From e33ec746a163507ee3ed4f50d0b16c0758256fbb Mon Sep 17 00:00:00 2001 From: Rizwana Naaz Date: Wed, 14 Feb 2024 19:06:22 +0530 Subject: [PATCH] Fix webhook route fail in argocd due to long route name (#1129) * Fix webhook route failure when hostname is too long --------- Signed-off-by: Rizwana777 --- controllers/argocd/ingress.go | 7 ++- controllers/argocd/route.go | 71 ++++++++++++++++++++++++++ controllers/argocd/route_test.go | 85 ++++++++++++++++++++++++++++++++ controllers/argocd/util.go | 10 ++-- 4 files changed, 169 insertions(+), 4 deletions(-) diff --git a/controllers/argocd/ingress.go b/controllers/argocd/ingress.go index 4092724e4..0a1943052 100644 --- a/controllers/argocd/ingress.go +++ b/controllers/argocd/ingress.go @@ -363,10 +363,15 @@ func (r *ReconcileArgoCD) reconcileApplicationSetControllerIngress(cr *argoproj. ingress.ObjectMeta.Annotations = atns pathType := networkingv1.PathTypeImplementationSpecific + httpServerHost, err := getApplicationSetHTTPServerHost(cr) + if err != nil { + return err + } + // Add rules ingress.Spec.Rules = []networkingv1.IngressRule{ { - Host: getApplicationSetHTTPServerHost(cr), + Host: httpServerHost, IngressRuleValue: networkingv1.IngressRuleValue{ HTTP: &networkingv1.HTTPIngressRuleValue{ Paths: []networkingv1.HTTPIngressPath{ diff --git a/controllers/argocd/route.go b/controllers/argocd/route.go index c659d9995..57db8b0ae 100644 --- a/controllers/argocd/route.go +++ b/controllers/argocd/route.go @@ -17,6 +17,7 @@ package argocd import ( "context" "fmt" + "strings" routev1 "github.com/openshift/api/route/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -28,6 +29,12 @@ import ( "github.com/argoproj-labs/argocd-operator/controllers/argoutil" ) +const ( + maxLabelLength = 63 + maxHostnameLength = 253 + minFirstLabelSize = 20 +) + var routeAPIFound = false // IsRouteAPIAvailable returns true if the Route API is present. @@ -207,6 +214,13 @@ func (r *ReconcileArgoCD) reconcileServerRoute(cr *argoproj.ArgoCD) error { route.Spec.Host = cr.Spec.Server.Host // TODO: What additional role needed for this? } + hostname, err := shortenHostname(route.Spec.Host) + if err != nil { + return err + } + + route.Spec.Host = hostname + if cr.Spec.Server.Insecure { // Disable TLS and rely on the cluster certificate. route.Spec.Port = &routev1.RoutePort{ @@ -284,6 +298,13 @@ func (r *ReconcileArgoCD) reconcileApplicationSetControllerWebhookRoute(cr *argo route.Spec.Host = cr.Spec.ApplicationSet.WebhookServer.Host } + hostname, err := shortenHostname(route.Spec.Host) + if err != nil { + return err + } + + route.Spec.Host = hostname + route.Spec.Port = &routev1.RoutePort{ TargetPort: intstr.FromString("webhook"), } @@ -351,3 +372,53 @@ func (r *ReconcileArgoCD) reconcileApplicationSetControllerWebhookRoute(cr *argo } return r.Client.Update(context.TODO(), route) } + +// The algorithm used by this function is: +// - If the FIRST label ("console-openshift-console" in the above case) is longer than 63 characters, shorten (truncate the end) it to 63. +// - If any other label is longer than 63 characters, return an error +// - After all the labels are 63 characters or less, check the length of the overall hostname: +// - If the overall hostname is > 253, then shorten the FIRST label until the host name is < 253 +// - After the FIRST label has been shortened, if it is < 20, then return an error (this is a sanity test to ensure the label is likely to be unique) +func shortenHostname(hostname string) (string, error) { + if hostname == "" { + return "", nil + } + + // Return the hostname as it is if hostname is already within the size limit + if len(hostname) <= maxHostnameLength { + return hostname, nil + } + + // Split the hostname into labels + labels := strings.Split(hostname, ".") + + // Check and truncate the FIRST label if longer than 63 characters + if len(labels[0]) > maxLabelLength { + labels[0] = labels[0][:maxLabelLength] + } + + // Check other labels and return an error if any is longer than 63 characters + for _, label := range labels[1:] { + if len(label) > maxLabelLength { + return "", fmt.Errorf("label length exceeds 63 characters") + } + } + + // Join the labels back into a hostname + resultHostname := strings.Join(labels, ".") + + // Check and shorten the overall hostname + if len(resultHostname) > maxHostnameLength { + // Shorten the first label until the length is less than 253 + for len(resultHostname) > maxHostnameLength && len(labels[0]) > 20 { + labels[0] = labels[0][:len(labels[0])-1] + resultHostname = strings.Join(labels, ".") + } + + // Check if the first label is still less than 20 characters + if len(labels[0]) < minFirstLabelSize { + return "", fmt.Errorf("shortened first label is less than 20 characters") + } + } + return resultHostname, nil +} diff --git a/controllers/argocd/route_test.go b/controllers/argocd/route_test.go index fbd8f24e6..af83bc166 100644 --- a/controllers/argocd/route_test.go +++ b/controllers/argocd/route_test.go @@ -3,6 +3,7 @@ package argocd import ( "context" "fmt" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -405,6 +406,90 @@ func TestReconcileRouteApplicationSetTls(t *testing.T) { } } +func TestReconcileRouteForShorteningHostname(t *testing.T) { + routeAPIFound = true + ctx := context.Background() + logf.SetLogger(ZapLogger(true)) + + tests := []struct { + testName string + expected string + hostname string + }{ + { + testName: "longHostname", + hostname: "myhostnameaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.redhat.com", + expected: "myhostnameaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.redhat.com", + }, + { + testName: "twentySixLetterHostname", + hostname: "myhostnametwentysixletteraaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.redhat.com", + expected: "myhostnametwentysixletteraaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.redhat.com", + }, + } + + for _, v := range tests { + t.Run(v.testName, func(t *testing.T) { + + argoCD := makeArgoCD(func(a *argoproj.ArgoCD) { + a.Spec.Server.Route.Enabled = true + a.Spec.ApplicationSet = &argoproj.ArgoCDApplicationSet{ + WebhookServer: argoproj.WebhookServerSpec{ + Route: argoproj.ArgoCDRouteSpec{ + Enabled: true, + }, + Host: v.hostname, + }, + } + }) + + resObjs := []client.Object{argoCD} + subresObjs := []client.Object{argoCD} + runtimeObjs := []runtime.Object{} + sch := makeTestReconcilerScheme(argoproj.AddToScheme, configv1.Install, routev1.Install) + cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs) + r := makeTestReconciler(cl, sch) + + assert.NoError(t, createNamespace(r, argoCD.Namespace, "")) + + req := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: testArgoCDName, + Namespace: testNamespace, + }, + } + + // Check if it returns nil when hostname is empty + _, err := r.Reconcile(context.TODO(), req) + assert.NoError(t, err) + + // second reconciliation after changing the hostname. + err = r.Client.Get(ctx, req.NamespacedName, argoCD) + fatalIfError(t, err, "failed to load ArgoCD %q: %s", testArgoCDName+"-server", err) + + argoCD.Spec.Server.Host = v.hostname + err = r.Client.Update(ctx, argoCD) + fatalIfError(t, err, "failed to update the ArgoCD: %s", err) + + _, err = r.Reconcile(context.TODO(), req) + assert.NoError(t, err) + + loaded := &routev1.Route{} + err = r.Client.Get(ctx, types.NamespacedName{Name: testArgoCDName + "-server", Namespace: testNamespace}, loaded) + fatalIfError(t, err, "failed to load route %q: %s", testArgoCDName+"-server", err) + + if diff := cmp.Diff(v.expected, loaded.Spec.Host); diff != "" { + t.Fatalf("failed to reconcile route:\n%s", diff) + } + + // Check if first label is greater than 20 + labels := strings.Split(loaded.Spec.Host, ".") + assert.True(t, len(labels[0]) > 20) + + }) + } +} + func makeReconciler(t *testing.T, acd *argoproj.ArgoCD, objs ...runtime.Object) *ReconcileArgoCD { t.Helper() s := scheme.Scheme diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index b0c98bbdd..a9c34bfa1 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1574,10 +1574,14 @@ func contains(s []string, g string) bool { } // getApplicationSetHTTPServerHost will return the host for the given ArgoCD. -func getApplicationSetHTTPServerHost(cr *argoproj.ArgoCD) string { +func getApplicationSetHTTPServerHost(cr *argoproj.ArgoCD) (string, error) { host := cr.Name if len(cr.Spec.ApplicationSet.WebhookServer.Host) > 0 { - host = cr.Spec.ApplicationSet.WebhookServer.Host + hostname, err := shortenHostname(cr.Spec.ApplicationSet.WebhookServer.Host) + if err != nil { + return "", err + } + host = hostname } - return host + return host, nil }