fix(service-proxy): well defined urls for exposed services with max 63

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)

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()

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",
 		},

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]

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
+}

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
 }