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.

Sign up for GitHub

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

🌱 KCP: improve validation webhooks #6129

Merged
merged 2 commits into from
Mar 10, 2022
Merged

🌱 KCP: improve validation webhooks #6129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f4474a
KCP: improve validation webhooks
sbueringer Feb 15, 2022
7a02662
fix review finding
sbueringer Feb 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 20 additions & 22 deletions bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (c *KubeadmConfig) ValidateDelete() error {
}

func (c *KubeadmConfigSpec) validate(name string) error {
allErrs := c.Validate()
allErrs := c.Validate(field.NewPath("spec"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is passing the spec field inside for code or is it solving an actual problem? While going through the changes, the older version with e.g. "spec" "x" "y" "z" is a bit more clear than the current proposed changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solves the problem that we are reporting wrong field paths in our field errors in some webhooks.

We have different places where KubeadmConfigSpec are used, e.g. in KCP. But today all our webhooks report that the error is in .spec... without considering that it's actually e.g. .spec.kubeadmConfigSpec...

Furthermore (in my opinion) it's a best practice to build up those field paths incrementally, so each function only has to append new relative paths and we don't have to look up and down complex call hierarchies to figure out if the resulting field error has a correct field path. We are e.g. using that in our ClusterClass validation quite extensively (and in my opinion successfully :) ).


if len(allErrs) == 0 {
return nil
Expand All @@ -73,16 +73,16 @@ func (c *KubeadmConfigSpec) validate(name string) error {
}

// Validate ensures the KubeadmConfigSpec is valid.
func (c *KubeadmConfigSpec) Validate() field.ErrorList {
func (c *KubeadmConfigSpec) Validate(pathPrefix *field.Path) field.ErrorList {
var allErrs field.ErrorList

allErrs = append(allErrs, c.validateFiles()...)
allErrs = append(allErrs, c.validateIgnition()...)
allErrs = append(allErrs, c.validateFiles(pathPrefix)...)
allErrs = append(allErrs, c.validateIgnition(pathPrefix)...)

return allErrs
}

func (c *KubeadmConfigSpec) validateFiles() field.ErrorList {
func (c *KubeadmConfigSpec) validateFiles(pathPrefix *field.Path) field.ErrorList {
var allErrs field.ErrorList

knownPaths := map[string]struct{}{}
Expand All @@ -93,7 +93,7 @@ func (c *KubeadmConfigSpec) validateFiles() field.ErrorList {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "files").Index(i),
pathPrefix.Child("files").Index(i),
file,
conflictingFileSourceMsg,
),
Expand All @@ -106,19 +106,17 @@ func (c *KubeadmConfigSpec) validateFiles() field.ErrorList {
if file.ContentFrom.Secret.Name == "" {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "files").Index(i).Child("contentFrom", "secret", "name"),
file,
field.Required(
pathPrefix.Child("files").Index(i).Child("contentFrom", "secret", "name"),
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
missingSecretNameMsg,
),
)
}
if file.ContentFrom.Secret.Key == "" {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "files").Index(i).Child("contentFrom", "secret", "key"),
file,
field.Required(
pathPrefix.Child("files").Index(i).Child("contentFrom", "secret", "key"),
missingSecretKeyMsg,
),
)
Expand All @@ -129,7 +127,7 @@ func (c *KubeadmConfigSpec) validateFiles() field.ErrorList {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "files").Index(i).Child("path"),
pathPrefix.Child("files").Index(i).Child("path"),
file,
pathConflictMsg,
),
Expand All @@ -141,18 +139,18 @@ func (c *KubeadmConfigSpec) validateFiles() field.ErrorList {
return allErrs
}

func (c *KubeadmConfigSpec) validateIgnition() field.ErrorList {
func (c *KubeadmConfigSpec) validateIgnition(pathPrefix *field.Path) field.ErrorList {
var allErrs field.ErrorList

if !feature.Gates.Enabled(feature.KubeadmBootstrapFormatIgnition) {
if c.Format == Ignition {
allErrs = append(allErrs, field.Forbidden(
field.NewPath("spec", "format"), kubeadmBootstrapFormatIgnitionFeatureDisabledMsg))
pathPrefix.Child("format"), kubeadmBootstrapFormatIgnitionFeatureDisabledMsg))
}

if c.Ignition != nil {
allErrs = append(allErrs, field.Forbidden(
field.NewPath("spec", "ignition"), kubeadmBootstrapFormatIgnitionFeatureDisabledMsg))
pathPrefix.Child("ignition"), kubeadmBootstrapFormatIgnitionFeatureDisabledMsg))
}

return allErrs
Expand All @@ -163,7 +161,7 @@ func (c *KubeadmConfigSpec) validateIgnition() field.ErrorList {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "format"),
pathPrefix.Child("format"),
c.Format,
fmt.Sprintf("must be set to %q if spec.ignition is set", Ignition),
),
Expand All @@ -178,7 +176,7 @@ func (c *KubeadmConfigSpec) validateIgnition() field.ErrorList {
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "users").Index(i).Child("inactive"),
pathPrefix.Child("users").Index(i).Child("inactive"),
cannotUseWithIgnition,
),
)
Expand All @@ -189,7 +187,7 @@ func (c *KubeadmConfigSpec) validateIgnition() field.ErrorList {
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "useExperimentalRetryJoin"),
pathPrefix.Child("useExperimentalRetryJoin"),
cannotUseWithIgnition,
),
)
Expand All @@ -204,7 +202,7 @@ func (c *KubeadmConfigSpec) validateIgnition() field.ErrorList {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "diskSetup", "partitions").Index(i).Child("tableType"),
pathPrefix.Child("diskSetup", "partitions").Index(i).Child("tableType"),
*partition.TableType,
fmt.Sprintf(
"only partition type %q is supported when spec.format is set to %q",
Expand All @@ -221,7 +219,7 @@ func (c *KubeadmConfigSpec) validateIgnition() field.ErrorList {
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "diskSetup", "filesystems").Index(i).Child("replaceFS"),
pathPrefix.Child("diskSetup", "filesystems").Index(i).Child("replaceFS"),
cannotUseWithIgnition,
),
)
Expand All @@ -231,7 +229,7 @@ func (c *KubeadmConfigSpec) validateIgnition() field.ErrorList {
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "diskSetup", "filesystems").Index(i).Child("partition"),
pathPrefix.Child("diskSetup", "filesystems").Index(i).Child("partition"),
cannotUseWithIgnition,
),
)
Expand Down
2 changes: 1 addition & 1 deletion bootstrap/kubeadm/api/v1beta1/kubeadmconfigtemplate_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (r *KubeadmConfigTemplate) ValidateDelete() error {
func (r *KubeadmConfigTemplateSpec) validate(name string) error {
var allErrs field.ErrorList

allErrs = append(allErrs, r.Template.Spec.Validate()...)
allErrs = append(allErrs, r.Template.Spec.Validate(field.NewPath("spec", "template", "spec"))...)

if len(allErrs) == 0 {
return nil
Expand Down
23 changes: 18 additions & 5 deletions controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (in *KubeadmControlPlane) ValidateCreate() error {
spec := in.Spec
allErrs := validateKubeadmControlPlaneSpec(spec, in.Namespace, field.NewPath("spec"))
allErrs = append(allErrs, validateClusterConfiguration(spec.KubeadmConfigSpec.ClusterConfiguration, nil, field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration"))...)
allErrs = append(allErrs, in.Spec.KubeadmConfigSpec.Validate()...)
allErrs = append(allErrs, spec.KubeadmConfigSpec.Validate(field.NewPath("spec", "kubeadmConfigSpec"))...)
if len(allErrs) > 0 {
return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlane").GroupKind(), in.Name, allErrs)
}
Expand Down Expand Up @@ -208,7 +208,7 @@ func (in *KubeadmControlPlane) ValidateUpdate(old runtime.Object) error {
allErrs = append(allErrs, in.validateVersion(prev.Spec.Version)...)
allErrs = append(allErrs, validateClusterConfiguration(in.Spec.KubeadmConfigSpec.ClusterConfiguration, prev.Spec.KubeadmConfigSpec.ClusterConfiguration, field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration"))...)
allErrs = append(allErrs, in.validateCoreDNSVersion(prev)...)
allErrs = append(allErrs, in.Spec.KubeadmConfigSpec.Validate()...)
allErrs = append(allErrs, in.Spec.KubeadmConfigSpec.Validate(field.NewPath("spec", "kubeadmConfigSpec"))...)

if len(allErrs) > 0 {
return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlane").GroupKind(), in.Name, allErrs)
Expand Down Expand Up @@ -371,6 +371,18 @@ func validateClusterConfiguration(newClusterConfiguration, oldClusterConfigurati
)
}

if newClusterConfiguration.DNS.ImageTag != "" {
if _, err := version.ParseMajorMinorPatchTolerant(newClusterConfiguration.DNS.ImageTag); err != nil {
allErrs = append(allErrs,
field.Invalid(
field.NewPath("dns", "imageTag"),
newClusterConfiguration.DNS.ImageTag,
fmt.Sprintf("failed to parse CoreDNS version: %v", err),
),
)
}
}

// TODO: Remove when kubeadm types include OpenAPI validation
if newClusterConfiguration.Etcd.Local != nil && !container.ImageTagIsValid(newClusterConfiguration.Etcd.Local.ImageTag) {
allErrs = append(
Expand Down Expand Up @@ -481,9 +493,10 @@ func (in *KubeadmControlPlane) validateCoreDNSVersion(prev *KubeadmControlPlane)
fromVersion, err := version.ParseMajorMinorPatchTolerant(prev.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag)
if err != nil {
allErrs = append(allErrs,
field.InternalError(
field.Invalid(
field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "dns", "imageTag"),
fmt.Errorf("failed to parse CoreDNS current version: %v", prev.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag),
prev.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag,
fmt.Sprintf("failed to parse current CoreDNS version: %v", err),
),
)
return allErrs
Expand All @@ -495,7 +508,7 @@ func (in *KubeadmControlPlane) validateCoreDNSVersion(prev *KubeadmControlPlane)
field.Invalid(
field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "dns", "imageTag"),
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
targetDNS.ImageTag,
fmt.Sprintf("failed to parse CoreDNS target version: %v", targetDNS.ImageTag),
fmt.Sprintf("failed to parse target CoreDNS version: %v", err),
),
)
return allErrs
Expand Down
11 changes: 11 additions & 0 deletions controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ func TestKubeadmControlPlaneValidateCreate(t *testing.T) {
Name: "infraTemplate",
},
},
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{},
},
Replicas: pointer.Int32Ptr(1),
Version: "v1.19.0",
RolloutStrategy: &RolloutStrategy{
Expand Down Expand Up @@ -129,6 +132,9 @@ func TestKubeadmControlPlaneValidateCreate(t *testing.T) {
invalidVersion2 := valid.DeepCopy()
invalidVersion2.Spec.Version = "1.16.6"

invalidCoreDNSVersion := valid.DeepCopy()
invalidCoreDNSVersion.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag = "v1.7" // not a valid semantic version

invalidIgnitionConfiguration := valid.DeepCopy()
invalidIgnitionConfiguration.Spec.KubeadmConfigSpec.Ignition = &bootstrapv1.IgnitionSpec{}

Expand Down Expand Up @@ -187,6 +193,11 @@ func TestKubeadmControlPlaneValidateCreate(t *testing.T) {
expectErr: true,
kcp: invalidVersion1,
},
{
name: "should return error when given an invalid semantic CoreDNS version",
expectErr: true,
kcp: invalidCoreDNSVersion,
},
{
name: "should return error when maxSurge is not 1",
expectErr: true,
Expand Down
1 change: 1 addition & 0 deletions controlplane/kubeadm/api/v1beta1/kubeadmcontrolplanetemplate_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ func (r *KubeadmControlPlaneTemplate) ValidateCreate() error {
spec := r.Spec.Template.Spec
allErrs := validateKubeadmControlPlaneTemplateResourceSpec(spec, field.NewPath("spec", "template", "spec"))
allErrs = append(allErrs, validateClusterConfiguration(spec.KubeadmConfigSpec.ClusterConfiguration, nil, field.NewPath("spec", "template", "spec", "kubeadmConfigSpec", "clusterConfiguration"))...)
allErrs = append(allErrs, spec.KubeadmConfigSpec.Validate(field.NewPath("spec", "template", "spec", "kubeadmConfigSpec"))...)
if len(allErrs) > 0 {
return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlaneTemplate").GroupKind(), r.Name, allErrs)
}
Expand Down