kubeadm: enable strict config unmarhaling #70901

neolit123 · 2018-11-10T03:18:11Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

EDITED

my PR here has the potential to break the whole k8s:
#70839

so instead this kubeadm localized PR can be used, which enables YAML field validation outside of codecs.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes kubernetes/kubeadm#1066

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

kubeadm: enable strict unmarshaling of YAML configuration files and show warnings for unknown and duplicate fields.

/assign @fabriziopandini @rosti
/priority important-longterm
cc @kubernetes/sig-cluster-lifecycle-pr-reviews
cc @luxas @dims

cmd/kubeadm/app/util/config/initconfiguration.go

neolit123 · 2018-11-10T03:20:23Z

cmd/kubeadm/app/cmd/config_test.go

@@ -242,7 +242,6 @@ func TestMigrate(t *testing.T) {
 		# This is intentionally testing an old API version and the old kind naming and making sure the output is correct
 		apiVersion: kubeadm.k8s.io/v1alpha3
 		kind: InitConfiguration
-		kubernetesVersion: v1.12.0


some of these _test changes are bugs in our test config data..

cmd/kubeadm/app/util/config/initconfiguration.go

cmd/kubeadm/app/util/config/testdata/conversion/master/v1alpha3.yaml

neolit123 · 2018-11-11T21:39:26Z

updated the PR.
the solution is much cleaner now, but comments would be appreciated.

cmd/kubeadm/app/componentconfigs/registrations.go

neolit123 · 2018-11-11T23:49:26Z

/hold

neolit123 · 2018-11-12T01:09:37Z

/retest

fabriziopandini

@neolit123 thanks for this PR!
As you are saying this change has potential implications and so I would like it to be discussed at sig level.
My personal first impression about this is that I would like kubeadm to be aligned with kubernetes, and that this change is kind of risky at this stage of the cycle

rosti

I like the general idea. I see a couple things however:

I don't like the idea of imposing this on component configs (Kube-Proxy and Kubelet). Technically speaking, this validation should be done on their side and we should simply invoke their code to pre-validate things. Ideally, this should be done centrally like you suggested in [WIP] runtime/serializer: use DisallowUnknownFields for the json-iter config #70839 . I am not convinced, that we should single handedly impose restrictions in other people's configs.
Missing tests for InitConfiguration. We need those in the spirit of ClusterConfiguration.

cmd/kubeadm/app/util/config/testdata/validation/invalid_duplicate_field_mastercfg.yaml

cmd/kubeadm/app/util/config/testdata/validation/invalid_duplicate_field_nodecfg.yaml

cmd/kubeadm/app/util/config/testdata/validation/invalid_unknown_field_mastercfg.yaml

cmd/kubeadm/app/util/config/testdata/validation/invalid_unknown_field_nodecfg.yaml

neolit123 · 2018-11-12T13:20:44Z

@fabriziopandini

My personal first impression about this is that I would like kubeadm to be aligned with kubernetes, and that this change is kind of risky at this stage of the cycle

api machinery will not enable strict unmarshaling for the core codecs.
i don't see it happening in the next +4 cycles.

i will have office hours agenda on this. the change on the kubeadm side will always be risky, due to users having typos or duplicate fields in production YAMLs.

@rosti

I don't like the idea of imposing this on component configs (Kube-Proxy and Kubelet). Technically speaking, this validation should be done on their side and we should simply invoke their code to pre-validate things. Ideally, this should be done centrally like you suggested in #70839 . I am not convinced, that we should single handedly impose restrictions in other people's configs.

70839 is not going to happen. i just did it to see how many bogus YAML documents will fail the CI and there are quite a few all over the project.

i do agree about kubeproxy and kubelet not being our responsibility, but the maintainers are not going to do it anytime in the future. given kubeadm is a tool for the end user it feels to me that we have to do it now, even if temporary.

/assign @timothysc
for comments too please.

timothysc

We'll
/hold
until group consensus.

cmd/kubeadm/app/componentconfigs/registrations.go

cmd/kubeadm/app/util/config/common.go

cmd/kubeadm/app/util/config/initconfiguration.go

neolit123 · 2018-11-15T22:53:38Z

cmd/kubeadm/app/util/config/strict/strict.go

+	"sigs.k8s.io/yaml"
+)
+
+var localScheme = runtime.NewScheme()


the reason for this localscheme is that i cannot find a way to obtain external empty objects from our schemes such as:

&kubeadmapiv1beta1.ClusterConfiguration{}

