From ba325bdf7e1165f312456dfc3e52b9b9cce338e4 Mon Sep 17 00:00:00 2001 From: Simon Croome Date: Thu, 25 Feb 2021 21:36:23 +0000 Subject: [PATCH] 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) - } - } + }) } }