kubeadm: add the "config validate" subcommand #118013

neolit123 · 2023-05-15T12:00:22Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

kubeadm: add the "config validate" subcommand

The command can be used to validate an input --config and throw
warnings and errors.

Add a new argument strctErrors to the functions
documentMapTo{Init|Join}Configuration(). This allows
to return errors from the calls to VerifyUnmarshalStrict().

Add a new function verifyKnownGVKs() in config/common.go
that is used to verify if a list of GVKs in a config file is
known. This function is used by the "validate" and "migrate"
commands.

Both commands now throw errors for unknown APIs or fields.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#2871

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

kubeadm: add a new "kubeadm config validate" command that can be used to validate any input config file. Use the --config flag to pass a config file to it. See the command --help screen for more information. As a result of adding this new command, enhance the validation capabilities of the existing "kubeadm config migrate" command. For both commands unknown APIs or fields will throw errors.

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

neolit123 · 2023-05-15T12:09:29Z

cmd/kubeadm/app/cmd/config.go

+ This command lets you validate a kubeadm configuration API file and report any warnings and errors.
+ If there are no errors the exit status will be zero, otherwise it will be non-zero.
+ Any unmarshaling problems such as unknown API fields will trigger warnings. Unknown API versions and
+ fields with invalid values will trigger fatal errors. Any other errors or warnings may be reported
+ depending on contents of the input file.
+
+ In this version of kubeadm, the following API versions are supported:
+ - %s


this behavior is exactly the same as any kubeadm command that accepts a --config flag. same for "kubeadm config migrate". my thinking was that we don't want to make e.g. warnings into errors when validating but instead keep the validation behavior consistent across commands. warnings are shown with klog on stderr, so it is possible for the user to check if stderr is empty on bash from the command line.

@howardjohn @jordanbreen28

for feedback

also please check these comments about printing to stdout
#118013 (comment)

neolit123 · 2023-05-15T12:10:23Z

cmd/kubeadm/app/cmd/config.go

