Skip to content

Commit

Permalink
Merge pull request #6129 from sbueringer/pr-improve-kcp-validation
Browse files Browse the repository at this point in the history 
🌱 KCP: improve validation webhooks
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Mar 10, 2022
2 parents 338ed6b + 7a02662 commit ffc9943
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 28 deletions.
42 changes: 20 additions & 22 deletions bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (c *KubeadmConfig) ValidateDelete() error {
}

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

if len(allErrs) == 0 {
return nil
Expand All @@ -89,16 +89,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 @@ -109,7 +109,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 @@ -122,19 +122,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"),
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 @@ -145,7 +143,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 @@ -157,18 +155,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 @@ -179,7 +177,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 @@ -194,7 +192,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 @@ -205,7 +203,7 @@ func (c *KubeadmConfigSpec) validateIgnition() field.ErrorList {
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "useExperimentalRetryJoin"),
pathPrefix.Child("useExperimentalRetryJoin"),
cannotUseWithIgnition,
),
)
Expand All @@ -220,7 +218,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 @@ -237,7 +235,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 @@ -247,7 +245,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
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,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
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,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 @@ -211,7 +211,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 @@ -374,6 +374,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 @@ -484,9 +496,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 @@ -498,7 +511,7 @@ func (in *KubeadmControlPlane) validateCoreDNSVersion(prev *KubeadmControlPlane)
field.Invalid(
field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "dns", "imageTag"),
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
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,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 @@ -130,6 +133,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 @@ -188,6 +194,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
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,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

0 comments on commit ffc9943

Please sign in to comment.