in the case of: constants.ClusterConfigurationKind and kubeadmapiv1alpha3.SchemeGroupVersion

@fabriziopandini @rosti @timothysc please advise.
is there a magical way to convert from GroupVersionKind to such an external empty object for our already registered config types including component configs?

@neolit123 Isn't this one working for you?

kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme/scheme.go

Line 31 in 90245be

var Scheme = runtime.NewScheme()

rosti

I'll do a more thorough review tomorrow. Hope the suggestion works.

rosti · 2018-11-15T23:23:03Z

cmd/kubeadm/app/util/config/strict/strict.go

+	"sigs.k8s.io/yaml"
+)
+
+var localScheme = runtime.NewScheme()


@neolit123 Isn't this one working for you?

kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme/scheme.go

Line 31 in 90245be

var Scheme = runtime.NewScheme()

cmd/kubeadm/app/util/config/strict/strict.go

neolit123 · 2018-11-16T00:27:04Z

cmd/kubeadm/app/util/config/strict/strict.go

+		iface, err = componentconfigs.Scheme.New(gvk)
+		if err != nil {
+			err := errors.Errorf("unknown configuration %#v for scheme definitions in %q and %q",
+				gvk, scheme.Scheme.Name(), componentconfigs.Scheme.Name())


after the suggestion by @rosti this was greatly simplified and the init function was removed. 🎉
what this is doing is to check if a certain GVK exists in our kubeadm configs and if it doesn't it checks the component configs.

if it's missing for both it outputs such an error:

W1116 02:17:13.388274 30409 strict.go:47] unknown configuration schema.GroupVersionKind{Group:"kubeproxy.config.k8s.io", Version:"v1alpha1", Kind:"KubeProxyConfiguration1"} for scheme definitions "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme/scheme.go:31" and "k8s.io/kubernetes/cmd/kubeadm/app/componentconfigs/scheme.go:28"

@timothysc please let me know if you want me to further amend the error output.

timothysc

thx @neolit123 !
/lgtm
/approve

k8s-ci-robot · 2018-11-16T00:31:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, timothysc

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/kubeadm/OWNERS~~ [timothysc]

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

timothysc · 2018-11-16T00:32:06Z

/hold cancel

neolit123 · 2018-11-16T03:21:48Z

/test pull-kubernetes-e2e-gce-100-performance

luxas

LGTM, thanks, but some small cleanup notes

luxas · 2018-11-16T13:22:15Z

cmd/kubeadm/app/util/config/strict/strict.go

+// VerifyUnmarshalStrict takes a YAML byte slice and a GroupVersionKind and verifies if the YAML
+// schema is known and if it unmarshals with strict mode.
+//
+// TODO(neolit123): The returned error here is currently ignored everywhere and a klog warning is thrown instead.


Why don't you run klog if err != nil on the caller side instead?

i did this to contain the klog messages in one location.
otherwise i have to copy them two times - one for init and one for join config.

luxas · 2018-11-16T13:22:42Z

cmd/kubeadm/app/util/config/strict/strict.go

