Optimize common package functions for improved performance (#187)

* optimisation: Improve performance of UnMarshallObjectKey

This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys.

* optimisation: Improve performance of UnMarshallLimitNamespace

This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made:

- Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead.
- Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations.
- Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings.

* optimisation: improve performance of HostnamesToStrings (#167)

- Pre-allocated output slice using make function to reduce append overhead

* refactor: Replace errors.New(...) with fmt.Errorf(...)

* refactor: Simplify if-condition in UnMarshallObjectKey to handle empty namespace and name values

+ refactor the error message

* fix: Use IndexRune instead of IndexByte to handle non-ascii characters in UnMarshallLimitNamespace and UnMarshallObjectKey

Discussion: #187 (comment)

Kudos to Eguzki

* [test] Unit-tests for common/common.go (part 3 of 3) (#186)

* test: Add unit-tests for UnMarshallLimitNamespace (#167)

* test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167)

The two failing test cases include:
- when namespace has no gateway name then return an error
- when namespace has no domain name then return an error

In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases:
- when namespace only has gateway name (missing 'namespace/') and domain then return an error
- when namespace only has namespace name (missing '/gwName') and domain then return an error
the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message.

* refactor: Add comment to UnMarshallLimitNamespace (#167)

* test: Add unit-tests for MarshallNamespace (#167)

* test: Add unit-tests for UnMarshallObjectKey (#167)

* test: Add unit-tests for HostnamesToStrings (#167)

* [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182)

* test: Add unit-tests for Contains (#167)

* refactor: Add comment to Contains (#167)

* test: Add unit-tests for Map (#167)

* refactor: Add comment to Map (#167)

* refactor: Add names to Map test cases (#167)

* test: Add unit-tests for SliceCopy (#167)

* refactor: Add comment to SliceCopy (#167)

* test: Add unit-tests for ReverseSlice (#167)

* refactor: Add comment to ReverseSlice (#167)

* test: Add unit-tests for MergeMapStringString (#167)

* refactor: Add missing import 'reflect'

* fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185)

* changelog (#184)

* test: Add two test-cases for UnMarshallObjectKey (#167)

1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only
2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only

Changes were discussed here: #187 (comment)

* test: Delete test case for UnMarshallLimitNamespace

The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace.

* refactor: Add clarity to the test name

Describing the '/' separator as a default one we show that it is a default constant value.

---------

Co-authored-by: Guilherme Cassolato <[email protected]>
Co-authored-by: Eguzki Astiz Lezaun <[email protected]>

---------

Co-authored-by: Guilherme Cassolato <[email protected]>
Co-authored-by: Eguzki Astiz Lezaun <[email protected]>

pkg/common/common.go

@@ -17,7 +17,6 @@ limitations under the License.
 package common
 
 import (
-	"errors"
 	"fmt"
 	"os"
 	"reflect"
@@ -156,40 +155,45 @@ func MergeMapStringString(existing *map[string]string, desired map[string]string
 
 // UnMarshallLimitNamespace parses limit namespace with format "gwNS/gwName#domain"
 func UnMarshallLimitNamespace(ns string) (client.ObjectKey, string, error) {
-	split := strings.Split(ns, "#")
-	if len(split) != 2 {
-		return client.ObjectKey{}, "", errors.New("failed to split on #")
+	delimIndex := strings.IndexRune(ns, '#')
+	if delimIndex == -1 {
+		return client.ObjectKey{}, "", fmt.Errorf("failed to split on #")
 	}
 
-	domain := split[1]
+	gwSplit := ns[:delimIndex]
+	domain := ns[delimIndex+1:]
 
-	gwKey, err := UnMarshallObjectKey(split[0])
+	objKey, err := UnMarshallObjectKey(gwSplit)
 	if err != nil {
 		return client.ObjectKey{}, "", err
 	}
 
-	return gwKey, domain, nil
+	return objKey, domain, nil
 }
 
 // MarshallNamespace serializes limit namespace with format "gwNS/gwName#domain"
 func MarshallNamespace(gwKey client.ObjectKey, domain string) string {
 	return fmt.Sprintf("%s/%s#%s", gwKey.Namespace, gwKey.Name, domain)
 }
 
+// UnMarshallObjectKey takes a string input and converts it into an ObjectKey struct that
+// can be used to access a specific Kubernetes object. The input string is expected to be in the format "namespace/name".
+// If the input string does not contain a NamespaceSeparator (typically '/')
+// or has too few components, this function returns an error.
 func UnMarshallObjectKey(keyStr string) (client.ObjectKey, error) {
-	keySplit := strings.Split(keyStr, string(NamespaceSeparator))
-	if len(keySplit) < 2 {
-		return client.ObjectKey{}, fmt.Errorf("failed to split on %s: '%s'", string(NamespaceSeparator), keyStr)
+	namespaceEndIndex := strings.IndexRune(keyStr, NamespaceSeparator)
+	if namespaceEndIndex < 0 {
+		return client.ObjectKey{}, fmt.Errorf(fmt.Sprintf("failed to split on %s: '%s'", string(NamespaceSeparator), keyStr))
 	}
 
-	return client.ObjectKey{Namespace: keySplit[0], Name: keySplit[1]}, nil
+	return client.ObjectKey{Namespace: keyStr[:namespaceEndIndex], Name: keyStr[namespaceEndIndex+1:]}, nil
 }
 
 // HostnamesToStrings converts []gatewayapi_v1alpha2.Hostname to []string
 func HostnamesToStrings(hostnames []gatewayapiv1alpha2.Hostname) []string {
-	hosts := []string{}
-	for idx := range hostnames {
-		hosts = append(hosts, string(hostnames[idx]))
+	hosts := make([]string, len(hostnames))
+	for i, h := range hostnames {
+		hosts[i] = string(h)
 	}
 	return hosts
 }

pkg/common/common_test.go

@@ -3,11 +3,13 @@
 package common
 
 import (
+	"fmt"
 	"os"
 	"reflect"
 	"testing"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
 )
 
 func TestValidSubdomains(t *testing.T) {
@@ -411,3 +413,242 @@ func TestMergeMapStringString(t *testing.T) {
 		})
 	}
 }
+
+func TestUnMarshallLimitNamespace(t *testing.T) {
+	testCases := []struct {
+		name           string
+		namespace      string
+		expectedKey    client.ObjectKey
+		expectedDomain string
+		expectedError  bool
+	}{
+		{
+			name:           "when namespace is valid and contains both namespace and domain then return the correct values",
+			namespace:      "exampleNS/exampleGW#domain.com",
+			expectedKey:    client.ObjectKey{Name: "exampleGW", Namespace: "exampleNS"},
+			expectedDomain: "domain.com",
+			expectedError:  false,
+		},
+		{
+			name:           "when namespace is invalid (no '#domain') then return an error",
+			namespace:      "exampleNS/exampleGW",
+			expectedKey:    client.ObjectKey{},
+			expectedDomain: "",
+			expectedError:  true,
+		},
+		{
+			name:           "when namespace missing both namespace and gateway parts then return an error",
+			namespace:      "#domain.com",
+			expectedKey:    client.ObjectKey{},
+			expectedDomain: "",
+			expectedError:  true,
+		},
+		{
+			name:           "when namespace has no domain name then return correct values",
+			namespace:      "exampleNS/exampleGW#",
+			expectedKey:    client.ObjectKey{"exampleNS", "exampleGW"},
+			expectedDomain: "",
+			expectedError:  false,
+		},
+		{
+			name:           "when namespace only has gateway name (missing 'namespace/') and domain then return an error",
+			namespace:      "exampleGW#domain.com",
+			expectedKey:    client.ObjectKey{},
+			expectedDomain: "",
+			expectedError:  true,
+		},
+		{
+			name:           "when namespace only has namespace name (missing '/gwName') and domain then return an error",
+			namespace:      "exampleNS#domain.com",
+			expectedKey:    client.ObjectKey{},
+			expectedDomain: "",
+			expectedError:  true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			key, domain, err := UnMarshallLimitNamespace(tc.namespace)
+
+			if tc.expectedError {
+				if err == nil {
+					t.Errorf("Expected an error, but got %v", err)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("Unexpected error: %v", err)
+				}
+
+				if key != tc.expectedKey {
+					t.Errorf("Expected %v, but got %v", tc.expectedKey, key)
+				}
+
+				if domain != tc.expectedDomain {
+					t.Errorf("Expected %v, but got %v", tc.expectedDomain, domain)
+				}
+			}
+		})
+	}
+}
+
+func TestMarshallNamespace(t *testing.T) {
+	testCases := []struct {
+		name     string
+		gwKey    client.ObjectKey
+		domain   string
+		expected string
+	}{
+		{
+			name: "when input is valid then return expected output",
+			gwKey: client.ObjectKey{
+				Namespace: "test",
+				Name:      "myGwName",
+			},
+			domain:   "example.com",
+			expected: "test/myGwName#example.com",
+		},
+		{
+			name:     "when input is empty then return expected output",
+			gwKey:    client.ObjectKey{},
+			domain:   "",
+			expected: "/#",
+		},
+		{
+			name: "when input contains special characters then return expected output",
+			gwKey: client.ObjectKey{
+				Namespace: "test",
+				Name:      "myG.w-N*ame",
+			},
+			domain:   "example%-com",
+			expected: "test/myG.w-N*ame#example%-com",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := MarshallNamespace(tc.gwKey, tc.domain)
+			if !reflect.DeepEqual(result, tc.expected) {
+				t.Errorf("Expected %v, but got %v", tc.expected, result)
+			}
+		})
+	}
+}
+
+func TestUnMarshallObjectKey(t *testing.T) {
+	testCases := []struct {
+		name           string
+		input          string
+		expectedOutput client.ObjectKey
+		expectedError  error
+	}{
+		{
+			name:           "when valid key string then return valid ObjectKey",
+			input:          "default/object1",
+			expectedOutput: client.ObjectKey{Namespace: "default", Name: "object1"},
+			expectedError:  nil,
+		},
+		{
+			name:           "when valid key string with non-default namespace then return valid ObjectKey",
+			input:          "kube-system/object2",
+			expectedOutput: client.ObjectKey{Namespace: "kube-system", Name: "object2"},
+			expectedError:  nil,
+		},
+		{
+			name:           "when invalid namespace and name then return empty ObjectKey and error",
+			input:          "invalid",
+			expectedOutput: client.ObjectKey{},
+			expectedError:  fmt.Errorf("failed to split on %s: 'invalid'", string(NamespaceSeparator)),
+		},
+		{
+			name:           "when '#' separator used instead of default separator ('/') then return an error",
+			input:          "default#object1",
+			expectedOutput: client.ObjectKey{},
+			expectedError:  fmt.Errorf("failed to split on %s: 'default#object1'", string(NamespaceSeparator)),
+		},
+		{
+			name:           "when input string is empty then return an error",
+			input:          "",
+			expectedOutput: client.ObjectKey{},
+			expectedError:  fmt.Errorf("failed to split on %s: ''", string(NamespaceSeparator)),
+		},
+		{
+			name:           "when empty namespace and name then return valid empty ObjectKey",
+			input:          "/",
+			expectedOutput: client.ObjectKey{},
+			expectedError:  nil,
+		},
+		{
+			name:           "when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only",
+			input:          "default/",
+			expectedOutput: client.ObjectKey{Namespace: "default", Name: ""},
+			expectedError:  nil,
+		},
+		{
+			name:           "when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only",
+			input:          "/object",
+			expectedOutput: client.ObjectKey{Namespace: "", Name: "object"},
+			expectedError:  nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			output, err := UnMarshallObjectKey(tc.input)
+
+			if err != nil && tc.expectedError == nil {
+				t.Errorf("unexpected error: got %v, want nil", err)
+			} else if err == nil && tc.expectedError != nil {
+				t.Errorf("expected error but got nil")
+			} else if err != nil && tc.expectedError != nil && err.Error() != tc.expectedError.Error() {
+				t.Errorf("unexpected error: got '%v', want '%v'", err, tc.expectedError)
+			}
+
+			if output != tc.expectedOutput {
+				t.Errorf("unexpected output: got %v, want %v", output, tc.expectedOutput)
+			}
+		})
+	}
+}
+
+func TestHostnamesToStrings(t *testing.T) {
+	testCases := []struct {
+		name           string
+		inputHostnames []gatewayapiv1alpha2.Hostname
+		expectedOutput []string
+	}{
+		{
+			name:           "when input is empty then return empty output",
+			inputHostnames: []gatewayapiv1alpha2.Hostname{},
+			expectedOutput: []string{},
+		},
+		{
+			name:           "when input has a single precise hostname then return a single string",
+			inputHostnames: []gatewayapiv1alpha2.Hostname{"example.com"},
+			expectedOutput: []string{"example.com"},
+		},
+		{
+			name:           "when input has multiple precise hostnames then return the corresponding strings",
+			inputHostnames: []gatewayapiv1alpha2.Hostname{"example.com", "test.com", "localhost"},
+			expectedOutput: []string{"example.com", "test.com", "localhost"},
+		},
+		{
+			name:           "when input has a wildcard hostname then return the wildcard string",
+			inputHostnames: []gatewayapiv1alpha2.Hostname{"*.example.com"},
+			expectedOutput: []string{"*.example.com"},
+		},
+		{
+			name:           "when input has both precise and wildcard hostnames then return the corresponding strings",
+			inputHostnames: []gatewayapiv1alpha2.Hostname{"example.com", "*.test.com"},
+			expectedOutput: []string{"example.com", "*.test.com"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			output := HostnamesToStrings(tc.inputHostnames)
+			if !reflect.DeepEqual(tc.expectedOutput, output) {
+				t.Errorf("Unexpected output. Expected %v but got %v", tc.expectedOutput, output)
+			}
+		})
+	}
+}