Enable strict serializer in kube-proxy #82927

obitech · 2019-09-20T08:50:12Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
With strict serializers enabled, an error will be thrown on things such as duplicate or unknown fields when parsing a YAML file. This will improve code quality of user's configuration files as it enforces a proper configuration file to be present when starting a component.

Which issue(s) this PR fixes:

Ref #82924

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-proxy: a configuration file specified via `--config` is now loaded with strict deserialization, which fails if the config file contains duplicate or unknown fields. This protects against accidentally running with config files that are malformed, mis-indented, or have typos in field names, and getting unexpected behavior.

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

NONE

/sig cluster-lifecycle
/sig api-machinery
/wg component-standard
/cc @mtaufen @stealthybox

obitech · 2019-09-20T08:52:12Z

/assign @thockin

stealthybox

Nice job on this!
Here's my feedback:

stealthybox · 2019-09-21T00:42:12Z

cmd/kube-proxy/app/server_test.go

@@ -279,35 +281,130 @@ nodePortAddresses:

 // TestLoadConfigFailures tests failure modes for loadConfig()
 func TestLoadConfigFailures(t *testing.T) {
+	duplicateKeyYAMLTemplate := `bindAddress: 0.0.0.0
+bindAddress: 1.2.3.4


Maybe we could just represent the minimal config for these:

duplicateKeyYAMLTemplate := `bindAddress: 0.0.0.0 bindAddress: 1.2.3.4 clusterCIDR: "1.2.3.0/24" configSyncPeriod: 15s configSyncPeriod: 30s kind: KubeProxyConfiguration`

Looking at the test runner, the apiVersion is appended to every test to reduce maintenance burden.
Being less specific about the fields make these strings easier to maintain.

There's already a happy path test for TestLoadConfig

It's possible to prepend some unrelated fields if a more robust test is desired, but this isn't a rigorous suite for the YAML parser.

WDYT?

Yeah that's much cleaner. I provided a basicYAMLTemplate which gets appended to in each tests accordingly.

stealthybox · 2019-09-21T01:18:22Z

cmd/kube-proxy/app/server_test.go

+		{
+			name:   "Duplicate fields",
+			config: duplicateKeyYAMLTemplate,
+			errFn: func(err error) bool {


This can change to:

errFn: kuberuntime.IsStrictDecodingError

That function contains a nil check, and it should already have been asserted an error exists before running it because of the nature of this range test.

stealthybox · 2019-09-21T01:19:21Z

cmd/kube-proxy/app/server_test.go

 		},
 		{
 			name:   "Bad config type test",
 			config: "kind: KubeSchedulerConfiguration",
-			expErr: "no kind",
+			errFn:  func(err error) bool { return err != nil },


These functions can be omitted since the test will assert an error exists for every case.
Leaving the fields unspecified will result in a zero-value of nil.

stealthybox · 2019-09-21T01:31:19Z

cmd/kube-proxy/app/server_test.go

-		options := NewOptions()
-		config := fmt.Sprintf("%s\n%s", version, tc.config)
-		_, err := options.loadConfig([]byte(config))
-		if assert.Error(t, err, tc.name) {


Nice addition of t.Run 👍

We should keep this:

if assert.Error(t, err, tc.name) {

This whole test is about failures, so we assert that one should exist.
This is equivalent to if err != nil. It protects us from operating on nil errors.
The sneaky benefit of using assert.Error is that it will also fail the test if it is a nil error.

We can keep the old test behavior for string matching the error text.
To gate it, just check for the zero value "".
The gate is not strictly necessary since Contains will always pass with an empty string, but it's more clear.

We should also check that errFn != nil before running it, otherwise go will
panic: runtime error: invalid memory address or nil pointer dereference.
Gating this has the added benefit of making this errFn optional as well.

Finally, we should use the assert library to maintain style.

ex:

t.Run(tc.name, func(t *testing.T) { options := NewOptions() config := fmt.Sprintf("%s\n%s", version, tc.config) _, err := options.loadConfig([]byte(config)) if assert.Error(t, err, tc.name) { if tc.expErr != "" { assert.Contains(t, err.Error(), tc.expErr, tc.name) } if tc.errFn != nil { assert.True(t, tc.errFn(err), tc.name) } } })

errFn may be better named as checkFn.
The verb/purpose is not "to error" but rather "to check and return a bool".

I like it much better! I adjusted it accordingly

stealthybox · 2019-09-23T16:15:13Z

/retest
/lgtm

stealthybox · 2019-09-23T17:29:58Z

/retest

obitech · 2019-09-23T18:59:11Z

/retest

roycaihw · 2019-09-23T21:12:54Z

/cc @caesarxuchao

caesarxuchao · 2019-09-23T21:21:13Z

/assign

obitech · 2019-10-07T15:46:12Z

/assign @caseydavenport @bowei @dcbw

thockin · 2019-10-09T16:31:20Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2019-10-09T16:32:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: obitech, thockin

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:

~~cmd/kube-proxy/OWNERS~~ [thockin]
~~pkg/proxy/apis/config/OWNERS~~ [thockin]

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

mtaufen · 2019-10-11T23:51:13Z

pkg/proxy/apis/config/scheme/scheme.go

@@ -29,7 +29,7 @@ var (
 	Scheme = runtime.NewScheme()
 	// Codecs provides methods for retrieving codecs and serializers for specific
 	// versions and content types.
-	Codecs = serializer.NewCodecFactory(Scheme)
+	Codecs = serializer.NewCodecFactory(Scheme, serializer.EnableStrict)


@obitech I noticed this merged, did you plan to add a lenient path to this PR before then?

…rializer Enable strict serializer in kube-proxy

k8s-ci-robot requested a review from mtaufen September 20, 2019 08:50

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 20, 2019

k8s-ci-robot requested a review from stealthybox September 20, 2019 08:50

obitech mentioned this pull request Sep 20, 2019

Enable strict serializers in components by default #82924

Closed

6 tasks

obitech added this to In progress in component-base Sep 20, 2019

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Sep 20, 2019

k8s-ci-robot assigned thockin Sep 20, 2019

stealthybox suggested changes Sep 21, 2019

View reviewed changes

obitech force-pushed the kube_proxy_strict_serializer branch from bef5f93 to 0c692fd Compare September 23, 2019 15:03

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2019

k8s-ci-robot assigned stealthybox Sep 23, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2019

Add strict serializer to codec factory in kube-proxy

f9b152a

obitech force-pushed the kube_proxy_strict_serializer branch from 7addd7a to f9b152a Compare September 23, 2019 17:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2019

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 23, 2019

k8s-ci-robot requested a review from caesarxuchao September 23, 2019 21:12

k8s-ci-robot assigned caesarxuchao Sep 23, 2019

k8s-ci-robot assigned bowei, caseydavenport and dcbw Oct 7, 2019

mtaufen added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Oct 8, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 8, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2019

k8s-ci-robot merged commit 0ff761b into kubernetes:master Oct 9, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 9, 2019

obitech moved this from In progress to Done in component-base Oct 10, 2019

mtaufen reviewed Oct 11, 2019

View reviewed changes

ohsewon pushed a commit to ohsewon/kubernetes that referenced this pull request Oct 16, 2019

Merge pull request kubernetes#82927 from obitech/kube_proxy_strict_se…

2316ce4

…rializer Enable strict serializer in kube-proxy

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable strict serializer in kube-proxy #82927

Enable strict serializer in kube-proxy #82927

obitech commented Sep 20, 2019 •

edited

obitech commented Sep 20, 2019

stealthybox left a comment

stealthybox Sep 21, 2019 •

edited

obitech Sep 23, 2019

stealthybox Sep 21, 2019

obitech Sep 23, 2019

stealthybox Sep 21, 2019

obitech Sep 23, 2019

stealthybox Sep 21, 2019 •

edited

stealthybox Sep 21, 2019

obitech Sep 23, 2019

stealthybox commented Sep 23, 2019

stealthybox commented Sep 23, 2019

obitech commented Sep 23, 2019

roycaihw commented Sep 23, 2019

caesarxuchao commented Sep 23, 2019

obitech commented Oct 7, 2019

thockin commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

mtaufen Oct 11, 2019

Enable strict serializer in kube-proxy #82927

Enable strict serializer in kube-proxy #82927

Conversation

obitech commented Sep 20, 2019 • edited

obitech commented Sep 20, 2019

stealthybox left a comment

Choose a reason for hiding this comment

stealthybox Sep 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stealthybox Sep 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stealthybox commented Sep 23, 2019

stealthybox commented Sep 23, 2019

obitech commented Sep 23, 2019

roycaihw commented Sep 23, 2019

caesarxuchao commented Sep 23, 2019

obitech commented Oct 7, 2019

thockin commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

Choose a reason for hiding this comment

obitech commented Sep 20, 2019 •

edited

stealthybox Sep 21, 2019 •

edited

stealthybox Sep 21, 2019 •

edited