+func VerifyUnmarshalStrict(bytes []byte, gvk schema.GroupVersionKind) error {
+
+	var (
+		iface interface{}


nit: this isn't needed, just use := on first invocation instead

neolit123 · 2018-11-16T15:53:33Z

cmd/kubeadm/app/util/config/strict/strict_test.go

+			}
+			err = VerifyUnmarshalStrict(bytes, gvk)
+			if (err != nil) != test.expectedError {
+				t.Errorf("expected error %v, got %v, error: %v", err != nil, test.expectedError, err)


i managed to flip "got" and "expected" here..

k8s-ci-robot assigned fabriziopandini and rosti Nov 10, 2018

k8s-ci-robot requested review from fabriziopandini and stealthybox November 10, 2018 03:18

neolit123 commented Nov 10, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2018

neolit123 force-pushed the kubeadm-strict-config branch from e298aa7 to ff6bbe2 Compare November 11, 2018 21:33

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 11, 2018

liggitt reviewed Nov 11, 2018

View reviewed changes

cmd/kubeadm/app/componentconfigs/registrations.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/componentconfigs/registrations.go Outdated Show resolved Hide resolved

neolit123 force-pushed the kubeadm-strict-config branch from ff6bbe2 to ce663f3 Compare November 11, 2018 22:59

neolit123 changed the title ~~[WIP] kubeadm: enable strict config unmarhaling~~ kubeadm: enable strict config unmarhaling Nov 11, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2018

neolit123 force-pushed the kubeadm-strict-config branch 2 times, most recently from f4eea43 to d1d27ef Compare November 12, 2018 00:38

fabriziopandini reviewed Nov 12, 2018

View reviewed changes

rosti suggested changes Nov 12, 2018

View reviewed changes

k8s-ci-robot assigned timothysc Nov 12, 2018

neolit123 mentioned this pull request Nov 12, 2018

switch to yaml.UnmarshallStrict for config kubernetes-sigs/kind#96

Merged

neolit123 changed the title ~~kubeadm: enable strict config unmarhaling~~ [WIP] kubeadm: enable strict config unmarhaling Nov 12, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2018

timothysc reviewed Nov 13, 2018

View reviewed changes

neolit123 force-pushed the kubeadm-strict-config branch from d1d27ef to e4b48bb Compare November 15, 2018 22:47

neolit123 commented Nov 15, 2018

View reviewed changes

neolit123 force-pushed the kubeadm-strict-config branch 2 times, most recently from 73a0546 to 5552c66 Compare November 15, 2018 23:13

rosti suggested changes Nov 15, 2018

View reviewed changes

timothysc suggested changes Nov 15, 2018

View reviewed changes

cmd/kubeadm/app/util/config/strict/strict.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/util/config/strict/strict.go Outdated Show resolved Hide resolved

neolit123 added 2 commits November 16, 2018 02:21

kubeadm: handle strict config unmarshaling

cc60625

kubeadm: update autogenerated bazel

2473bba

neolit123 force-pushed the kubeadm-strict-config branch from 5552c66 to 2473bba Compare November 16, 2018 00:23

neolit123 commented Nov 16, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2018

timothysc approved these changes Nov 16, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2018

timothysc added this to the v1.13 milestone Nov 16, 2018

timothysc changed the title ~~[WIP] kubeadm: enable strict config unmarhaling~~ kubeadm: enable strict config unmarhaling Nov 16, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2018

k8s-ci-robot merged commit 4b98060 into kubernetes:master Nov 16, 2018

luxas reviewed Nov 16, 2018

View reviewed changes

neolit123 commented Nov 16, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: enable strict config unmarhaling #70901

kubeadm: enable strict config unmarhaling #70901

neolit123 commented Nov 10, 2018 •

edited

neolit123 Nov 10, 2018 •

edited

neolit123 commented Nov 11, 2018

neolit123 commented Nov 11, 2018

neolit123 commented Nov 12, 2018

fabriziopandini left a comment

rosti left a comment

neolit123 commented Nov 12, 2018

timothysc left a comment

neolit123 Nov 15, 2018 •

edited

rosti Nov 15, 2018

rosti left a comment

rosti Nov 15, 2018

neolit123 Nov 16, 2018 •

edited

timothysc left a comment

k8s-ci-robot commented Nov 16, 2018

timothysc commented Nov 16, 2018

neolit123 commented Nov 16, 2018

luxas left a comment

luxas Nov 16, 2018

neolit123 Nov 16, 2018

luxas Nov 16, 2018

neolit123 Nov 16, 2018

kubeadm: enable strict config unmarhaling #70901

kubeadm: enable strict config unmarhaling #70901

Conversation

neolit123 commented Nov 10, 2018 • edited

neolit123 Nov 10, 2018 • edited

Choose a reason for hiding this comment

neolit123 commented Nov 11, 2018

neolit123 commented Nov 11, 2018

neolit123 commented Nov 12, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

rosti left a comment

Choose a reason for hiding this comment

neolit123 commented Nov 12, 2018

timothysc left a comment

Choose a reason for hiding this comment

neolit123 Nov 15, 2018 • edited

Choose a reason for hiding this comment

rosti Nov 15, 2018

Choose a reason for hiding this comment

rosti left a comment

Choose a reason for hiding this comment

rosti Nov 15, 2018

Choose a reason for hiding this comment

neolit123 Nov 16, 2018 • edited

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 16, 2018

timothysc commented Nov 16, 2018

neolit123 commented Nov 16, 2018

luxas left a comment

Choose a reason for hiding this comment

luxas Nov 16, 2018

Choose a reason for hiding this comment

neolit123 Nov 16, 2018

Choose a reason for hiding this comment

luxas Nov 16, 2018

Choose a reason for hiding this comment

neolit123 Nov 16, 2018

Choose a reason for hiding this comment

neolit123 commented Nov 10, 2018 •

edited

neolit123 Nov 10, 2018 •

edited

neolit123 Nov 15, 2018 •

edited

neolit123 Nov 16, 2018 •

edited