diff --git a/cmd/service-proxy/instrumentation.go b/cmd/service-proxy/instrumentation.go index 5a6092daa..3f8cbfd08 100644 --- a/cmd/service-proxy/instrumentation.go +++ b/cmd/service-proxy/instrumentation.go @@ -13,31 +13,24 @@ 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 { requestCounter := prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "http_requests_total", @@ -69,11 +62,14 @@ 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) + 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) @@ -84,7 +80,7 @@ func InstrumentHandler(next http.Handler, registry prometheus.Registerer) http.H promhttp.InstrumentHandlerCounter(requestCounter, promhttp.InstrumentHandlerDuration(requestDuration, promhttp.InstrumentHandlerResponseSize(responseSizeHistogram, - next, + pm.ReverseProxy(), clusterFromContext, namespaceFromContext, nameFromContext, ), clusterFromContext, namespaceFromContext, nameFromContext, 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 ad78607b4..964ea6d06 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 ( @@ -36,22 +51,36 @@ 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 { } +// 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/`) func (pm *ProxyManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -90,26 +119,26 @@ 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 { 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 { 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,6 +173,7 @@ 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() @@ -157,10 +187,14 @@ 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 } +// 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 +205,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 - name, cluster, namespace, err := common.SplitHost(req.In.Host) + // 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, "namespace", namespace, "name", name) - 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 +264,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 e761eb415..31eddd352 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", }, @@ -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) @@ -101,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, @@ -115,7 +119,7 @@ func TestReconcile(t *testing.T) { }, &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", + Name: "cluster-1", Namespace: "namespace", }, Type: "greenhouse.sap/kubeconfig", @@ -138,20 +142,18 @@ users: `), }, }).Build() - pm.clusters["cluster"] = clusterRoutes{ - routes: map[string]*url.URL{}, - } 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) } - targetURL, 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") } + 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. diff --git a/pkg/admission/cluster_webhook.go b/pkg/admission/cluster_webhook.go index b3002dbc1..be57e124c 100644 --- a/pkg/admission/cluster_webhook.go +++ b/pkg/admission/cluster_webhook.go @@ -88,6 +88,13 @@ func ValidateCreateCluster(ctx context.Context, _ client.Client, obj runtime.Obj if !ok { return nil, nil } + 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 err := capName(cluster, logger, 40); err != nil { + return nil, 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..2af7961c3 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) 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 err + } + return nil +} + +// capName validates that the name is not longer than the provided length. + +func capName(obj client.Object, l logr.Logger, length int) 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 err + } + return nil +} diff --git a/pkg/common/url.go b/pkg/common/url.go index 9d12ead0a..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,32 +16,31 @@ 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))) + 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, ) } -// 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 $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) + 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 } 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")