From 294f2f52cd1806ada3b19bb5f2c49dc52b3fbad9 Mon Sep 17 00:00:00 2001 From: leovct Date: Wed, 5 Jun 2024 10:07:33 +0200 Subject: [PATCH 1/6] feat: port name validation RFC-6335 --- .../objects/service/service.go | 38 +++++++++++++++---- .../objects/service/service_test.go | 35 +++++++++++++++-- .../add_service/add_service_shared.go | 15 +++++++- 3 files changed, 77 insertions(+), 11 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..99911b7507 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,51 @@ 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 ( // 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 + serviceNameMaxLength = 63 + + // 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 + portNameMaxLength = 15 ) var ( - compiledWordWrappedServiceNameRegex = regexp.MustCompile(WordWrappedServiceNameRegex) + ServiceNameRegex = regexp.MustCompile(generateRegex(serviceNameMaxLength)) + PortNameRegex = regexp.MustCompile(generateRegex(portNameMaxLength)) ) +// generateRegex creates a regex string based on the provided constraints (RFC-1035 and RFC-6335). +func generateRegex(maxLen int) string { + return "^[a-z]([-a-z0-9]{0," + strconv.Itoa(maxLen-2) + "}[a-z0-9])?$" +} + type ServiceName string type ServiceUUID string @@ -79,5 +99,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..d857311047 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,18 +1,29 @@ 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"))) + require.True(t, IsServiceNameValid(ServiceName("abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef123"))) // 63 characters +} + +func TestValidPortName(t *testing.T) { + require.True(t, IsPortNameValid("a")) + require.True(t, IsPortNameValid("abc")) + require.True(t, IsPortNameValid("abcdefabcdef123")) // 15 characters } func TestInvalidServiceName(t *testing.T) { - require.False(t, IsServiceNameValid(ServiceName("1-geth-lighthouse"))) + require.False(t, IsServiceNameValid(ServiceName("1-geth-lighthouse"))) // 17 characters +} + +func TestInvalidPortName(t *testing.T) { + require.False(t, IsPortNameValid("1-dummy-port")) // 12 characters } func TestServiceNameWithSpecialChars(t *testing.T) { @@ -25,6 +36,16 @@ func TestServiceNameWithSpecialChars(t *testing.T) { require.False(t, IsServiceNameValid(ServiceName("a/b"))) } +func TestPortNameWithSpecialChars(t *testing.T) { + require.True(t, IsPortNameValid("a-b")) + require.False(t, IsPortNameValid("-bc")) + require.False(t, IsPortNameValid("a--")) + require.False(t, IsPortNameValid("a_b")) + require.False(t, IsPortNameValid("a%b")) + require.False(t, IsPortNameValid("a:b")) + require.False(t, IsPortNameValid("a/b")) +} + func TestServiceNameLength(t *testing.T) { require.False(t, IsServiceNameValid(ServiceName(""))) require.True(t, IsServiceNameValid(ServiceName("a"))) @@ -32,3 +53,11 @@ func TestServiceNameLength(t *testing.T) { require.True(t, IsServiceNameValid(ServiceName("abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef123"))) require.False(t, IsServiceNameValid(ServiceName("abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef1234"))) } + +func TestPortNameLength(t *testing.T) { + require.False(t, IsPortNameValid("")) + require.True(t, IsPortNameValid("a")) + require.True(t, IsPortNameValid("abc")) + require.True(t, IsPortNameValid("abcdefabcdef123")) // 15 characters + require.False(t, IsPortNameValid("abcdefabcdef1234")) // 16 characters +} 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, ) } From 6c93cb294c8cb18b3d0e699334c2374149059d92 Mon Sep 17 00:00:00 2001 From: leovct Date: Wed, 5 Jun 2024 10:14:45 +0200 Subject: [PATCH 2/6] chore: refactor tests --- .../objects/service/service_test.go | 94 ++++++++++--------- 1 file changed, 48 insertions(+), 46 deletions(-) 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 d857311047..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 @@ -6,58 +6,60 @@ import ( "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"))) // 63 characters -} +func TestServiceNameValidation(t *testing.T) { + validServiceNames := []string{ + "a", + "abc", + "abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef123", // 63 characters + "a-b", + } -func TestValidPortName(t *testing.T) { - require.True(t, IsPortNameValid("a")) - require.True(t, IsPortNameValid("abc")) - require.True(t, IsPortNameValid("abcdefabcdef123")) // 15 characters -} + invalidServiceNames := []string{ + "1-geth-lighthouse", // 17 characters + "-bc", + "a--", + "a_b", + "a%b", + "a:b", + "a/b", + "", + "abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef1234", // 64 characters + } -func TestInvalidServiceName(t *testing.T) { - require.False(t, IsServiceNameValid(ServiceName("1-geth-lighthouse"))) // 17 characters -} + for _, name := range validServiceNames { + require.True(t, IsServiceNameValid(ServiceName(name)), "expected valid service name: %s", name) + } -func TestInvalidPortName(t *testing.T) { - require.False(t, IsPortNameValid("1-dummy-port")) // 12 characters + for _, name := range invalidServiceNames { + require.False(t, IsServiceNameValid(ServiceName(name)), "expected invalid service name: %s", name) + } } -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"))) -} +func TestPortNameValidation(t *testing.T) { + validPortNames := []string{ + "a", + "abc", + "abcdefabcdef123", // 15 characters + "a-b", + } -func TestPortNameWithSpecialChars(t *testing.T) { - require.True(t, IsPortNameValid("a-b")) - require.False(t, IsPortNameValid("-bc")) - require.False(t, IsPortNameValid("a--")) - require.False(t, IsPortNameValid("a_b")) - require.False(t, IsPortNameValid("a%b")) - require.False(t, IsPortNameValid("a:b")) - require.False(t, IsPortNameValid("a/b")) -} + invalidPortNames := []string{ + "1-dummy-port", // 12 characters + "-bc", + "a--", + "a_b", + "a%b", + "a:b", + "a/b", + "", + "abcdefabcdef1234", // 16 characters + } -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"))) -} + for _, name := range validPortNames { + require.True(t, IsPortNameValid(name), "expected valid port name: %s", name) + } -func TestPortNameLength(t *testing.T) { - require.False(t, IsPortNameValid("")) - require.True(t, IsPortNameValid("a")) - require.True(t, IsPortNameValid("abc")) - require.True(t, IsPortNameValid("abcdefabcdef123")) // 15 characters - require.False(t, IsPortNameValid("abcdefabcdef1234")) // 16 characters + for _, name := range invalidPortNames { + require.False(t, IsPortNameValid(name), "expected invalid port name: %s", name) + } } From 871bededf21dcfc267bb8722e1daa989501700f1 Mon Sep 17 00:00:00 2001 From: leovct Date: Wed, 5 Jun 2024 10:16:03 +0200 Subject: [PATCH 3/6] fix: typo --- .../lib/backend_interface/objects/service/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 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 99911b7507..d094c3d782 100644 --- a/container-engine-lib/lib/backend_interface/objects/service/service.go +++ b/container-engine-lib/lib/backend_interface/objects/service/service.go @@ -99,9 +99,9 @@ func (service *Service) GetMaybePublicPorts() map[string]*port_spec.PortSpec { } func IsServiceNameValid(serviceName ServiceName) bool { - return serviceNameRegex.MatchString(string(serviceName)) + return ServiceNameRegex.MatchString(string(serviceName)) } func IsPortNameValid(portName string) bool { - return portNameRegex.MatchString(portName) + return PortNameRegex.MatchString(portName) } From b5f273c51641bfe47172627294e2881ea8f35fba Mon Sep 17 00:00:00 2001 From: leovct Date: Wed, 5 Jun 2024 10:22:37 +0200 Subject: [PATCH 4/6] chore: silence `gomnd` magic number issue --- .../lib/backend_interface/objects/service/service.go | 1 + 1 file changed, 1 insertion(+) 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 d094c3d782..21404b3699 100644 --- a/container-engine-lib/lib/backend_interface/objects/service/service.go +++ b/container-engine-lib/lib/backend_interface/objects/service/service.go @@ -43,6 +43,7 @@ var ( // 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])?$" } From 8f0070af263fd06bd0813149664307c2ab470237 Mon Sep 17 00:00:00 2001 From: leovct Date: Wed, 5 Jun 2024 11:03:46 +0200 Subject: [PATCH 5/6] chore: nit --- .../lib/backend_interface/objects/service/service.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 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 21404b3699..3af317317b 100644 --- a/container-engine-lib/lib/backend_interface/objects/service/service.go +++ b/container-engine-lib/lib/backend_interface/objects/service/service.go @@ -9,7 +9,7 @@ import ( "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 @@ -21,7 +21,7 @@ const ( // 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/ - serviceNameMaxLength = 63 + ServiceNameRegex = regexp.MustCompile(generateRegex(63)) // PortNameRegex implements RFC-6335 for naming ports, namely: // * contain at least 1 character @@ -33,12 +33,7 @@ const ( // 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 - portNameMaxLength = 15 -) - -var ( - ServiceNameRegex = regexp.MustCompile(generateRegex(serviceNameMaxLength)) - PortNameRegex = regexp.MustCompile(generateRegex(portNameMaxLength)) + PortNameRegex = regexp.MustCompile(generateRegex(15)) ) // generateRegex creates a regex string based on the provided constraints (RFC-1035 and RFC-6335). From b78ce9c95ded9cf8cfa68e7e1a099e3ebd17be77 Mon Sep 17 00:00:00 2001 From: leovct Date: Wed, 5 Jun 2024 11:05:46 +0200 Subject: [PATCH 6/6] chore: magic number issues --- .../lib/backend_interface/objects/service/service.go | 2 ++ 1 file changed, 2 insertions(+) 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 3af317317b..6fc54009ec 100644 --- a/container-engine-lib/lib/backend_interface/objects/service/service.go +++ b/container-engine-lib/lib/backend_interface/objects/service/service.go @@ -21,6 +21,7 @@ var ( // 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/ + // nolint: gomnd ServiceNameRegex = regexp.MustCompile(generateRegex(63)) // PortNameRegex implements RFC-6335 for naming ports, namely: @@ -33,6 +34,7 @@ var ( // 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)) )