From 930f67cc5eb9d801ac2a0fb8df9c7723a63ca582 Mon Sep 17 00:00:00 2001 From: Uwe Mayer Date: Mon, 28 Oct 2024 15:07:24 +0100 Subject: [PATCH 1/8] fix(service-proxy): Well defined urls for exposed services with max 63 chars Signed-off-by: Uwe Mayer --- cmd/service-proxy/instrumentation.go | 4 +-- cmd/service-proxy/proxy.go | 10 +++---- cmd/service-proxy/proxy_test.go | 2 +- pkg/admission/cluster_webhook.go | 7 +++++ pkg/admission/utils.go | 39 ++++++++++++++++++++++++++++ pkg/common/url.go | 29 +++++++++------------ 6 files changed, 65 insertions(+), 26 deletions(-) diff --git a/cmd/service-proxy/instrumentation.go b/cmd/service-proxy/instrumentation.go index 5a6092daa..06f0bdf2a 100644 --- a/cmd/service-proxy/instrumentation.go +++ b/cmd/service-proxy/instrumentation.go @@ -69,11 +69,9 @@ func InstrumentHandler(next http.Handler, registry prometheus.Registerer) http.H injector := func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { - if name, cluster, namespace, err := common.SplitHost(req.Host); err == nil { + if cluster, err := common.ExtractCluster(req.Host); err == nil { ctx := req.Context() ctx = context.WithValue(ctx, ctxClusterKey{}, cluster) - ctx = context.WithValue(ctx, ctxNamespaceKey{}, namespace) - ctx = context.WithValue(ctx, ctxNameKey{}, name) req = req.WithContext(ctx) } next.ServeHTTP(rw, req) diff --git a/cmd/service-proxy/proxy.go b/cmd/service-proxy/proxy.go index ad78607b4..5517230de 100644 --- a/cmd/service-proxy/proxy.go +++ b/cmd/service-proxy/proxy.go @@ -97,12 +97,12 @@ func (pm *ProxyManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R return ctrl.Result{}, fmt.Errorf("failed to parse api url: %w", err) } - configs, err := pm.pluginsForCluster(ctx, req.Name, req.Namespace) + plugins, err := pm.pluginsForCluster(ctx, req.Name, req.Namespace) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to get plugins for cluster %s: %w", req.Name, err) } - for _, cfg := range configs { - for url, svc := range cfg.Status.ExposedServices { + for _, plugin := range plugins { + for url, svc := range plugin.Status.ExposedServices { u := *k8sAPIURL // copy URL struct proto := "http" if svc.Protocol != nil { @@ -172,11 +172,11 @@ func (pm *ProxyManager) rewrite(req *httputil.ProxyRequest) { }() // hostname is expected to have the format $name--$cluster--$namespace.$organisation.$basedomain - name, cluster, namespace, err := common.SplitHost(req.In.Host) + cluster, err := common.ExtractCluster(req.In.Host) if err != nil { return } - l = l.WithValues("cluster", cluster, "namespace", namespace, "name", name) + l = l.WithValues("cluster", cluster) pm.mu.RLock() defer pm.mu.RUnlock() diff --git a/cmd/service-proxy/proxy_test.go b/cmd/service-proxy/proxy_test.go index e761eb415..7906fc5ba 100644 --- a/cmd/service-proxy/proxy_test.go +++ b/cmd/service-proxy/proxy_test.go @@ -41,7 +41,7 @@ func TestRewrite(t *testing.T) { }{ { name: "valid host", - url: "https://name--cluster--namespace.organisation.basedomain/abcd", + url: "https://cluster--1234567.organisation.basedomain/abcd", expectedURL: "https://apiserver/proxy/url/abcd", contextVal: "cluster", }, diff --git a/pkg/admission/cluster_webhook.go b/pkg/admission/cluster_webhook.go index 11db3eecf..bfdffdd90 100644 --- a/pkg/admission/cluster_webhook.go +++ b/pkg/admission/cluster_webhook.go @@ -87,6 +87,13 @@ func ValidateCreateCluster(ctx context.Context, _ client.Client, obj runtime.Obj if !ok { return nil, nil } + if warning, err := invalidateDoubleDashesInName(cluster, logger); err != nil { + return warning, err + } + // capping the name at 40 chars, so we ensure to get unique urls for exposed services per cluster. service-name/namespace hash needs to fit (max 63 chars) + if warning, err := capName(cluster, logger, 40); err != nil { + return warning, err + } annotations := cluster.GetAnnotations() _, deletionMarked := annotations[apis.MarkClusterDeletionAnnotation] _, scheduleExists := annotations[apis.ScheduleClusterDeletionAnnotation] diff --git a/pkg/admission/utils.go b/pkg/admission/utils.go index 0d3133be7..c1cf5ac7f 100644 --- a/pkg/admission/utils.go +++ b/pkg/admission/utils.go @@ -5,10 +5,16 @@ package admission import ( "context" + "fmt" "net/url" + "strings" + "github.com/go-logr/logr" + + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -126,3 +132,36 @@ func logAdmissionRequest(ctx context.Context) { ctrl.Log.Info("AdmissionRequest", "Request", admissionRequest) } + +// invalidateDoubleDashes validates that the object name does not contain double dashes. +func invalidateDoubleDashesInName(obj client.Object, l logr.Logger) (admission.Warnings, error) { + if strings.Contains(obj.GetName(), "--") { + err := apierrors.NewInvalid( + obj.GetObjectKind().GroupVersionKind().GroupKind(), + obj.GetName(), + field.ErrorList{ + field.Invalid(field.NewPath("metadata", "name"), obj.GetName(), "name cannot contain double dashes"), + }, + ) + l.Error(err, "found object name with double dashes, admission will be denied") + return admission.Warnings{"you cannot create an object with double dashes in the name"}, err + } + return nil, nil +} + +// capName validates that the name is not longer than the provided length. + +func capName(obj client.Object, l logr.Logger, length int) (admission.Warnings, error) { + if len(obj.GetName()) > length { + err := apierrors.NewInvalid( + obj.GetObjectKind().GroupVersionKind().GroupKind(), + obj.GetName(), + field.ErrorList{ + field.Invalid(field.NewPath("metadata", "name"), obj.GetName(), fmt.Sprintf("name must be less than or equal to %d", length)), + }, + ) + l.Error(err, fmt.Sprintf("found object name too long, admission will be denied, name must be less than or equal to %d", length)) + return admission.Warnings{fmt.Sprintf("you cannot create an object with a name longer than %d", length)}, err + } + return nil, nil +} diff --git a/pkg/common/url.go b/pkg/common/url.go index 9d12ead0a..c005f72d6 100644 --- a/pkg/common/url.go +++ b/pkg/common/url.go @@ -15,32 +15,27 @@ import ( var DNSDomain string // URLForExposedServiceInPlugin returns the URL that shall be used to expose a service centrally via Greenhouse. -// The pattern shall be $https://$service--$cluster--$namespace.$organisation.$basedomain . -// If the first subdomain exceeds 63 characters, it will be shortened to 63 characters by appending a hash. +// The pattern shall be $https://$cluster--$hash.$organisation.$basedomain, where $hash = service--$namespace +// We know $cluster is no longer than 40 characters and does not contain "--" func URLForExposedServiceInPlugin(serviceName string, plugin *greenhousev1alpha1.Plugin) string { - subdomain := fmt.Sprintf("%s--%s--%s", serviceName, plugin.Spec.ClusterName, plugin.Spec.ReleaseNamespace) - if len(subdomain) > 63 { - hashedSubdomain := sha256.Sum256([]byte(subdomain)) - subdomain = fmt.Sprintf("%s-%x", subdomain[:54], hashedSubdomain[:4]) - } + hash := sha256.Sum256([]byte(fmt.Sprintf("%s--%s", serviceName, plugin.Spec.ReleaseNamespace))) + subdomain := fmt.Sprintf("%s--%s", plugin.Spec.ClusterName, hash[:8]) return fmt.Sprintf( "https://%s.%s.%s", subdomain, plugin.GetNamespace(), DNSDomain, ) } -// SplitHost splits an exposed service host into its name, namespace and cluster parts -// The pattern shall be $service--$cluster--$namespace -// TODO, need to fix: We know that information on the namespace and even the cluster might be lost when the host was hashed due to it's length. -// Currently only cluster is critical, need to fix this. -func SplitHost(host string) (name, cluster, namespace string, err error) { +// ExtractCluster extracts the cluster name from the host. +// The pattern shall be $https://$cluster--$hash, where $hash = service--$namespace +func ExtractCluster(host string) (cluster string, err error) { parts := strings.SplitN(host, ".", 2) if len(parts) < 2 { - return "", "", "", fmt.Errorf("invalid host: %s", host) + return "", fmt.Errorf("invalid host: %s", host) } - parts = strings.SplitN(parts[0], "--", 3) - if len(parts) < 3 { - return "", "", "", fmt.Errorf("invalid host: %s", host) + parts = strings.SplitN(parts[0], "--", 2) + if len(parts) < 2 { + return "", fmt.Errorf("invalid host: %s", host) } - return parts[0], parts[1], parts[2], nil + return parts[0], nil } From 1613d4aefab8f9067a6adc200eb30ca95fac7b3a Mon Sep 17 00:00:00 2001 From: Uwe Mayer Date: Fri, 1 Nov 2024 12:49:24 +0100 Subject: [PATCH 2/8] refac(service-proxy): Persist nam+namespace in clusterRoutes Signed-off-by: Uwe Mayer --- cmd/service-proxy/instrumentation.go | 25 +++---- cmd/service-proxy/main.go | 2 +- cmd/service-proxy/proxy.go | 69 ++++++++++++++------ cmd/service-proxy/proxy_test.go | 17 +++-- docs/user-guides/plugin/plugin-deployment.md | 2 +- 5 files changed, 75 insertions(+), 40 deletions(-) diff --git a/cmd/service-proxy/instrumentation.go b/cmd/service-proxy/instrumentation.go index 06f0bdf2a..f37a7f565 100644 --- a/cmd/service-proxy/instrumentation.go +++ b/cmd/service-proxy/instrumentation.go @@ -13,31 +13,25 @@ import ( "github.com/cloudoperators/greenhouse/pkg/common" ) -type ctxClusterKey struct { -} -type ctxNamespaceKey struct { -} -type ctxNameKey struct { -} - var ( clusterFromContext = promhttp.WithLabelFromCtx("cluster", func(ctx context.Context) string { - cluster, _ := ctx.Value(ctxClusterKey{}).(string) //nolint:errcheck + cluster, _ := ctx.Value(ContextClusterKey{}).(string) //nolint:errcheck return cluster }) namespaceFromContext = promhttp.WithLabelFromCtx("namespace", func(ctx context.Context) string { - namespace, _ := ctx.Value(ctxNamespaceKey{}).(string) //nolint:errcheck + namespace, _ := ctx.Value(ContextNamespaceKey{}).(string) //nolint:errcheck return namespace }) nameFromContext = promhttp.WithLabelFromCtx("name", func(ctx context.Context) string { - name, _ := ctx.Value(ctxNameKey{}).(string) //nolint:errcheck + name, _ := ctx.Value(ContextNameKey{}).(string) //nolint:errcheck return name }) ) -func InstrumentHandler(next http.Handler, registry prometheus.Registerer) http.Handler { +func InstrumentHandler(pm *ProxyManager, registry prometheus.Registerer) http.Handler { + next := pm.ReverseProxy() requestCounter := prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "http_requests_total", @@ -70,8 +64,15 @@ func InstrumentHandler(next http.Handler, registry prometheus.Registerer) http.H injector := func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { if cluster, err := common.ExtractCluster(req.Host); err == nil { + pm.mu.Lock() + defer pm.mu.Unlock() ctx := req.Context() - ctx = context.WithValue(ctx, ctxClusterKey{}, cluster) + ctx = context.WithValue(ctx, ContextClusterKey{}, cluster) + route, found := pm.GetClusterRoute(cluster, req.Host) + if found { + ctx = context.WithValue(ctx, ContextNamespaceKey{}, route.namespace) + ctx = context.WithValue(ctx, ContextNameKey{}, route.serviceName) + } req = req.WithContext(ctx) } next.ServeHTTP(rw, req) diff --git a/cmd/service-proxy/main.go b/cmd/service-proxy/main.go index 0b0fd1a38..77c629016 100644 --- a/cmd/service-proxy/main.go +++ b/cmd/service-proxy/main.go @@ -109,7 +109,7 @@ func main() { frontend := http.Server{ Addr: listenAddr, - Handler: InstrumentHandler(pm.ReverseProxy(), metrics.Registry), + Handler: InstrumentHandler(pm, metrics.Registry), } g.Add( diff --git a/cmd/service-proxy/proxy.go b/cmd/service-proxy/proxy.go index 5517230de..09a43d911 100644 --- a/cmd/service-proxy/proxy.go +++ b/cmd/service-proxy/proxy.go @@ -36,20 +36,34 @@ func NewProxyManager() *ProxyManager { } type ProxyManager struct { - client client.Client - logger logr.Logger - + client client.Client + logger logr.Logger clusters map[string]clusterRoutes mu sync.RWMutex } type clusterRoutes struct { transport http.RoundTripper - routes map[string]*url.URL + routes map[string]route +} + +// route holds the url the request should be forwarded to and the service name and namespace as metadata +type route struct { + url *url.URL + serviceName string + namespace string } -// contextClusterKey is used to embed a cluster in the context -type contextClusterKey struct { +// ContextClusterKey is used to embed a cluster in the context +type ContextClusterKey struct { +} + +// contextNamespaceKey is used to embed a namespace in the context +type ContextNamespaceKey struct { +} + +// contextNameKey is used to embed a name in the context +type ContextNameKey struct { } var apiServerProxyPathRegex = regexp.MustCompile(`/api/v1/namespaces/[^/]+/services/[^/]+/proxy/`) @@ -90,7 +104,7 @@ func (pm *ProxyManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R return ctrl.Result{}, fmt.Errorf("failed to create transport for cluster %s: %w", req.Name, err) } - cls.routes = make(map[string]*url.URL) + cls.routes = make(map[string]route) k8sAPIURL, err := url.Parse(restConfig.Host) if err != nil { @@ -109,7 +123,7 @@ func (pm *ProxyManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R proto = *svc.Protocol } u.Path = fmt.Sprintf("/api/v1/namespaces/%s/services/%s:%s:%d/proxy", svc.Namespace, proto, svc.Name, svc.Port) - cls.routes[url] = &u + cls.routes[url] = route{url: &u, namespace: svc.Namespace, serviceName: svc.Name} } } logger.Info("Added routes for cluster", "cluster", req.Name, "routes", cls.routes) @@ -144,10 +158,11 @@ func (pm *ProxyManager) ReverseProxy() *httputil.ReverseProxy { } } +// RoundTrip executes the rewritten request and uses the transport created when reconciling the cluster with respective credentials func (pm *ProxyManager) RoundTrip(req *http.Request) (resp *http.Response, err error) { pm.mu.RLock() defer pm.mu.RUnlock() - cluster, ok := req.Context().Value(contextClusterKey{}).(string) + cluster, ok := req.Context().Value(ContextClusterKey{}).(string) if !ok { return nil, fmt.Errorf("no upstream found for: %s", req.URL.String()) @@ -161,6 +176,7 @@ func (pm *ProxyManager) RoundTrip(req *http.Request) (resp *http.Response, err e 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() @@ -171,26 +187,24 @@ func (pm *ProxyManager) rewrite(req *httputil.ProxyRequest) { req.Out = req.Out.WithContext(log.IntoContext(req.Out.Context(), l)) }() - // hostname is expected to have the format $name--$cluster--$namespace.$organisation.$basedomain + // hostname is expected to have the format specified in ./pkg/common/url.go cluster, err := common.ExtractCluster(req.In.Host) if err != nil { return } - l = l.WithValues("cluster", cluster) - pm.mu.RLock() - defer pm.mu.RUnlock() - cls, found := pm.clusters[cluster] - if !found { - return - } - backendURL, found := cls.routes["https://"+req.In.Host] - if !found { + 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()) return } + backendURL := route.url // set cluster in context - req.Out = req.Out.WithContext(context.WithValue(req.Out.Context(), contextClusterKey{}, cluster)) + ctx := context.WithValue(req.Out.Context(), ContextClusterKey{}, cluster) + + l.WithValues("cluster", cluster, "namespace", route.namespace, "name", route.serviceName) + req.Out = req.Out.WithContext(ctx) req.SetURL(backendURL) } @@ -232,6 +246,21 @@ func (pm *ProxyManager) pluginsForCluster(ctx context.Context, cluster, namespac return configs, nil } +// GetClusterRoute returns the route information for a given cluster and incoming URL +func (pm *ProxyManager) GetClusterRoute(cluster, inURL string) (*route, bool) { + pm.mu.RLock() + defer pm.mu.RUnlock() + cls, ok := pm.clusters[cluster] + if !ok { + return nil, false + } + getRoute, ok := cls.routes[inURL] + if !ok { + return nil, false + } + return &getRoute, true +} + func enqueuePluginForCluster(_ context.Context, o client.Object) []ctrl.Request { plugin, ok := o.(*greenhousev1alpha1.Plugin) if !ok { diff --git a/cmd/service-proxy/proxy_test.go b/cmd/service-proxy/proxy_test.go index 7906fc5ba..3c60d1935 100644 --- a/cmd/service-proxy/proxy_test.go +++ b/cmd/service-proxy/proxy_test.go @@ -60,8 +60,12 @@ func TestRewrite(t *testing.T) { } pm := NewProxyManager() pm.clusters["cluster"] = clusterRoutes{ - routes: map[string]*url.URL{ - inputURL.Scheme + "://" + inputURL.Host: proxyURL, + routes: map[string]route{ + inputURL.Scheme + "://" + inputURL.Host: { + url: proxyURL, + namespace: "namespace", + serviceName: "test", + }, }, } r, err := http.NewRequestWithContext(context.Background(), http.MethodGet, inputURL.String(), http.NoBody) @@ -81,8 +85,8 @@ func TestRewrite(t *testing.T) { if req.Out.URL.String() != tt.expectedURL { t.Errorf("expected url %s, got %s", tt.expectedURL, req.Out.URL.String()) } - if req.Out.Context().Value(contextClusterKey{}) != tt.contextVal { - t.Errorf("expected cluster %s in context, got %s", "cluster", req.Out.Context().Value(contextClusterKey{})) + if req.Out.Context().Value(ContextClusterKey{}) != tt.contextVal { + t.Errorf("expected cluster %s in context, got %s", "cluster", req.Out.Context().Value(ContextClusterKey{})) } }) } @@ -139,7 +143,7 @@ users: }, }).Build() pm.clusters["cluster"] = clusterRoutes{ - routes: map[string]*url.URL{}, + routes: map[string]route{}, } ctx := context.Background() _, err := pm.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "cluster", Namespace: "namespace"}}) @@ -148,10 +152,11 @@ users: t.Errorf("expected no error, got: %s", err) } - targetURL, ok := pm.clusters["cluster"].routes["https://service--namespace--cluster.org.basedomain"] + route, ok := pm.clusters["cluster"].routes["https://service--namespace--cluster.org.basedomain"] if !ok { t.Fatal("expected route to be added") } + targetURL := route.url expectedURL := fmt.Sprintf("%s/api/v1/namespaces/%s/services/%s:%s:%s/proxy", "https://apiserver.test", "namespace", "http", "test", "8080") if targetURL.String() != expectedURL { t.Errorf("expected url %s, got %s", expectedURL, targetURL.String()) diff --git a/docs/user-guides/plugin/plugin-deployment.md b/docs/user-guides/plugin/plugin-deployment.md index 00bd2f210..d89573037 100644 --- a/docs/user-guides/plugin/plugin-deployment.md +++ b/docs/user-guides/plugin/plugin-deployment.md @@ -53,5 +53,5 @@ kubectl --namespace= create -f plugin.yaml After deploying the plugin to a remote cluster, ExposedServices section in Plugin's status provides an overview of the Plugins services that are centrally exposed. It maps the exposed URL to the service found in the manifest. -- The URLs for exposed services are created in the following pattern: `$https://$service--$cluster--$namespace.$organisation.$basedomain`. If `$service--$cluster--$namespace` exceeds 63 characters it is capped at 54 characters and a hash is appended.s +- The URLs for exposed services are created in the following pattern: `$https://$cluster--$hash.$organisation.$basedomain`. The `$hash` is computed from `service--$namespace`. - When deploying a plugin to the central cluster, the exposed services won't have their URLs defined, which will be reflected in the Plugin's Status. From 1feee222768ce0c0cc7416b2bd59fd77172f3e44 Mon Sep 17 00:00:00 2001 From: Uwe Mayer Date: Fri, 1 Nov 2024 12:51:28 +0100 Subject: [PATCH 3/8] fix(service-proxy): Do not expose internal vars Signed-off-by: Uwe Mayer --- cmd/service-proxy/instrumentation.go | 12 ++++++------ cmd/service-proxy/proxy.go | 12 ++++++------ cmd/service-proxy/proxy_test.go | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/cmd/service-proxy/instrumentation.go b/cmd/service-proxy/instrumentation.go index f37a7f565..2c1ee2ec8 100644 --- a/cmd/service-proxy/instrumentation.go +++ b/cmd/service-proxy/instrumentation.go @@ -15,17 +15,17 @@ import ( var ( clusterFromContext = promhttp.WithLabelFromCtx("cluster", func(ctx context.Context) string { - cluster, _ := ctx.Value(ContextClusterKey{}).(string) //nolint:errcheck + cluster, _ := ctx.Value(contextClusterKey{}).(string) //nolint:errcheck return cluster }) namespaceFromContext = promhttp.WithLabelFromCtx("namespace", func(ctx context.Context) string { - namespace, _ := ctx.Value(ContextNamespaceKey{}).(string) //nolint:errcheck + namespace, _ := ctx.Value(contextNamespaceKey{}).(string) //nolint:errcheck return namespace }) nameFromContext = promhttp.WithLabelFromCtx("name", func(ctx context.Context) string { - name, _ := ctx.Value(ContextNameKey{}).(string) //nolint:errcheck + name, _ := ctx.Value(contextNameKey{}).(string) //nolint:errcheck return name }) ) @@ -67,11 +67,11 @@ func InstrumentHandler(pm *ProxyManager, registry prometheus.Registerer) http.Ha pm.mu.Lock() defer pm.mu.Unlock() ctx := req.Context() - ctx = context.WithValue(ctx, ContextClusterKey{}, cluster) + ctx = context.WithValue(ctx, contextClusterKey{}, cluster) route, found := pm.GetClusterRoute(cluster, req.Host) if found { - ctx = context.WithValue(ctx, ContextNamespaceKey{}, route.namespace) - ctx = context.WithValue(ctx, ContextNameKey{}, route.serviceName) + ctx = context.WithValue(ctx, contextNamespaceKey{}, route.namespace) + ctx = context.WithValue(ctx, contextNameKey{}, route.serviceName) } req = req.WithContext(ctx) } diff --git a/cmd/service-proxy/proxy.go b/cmd/service-proxy/proxy.go index 09a43d911..7b7bcec45 100644 --- a/cmd/service-proxy/proxy.go +++ b/cmd/service-proxy/proxy.go @@ -54,16 +54,16 @@ type route struct { namespace string } -// ContextClusterKey is used to embed a cluster in the context -type ContextClusterKey struct { +// contextClusterKey is used to embed a cluster in the context +type contextClusterKey struct { } // contextNamespaceKey is used to embed a namespace in the context -type ContextNamespaceKey struct { +type contextNamespaceKey struct { } // contextNameKey is used to embed a name in the context -type ContextNameKey struct { +type contextNameKey struct { } var apiServerProxyPathRegex = regexp.MustCompile(`/api/v1/namespaces/[^/]+/services/[^/]+/proxy/`) @@ -162,7 +162,7 @@ func (pm *ProxyManager) ReverseProxy() *httputil.ReverseProxy { func (pm *ProxyManager) RoundTrip(req *http.Request) (resp *http.Response, err error) { pm.mu.RLock() defer pm.mu.RUnlock() - cluster, ok := req.Context().Value(ContextClusterKey{}).(string) + cluster, ok := req.Context().Value(contextClusterKey{}).(string) if !ok { return nil, fmt.Errorf("no upstream found for: %s", req.URL.String()) @@ -200,7 +200,7 @@ func (pm *ProxyManager) rewrite(req *httputil.ProxyRequest) { } backendURL := route.url // set cluster in context - ctx := context.WithValue(req.Out.Context(), ContextClusterKey{}, cluster) + ctx := context.WithValue(req.Out.Context(), contextClusterKey{}, cluster) l.WithValues("cluster", cluster, "namespace", route.namespace, "name", route.serviceName) diff --git a/cmd/service-proxy/proxy_test.go b/cmd/service-proxy/proxy_test.go index 3c60d1935..96ff5ff5c 100644 --- a/cmd/service-proxy/proxy_test.go +++ b/cmd/service-proxy/proxy_test.go @@ -85,8 +85,8 @@ func TestRewrite(t *testing.T) { if req.Out.URL.String() != tt.expectedURL { t.Errorf("expected url %s, got %s", tt.expectedURL, req.Out.URL.String()) } - if req.Out.Context().Value(ContextClusterKey{}) != tt.contextVal { - t.Errorf("expected cluster %s in context, got %s", "cluster", req.Out.Context().Value(ContextClusterKey{})) + if req.Out.Context().Value(contextClusterKey{}) != tt.contextVal { + t.Errorf("expected cluster %s in context, got %s", "cluster", req.Out.Context().Value(contextClusterKey{})) } }) } From fc2afe21feeccb58c1ed230cf019df129376eb7b Mon Sep 17 00:00:00 2001 From: Uwe Mayer Date: Fri, 1 Nov 2024 13:19:40 +0100 Subject: [PATCH 4/8] Add some remarks and use coherent naming in tests Signed-off-by: Uwe Mayer --- cmd/service-proxy/proxy.go | 15 +++++++++++++++ cmd/service-proxy/proxy_test.go | 13 +++++-------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/cmd/service-proxy/proxy.go b/cmd/service-proxy/proxy.go index 7b7bcec45..b663607f0 100644 --- a/cmd/service-proxy/proxy.go +++ b/cmd/service-proxy/proxy.go @@ -1,6 +1,21 @@ // SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Greenhouse contributors // SPDX-License-Identifier: Apache-2.0 +// The ProxyManager struct is used to manage the reverse proxy and the cluster routes. +// The Reconcile method is used to add or update the cluster routes. +// When reconciling cluster routes from a Plugin with an exposed service, the ProxyManager persists the transport and URL necessary to proxy an incoming request in the clusters map. +// The transport is created using the credentials from the Secret associated with the cluster. +// The URL is created using the k8s API server proxy: https://kubernetes.io/docs/tasks/access-application-cluster/access-cluster-services/#discovering-builtin-services +// Entries are saved by cluster and exposed URL. E.g., if a Plugin exposes a service with the URL "https://cluster1--1234567.example.com" on cluster-1, the route is saved in +// clusters["cluster-1"]clusterRoutes{ +// transport: net/http.RoundTripper{$TransportCreatedFromClusterKubeConfig}, +// routes: map[string]route ["https://cluster-1--1234567.organisation.basedomain"]route{ +// url: *net/url.URL {$BackenURL}, +// serviceName: $serviceName, +// namespace: $serviceNamespace +// } +// }. + package main import ( diff --git a/cmd/service-proxy/proxy_test.go b/cmd/service-proxy/proxy_test.go index 96ff5ff5c..31eddd352 100644 --- a/cmd/service-proxy/proxy_test.go +++ b/cmd/service-proxy/proxy_test.go @@ -105,11 +105,11 @@ func TestReconcile(t *testing.T) { Namespace: "namespace", }, Spec: greenhousev1alpha1.PluginSpec{ - ClusterName: "cluster", + ClusterName: "cluster-1", }, Status: greenhousev1alpha1.PluginStatus{ ExposedServices: map[string]greenhousev1alpha1.Service{ - "https://service--namespace--cluster.org.basedomain": { + "https://cluster-1--1234567.org.basedomain": { Namespace: "namespace", Name: "test", Port: 8080, @@ -119,7 +119,7 @@ func TestReconcile(t *testing.T) { }, &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", + Name: "cluster-1", Namespace: "namespace", }, Type: "greenhouse.sap/kubeconfig", @@ -142,17 +142,14 @@ users: `), }, }).Build() - pm.clusters["cluster"] = clusterRoutes{ - routes: map[string]route{}, - } ctx := context.Background() - _, err := pm.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "cluster", Namespace: "namespace"}}) + _, err := pm.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "cluster-1", Namespace: "namespace"}}) if err != nil { t.Errorf("expected no error, got: %s", err) } - route, ok := pm.clusters["cluster"].routes["https://service--namespace--cluster.org.basedomain"] + route, ok := pm.clusters["cluster-1"].routes["https://cluster-1--1234567.org.basedomain"] if !ok { t.Fatal("expected route to be added") } From 0a2f8eb24a393d12051c54942ffb6df65d157c3d Mon Sep 17 00:00:00 2001 From: Uwe Mayer Date: Fri, 1 Nov 2024 13:25:38 +0100 Subject: [PATCH 5/8] remove redundant warning Signed-off-by: Uwe Mayer --- pkg/admission/cluster_webhook.go | 8 ++++---- pkg/admission/utils.go | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/admission/cluster_webhook.go b/pkg/admission/cluster_webhook.go index 667bcd45a..be57e124c 100644 --- a/pkg/admission/cluster_webhook.go +++ b/pkg/admission/cluster_webhook.go @@ -88,12 +88,12 @@ func ValidateCreateCluster(ctx context.Context, _ client.Client, obj runtime.Obj if !ok { return nil, nil } - if warning, err := invalidateDoubleDashesInName(cluster, logger); err != nil { - return warning, err + if err := invalidateDoubleDashesInName(cluster, logger); err != nil { + return nil, err } // capping the name at 40 chars, so we ensure to get unique urls for exposed services per cluster. service-name/namespace hash needs to fit (max 63 chars) - if warning, err := capName(cluster, logger, 40); err != nil { - return warning, err + if err := capName(cluster, logger, 40); err != nil { + return nil, err } annotations := cluster.GetAnnotations() _, deletionMarked := annotations[apis.MarkClusterDeletionAnnotation] diff --git a/pkg/admission/utils.go b/pkg/admission/utils.go index c1cf5ac7f..2af7961c3 100644 --- a/pkg/admission/utils.go +++ b/pkg/admission/utils.go @@ -134,7 +134,7 @@ func logAdmissionRequest(ctx context.Context) { } // invalidateDoubleDashes validates that the object name does not contain double dashes. -func invalidateDoubleDashesInName(obj client.Object, l logr.Logger) (admission.Warnings, error) { +func invalidateDoubleDashesInName(obj client.Object, l logr.Logger) error { if strings.Contains(obj.GetName(), "--") { err := apierrors.NewInvalid( obj.GetObjectKind().GroupVersionKind().GroupKind(), @@ -144,14 +144,14 @@ func invalidateDoubleDashesInName(obj client.Object, l logr.Logger) (admission.W }, ) l.Error(err, "found object name with double dashes, admission will be denied") - return admission.Warnings{"you cannot create an object with double dashes in the name"}, err + return err } - return nil, nil + return nil } // capName validates that the name is not longer than the provided length. -func capName(obj client.Object, l logr.Logger, length int) (admission.Warnings, error) { +func capName(obj client.Object, l logr.Logger, length int) error { if len(obj.GetName()) > length { err := apierrors.NewInvalid( obj.GetObjectKind().GroupVersionKind().GroupKind(), @@ -161,7 +161,7 @@ func capName(obj client.Object, l logr.Logger, length int) (admission.Warnings, }, ) l.Error(err, fmt.Sprintf("found object name too long, admission will be denied, name must be less than or equal to %d", length)) - return admission.Warnings{fmt.Sprintf("you cannot create an object with a name longer than %d", length)}, err + return err } - return nil, nil + return nil } From c58d4a3eb300142803e4dfe4356c994cccced9be Mon Sep 17 00:00:00 2001 From: Uwe Mayer Date: Fri, 1 Nov 2024 13:25:51 +0100 Subject: [PATCH 6/8] Fix tests Signed-off-by: Uwe Mayer --- pkg/common/url.go | 11 +++++-- pkg/common/url_test.go | 29 ++++++++++++------- pkg/controllers/plugin/remote_cluster_test.go | 4 +-- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/pkg/common/url.go b/pkg/common/url.go index c005f72d6..8f7769db0 100644 --- a/pkg/common/url.go +++ b/pkg/common/url.go @@ -5,6 +5,7 @@ package common import ( "crypto/sha256" + "encoding/hex" "fmt" "strings" @@ -15,11 +16,12 @@ import ( var DNSDomain string // URLForExposedServiceInPlugin returns the URL that shall be used to expose a service centrally via Greenhouse. -// The pattern shall be $https://$cluster--$hash.$organisation.$basedomain, where $hash = service--$namespace +// The pattern shall be $https://$cluster--$hash.$organisation.$basedomain, where $hash = $service--$namespace // We know $cluster is no longer than 40 characters and does not contain "--" func URLForExposedServiceInPlugin(serviceName string, plugin *greenhousev1alpha1.Plugin) string { hash := sha256.Sum256([]byte(fmt.Sprintf("%s--%s", serviceName, plugin.Spec.ReleaseNamespace))) - subdomain := fmt.Sprintf("%s--%s", plugin.Spec.ClusterName, hash[:8]) + hashString := hex.EncodeToString(hash[:]) + subdomain := fmt.Sprintf("%s--%s", plugin.Spec.ClusterName, hashString[:7]) return fmt.Sprintf( "https://%s.%s.%s", subdomain, plugin.GetNamespace(), DNSDomain, @@ -27,8 +29,11 @@ func URLForExposedServiceInPlugin(serviceName string, plugin *greenhousev1alpha1 } // ExtractCluster extracts the cluster name from the host. -// The pattern shall be $https://$cluster--$hash, where $hash = service--$namespace +// The pattern shall be $cluster--$hash, where $hash = service--$namespace func ExtractCluster(host string) (cluster string, err error) { + if strings.HasPrefix(host, "https://") { + return "", fmt.Errorf("invalid host: %s, no protocol expected", host) + } parts := strings.SplitN(host, ".", 2) if len(parts) < 2 { return "", fmt.Errorf("invalid host: %s", host) diff --git a/pkg/common/url_test.go b/pkg/common/url_test.go index b32b3fe68..07db04b31 100644 --- a/pkg/common/url_test.go +++ b/pkg/common/url_test.go @@ -30,20 +30,27 @@ var _ = Describe("validate url methods", Ordered, func() { plugin.SetNamespace("test-organisation") url := common.URLForExposedServiceInPlugin("test-service", plugin) - Expect(url).To(Equal("https://test-service--test-cluster--test-namespace.test-organisation.example.com")) + Expect(url).To(Equal("https://test-cluster--e30cc9f.test-organisation.example.com")) }) - It("should correctly cap the url with a hash on urls with subdomains exceeding 63 characters", func() { - common.DNSDomain = "example.com" - plugin := &v1alpha1.Plugin{ - Spec: v1alpha1.PluginSpec{ - ReleaseNamespace: "test-long-namespace", - ClusterName: "test-cluster", - }, + It("should correctly extract the cluster from an host", func() { + cluster, err := common.ExtractCluster("test-cluster--e30cc9f.test-organisation.example.com") + + Expect(err).ToNot(HaveOccurred()) + Expect(cluster).To(Equal("test-cluster")) + }) + + It("should return an error if the host is invalid", func() { + invalidHosts := []string{ + "https://test-cluster-e30cc9f.test-organisation.example.com", + "test-cluster-e30cc9f.example.com", + "test-cluster--e30cc9f", } - plugin.SetNamespace("test-organisation") - url := common.URLForExposedServiceInPlugin("this-is-a-very-long-service-name", plugin) - Expect(url).To(Equal("https://this-is-a-very-long-service-name--test-cluster--test-l-7982a2e3.test-organisation.example.com")) + for _, host := range invalidHosts { + _, err := common.ExtractCluster(host) + Expect(err).To(HaveOccurred(), "Expected error for host: %s", host) + } }) + }) diff --git a/pkg/controllers/plugin/remote_cluster_test.go b/pkg/controllers/plugin/remote_cluster_test.go index 0d2d0fd53..d5211c283 100644 --- a/pkg/controllers/plugin/remote_cluster_test.go +++ b/pkg/controllers/plugin/remote_cluster_test.go @@ -511,8 +511,8 @@ var _ = Describe("HelmController reconciliation", Ordered, func() { for exposedServiceURL = range testPluginWithExposedService1.Status.ExposedServices { break } - // URL pattern: $https://$service--$cluster--$namespace.$organisation.$basedomain - g.Expect(exposedServiceURL).To(Equal("https://exposed-service--test-cluster--test-org.test-org.example.com"), "exposed service URL should be generated correctly") + expectedURL := common.URLForExposedServiceInPlugin("exposed-service", testPluginWithExposedService) + g.Expect(exposedServiceURL).To(Equal(expectedURL), "exposed service URL should be generated correctly") }).Should(Succeed(), "plugin should have correct status") By("cleaning up test") From cd9a82a29b70d2ca86bc2818e05ac89fe1bc01f7 Mon Sep 17 00:00:00 2001 From: Uwe Mayer Date: Mon, 4 Nov 2024 15:10:16 +0100 Subject: [PATCH 7/8] Fix nil pointer Signed-off-by: Uwe Mayer --- cmd/service-proxy/instrumentation.go | 2 -- cmd/service-proxy/proxy.go | 5 ++++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cmd/service-proxy/instrumentation.go b/cmd/service-proxy/instrumentation.go index 2c1ee2ec8..04e943faa 100644 --- a/cmd/service-proxy/instrumentation.go +++ b/cmd/service-proxy/instrumentation.go @@ -64,8 +64,6 @@ func InstrumentHandler(pm *ProxyManager, registry prometheus.Registerer) http.Ha injector := func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { if cluster, err := common.ExtractCluster(req.Host); err == nil { - pm.mu.Lock() - defer pm.mu.Unlock() ctx := req.Context() ctx = context.WithValue(ctx, contextClusterKey{}, cluster) route, found := pm.GetClusterRoute(cluster, req.Host) diff --git a/cmd/service-proxy/proxy.go b/cmd/service-proxy/proxy.go index b663607f0..964ea6d06 100644 --- a/cmd/service-proxy/proxy.go +++ b/cmd/service-proxy/proxy.go @@ -187,7 +187,10 @@ func (pm *ProxyManager) RoundTrip(req *http.Request) (resp *http.Response, err e return nil, fmt.Errorf("cluster %s not found", cluster) } resp, err = cls.transport.RoundTrip(req) - log.FromContext(req.Context()).Info("Forwarded request", "status", resp.StatusCode, "upstream", req.URL.String()) + // errors are logged by pm.Errorhandler + if err == nil { + log.FromContext(req.Context()).Info("Forwarded request", "status", resp.StatusCode, "upstream", req.URL.String()) + } return } From 7fee7a63140bbb34d74823f1e00917598d88ef6d Mon Sep 17 00:00:00 2001 From: Uwe Mayer Date: Wed, 6 Nov 2024 13:12:13 +0100 Subject: [PATCH 8/8] PR remarks Signed-off-by: Uwe Mayer --- cmd/service-proxy/instrumentation.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/service-proxy/instrumentation.go b/cmd/service-proxy/instrumentation.go index 04e943faa..3f8cbfd08 100644 --- a/cmd/service-proxy/instrumentation.go +++ b/cmd/service-proxy/instrumentation.go @@ -31,7 +31,6 @@ var ( ) func InstrumentHandler(pm *ProxyManager, registry prometheus.Registerer) http.Handler { - next := pm.ReverseProxy() requestCounter := prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "http_requests_total", @@ -81,7 +80,7 @@ func InstrumentHandler(pm *ProxyManager, registry prometheus.Registerer) http.Ha promhttp.InstrumentHandlerCounter(requestCounter, promhttp.InstrumentHandlerDuration(requestDuration, promhttp.InstrumentHandlerResponseSize(responseSizeHistogram, - next, + pm.ReverseProxy(), clusterFromContext, namespaceFromContext, nameFromContext, ), clusterFromContext, namespaceFromContext, nameFromContext,