Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lenient decoding path for v1alpha1 kube-proxy config #84143

Merged
merged 1 commit into from Oct 30, 2019

Conversation

@phenixblue
Copy link
Member

phenixblue commented Oct 21, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:

This implements a lenient path for decoding a kube-proxy config file.
The config file gets decoded with a strict serializer first, if that fails a lenient
CodecFactory that has just v1alpha1 registered into it is used for decoding. The lenient
path is to be dropped when support for v1alpha1 is dropped.

The reason for this patch is that after discussion with sig-cluster-lifecycle we realized that
Kustomize needs a metadata.name field to be present and right now users are adding a dummy metadata.name field so they can use Kustomize for component configs. However this would not be possible with strict decoding. Additionally, after discussion with wg-component-standard we agreed to implement a lenient decoding path regardless of versions, so we can warn users before we introduce breaking changes to their clusters.

Which issue(s) this PR fixes:

Ref #82924

Does this PR introduce a user-facing change?:

kube-proxy: emits a warning when a malformed component config file is used with v1alpha1.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE
@phenixblue

This comment has been minimized.

Copy link
Member Author

phenixblue commented Oct 21, 2019

/cc @mtaufen @stealthybox @obitech
/sig network
/wg component-standard

Copy link
Contributor

obitech left a comment

Thanks for the PR! I left a comment regarding the tests.

Also you will need to update the bazel BUILD files via ./hack/update-bazel.sh.

