fix(service-proxy): avoid Duplicate Proxy Path in URL Rewrite Function (

#868) * refactor(conditions): remove deprecated NoHelmChartTestFailures condition and related tests Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com> * fix(service-proxy): rewrite logic + a lot of tests Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com> * Automatic generation of CRD API Docs * refactor(tests): clean up import statements in proxy_test.go Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com> * refactor(service-proxy): reorder import statements in proxy_test.go Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com> * fix(service-proxy): inject logger into context in defer and update test URLs Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com> * chore(service-proxy): go fmt Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com> --------- Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com> Co-authored-by: Cloud Operator <169066274+cloud-operator@users.noreply.github.com> Co-authored-by: Abhijith Ravindra <137736216+abhijith-darshan@users.noreply.github.com>
cloudoperators · Jan 30, 2025 · d44875f · d44875f
1 parent 60de214
commit d44875f
Show file tree

Hide file tree

Showing 3 changed files with 151 additions and 30 deletions.
diff --git a/cmd/service-proxy/proxy.go b/cmd/service-proxy/proxy.go
@@ -25,6 +25,7 @@ import (
 	"net/http/httputil"
 	"net/url"
 	"regexp"
+	"strings"
 	"sync"
 
 	"github.com/go-logr/logr"
@@ -190,46 +191,72 @@ func (pm *ProxyManager) RoundTrip(req *http.Request) (resp *http.Response, err e
 	resp, err = cls.transport.RoundTrip(req)
 	// errors are logged by pm.Errorhandler
 	if err == nil {
-		log.FromContext(req.Context()).Info("Forwarded request", "status", resp.StatusCode, "upstream", req.URL.String())
+		log.FromContext(req.Context()).Info("Forwarded request", "status", resp.StatusCode, "upstreamServiceRouteURL", req.URL.String())
 	}
 	return
 }
 
-// rewrite rewrites the request to the backend URL and sets the cluster in the context to be used by RoundTrip to identify the correct transport
 func (pm *ProxyManager) rewrite(req *httputil.ProxyRequest) {
 	req.SetXForwarded()
 
-	l := pm.logger.WithValues("host", req.In.Host, "url", req.In.URL.String(), "method", req.In.Method)
+	// Create a logger with relevant request details
+	l := pm.logger.WithValues(
+		"incomingHost", req.In.Host,
+		"incomingRequestURL", req.In.URL.String(),
+		"incomingMethod", req.In.Method,
+	)
 
 	// inject current logger into context before returning
 	defer func() {
 		req.Out = req.Out.WithContext(log.IntoContext(req.Out.Context(), l))
 	}()
 
-	// hostname is expected to have the format specified in ./pkg/common/url.go
+	// Extract cluster from the incoming request host
 	cluster, err := common.ExtractCluster(req.In.Host)
 	if err != nil {
+		l.Error(err, "Failed to extract cluster from host", "host", req.In.Host)
 		return
 	}
 
+	// Retrieve the upstream service route for the cluster
 	route, ok := pm.GetClusterRoute(cluster, "https://"+req.In.Host)
 	if !ok {
-		l.Info("No route found for cluster and URL", "cluster", cluster, "url", req.In.URL.String())
+		l.Info("No route found for cluster and URL", "cluster", cluster, "incomingRequestURL", req.In.URL.String())
 		return
 	}
-	backendURL := route.url
-	// set cluster in context
-	ctx := context.WithValue(req.Out.Context(), contextClusterKey{}, cluster)
+	upstreamServiceRouteURL := route.url
+
+	// Ensure the outgoing request URL is properly updated
+	if !strings.HasPrefix(req.Out.URL.Path, upstreamServiceRouteURL.Path) {
+		// Append the original request path to the upstream service route URL path
+		req.Out.URL.Path = strings.TrimSuffix(upstreamServiceRouteURL.Path, "/") + req.Out.URL.Path
+	}
 
-	l.WithValues("cluster", cluster, "namespace", route.namespace, "name", route.serviceName)
+	// Set the correct upstream service route URL details
+	req.Out.URL.Scheme = upstreamServiceRouteURL.Scheme
+	req.Out.URL.Host = upstreamServiceRouteURL.Host
+
+	// Inject the cluster into the outgoing request context
+	ctx := context.WithValue(req.Out.Context(), contextClusterKey{}, cluster)
+	ctx = log.IntoContext(ctx, l)
 
 	req.Out = req.Out.WithContext(ctx)
-	req.SetURL(backendURL)
+
+	// Log the successful rewrite for debugging purposes
+	l.Info("Request rewrite completed",
+		"cluster", cluster,
+		"namespace", route.namespace,
+		"serviceName", route.serviceName,
+		"upstreamServiceRouteURL", req.Out.URL.String(),
+	)
 }
 
 // modifyResponse strips the k8s API server proxy path prepended to the location header during redirects:
 // https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go#L113
 func (pm *ProxyManager) modifyResponse(resp *http.Response) error {
+	logger := log.FromContext(resp.Request.Context())
+	logger.Info("Modifying response", "statusCode", resp.StatusCode, "originalLocation", resp.Header.Get("Location"))
+
 	if location := resp.Header.Get("Location"); location != "" {
 		location = apiServerProxyPathRegex.ReplaceAllString(location, "/")
 		resp.Header.Set("Location", location)

diff --git a/cmd/service-proxy/proxy_test.go b/cmd/service-proxy/proxy_test.go
@@ -27,63 +27,91 @@ import (
 // If checks if he url is properly rewritten and the request context contains the cluster name
 // and a logger with the correct values.
 func TestRewrite(t *testing.T) {
-	proxyURL, err := url.Parse("https://apiserver/proxy/url")
+	proxyURL, err := url.Parse("https://api.test-api-server.com/api/v1/namespaces/kube-monitoring/services/test-service:8080")
 	if err != nil {
-		t.Fatal("failed to parse proxy url")
+		t.Fatal("failed to parse proxy URL")
 	}
 
 	tests := []struct {
-		name        string
-		url         string
-		expectedURL string
-		contextVal  any
+		name                            string
+		url                             string
+		expectedupstreamServiceRouteURL string
+		contextVal                      any
 	}{
 		{
-			name:        "valid host",
-			url:         "https://cluster--1234567.organisation.basedomain/abcd",
-			expectedURL: "https://apiserver/proxy/url/abcd",
-			contextVal:  "cluster",
+			name:                            "valid host with path",
+			url:                             "https://cluster--1234567.organisation.basedomain/dashboard",
+			expectedupstreamServiceRouteURL: "https://api.test-api-server.com/api/v1/namespaces/kube-monitoring/services/test-service:8080/dashboard",
+			contextVal:                      "cluster",
 		},
 		{
-			name:        "invalid host",
-			url:         "https://something.organisation.basedomain/abcd",
-			expectedURL: "https://something.organisation.basedomain/abcd",
-			contextVal:  nil,
+			name:                            "valid host with deeper path",
+			url:                             "https://cluster--1234567.organisation.basedomain/api/resource",
+			expectedupstreamServiceRouteURL: "https://api.test-api-server.com/api/v1/namespaces/kube-monitoring/services/test-service:8080/api/resource",
+			contextVal:                      "cluster",
+		},
+		{
+			name:                            "valid host with already prefixed path",
+			url:                             "https://cluster--1234567.organisation.basedomain/api/v1/namespaces/kube-monitoring/services/test-service:8080/existing-path",
+			expectedupstreamServiceRouteURL: "https://api.test-api-server.com/api/v1/namespaces/kube-monitoring/services/test-service:8080/existing-path",
+			contextVal:                      "cluster",
+		},
+		{
+			name:                            "unknown cluster request",
+			url:                             "https://unknown-cluster.organisation.basedomain/dashboard",
+			expectedupstreamServiceRouteURL: "https://unknown-cluster.organisation.basedomain/dashboard", // No rewrite expected
+			contextVal:                      nil,
+		},
+		{
+			name:                            "invalid host format",
+			url:                             "https://something.organisation.basedomain/abcd",
+			expectedupstreamServiceRouteURL: "https://something.organisation.basedomain/abcd", // No rewrite expected
+			contextVal:                      nil,
 		},
 	}
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			inputURL, err := url.Parse(tt.url)
 			if err != nil {
-				t.Fatal("failed to parse url")
+				t.Fatal("failed to parse URL")
 			}
+
 			pm := NewProxyManager()
 			pm.clusters["cluster"] = clusterRoutes{
 				routes: map[string]route{
 					inputURL.Scheme + "://" + inputURL.Host: {
 						url:         proxyURL,
-						namespace:   "namespace",
-						serviceName: "test",
+						namespace:   "kube-monitoring",
+						serviceName: "test-service",
 					},
 				},
 			}
+
 			r, err := http.NewRequestWithContext(context.Background(), http.MethodGet, inputURL.String(), http.NoBody)
 			if err != nil {
 				t.Fatal("failed to create request")
 				return
 			}
+
 			req := httputil.ProxyRequest{
 				In:  r,
 				Out: r.Clone(r.Context()),
 			}
+
 			pm.rewrite(&req)
 
+			// Ensure logger is propagated
 			if _, err := logr.FromContext(req.Out.Context()); err != nil {
 				t.Error("expected logger in outgoing request context")
 			}
-			if req.Out.URL.String() != tt.expectedURL {
-				t.Errorf("expected url %s, got %s", tt.expectedURL, req.Out.URL.String())
+
+			// Validate the rewritten URL
+			if req.Out.URL.String() != tt.expectedupstreamServiceRouteURL {
+				t.Errorf("expected URL %s, got %s", tt.expectedupstreamServiceRouteURL, req.Out.URL.String())
 			}
+
+			// Validate the cluster context
 			if req.Out.Context().Value(contextClusterKey{}) != tt.contextVal {
 				t.Errorf("expected cluster %s in context, got %s", "cluster", req.Out.Context().Value(contextClusterKey{}))
 			}
@@ -185,6 +213,73 @@ users:
 	}
 }
 
+func TestModifyResponse(t *testing.T) {
+	tests := []struct {
+		name               string
+		locationHeader     string
+		expectedLocation   string
+		expectHeaderChange bool
+	}{
+		{
+			name:               "Valid input with proxy paths",
+			locationHeader:     "/api/v1/namespaces/kube-monitoring/services/test-service:8080/proxy/api/main.js",
+			expectedLocation:   "/api/main.js",
+			expectHeaderChange: true,
+		},
+		{
+			name:               "Single proxy path",
+			locationHeader:     "/api/v1/namespaces/kube-monitoring/services/test-service:8080/proxy/",
+			expectedLocation:   "/",
+			expectHeaderChange: true,
+		},
+		{
+			name:               "No match in location header",
+			locationHeader:     "/other/path/that/does/not/match",
+			expectedLocation:   "/other/path/that/does/not/match",
+			expectHeaderChange: false,
+		},
+		{
+			name:               "Empty location header",
+			locationHeader:     "",
+			expectedLocation:   "",
+			expectHeaderChange: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Prepare response
+			resp := &http.Response{
+				Header: http.Header{
+					"Location": []string{tt.locationHeader},
+				},
+				Request: &http.Request{},
+			}
+
+			// Call modifyResponse
+			pm := NewProxyManager() // Assuming NewProxyManager is implemented
+			err := pm.modifyResponse(resp)
+
+			// Check for errors
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+
+			// Validate the location header
+			location := resp.Header.Get("Location")
+			if location != tt.expectedLocation {
+				t.Errorf("expected location %s, got %s", tt.expectedLocation, location)
+			}
+
+			// Validate whether the header was modified
+			headerChanged := location != tt.locationHeader
+			if headerChanged != tt.expectHeaderChange {
+				t.Errorf("expected header change: %v, got: %v", tt.expectHeaderChange, headerChanged)
+			}
+		})
+	}
+}
+
 // helper function to create string pointer
 func pointer(s string) *string {
 	return &s

diff --git a/pkg/apis/greenhouse/v1alpha1/conditions_test.go b/pkg/apis/greenhouse/v1alpha1/conditions_test.go
@@ -353,5 +353,4 @@ var _ = Describe("Test conditions util functions", func() {
 		Expect(condition1.Equal(condition2)).To(BeTrue())
 
 	})
-
 })
Original file line number	Diff line number	Diff line change
Expand Up		@@ -353,5 +353,4 @@ var _ = Describe("Test conditions util functions", func() {
		Expect(condition1.Equal(condition2)).To(BeTrue())

		})

		})