Add some feedback

Author: bjee19

internal/controller/nginx/config/servers.go

@@ -225,21 +225,22 @@ type rewriteConfig struct {
 }
 
 // extractMirrorTargetsWithPercentages extracts mirror targets and their percentages from path rules.
-func extractMirrorTargetsWithPercentages(pathRules []dataplane.PathRule) map[string]float64 {
-	mirrorTargets := make(map[string]float64)
+func extractMirrorTargetsWithPercentages(pathRules []dataplane.PathRule) map[string]*float64 {
+	mirrorTargets := make(map[string]*float64)
 
 	for _, rule := range pathRules {
 		for _, matchRule := range rule.MatchRules {
 			for _, mirrorFilter := range matchRule.Filters.RequestMirrors {
 				if mirrorFilter.Target != nil {
 					if mirrorFilter.Percent == nil {
-						mirrorTargets[*mirrorFilter.Target] = 100.0
+						mirrorTargets[*mirrorFilter.Target] = helpers.GetPointer(100.0)
 						continue
 					}
 
-					percentage := *mirrorFilter.Percent
+					percentage := mirrorFilter.Percent
 
-					if _, exists := mirrorTargets[*mirrorFilter.Target]; !exists || percentage > mirrorTargets[*mirrorFilter.Target] {
+					if _, exists := mirrorTargets[*mirrorFilter.Target]; !exists ||
+						*percentage > *mirrorTargets[*mirrorFilter.Target] {
 						mirrorTargets[*mirrorFilter.Target] = percentage // set a higher percentage if it exists
 					}
 				}
@@ -278,12 +279,7 @@ func createLocations(
 			grpcServer = true
 		}
 
-		mirrorPercentage, exists := mirrorPathToPercentage[rule.Path]
-		if !exists {
-			// need a way to differentiate between no mirror filter and a mirror filter with 0 percent set, and
-			// I don't want to pass an extra boolean around.
-			mirrorPercentage = -1
-		}
+		mirrorPercentage := mirrorPathToPercentage[rule.Path]
 
 		extLocations := initializeExternalLocations(rule, pathsAndTypes)
 		for i := range extLocations {
@@ -464,7 +460,7 @@ func updateLocation(
 	path string,
 	grpc bool,
 	keepAliveCheck keepAliveChecker,
-	mirrorPercentage float64,
+	mirrorPercentage *float64,
 ) http.Location {
 	if filters.InvalidFilter != nil {
 		location.Return = &http.Return{Code: http.StatusInternalServerError}
@@ -533,7 +529,7 @@ func updateLocationMirrorFilters(
 	location http.Location,
 	mirrorFilters []*dataplane.HTTPRequestMirrorFilter,
 	path string,
-	mirrorPercentage float64,
+	mirrorPercentage *float64,
 ) http.Location {
 	for _, filter := range mirrorFilters {
 		if filter.Target != nil {
@@ -547,9 +543,9 @@ func updateLocationMirrorFilters(
 
 	// if the mirrorPercentage is 100.0 or negative (we set it to negative when there is no mirror filter because 0.0
 	// is valid), the split clients variable is not generated, and we want to let all the traffic get mirrored.
-	if mirrorPercentage != 100.0 && mirrorPercentage >= 0.0 {
+	if mirrorPercentage != nil && *mirrorPercentage != 100.0 {
 		location.MirrorSplitClientsVariableName = convertSplitClientVariableName(
-			fmt.Sprintf("%s_%.2f", path, mirrorPercentage),
+			fmt.Sprintf("%s_%.2f", path, *mirrorPercentage),
 		)
 	}
 
@@ -599,7 +595,7 @@ func updateLocations(
 	path string,
 	grpc bool,
 	keepAliveCheck keepAliveChecker,
-	mirrorPercentage float64,
+	mirrorPercentage *float64,
 ) []http.Location {
 	updatedLocations := make([]http.Location, len(buildLocations))
 
@@ -1070,12 +1066,12 @@ func getConnectionHeader(keepAliveCheck keepAliveChecker, backends []dataplane.B
 
 // deduplicateStrings removes duplicate strings from a slice while preserving order.
 func deduplicateStrings(content []string) []string {
-	seen := make(map[string]bool)
+	seen := make(map[string]struct{})
 	result := make([]string, 0, len(content))
 
 	for _, str := range content {
-		if !seen[str] {
-			seen[str] = true
+		if _, exists := seen[str]; !exists {
+			seen[str] = struct{}{}
 			result = append(result, str)
 		}
 	}

internal/controller/nginx/config/split_clients.go

@@ -42,15 +42,15 @@ func createRequestMirrorSplitClients(servers []dataplane.VirtualServer) []http.S
 		mirrorPathToPercentage := extractMirrorTargetsWithPercentages(server.PathRules)
 
 		for _, pathRule := range server.PathRules {
-			mirrorPercentage, exists := mirrorPathToPercentage[pathRule.Path]
+			mirrorPercentage := mirrorPathToPercentage[pathRule.Path]
 
-			if mirrorPercentage != 100 && exists {
+			if mirrorPercentage != nil && *mirrorPercentage != 100 {
 				splitClient := http.SplitClient{
-					// this has to be something unique and able to be accessed from the server side
-					VariableName: convertSplitClientVariableName(fmt.Sprintf("%s_%.2f", pathRule.Path, mirrorPercentage)),
+					// this has to be something unique and able to be accessed from the server block
+					VariableName: convertSplitClientVariableName(fmt.Sprintf("%s_%.2f", pathRule.Path, *mirrorPercentage)),
 					Distributions: []http.SplitClientDistribution{
 						{
-							Percent: fmt.Sprintf("%.2f", mirrorPercentage),
+							Percent: fmt.Sprintf("%.2f", *mirrorPercentage),
 							Value:   pathRule.Path,
 						},
 						{
@@ -79,12 +79,12 @@ func convertSplitClientVariableName(name string) string {
 }
 
 func removeDuplicateSplitClients(splitClients []http.SplitClient) []http.SplitClient {
-	seen := make(map[string]bool)
+	seen := make(map[string]struct{})
 	result := make([]http.SplitClient, 0, len(splitClients))
 
 	for _, client := range splitClients {
-		if !seen[client.VariableName] {
-			seen[client.VariableName] = true
+		if _, exists := seen[client.VariableName]; !exists {
+			seen[client.VariableName] = struct{}{}
 			result = append(result, client)
 		}
 	}

internal/controller/nginx/config/split_clients_template.go

@@ -6,9 +6,9 @@ split_clients $request_id ${{ $sc.VariableName }} {
     {{- range $d := $sc.Distributions }}
         {{- if eq $d.Percent "0.00" }}
     # {{ $d.Percent }}% {{ $d.Value }};
-		{{- else if eq $d.Percent "*" }}
-	* {{ $d.Value }};
-		{{- else }}
+        {{- else if eq $d.Percent "*" }}
+    * {{ $d.Value }};
+        {{- else }}
     {{ $d.Percent }}% {{ $d.Value }};
         {{- end }}
     {{- end }}

internal/controller/state/dataplane/convert.go

@@ -95,6 +95,10 @@ func convertHTTPRequestMirrorFilter(
 		result.Percent = helpers.GetPointer(float64(100))
 	}
 
+	if *result.Percent > 100.0 {
+		result.Percent = helpers.GetPointer(100.0)
+	}
+
 	return result
 }
 

internal/controller/state/dataplane/convert_test.go

@@ -361,6 +361,25 @@ func TestConvertHTTPMirrorFilter(t *testing.T) {
 			},
 			name: "fraction denominator not specified",
 		},
+		{
+			filter: &v1.HTTPRequestMirrorFilter{
+				BackendRef: v1.BackendObjectReference{
+					Name:      "backend",
+					Namespace: helpers.GetPointer[v1.Namespace]("namespace"),
+				},
+				Fraction: &v1.Fraction{
+					Numerator:   300,
+					Denominator: helpers.GetPointer(int32(1)),
+				},
+			},
+			expected: &HTTPRequestMirrorFilter{
+				Name:      helpers.GetPointer("backend"),
+				Namespace: helpers.GetPointer("namespace"),
+				Target:    helpers.GetPointer("/_ngf-internal-mirror-namespace/backend-test/route1-0"),
+				Percent:   helpers.GetPointer(float64(100)),
+			},
+			name: "fraction result over 100",
+		},
 		{
 			filter: &v1.HTTPRequestMirrorFilter{
 				BackendRef: v1.BackendObjectReference{

tests/Makefile

@@ -12,7 +12,7 @@ GW_SERVICE_TYPE = NodePort## Service type to use for the gateway
 NGF_VERSION ?= edge## NGF version to be tested
 PULL_POLICY = Never## Pull policy for the images
 NGINX_CONF_DIR = internal/controller/nginx/conf
-SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,GatewayAddressEmpty,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteBackendProtocolWebSocket,HTTPRouteRequestPercentageMirror
+SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,GatewayAddressEmpty,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteRequestPercentageMirror,HTTPRouteBackendProtocolWebSocket
 STANDARD_CONFORMANCE_PROFILES = GATEWAY-HTTP,GATEWAY-GRPC
 EXPERIMENTAL_CONFORMANCE_PROFILES = GATEWAY-TLS
 CONFORMANCE_PROFILES = $(STANDARD_CONFORMANCE_PROFILES) # by default we use the standard conformance profiles. If experimental is enabled we override this and add the experimental profiles.