From ba325bdf7e1165f312456dfc3e52b9b9cce338e4 Mon Sep 17 00:00:00 2001 From: Simon Croome Date: Thu, 25 Feb 2021 21:36:23 +0000 Subject: [PATCH 1/4] Add name validation rules for object kinds Signed-off-by: Simon Croome Developer Certificate of Origin Version 1.1 Copyright (C) 2004, 2006 The Linux Foundation and its contributors. 1 Letterman Drive Suite D4700 San Francisco, CA, 94129 Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed. Developer's Certificate of Origin 1.1 By making a contribution to this project, I certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. Signed-off-by: Simon Croome --- pkg/lint/rules/template.go | 67 +++++++++++++++++-- pkg/lint/rules/template_test.go | 110 ++++++++++++++++++++++---------- 2 files changed, 138 insertions(+), 39 deletions(-) diff --git a/pkg/lint/rules/template.go b/pkg/lint/rules/template.go index 10121c108..2addad122 100644 --- a/pkg/lint/rules/template.go +++ b/pkg/lint/rules/template.go @@ -28,6 +28,9 @@ import ( "strings" "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/api/validation" + apipath "k8s.io/apimachinery/pkg/api/validation/path" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/yaml" "helm.sh/helm/v3/pkg/chart/loader" @@ -202,18 +205,70 @@ func validateYamlContent(err error) error { return errors.Wrap(err, "unable to parse YAML") } +// validateMetadataName uses the correct validation function for the object +// Kind, or if not set, defaults to the standard definition of a subdomain in +// DNS (RFC 1123), used by most resources. func validateMetadataName(obj *K8sYamlStruct) error { - if len(obj.Metadata.Name) == 0 || len(obj.Metadata.Name) > 253 { - return fmt.Errorf("object name must be between 0 and 253 characters: %q", obj.Metadata.Name) + fn := validateMetadataNameFunc(obj) + if fn == nil { + fn = validation.NameIsDNSSubdomain } - // This will return an error if the characters do not abide by the standard OR if the - // name is left empty. - if err := chartutil.ValidateMetadataName(obj.Metadata.Name); err != nil { - return errors.Wrapf(err, "object name does not conform to Kubernetes naming requirements: %q", obj.Metadata.Name) + + allErrs := field.ErrorList{} + for _, msg := range fn(obj.Metadata.Name, false) { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), obj.Metadata.Name, msg)) + } + if len(allErrs) > 0 { + return fmt.Errorf("object name does not conform to Kubernetes naming requirements: %q (%s)", obj.Metadata.Name, allErrs.ToAggregate().Error()) } return nil } +// validateMetadataNameFunc will return a name validation function for the +// object kind, if defined below. +// +// Rules should match those set in the various api validations: +// https://github.com/kubernetes/kubernetes/blob/v1.20.0/pkg/apis/core/validation/validation.go#L205-L274 +// https://github.com/kubernetes/kubernetes/blob/v1.20.0/pkg/apis/apps/validation/validation.go#L39 +// ... +// +// Implementing here to avoid importing k/k. +// +// If no mapping is defined, returns nil. +func validateMetadataNameFunc(obj *K8sYamlStruct) validation.ValidateNameFunc { + switch strings.ToLower(obj.Kind) { + case "pod", "node", "secret", "endpoints", "resourcequota", // core + "controllerrevision", "daemonset", "deployment", "replicaset", "statefulset", // apps + "autoscaler", // autoscaler + "cronjob", "job", // batch + "lease", // coordination + "endpointslice", // discovery + "networkpolicy", "ingress", // networking + "podsecuritypolicy", // policy + "priorityclass", // scheduling + "podpreset", // settings + "storageclass", "volumeattachment", "csinode": // storage + return validation.NameIsDNSSubdomain + case "service": + return validation.NameIsDNS1035Label + case "namespace": + return validation.ValidateNamespaceName + case "serviceaccount": + return validation.ValidateServiceAccountName + case "certificatesigningrequest": + // No validation. + // https://github.com/kubernetes/kubernetes/blob/v1.20.0/pkg/apis/certificates/validation/validation.go#L137-L140 + return func(name string, prefix bool) []string { return nil } + case "role", "clusterrole", "rolebinding", "clusterrolebinding": + // https://github.com/kubernetes/kubernetes/blob/v1.20.0/pkg/apis/rbac/validation/validation.go#L32-L34 + return func(name string, prefix bool) []string { + return apipath.IsValidPathSegmentName(name) + } + default: + return nil + } +} + func validateNoCRDHooks(manifest []byte) error { if crdHookSearch.Match(manifest) { return errors.New("manifest is a crd-install hook. This hook is no longer supported in v3 and all CRDs should also exist the crds/ directory at the top level of the chart") diff --git a/pkg/lint/rules/template_test.go b/pkg/lint/rules/template_test.go index eb076a1bf..895f967a2 100644 --- a/pkg/lint/rules/template_test.go +++ b/pkg/lint/rules/template_test.go @@ -17,13 +17,12 @@ limitations under the License. package rules import ( + "fmt" "os" "path/filepath" "strings" "testing" - "github.com/Masterminds/goutils" - "helm.sh/helm/v3/internal/test/ensure" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" @@ -122,39 +121,84 @@ func TestMultiTemplateFail(t *testing.T) { } func TestValidateMetadataName(t *testing.T) { - names := map[string]bool{ - "": false, - "foo": true, - "foo.bar1234baz.seventyone": true, - "FOO": false, - "123baz": true, - "foo.BAR.baz": false, - "one-two": true, - "-two": false, - "one_two": false, - "a..b": false, - "%^&#$%*@^*@&#^": false, - } - - // The length checker should catch this first. So this is not true fuzzing. - tooLong, err := goutils.RandomAlphaNumeric(300) - if err != nil { - t.Fatalf("Randomizer failed to initialize: %s", err) + tests := []struct { + obj *K8sYamlStruct + wantErr bool + }{ + // Most kinds use IsDNS1123Subdomain. + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: ""}}, true}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "foo"}}, false}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "foo.bar1234baz.seventyone"}}, false}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "FOO"}}, true}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "123baz"}}, false}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "foo.BAR.baz"}}, true}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "one-two"}}, false}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "-two"}}, true}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "one_two"}}, true}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "a..b"}}, true}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "%^&#$%*@^*@&#^"}}, true}, + {&K8sYamlStruct{Kind: "Pod", Metadata: k8sYamlMetadata{Name: "operator:pod"}}, true}, + {&K8sYamlStruct{Kind: "ServiceAccount", Metadata: k8sYamlMetadata{Name: "foo"}}, false}, + {&K8sYamlStruct{Kind: "ServiceAccount", Metadata: k8sYamlMetadata{Name: "foo.bar1234baz.seventyone"}}, false}, + {&K8sYamlStruct{Kind: "ServiceAccount", Metadata: k8sYamlMetadata{Name: "FOO"}}, true}, + {&K8sYamlStruct{Kind: "ServiceAccount", Metadata: k8sYamlMetadata{Name: "operator:sa"}}, true}, + + // Service uses IsDNS1035Label. + {&K8sYamlStruct{Kind: "Service", Metadata: k8sYamlMetadata{Name: "foo"}}, false}, + {&K8sYamlStruct{Kind: "Service", Metadata: k8sYamlMetadata{Name: "123baz"}}, true}, + {&K8sYamlStruct{Kind: "Service", Metadata: k8sYamlMetadata{Name: "foo.bar"}}, true}, + + // Namespace uses IsDNS1123Label. + {&K8sYamlStruct{Kind: "Namespace", Metadata: k8sYamlMetadata{Name: "foo"}}, false}, + {&K8sYamlStruct{Kind: "Namespace", Metadata: k8sYamlMetadata{Name: "123baz"}}, false}, + {&K8sYamlStruct{Kind: "Namespace", Metadata: k8sYamlMetadata{Name: "foo.bar"}}, true}, + {&K8sYamlStruct{Kind: "Namespace", Metadata: k8sYamlMetadata{Name: "foo-bar"}}, false}, + + // CertificateSigningRequest has no validation. + {&K8sYamlStruct{Kind: "CertificateSigningRequest", Metadata: k8sYamlMetadata{Name: ""}}, false}, + {&K8sYamlStruct{Kind: "CertificateSigningRequest", Metadata: k8sYamlMetadata{Name: "123baz"}}, false}, + {&K8sYamlStruct{Kind: "CertificateSigningRequest", Metadata: k8sYamlMetadata{Name: "%^&#$%*@^*@&#^"}}, false}, + + // RBAC uses path validation. + {&K8sYamlStruct{Kind: "Role", Metadata: k8sYamlMetadata{Name: "foo"}}, false}, + {&K8sYamlStruct{Kind: "Role", Metadata: k8sYamlMetadata{Name: "123baz"}}, false}, + {&K8sYamlStruct{Kind: "Role", Metadata: k8sYamlMetadata{Name: "foo.bar"}}, false}, + {&K8sYamlStruct{Kind: "Role", Metadata: k8sYamlMetadata{Name: "operator:role"}}, false}, + {&K8sYamlStruct{Kind: "Role", Metadata: k8sYamlMetadata{Name: "operator/role"}}, true}, + {&K8sYamlStruct{Kind: "Role", Metadata: k8sYamlMetadata{Name: "operator%role"}}, true}, + {&K8sYamlStruct{Kind: "ClusterRole", Metadata: k8sYamlMetadata{Name: "foo"}}, false}, + {&K8sYamlStruct{Kind: "ClusterRole", Metadata: k8sYamlMetadata{Name: "123baz"}}, false}, + {&K8sYamlStruct{Kind: "ClusterRole", Metadata: k8sYamlMetadata{Name: "foo.bar"}}, false}, + {&K8sYamlStruct{Kind: "ClusterRole", Metadata: k8sYamlMetadata{Name: "operator:role"}}, false}, + {&K8sYamlStruct{Kind: "ClusterRole", Metadata: k8sYamlMetadata{Name: "operator/role"}}, true}, + {&K8sYamlStruct{Kind: "ClusterRole", Metadata: k8sYamlMetadata{Name: "operator%role"}}, true}, + {&K8sYamlStruct{Kind: "RoleBinding", Metadata: k8sYamlMetadata{Name: "operator:role"}}, false}, + {&K8sYamlStruct{Kind: "ClusterRoleBinding", Metadata: k8sYamlMetadata{Name: "operator:role"}}, false}, + + // Unknown Kind + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: ""}}, true}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "foo"}}, false}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "foo.bar1234baz.seventyone"}}, false}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "FOO"}}, true}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "123baz"}}, false}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "foo.BAR.baz"}}, true}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "one-two"}}, false}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "-two"}}, true}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "one_two"}}, true}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "a..b"}}, true}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "%^&#$%*@^*@&#^"}}, true}, + {&K8sYamlStruct{Kind: "FutureKind", Metadata: k8sYamlMetadata{Name: "operator:pod"}}, true}, + + // No kind + {&K8sYamlStruct{Metadata: k8sYamlMetadata{Name: "foo"}}, false}, + {&K8sYamlStruct{Metadata: k8sYamlMetadata{Name: "operator:pod"}}, true}, } - names[tooLong] = false - - for input, expectPass := range names { - obj := K8sYamlStruct{Metadata: k8sYamlMetadata{Name: input}} - if err := validateMetadataName(&obj); (err == nil) != expectPass { - st := "fail" - if expectPass { - st = "succeed" + for _, tt := range tests { + t.Run(fmt.Sprintf("%s/%s", tt.obj.Kind, tt.obj.Metadata.Name), func(t *testing.T) { + if err := validateMetadataName(tt.obj); (err != nil) != tt.wantErr { + t.Errorf("validateMetadataName() error = %v, wantErr %v", err, tt.wantErr) } - t.Errorf("Expected %q to %s", input, st) - if err != nil { - t.Log(err) - } - } + }) } } From 54de1c1f2549fb84db7f021c08cad6acd80ce7da Mon Sep 17 00:00:00 2001 From: Simon Croome Date: Mon, 22 Mar 2021 15:55:23 +0000 Subject: [PATCH 2/4] Move default to avoid nil check Signed-off-by: Simon Croome --- pkg/lint/rules/template.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/lint/rules/template.go b/pkg/lint/rules/template.go index 2addad122..826a31bfa 100644 --- a/pkg/lint/rules/template.go +++ b/pkg/lint/rules/template.go @@ -210,10 +210,6 @@ func validateYamlContent(err error) error { // DNS (RFC 1123), used by most resources. func validateMetadataName(obj *K8sYamlStruct) error { fn := validateMetadataNameFunc(obj) - if fn == nil { - fn = validation.NameIsDNSSubdomain - } - allErrs := field.ErrorList{} for _, msg := range fn(obj.Metadata.Name, false) { allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), obj.Metadata.Name, msg)) @@ -234,7 +230,9 @@ func validateMetadataName(obj *K8sYamlStruct) error { // // Implementing here to avoid importing k/k. // -// If no mapping is defined, returns nil. +// If no mapping is defined, returns NameIsDNSSubdomain. This is used by object +// kinds that don't have special requirements, so is the most likely to work if +// new kinds are added. func validateMetadataNameFunc(obj *K8sYamlStruct) validation.ValidateNameFunc { switch strings.ToLower(obj.Kind) { case "pod", "node", "secret", "endpoints", "resourcequota", // core @@ -265,7 +263,7 @@ func validateMetadataNameFunc(obj *K8sYamlStruct) validation.ValidateNameFunc { return apipath.IsValidPathSegmentName(name) } default: - return nil + return validation.NameIsDNSSubdomain } } From 6c82c83b3ab7f7664048f70c79b302cc6f7c1475 Mon Sep 17 00:00:00 2001 From: Simon Croome Date: Mon, 22 Mar 2021 15:58:07 +0000 Subject: [PATCH 3/4] Wrap validation error instead of recreating Signed-off-by: Simon Croome --- pkg/lint/rules/template.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/lint/rules/template.go b/pkg/lint/rules/template.go index 826a31bfa..c1c2ed84f 100644 --- a/pkg/lint/rules/template.go +++ b/pkg/lint/rules/template.go @@ -215,7 +215,7 @@ func validateMetadataName(obj *K8sYamlStruct) error { allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), obj.Metadata.Name, msg)) } if len(allErrs) > 0 { - return fmt.Errorf("object name does not conform to Kubernetes naming requirements: %q (%s)", obj.Metadata.Name, allErrs.ToAggregate().Error()) + return errors.Wrapf(allErrs.ToAggregate(), "object name does not conform to Kubernetes naming requirements: %q", obj.Metadata.Name) } return nil } From c50372a8c1b948f3a140cb6c47e94ba1f0c8438a Mon Sep 17 00:00:00 2001 From: Simon Croome Date: Mon, 22 Mar 2021 16:42:57 +0000 Subject: [PATCH 4/4] Add/update deprecation notices Signed-off-by: Simon Croome --- pkg/action/action.go | 2 +- pkg/chartutil/validate_name.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 79bb4f638..38ba638e4 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -65,7 +65,7 @@ var ( // ValidName is a regular expression for resource names. // // DEPRECATED: This will be removed in Helm 4, and is no longer used here. See -// pkg/chartutil.ValidateName for the replacement. +// pkg/lint/rules.validateMetadataNameFunc for the replacement. // // According to the Kubernetes help text, the regular expression it uses is: // diff --git a/pkg/chartutil/validate_name.go b/pkg/chartutil/validate_name.go index 913a477cf..284b8441b 100644 --- a/pkg/chartutil/validate_name.go +++ b/pkg/chartutil/validate_name.go @@ -96,6 +96,9 @@ func ValidateReleaseName(name string) error { // // The Kubernetes documentation is here, though it is not entirely correct: // https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names +// +// Deprecated: remove in Helm 4. Name validation now uses rules defined in +// pkg/lint/rules.validateMetadataNameFunc() func ValidateMetadataName(name string) error { if name == "" || len(name) > maxMetadataNameLen || !validName.MatchString(name) { return errInvalidKubernetesName