fix(kuma-cp): meta validation compatible with Kubernetes naming rules

UPGRADE.md

@@ -6,6 +6,20 @@ with `x.y.z` being the version you are planning to upgrade to.
 If such a section does not exist, the upgrade you want to perform
 does not have any particular instructions.
 
+## Upgrade to `2.5.x`
+
+#### More strict validation rules for resource names
+
+In order to be compatible with Kubernetes naming policy we updated the validation rules. Old rule:
+
+> Valid characters are numbers, lowercase latin letters and '-', '_' symbols.
+
+New rule:
+
+> A lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character
+
+New rule is applied for CREATE operations. The old rule is still applied for UPDATE, but this is going to change in Kuma 2.7.x or later.
+
 ## Upgrade to `2.4.x`
 
 ### Configuration change

app/kuma-dp/cmd/dataplane.go

@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"fmt"
 	"io"
 	"os"
 
@@ -46,8 +47,12 @@ func readResource(cmd *cobra.Command, r *kuma_dp.DataplaneRuntime) (model.Resour
 		return nil, err
 	}
 
-	if err := core_mesh.ValidateMeta(res.GetMeta().GetName(), res.GetMeta().GetMesh(), res.Descriptor().Scope); err.HasViolations() {
+	if err, msg := core_mesh.ValidateMetaBackwardsCompatible(res.GetMeta(), res.Descriptor().Scope); err.HasViolations() {
 		return nil, &err
+	} else if msg != "" {
+		if _, printErr := fmt.Fprintln(cmd.ErrOrStderr(), msg); printErr != nil {
+			return nil, printErr
+		}
 	}
 
 	return res, nil

app/kumactl/cmd/apply/apply.go

@@ -116,8 +116,12 @@ $ kumactl apply -f https://example.com/resource.yaml
 				if err != nil {
 					return errors.Wrap(err, "YAML contains invalid resource")
 				}
-				if err := mesh.ValidateMeta(res.GetMeta().GetName(), res.GetMeta().GetMesh(), res.Descriptor().Scope); err.HasViolations() {
+				if err, msg := mesh.ValidateMetaBackwardsCompatible(res.GetMeta(), res.Descriptor().Scope); err.HasViolations() {
 					return err.OrNil()
+				} else if msg != "" {
+					if _, printErr := fmt.Fprintln(cmd.ErrOrStderr(), msg); printErr != nil {
+						return printErr
+					}
 				}
 				resources = append(resources, res)
 			}

pkg/api-server/resource_endpoints.go

@@ -166,18 +166,21 @@ func (r *resourceEndpoints) createOrUpdateResource(request *restful.Request, res
 		return
 	}
 
-	if err := r.validateResourceRequest(request, resourceRest.GetMeta()); err != nil {
+	create := false
+	resource := r.descriptor.NewObject()
+	if err := r.resManager.Get(request.Request.Context(), resource, store.GetByKey(name, meshName)); err != nil && store.IsResourceNotFound(err) {
+		create = true
+	} else if err != nil {
+		rest_errors.HandleError(request.Request.Context(), response, err, "Could not find a resource")
+	}
+
+	if err := r.validateResourceRequest(request, resourceRest.GetMeta(), create); err != nil {
 		rest_errors.HandleError(request.Request.Context(), response, err, "Could not process a resource")
 		return
 	}
 
-	resource := r.descriptor.NewObject()
-	if err := r.resManager.Get(request.Request.Context(), resource, store.GetByKey(name, meshName)); err != nil {
-		if store.IsResourceNotFound(err) {
-			r.createResource(request.Request.Context(), name, meshName, resourceRest.GetSpec(), response)
-		} else {
-			rest_errors.HandleError(request.Request.Context(), response, err, "Could not find a resource")
-		}
+	if create {
+		r.createResource(request.Request.Context(), name, meshName, resourceRest.GetSpec(), response)
 	} else {
 		r.updateResource(request.Request.Context(), resource, resourceRest.GetSpec(), response)
 	}
@@ -272,7 +275,7 @@ func (r *resourceEndpoints) deleteResource(request *restful.Request, response *r
 	}
 }
 
-func (r *resourceEndpoints) validateResourceRequest(request *restful.Request, resourceMeta rest_v1alpha1.ResourceMeta) error {
+func (r *resourceEndpoints) validateResourceRequest(request *restful.Request, resourceMeta rest_v1alpha1.ResourceMeta, create bool) error {
 	var err validators.ValidationError
 	name := request.PathParameter("name")
 	meshName := r.meshFromRequest(request)
@@ -288,7 +291,15 @@ func (r *resourceEndpoints) validateResourceRequest(request *restful.Request, re
 	if r.descriptor.Scope == model.ScopeMesh && meshName != resourceMeta.Mesh {
 		err.AddViolation("mesh", "mesh from the URL has to be the same as in body")
 	}
-	err.AddError("", mesh.ValidateMeta(name, meshName, r.descriptor.Scope))
+	if create {
+		err.AddError("", mesh.ValidateMeta(resourceMeta, r.descriptor.Scope))
+	} else {
+		if verr, msg := mesh.ValidateMetaBackwardsCompatible(resourceMeta, r.descriptor.Scope); verr.HasViolations() {
+			err.AddError("", verr)
+		} else if msg != "" {
+			core.Log.Info(msg)
+		}
+	}
 	return err.OrNil()
 }
 

pkg/api-server/testdata/resource_name-mesh.golden.json

@@ -6,22 +6,22 @@
  "invalid_parameters": [
   {
    "field": "name",
-   "reason": "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_' symbols."
+   "reason": "invalid characters. A lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character"
   },
   {
    "field": "mesh",
-   "reason": "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_' symbols."
+   "reason": "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_', '.' symbols."
   }
  ],
  "details": "Resource is not valid",
  "causes": [
   {
    "field": "name",
-   "message": "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_' symbols."
+   "message": "invalid characters. A lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character"
   },
   {
    "field": "mesh",
-   "message": "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_' symbols."
+   "message": "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_', '.' symbols."
   }
  ]
 }

pkg/core/resources/apis/mesh/meta_validator.go

@@ -3,33 +3,70 @@ package mesh
 import (
 	"regexp"
 
-	"github.com/kumahq/kuma/pkg/core/resources/model"
+	core_model "github.com/kumahq/kuma/pkg/core/resources/model"
 	"github.com/kumahq/kuma/pkg/core/validators"
 )
 
-var nameMeshRegexp = regexp.MustCompile("^[0-9a-z-_]*$")
+var (
+	backwardCompatRegexp = regexp.MustCompile(`^[0-9a-z-_.]*$`)
+	backwardCompatErrMsg = "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_', '.' symbols."
+)
+
+var (
+	identifierRegexp = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`)
+	identifierErrMsg = "invalid characters. A lowercase RFC 1123 subdomain must consist of lower case alphanumeric " +
+		"characters, '-' or '.', and must start and end with an alphanumeric character"
+)
+
+func ValidateMeta(m core_model.ResourceMeta, scope core_model.ResourceScope) validators.ValidationError {
+	var err validators.ValidationError
+	err.AddError("name", validateIdentifier(m.GetName(), identifierRegexp, identifierErrMsg))
+	err.Add(ValidateMesh(m.GetMesh(), scope))
+	return err
+}
+
+// ValidateMetaBackwardsCompatible allows 'name' and 'mesh' to contain '_'. Should be called when updating existing
+// resources or on the clients (like 'kumactl' or 'kuma-dp') when it's not clear if it's create or update operation.
+//
+// We're going to remove this function in Kuma 2.7.x
+func ValidateMetaBackwardsCompatible(m core_model.ResourceMeta, scope core_model.ResourceScope) (validators.ValidationError, string) {
+	// trying to validate meta against the new (more strict) rules
+	if verr := ValidateMeta(m, scope); !verr.HasViolations() {
+		return validators.ValidationError{}, ""
+	}
 
-func ValidateMeta(name, mesh string, scope model.ResourceScope) validators.ValidationError {
 	var err validators.ValidationError
-	if name == "" {
-		err.AddViolation("name", "cannot be empty")
+	err.AddError("name", validateIdentifier(m.GetName(), backwardCompatRegexp, backwardCompatErrMsg))
+	err.Add(ValidateMesh(m.GetMesh(), scope))
+
+	if !err.HasViolations() {
+		// ResourceMeta doesn't pass new validation, but passes old validation. In that case we'd like to warn user about
+		// the new validation rules
+		return validators.ValidationError{}, "[WARNING] Kuma 2.5.x introduces more strict validation rules for resource name, it must consist of " +
+			"lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character. You can't " +
+			"CREATE resources with invalid names anymore, but it's possible to UPDATE such resources until Kuma 2.7.x is released."
 	}
-	if !nameMeshRegexp.MatchString(name) {
-		err.AddViolation("name", "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_' symbols.")
+
+	return err, ""
+}
+
+// ValidateMesh checks that resource's mesh matches the old regex (with '_'). Even if user creates entirely new resource,
+// we can't check resource's mesh against the new regex, because Mesh resource itself can be old and contain '_' in its name.
+// All new Mesh resources will have their name validated against new regex.
+func ValidateMesh(mesh string, scope core_model.ResourceScope) validators.ValidationError {
+	var err validators.ValidationError
+	if scope == core_model.ScopeMesh {
+		err.AddError("mesh", validateIdentifier(mesh, backwardCompatRegexp, backwardCompatErrMsg))
 	}
-	err.Add(ValidateMesh(mesh, scope))
 	return err
 }
 
-func ValidateMesh(mesh string, scope model.ResourceScope) validators.ValidationError {
+func validateIdentifier(identifier string, r *regexp.Regexp, errMsg string) validators.ValidationError {
 	var err validators.ValidationError
-	if scope == model.ScopeMesh {
-		if mesh == "" {
-			err.AddViolation("mesh", "cannot be empty")
-		}
-		if !nameMeshRegexp.MatchString(mesh) {
-			err.AddViolation("mesh", "invalid characters. Valid characters are numbers, lowercase latin letters and '-', '_' symbols.")
-		}
+	if identifier == "" {
+		err.AddViolation("", "cannot be empty")
+	} else if !r.MatchString(identifier) {
+		err.AddViolation("", errMsg)
 	}
 	return err
 }

pkg/core/resources/apis/mesh/meta_validator_test.go

@@ -0,0 +1,148 @@
+package mesh_test
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"sigs.k8s.io/yaml"
+
+	"github.com/kumahq/kuma/pkg/core/resources/apis/mesh"
+	core_model "github.com/kumahq/kuma/pkg/core/resources/model"
+	test_model "github.com/kumahq/kuma/pkg/test/resources/model"
+)
+
+var _ = Describe("Meta", func() {
+	type testCase struct {
+		meta           core_model.ResourceMeta
+		scope          core_model.ResourceScope
+		expected       string
+		printedWarning bool
+	}
+
+	Describe("ValidateMeta", func() {
+		DescribeTable("should pass validation",
+			func(given testCase) {
+				Expect(mesh.ValidateMeta(given.meta, given.scope).Violations).To(BeEmpty())
+			},
+			Entry("mesh-scoped valid name and mesh", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "mesh-1", Name: "name-1"},
+				scope: core_model.ScopeMesh,
+			}),
+			Entry("global-scoped valid name", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "", Name: "name-1"},
+				scope: core_model.ScopeGlobal,
+			}),
+			Entry("global-scoped valid name with dot", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "", Name: "default.name-1"},
+				scope: core_model.ScopeGlobal,
+			}),
+			Entry("mesh-scoped ", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "", Name: "default.name-1"},
+				scope: core_model.ScopeGlobal,
+			}),
+		)
+
+		DescribeTable("should validate fields",
+			func(given testCase) {
+				verr := mesh.ValidateMeta(given.meta, given.scope)
+				// and
+				actual, err := yaml.Marshal(verr)
+
+				// then
+				Expect(err).ToNot(HaveOccurred())
+				// and
+				Expect(actual).To(MatchYAML(given.expected))
+			},
+			Entry("empty name", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "mesh-1", Name: ""},
+				scope: core_model.ScopeMesh,
+				expected: `
+violations:
+ - field: name
+   message: cannot be empty`,
+			}),
+			Entry("empty mesh", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "", Name: "name-1"},
+				scope: core_model.ScopeMesh,
+				expected: `
+violations:
+ - field: mesh
+   message: cannot be empty`,
+			}),
+			Entry("name with 2 dot", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "mesh-1", Name: "two..dots"},
+				scope: core_model.ScopeMesh,
+				expected: `
+violations:
+ - field: name
+   message: invalid characters. A lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character`,
+			}),
+			Entry("name with underscore", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "mesh-1", Name: "under_score"},
+				scope: core_model.ScopeMesh,
+				expected: `
+violations:
+ - field: name
+   message: invalid characters. A lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character`,
+			}),
+		)
+	})
+
+	Describe("ValidateMetaBackwardsCompatible", func() {
+		DescribeTable("should pass validation",
+			func(given testCase) {
+				verr, msg := mesh.ValidateMetaBackwardsCompatible(given.meta, given.scope)
+				Expect(verr.Violations).To(BeEmpty())
+				if given.printedWarning {
+					Expect(msg).ToNot(BeEmpty())
+				}
+			},
+			Entry("mesh-scoped valid name and mesh", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "mesh-1", Name: "name-1"},
+				scope: core_model.ScopeMesh,
+			}),
+			Entry("global-scoped valid name", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "", Name: "name-1"},
+				scope: core_model.ScopeGlobal,
+			}),
+			Entry("global-scoped valid name with dot", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "", Name: "default.name-1"},
+				scope: core_model.ScopeGlobal,
+			}),
+			Entry("name with underscore", testCase{
+				meta:           &test_model.ResourceMeta{Mesh: "mesh-1", Name: "name-1_2"},
+				scope:          core_model.ScopeMesh,
+				printedWarning: true,
+			}),
+		)
+
+		DescribeTable("should validate fields",
+			func(given testCase) {
+				verr, warning := mesh.ValidateMetaBackwardsCompatible(given.meta, given.scope)
+				// and
+				actual, err := yaml.Marshal(verr)
+
+				// then
+				Expect(err).ToNot(HaveOccurred())
+				// and
+				Expect(actual).To(MatchYAML(given.expected))
+				Expect(warning).To(BeEmpty())
+			},
+			Entry("empty name", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "mesh-1", Name: ""},
+				scope: core_model.ScopeMesh,
+				expected: `
+violations:
+ - field: name
+   message: cannot be empty`,
+			}),
+			Entry("empty mesh", testCase{
+				meta:  &test_model.ResourceMeta{Mesh: "", Name: "name-1"},
+				scope: core_model.ScopeMesh,
+				expected: `
+violations:
+ - field: mesh
+   message: cannot be empty`,
+			}),
+		)
+	})
+})