+ // MigrateOldConfig performs conversion to internal type, strict unmarshaling,
+ // defaulting and validation. It can be used for the validation purpose with output bytes
+ // ignored.
+ if _, err = configutil.MigrateOldConfig(cfgBytes); err != nil {


this is essentially reusing the migrate backend and ignoring the output.
LMK, if you think we should refactor it for more clarity.

+1 to define another func for this command even if it just calls MigrateOldConfig instead.

I tried this command, one thing might worth to discuss here is that if only unknown obj defined in the cfg, there is no warning at all.

e.g. the config is defined as this,

kind: unknownKind apiVersion: kubeadm.k8s.io/v1beta3 unknowkey: unknownvalue

kubeadm config validate --config /home/davche02/backup/config/kubeadm/bootstrapfake.cfg

validation passed in this case.
We'd better to thrown a warning here.

we should throw an error for unknown GVK.
IIRC, the code paths for init, join do that.

+1 to define another func for this command even if it just calls MigrateOldConfig instead.

there could be migrate and validate funcs that share a common backend.

the validate func will not have the marshaling / output.

+1 to define another func for this command even if it just calls MigrateOldConfig instead.

added a new ValidateConfig util function now.

--- apiVersion: foo/v1beta1 kind: Bar

Should we add this to the unit test case?
BTW, an empty file would be a test case as well.

With the latest code 0d85d67, there is no warning if the configuration is not match Join/Init/Cluster according to https://github.com/kubernetes/kubernetes/pull/118013/files#diff-5569e71dfa98866b583171d92e018cf11680c6e1ab16e55ecd0c1e2a0f1f4f4eR250-R264.

Should we add this to the unit test case?

it's just a warning on stderr (klog) so there is no straightforward way to test it. we could switch it to error and it would be easier to unit test.

With the latest code 0d85d67, there is no warning

i think the problem is that i did not test just unknown GVK. it was combined with known GVK. the logic needs update..

updated, see new PR description and release note amend.

unknown APIs (GVK) and unknown fields now throw errors in migrate/validate commands

other commands like init/join are not affected

added better unit tests for both new commands

neolit123 · 2023-05-15T12:42:36Z

/hold
please keep the hold until discussion is resolved

neolit123 · 2023-05-15T13:17:45Z

/retest

untelated test failures

chendave · 2023-05-16T02:05:45Z

/assign

chendave · 2023-05-16T09:14:22Z

cmd/kubeadm/app/cmd/config.go

+ // MigrateOldConfig performs conversion to internal type, strict unmarshaling,
+ // defaulting and validation. It can be used for the validation purpose with output bytes
+ // ignored.
+ if _, err = configutil.MigrateOldConfig(cfgBytes); err != nil {


+1 to define another func for this command even if it just calls MigrateOldConfig instead.

chendave · 2023-05-16T09:15:22Z

cmd/kubeadm/app/cmd/config.go

+ // ignored.
+ if _, err = configutil.MigrateOldConfig(cfgBytes); err != nil {
+ return err
+ }


I'd prefer to print something if the validation is okay, nothing output to stdout now if everything is good.

perhaps, we should. i'd like to get more opinions on this. the exit code 0 / 1 may be sufficient.

looks like there are no comments on this. my preference is to not print on stdout on "OK", similarly to some go tools.

If making it like the dry-run, we can print a result for each configuration in the file

[root@daocloud ~]# ./kubeadm-dev config validate --config kubeadmcfg.yaml InitConfiguration validated ClusterConfiguration validated Bar unknown

This would be much clear if there are multi items in the configuration files.

Or, we can just print OK/Validated/Done when there is no error/warning. This is also good.

actually, there is no clean way to detect if a warning was printed. it just goes to stderr from klog. non warnings go there too.

printing per validated config is possible but it will complicate the plumbing of functions a bit. we don't want to print the same messages during normal init/join...but the backend functions are the same.

i will check what options work well.

updated to just print "ok" if validation has no errors.

chendave · 2023-05-16T09:18:08Z

cmd/kubeadm/app/cmd/config.go

+ // MigrateOldConfig performs conversion to internal type, strict unmarshaling,
+ // defaulting and validation. It can be used for the validation purpose with output bytes
+ // ignored.
+ if _, err = configutil.MigrateOldConfig(cfgBytes); err != nil {


I tried this command, one thing might worth to discuss here is that if only unknown obj defined in the cfg, there is no warning at all.

e.g. the config is defined as this,

kind: unknownKind apiVersion: kubeadm.k8s.io/v1beta3 unknowkey: unknownvalue

kubeadm config validate --config /home/davche02/backup/config/kubeadm/bootstrapfake.cfg

validation passed in this case.
We'd better to thrown a warning here.

pacoxu · 2023-05-21T12:49:18Z

/cc
/priority important-soon
/triage accepted

neolit123 · 2023-05-22T17:15:13Z

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

@@ -190,8 +191,37 @@ func ChooseAPIServerBindAddress(bindAddress net.IP) (net.IP, error) {
 return ip, nil
 }

+// verifyKnownGVKs takes a list of GVKs and verifies if they are known in kubeadm or component config schemes
+func verifyKnownGVKs(gvks []schema.GroupVersionKind) error {


documentMapTo{Init|Join}Configuration() calls VerifyUnmarshalStrict() which contains similar logic to test for known GVKs. having verifyKnownGVKs() and calling it early in MigrateOldConfig() and ValidateConfig() has the benefits of exiting early for unknown GVKs. logic is a bit duplicated, yet for both migrate/validate commands we want a common method to check if an API is known early on and include component config in there as well. during VerifyUnmarshalStrict() for documentMapToJoinConfiguration() component config is not considered.

please LMK, if someone has strong opinions for refactors here.

pacoxu · 2023-05-23T02:29:59Z

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

+ gvks = append(gvks, gvk)
+ }
+
+ if err := verifyKnownGVKs(gvks); err != nil {


When a user is using the v1beta2, it will just be shown as unknown.

For kubeadm config migrate, it will show the message that you can migrate v1beta2 to v1beta3 using kubeadm v1.22. I think the message helps when validating an old version of the kubeadm configuration.

https://github.com/kubernetes/kubernetes/blob/ed1e45b1092a6d736895b6fd0764b2b01b258e04/cmd/kubeadm/app/util/config/common.go#L57-L58

Using validateSupportedVersion can show that message.

i also think the message is valuable.

this is a bit problematic because the validateSupportedVersion function only validates a known GroupVersion. it does not know what Kinds existed in the version.

i need to think what is best here.

updated to keep legacy versions in mind.

pacoxu · 2023-05-24T02:16:51Z

/lgtm

The current behavior and warning messages are LGTM.

k8s-ci-robot · 2023-05-24T02:16:58Z

LGTM label has been added.

Git tree hash: 81efb3435b92bb18176b93d022747b35927a02b1

chendave

/lgtm

Looks great!

chendave · 2023-05-24T02:53:14Z

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

+ return err
+ }
+
+ // Migrate InitConfiguration and ClusterConfiguration if there are any in the config


Looks like these comments is from MigrateOldConfig, it is a little confusion here to say Migrate?

changed comments to // Validate ...

chendave · 2023-05-24T02:54:37Z

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

+ }
+ }
+
+ // Migrate JoinConfiguration if there is any


The command can be used to validate an input --config and throw warnings and errors. Add a new argument strctErrors to the functions documentMapTo{Init|Join}Configuration(). This allows to return errors from the calls to VerifyUnmarshalStrict(). Add a new function verifyKnownGVKs() in config/common.go that is used to verify if a list of GVKs in a config file is known. This function is used by the "validate" and "migrate" commands. Both commands now throw errors for unknown APIs or fields.

chendave

/lgtm
thanks!

k8s-ci-robot · 2023-05-25T01:58:52Z

LGTM label has been added.

Git tree hash: 5e2a878812ce3011bb60b479e3fec3853ef3775a

pacoxu · 2023-05-25T02:15:22Z

/unhold
as all discussions are addressed.
/approve

k8s-ci-robot · 2023-05-25T02:16:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendave, neolit123, pacoxu

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~~ [neolit123]

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 requested review from chendave and fabriziopandini May 15, 2023 12:01

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2023

neolit123 commented May 15, 2023

View reviewed changes

This was referenced May 15, 2023

Add ability to validate configuration yaml kubernetes/kubeadm#2871

Closed

kubeadm: add the "config validate" command as placeholder kubernetes/website#41150

Merged

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

k8s-ci-robot assigned chendave May 16, 2023

chendave reviewed May 16, 2023

View reviewed changes

k8s-ci-robot requested a review from pacoxu May 21, 2023 12:49

neolit123 force-pushed the 1.28-add-config-validate branch from 6e19f0a to 0d85d67 Compare May 22, 2023 09:57

neolit123 force-pushed the 1.28-add-config-validate branch from 0d85d67 to ed1e45b Compare May 22, 2023 17:00

neolit123 commented May 22, 2023

View reviewed changes

pacoxu reviewed May 23, 2023

View reviewed changes

neolit123 force-pushed the 1.28-add-config-validate branch from ed1e45b to 797e726 Compare May 23, 2023 10:47

k8s-ci-robot assigned pacoxu May 24, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2023

chendave reviewed May 24, 2023

View reviewed changes

neolit123 force-pushed the 1.28-add-config-validate branch from 797e726 to 72e4c9a Compare May 24, 2023 18:22

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2023

k8s-ci-robot requested review from chendave and pacoxu May 24, 2023 18:22

chendave approved these changes May 25, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2023

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

k8s-ci-robot merged commit 90ed8ba into kubernetes:master May 25, 2023
12 checks passed

k8s-ci-robot added this to the v1.28 milestone May 25, 2023

jaskaransarkaria mentioned this pull request Jun 5, 2024

Planning upgrade to EKS 1.28 ministryofjustice/cloud-platform#5570

Open

kubeadm: add the "config validate" subcommand #118013

kubeadm: add the "config validate" subcommand #118013

Conversation

neolit123 commented May 15, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

neolit123 May 15, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacoxu May 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 May 22, 2023 • edited

Choose a reason for hiding this comment

neolit123 commented May 15, 2023

neolit123 commented May 15, 2023

chendave commented May 16, 2023

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

neolit123 May 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacoxu commented May 21, 2023

neolit123 May 22, 2023 • 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

pacoxu commented May 24, 2023 • edited

k8s-ci-robot commented May 24, 2023

chendave left a 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

chendave left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 25, 2023

pacoxu commented May 25, 2023

k8s-ci-robot commented May 25, 2023

neolit123 commented May 15, 2023 •

edited

neolit123 May 15, 2023 •

edited

pacoxu May 22, 2023 •

edited

neolit123 May 22, 2023 •

edited

neolit123 May 23, 2023 •

edited

neolit123 May 22, 2023 •

edited

pacoxu commented May 24, 2023 •

edited