From ffbd30bbb55bf7d4c5cf3d955d221c1c48f67ce4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Vincent?= <28714795+leovct@users.noreply.github.com> Date: Thu, 6 Jun 2024 16:38:41 +0200 Subject: [PATCH] feat: port name validation RFC-6335 (#2474) ## Description Kubernetes adheres to [RFC-6335](https://www.rfc-archive.org/getrfc.php?rfc=6335#gsc.tab=0), which specifies that port names must be between 1 and 15 characters. However, this constraint is currently not enforced by the Kurtosis engine, leading to deployment failures in Kubernetes environments when port names exceed 15 characters. Here is an example that the Kurtosis validation step failed to catch: ![image (1)](https://github.com/kurtosis-tech/kurtosis/assets/28714795/cb2f8ee4-038f-4368-9901-e5432fc58e45) This PR resolves the issue by adding an additional validation step for port names. It also refactors the regex definitions in `service.go` and improves the test design for validating service and port names in `service_test.go`. ## Is this change user facing? YES ## References (if applicable) - https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L326-L351 - https://github.com/0xPolygon/kurtosis-cdk/pull/146#issue-2332946061 --- .../objects/service/service.go | 42 +++++++--- .../objects/service/service_test.go | 77 +++++++++++++------ .../add_service/add_service_shared.go | 15 +++- 3 files changed, 100 insertions(+), 34 deletions(-) diff --git a/container-engine-lib/lib/backend_interface/objects/service/service.go b/container-engine-lib/lib/backend_interface/objects/service/service.go index 969ae4a20f..6fc54009ec 100644 --- a/container-engine-lib/lib/backend_interface/objects/service/service.go +++ b/container-engine-lib/lib/backend_interface/objects/service/service.go @@ -1,31 +1,49 @@ package service import ( - "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_interface/objects/container" - "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_interface/objects/port_spec" "net" "regexp" + "strconv" + + "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_interface/objects/container" + "github.com/kurtosis-tech/kurtosis/container-engine-lib/lib/backend_interface/objects/port_spec" ) -const ( +var ( // ServiceNameRegex implements RFC-1035 for naming services, namely: + // * contain at least 1 character // * contain at most 63 characters // * contain only lowercase alphanumeric characters or '-' + // * contain at least one letter ('A'-'Z' or 'a'-'z') // * start with an alphabetic character // * end with an alphanumeric character // The adoption of RFC-1035 is to maintain compatability with current Kubernetes service and pod naming standards: // We use this over RFC-1035 as Service Names require 1035 to be followed // https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names // https://kubernetes.io/docs/concepts/services-networking/service/ - ServiceNameRegex = "[a-z]([-a-z0-9]{0,61}[a-z0-9])?" - WordWrappedServiceNameRegex = "^" + ServiceNameRegex + "$" - serviceNameMaxLength = 63 -) + // nolint: gomnd + ServiceNameRegex = regexp.MustCompile(generateRegex(63)) -var ( - compiledWordWrappedServiceNameRegex = regexp.MustCompile(WordWrappedServiceNameRegex) + // PortNameRegex implements RFC-6335 for naming ports, namely: + // * contain at least 1 character + // * contain at most 15 characters + // * contain only lowercase alphanumeric characters or '-' + // * contain at least one letter ('A'-'Z' or 'a'-'z') + // * start with an alphabetic character + // * end with an alphanumeric character + // The adpoption of RFC-6335 is to maintain compatability with current Kubernetes port naming standards: + // We use this over RFC-6335 as Port Names require 6335 to be followed + // https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L326-L351 + // nolint: gomnd + PortNameRegex = regexp.MustCompile(generateRegex(15)) ) +// generateRegex creates a regex string based on the provided constraints (RFC-1035 and RFC-6335). +func generateRegex(maxLen int) string { + // nolint: gomnd + return "^[a-z]([-a-z0-9]{0," + strconv.Itoa(maxLen-2) + "}[a-z0-9])?$" +} + type ServiceName string type ServiceUUID string @@ -79,5 +97,9 @@ func (service *Service) GetMaybePublicPorts() map[string]*port_spec.PortSpec { } func IsServiceNameValid(serviceName ServiceName) bool { - return compiledWordWrappedServiceNameRegex.MatchString(string(serviceName)) + return ServiceNameRegex.MatchString(string(serviceName)) +} + +func IsPortNameValid(portName string) bool { + return PortNameRegex.MatchString(portName) } diff --git a/container-engine-lib/lib/backend_interface/objects/service/service_test.go b/container-engine-lib/lib/backend_interface/objects/service/service_test.go index ca9b6b97c6..f5b67864bb 100644 --- a/container-engine-lib/lib/backend_interface/objects/service/service_test.go +++ b/container-engine-lib/lib/backend_interface/objects/service/service_test.go @@ -1,34 +1,65 @@ package service import ( - "github.com/stretchr/testify/require" "testing" + + "github.com/stretchr/testify/require" ) -func TestValidServiceName(t *testing.T) { - require.True(t, IsServiceNameValid(ServiceName("a"))) - require.True(t, IsServiceNameValid(ServiceName("abc"))) - require.True(t, IsServiceNameValid(ServiceName("abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef123"))) -} +func TestServiceNameValidation(t *testing.T) { + validServiceNames := []string{ + "a", + "abc", + "abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef123", // 63 characters + "a-b", + } -func TestInvalidServiceName(t *testing.T) { - require.False(t, IsServiceNameValid(ServiceName("1-geth-lighthouse"))) -} + invalidServiceNames := []string{ + "1-geth-lighthouse", // 17 characters + "-bc", + "a--", + "a_b", + "a%b", + "a:b", + "a/b", + "", + "abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef1234", // 64 characters + } -func TestServiceNameWithSpecialChars(t *testing.T) { - require.True(t, IsServiceNameValid(ServiceName("a-b"))) - require.False(t, IsServiceNameValid(ServiceName("-bc"))) - require.False(t, IsServiceNameValid(ServiceName("a--"))) - require.False(t, IsServiceNameValid(ServiceName("a_b"))) - require.False(t, IsServiceNameValid(ServiceName("a%b"))) - require.False(t, IsServiceNameValid(ServiceName("a:b"))) - require.False(t, IsServiceNameValid(ServiceName("a/b"))) + for _, name := range validServiceNames { + require.True(t, IsServiceNameValid(ServiceName(name)), "expected valid service name: %s", name) + } + + for _, name := range invalidServiceNames { + require.False(t, IsServiceNameValid(ServiceName(name)), "expected invalid service name: %s", name) + } } -func TestServiceNameLength(t *testing.T) { - require.False(t, IsServiceNameValid(ServiceName(""))) - require.True(t, IsServiceNameValid(ServiceName("a"))) - require.True(t, IsServiceNameValid(ServiceName("abc"))) - require.True(t, IsServiceNameValid(ServiceName("abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef123"))) - require.False(t, IsServiceNameValid(ServiceName("abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef1234"))) +func TestPortNameValidation(t *testing.T) { + validPortNames := []string{ + "a", + "abc", + "abcdefabcdef123", // 15 characters + "a-b", + } + + invalidPortNames := []string{ + "1-dummy-port", // 12 characters + "-bc", + "a--", + "a_b", + "a%b", + "a:b", + "a/b", + "", + "abcdefabcdef1234", // 16 characters + } + + for _, name := range validPortNames { + require.True(t, IsPortNameValid(name), "expected valid port name: %s", name) + } + + for _, name := range invalidPortNames { + require.False(t, IsPortNameValid(name), "expected invalid port name: %s", name) + } } diff --git a/core/server/api_container/server/startosis_engine/kurtosis_instruction/add_service/add_service_shared.go b/core/server/api_container/server/startosis_engine/kurtosis_instruction/add_service/add_service_shared.go index 69ba481a3c..4728c8e18c 100644 --- a/core/server/api_container/server/startosis_engine/kurtosis_instruction/add_service/add_service_shared.go +++ b/core/server/api_container/server/startosis_engine/kurtosis_instruction/add_service/add_service_shared.go @@ -117,6 +117,9 @@ func validateSingleService(validatorEnvironment *startosis_validator.ValidatorEn var portIds []string for portId := range serviceConfig.GetPrivatePorts() { + if isValidPortName := service.IsPortNameValid(portId); !isValidPortName { + return startosis_errors.NewValidationError(invalidPortNameErrorText(portId)) + } portIds = append(portIds, portId) } validatorEnvironment.AddPrivatePortIDForService(portIds, serviceName) @@ -131,7 +134,17 @@ func invalidServiceNameErrorText( return fmt.Sprintf( "Service name '%v' is invalid as it contains disallowed characters. Service names must adhere to the RFC 1035 standard, specifically implementing this regex and be 1-63 characters long: %s. This means the service name must only contain lowercase alphanumeric characters or '-', and must start with a lowercase alphabet and end with a lowercase alphanumeric character.", serviceName, - service.WordWrappedServiceNameRegex, + service.ServiceNameRegex, + ) +} + +func invalidPortNameErrorText( + portName string, +) string { + return fmt.Sprintf( + "Port name '%v' is invalid as it contains disallowed characters. Service names must adhere to the RFC 6335 standard, specifically implementing this regex and be 1-15 characters long: %s. This means the service name must only contain lowercase alphanumeric characters or '-', and must start with a lowercase alphabet and end with a lowercase alphanumeric character.", + portName, + service.PortNameRegex, ) }