name: "Duplicate fields",
config: fmt.Sprintf("%s\nbindAddess: 1.2.3.4", yamlTemplate),
checkFn: kuberuntime.IsStrictDecodingError,
},
{
// This should fail from v1alpha2+
name: "Unknown field",

This comment has been minimized.

Copy link
@obitech

obitech Oct 21, 2019

Contributor

With the lenient path these tests should not fail anymore, but yield a valid config.

This comment has been minimized.

Copy link
@phenixblue

phenixblue Oct 21, 2019

Author Member

OK, I was wondering about the BUILD file. I'll update that and the comments.

This comment has been minimized.

Copy link
@phenixblue

phenixblue Oct 21, 2019

Author Member

^^^ Done, and squashed

@obitech obitech mentioned this pull request Oct 21, 2019
6 of 6 tasks complete
@phenixblue phenixblue force-pushed the phenixblue:master branch from 526e643 to d002455 Oct 21, 2019
Copy link
Contributor

mtaufen left a comment

Overall looks good. Keep an eye on #83204 so we keep the style consistent.

if !runtime.IsStrictDecodingError(err) {
return nil, fmt.Errorf("failed to decode: %v", err)
}
klog.Warningf("using lenient decoding as strict decoding failed: %v", err)

This comment has been minimized.

Copy link
@mtaufen

mtaufen Oct 22, 2019

Contributor

@liggitt had a comment (https://github.com/kubernetes/kubernetes/pull/83204/files#r337558047) about only printing this warning when we successfully decode with the lenient codec

This comment has been minimized.

Copy link
@phenixblue

phenixblue Oct 25, 2019

Author Member

Updated to match

@mtaufen

This comment has been minimized.

Copy link
Contributor

mtaufen commented Oct 22, 2019

Would also be worth adding some tests.

Copy link
Member

rosti left a comment

Thanks @phenixblue !


var (
configObj runtime.Object
gvk *schema.GroupVersionKind
)

Comment on lines 402 to 407

This comment has been minimized.

Copy link
@rosti

rosti Oct 23, 2019

Member

I don't think, that you need this block here.

This comment has been minimized.

Copy link
@phenixblue

phenixblue Oct 25, 2019

Author Member

Ok, cleaned up.

@phenixblue phenixblue force-pushed the phenixblue:master branch 2 times, most recently from 7a99b5e to a8bc9ce Oct 25, 2019
@phenixblue

This comment has been minimized.

Copy link
Member Author

phenixblue commented Oct 25, 2019

/test pull-kubernetes-bazel-test
/test pull-kubernetes-integration

@phenixblue phenixblue force-pushed the phenixblue:master branch from a8bc9ce to 54bafb4 Oct 27, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Oct 27, 2019
@phenixblue phenixblue force-pushed the phenixblue:master branch from 54bafb4 to 9c0a03e Oct 27, 2019
@phenixblue

This comment has been minimized.

Copy link
Member Author

phenixblue commented Oct 27, 2019

Would also be worth adding some tests.

I've taken a stab at testing the lenient path for unknown/duplicate fields. Wasn't sure the best way to handle this since the behavior will change as we implement config API versions > v1alpha1 and the strict decoding behavior is expected.

Was thinking of maybe adding in a mechanism to specify the config API version per test, but wasn't sure the effort is warranted if things will just be removed once support for v1alpha1 is dropped.

Thoughts?

@phenixblue phenixblue force-pushed the phenixblue:master branch from 9c0a03e to 0ee01f0 Oct 27, 2019
@phenixblue phenixblue changed the title Add lenient decoding path for v1alpha1 kube-proxy [WIP] Add lenient decoding path for v1alpha1 kube-proxy config [WIP] Oct 27, 2019
@@ -219,6 +219,30 @@ nodePortAddresses:
healthzBindAddress: "[fd00:1::5]:12345",
metricsBindAddress: "[fd00:2::5]:23456",
},
{
// Test for unknown field within config.
// For v1alpha1 a lenient path is implemented and will throw a

This comment has been minimized.

Copy link
@mtaufen

mtaufen Oct 29, 2019

Contributor

If you need to specify the version, we can update the yamlTemplate.

This comment has been minimized.

Copy link
@phenixblue

phenixblue Oct 29, 2019

Author Member

Based on our chat earlier today, I think we have a gameplay for further cleanup around this and that will come as a separate PR.

@@ -268,23 +292,36 @@ nodePortAddresses:

options := NewOptions()

yaml := fmt.Sprintf(
baseYAML := fmt.Sprintf(

This comment has been minimized.

Copy link
@mtaufen

mtaufen Oct 29, 2019

Contributor

May be better to use Go templates rather than fmt.Sprintf so we don't have to remember the order of parameters in a big format string.

This comment has been minimized.

Copy link
@phenixblue

phenixblue Oct 29, 2019

Author Member

I will make this part of the cleanup efforts in a separate PR, as discussed.

yamlTemplate, tc.bindAddress, tc.clusterCIDR,
tc.healthzBindAddress, tc.metricsBindAddress, tc.mode)

// Append additional configuration to the base yaml template
yaml := fmt.Sprintf("%s"+tc.extraConfig, baseYAML)

This comment has been minimized.

Copy link
@mtaufen

mtaufen Oct 29, 2019

Contributor

Recommend fmt.Sprintf("%s\n%s", baseYAML, tc.extraConfig) (best practice not to compute the format string).

This comment has been minimized.

Copy link
@mtaufen

mtaufen Oct 29, 2019

Contributor

I'd like this fix (even though technically the yaml template has a newline at the end, it's good defense), otherwise LGTM.

This comment has been minimized.

Copy link
@phenixblue

phenixblue Oct 30, 2019

Author Member

Fixed

@mtaufen

This comment has been minimized.

Copy link
Contributor

mtaufen commented Oct 29, 2019

Was thinking of maybe adding in a mechanism to specify the config API version per test, but wasn't sure the effort is warranted if things will just be removed once support for v1alpha1 is dropped.

I think it's worth it, whenever we have multiple concurrent versions, we should be testing that they all work.

config: fmt.Sprintf("%s\nfoo: bar", yamlTemplate),
checkFn: kuberuntime.IsStrictDecodingError,
},
// to-do: Uncomment below tests when v1alpha2+ of kube-proxy config is

This comment has been minimized.

Copy link
@mtaufen

mtaufen Oct 29, 2019

Contributor

nit: TODO(username)

This comment has been minimized.

Copy link
@phenixblue

phenixblue Oct 29, 2019

Author Member

Fixed

@phenixblue phenixblue force-pushed the phenixblue:master branch from 0ee01f0 to 4154de5 Oct 29, 2019
Removed unneeded comments

Matched style from other PR's

Only print error when lenient decoding is successful

Update Bazel for BUILD

Comment out existing strict decoder tests

Added tests for leniant path

Added comments to explain test additions

Cleanup TODO's and tests

Add explicit newline for appended config
@phenixblue phenixblue force-pushed the phenixblue:master branch from 4154de5 to 10879d3 Oct 30, 2019
@phenixblue

This comment has been minimized.

Copy link
Member Author

phenixblue commented Oct 30, 2019

/test pull-kubernetes-e2e-gce

@phenixblue phenixblue changed the title Add lenient decoding path for v1alpha1 kube-proxy config [WIP] Add lenient decoding path for v1alpha1 kube-proxy config Oct 30, 2019
@mtaufen

This comment has been minimized.

Copy link
Contributor

mtaufen commented Oct 30, 2019

/lgtm
/priority important-soon

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Oct 30, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, phenixblue

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 7fd399e into kubernetes:master Oct 30, 2019
15 checks passed
15 checks passed
cla/linuxfoundation phenixblue authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.