fix (bug): Host name error for ApplicationSet Webhook Server route (argoproj-labs#1730)

* fix Host name error for ApplicationSet Webhook Server route

Signed-off-by: Atif Ali <atali@redhat.com>

* ensure route name and its label value is always ≤ 63 characters

Signed-off-by: Atif Ali <atali@redhat.com>

* added tests

Signed-off-by: Atif Ali <atali@redhat.com>

* resolve linter error

Signed-off-by: Atif Ali <atali@redhat.com>

* fix test failure

Signed-off-by: Atif Ali <atali@redhat.com>

* define a constant for appset webhook in commons

Signed-off-by: Atif Ali <atali@redhat.com>

* fix linter

Signed-off-by: Atif Ali <atali@redhat.com>

---------

Signed-off-by: Atif Ali <atali@redhat.com>

Author: aali309

common/values.go

@@ -86,6 +86,9 @@ const (
 	//ApplicationSetServiceNameSuffix is the suffix for Apllication Set Controller Service
 	ApplicationSetServiceNameSuffix = "applicationset-controller"
 
+	// ApplicationSetControllerWebhookSuffix is the suffix for ApplicationSet Webhook
+	ApplicationSetControllerWebhookSuffix = "appset-webhook"
+
 	// ArgoCDAggregateToControllerLabelKey is label to configure base aggregated ClusterRole for Argo CD Application Controller.
 	ArgoCDAggregateToControllerLabelKey = "argocd/aggregate-to-controller"
 

controllers/argocd/route.go

@@ -25,6 +25,9 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	configv1 "github.com/openshift/api/config/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
 	argoproj "github.com/argoproj-labs/argocd-operator/api/v1beta1"
 	"github.com/argoproj-labs/argocd-operator/common"
 	"github.com/argoproj-labs/argocd-operator/controllers/argoutil"
@@ -325,8 +328,14 @@ func isCreatedByServiceCA(crName string, secret corev1.Secret) bool {
 
 // reconcileApplicationSetControllerWebhookRoute will ensure that the ArgoCD Server Route is present.
 func (r *ReconcileArgoCD) reconcileApplicationSetControllerWebhookRoute(cr *argoproj.ArgoCD) error {
-	name := fmt.Sprintf("%s-%s", common.ApplicationSetServiceNameSuffix, "webhook")
-	route := newRouteWithSuffix(name, cr)
+	// Generate a base name for the route
+	baseName := fmt.Sprintf("%s-%s", cr.Name, common.ApplicationSetControllerWebhookSuffix)
+	// Truncate to 63 characters if needed (Kubernetes label value limit)
+	if len(baseName) > maxLabelLength {
+		baseName = baseName[:maxLabelLength]
+	}
+	route := newRouteWithName(baseName, cr)
+
 	found := argoutil.IsObjectFound(r.Client, cr.Namespace, route.Name, route)
 	if found {
 		if cr.Spec.ApplicationSet == nil || !cr.Spec.ApplicationSet.WebhookServer.Route.Enabled {
@@ -361,15 +370,36 @@ func (r *ReconcileArgoCD) reconcileApplicationSetControllerWebhookRoute(cr *argo
 	}
 
 	// Allow override of the Host for the Route.
+	var host string
 	if len(cr.Spec.ApplicationSet.WebhookServer.Host) > 0 {
-		route.Spec.Host = cr.Spec.ApplicationSet.WebhookServer.Host
+		host = cr.Spec.ApplicationSet.WebhookServer.Host
+	} else {
+		// Generate the default host
+		baseHost := fmt.Sprintf("%s-%s-%s", cr.Name, common.ApplicationSetControllerWebhookSuffix, cr.Namespace)
+		ingressConfig := &configv1.Ingress{}
+		err := r.Get(context.TODO(), client.ObjectKey{Name: "cluster"}, ingressConfig)
+		if err != nil {
+			return err
+		}
+		appsDomain := ingressConfig.Spec.Domain
+		host = fmt.Sprintf("%s.%s", baseHost, appsDomain)
+	}
+
+	// Truncate the first label if needed
+	labels := strings.SplitN(host, ".", 2)
+	if len(labels[0]) > maxLabelLength {
+		labels[0] = labels[0][:maxLabelLength]
+	}
+	if len(labels) > 1 {
+		route.Spec.Host = fmt.Sprintf("%s.%s", labels[0], labels[1])
+	} else {
+		route.Spec.Host = labels[0]
 	}
 
 	hostname, err := shortenHostname(route.Spec.Host)
 	if err != nil {
 		return err
 	}
-
 	route.Spec.Host = hostname
 
 	route.Spec.Port = &routev1.RoutePort{

controllers/argocd/route_test.go

@@ -253,7 +253,7 @@ func TestReconcileRouteApplicationSetHost(t *testing.T) {
 	assert.NoError(t, err)
 
 	loaded := &routev1.Route{}
-	err = r.Client.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-%s-%s", testArgoCDName, common.ApplicationSetServiceNameSuffix, "webhook"), Namespace: testNamespace}, loaded)
+	err = r.Client.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-%s", testArgoCDName, common.ApplicationSetControllerWebhookSuffix), Namespace: testNamespace}, loaded)
 	fatalIfError(t, err, "failed to load route %q: %s", testArgoCDName+"-server", err)
 
 	wantTLSConfig := &routev1.TLSConfig{
@@ -309,7 +309,7 @@ func TestReconcileRouteApplicationSetTlsTermination(t *testing.T) {
 	assert.NoError(t, err)
 
 	loaded := &routev1.Route{}
-	err = r.Client.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-%s-%s", testArgoCDName, common.ApplicationSetServiceNameSuffix, "webhook"), Namespace: testNamespace}, loaded)
+	err = r.Client.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-%s", testArgoCDName, common.ApplicationSetControllerWebhookSuffix), Namespace: testNamespace}, loaded)
 	fatalIfError(t, err, "failed to load route %q: %s", testArgoCDName+"-server", err)
 
 	wantTLSConfig := &routev1.TLSConfig{
@@ -351,7 +351,17 @@ func TestReconcileRouteApplicationSetTls(t *testing.T) {
 		}
 	})
 
-	resObjs := []client.Object{argoCD}
+	// Create the Ingress configuration
+	ingressConfig := &configv1.Ingress{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "cluster",
+		},
+		Spec: configv1.IngressSpec{
+			Domain: "apps.example.com",
+		},
+	}
+
+	resObjs := []client.Object{argoCD, ingressConfig}
 	subresObjs := []client.Object{argoCD}
 	runtimeObjs := []runtime.Object{}
 	sch := makeTestReconcilerScheme(argoproj.AddToScheme, configv1.Install, routev1.Install)
@@ -370,10 +380,17 @@ func TestReconcileRouteApplicationSetTls(t *testing.T) {
 	_, err := r.Reconcile(context.TODO(), req)
 	assert.NoError(t, err)
 
+	// The route name should be based on the ArgoCD instance name
+	expectedRouteName := fmt.Sprintf("%s-%s", testArgoCDName, common.ApplicationSetControllerWebhookSuffix)
+	if len(expectedRouteName) > 63 {
+		expectedRouteName = expectedRouteName[:63]
+	}
+
 	loaded := &routev1.Route{}
-	err = r.Client.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-%s-%s", testArgoCDName, common.ApplicationSetServiceNameSuffix, "webhook"), Namespace: testNamespace}, loaded)
-	fatalIfError(t, err, "failed to load route %q: %s", testArgoCDName+"-server", err)
+	err = r.Client.Get(ctx, types.NamespacedName{Name: expectedRouteName, Namespace: testNamespace}, loaded)
+	fatalIfError(t, err, "failed to load route %q: %s", expectedRouteName, err)
 
+	// Verify TLS configuration
 	wantTLSConfig := &routev1.TLSConfig{
 		Termination:                   routev1.TLSTerminationEdge,
 		Certificate:                   "test-certificate",
@@ -383,28 +400,36 @@ func TestReconcileRouteApplicationSetTls(t *testing.T) {
 		InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
 	}
 	if diff := cmp.Diff(wantTLSConfig, loaded.Spec.TLS); diff != "" {
-		t.Fatalf("failed to reconcile route:\n%s", diff)
+		t.Fatalf("failed to reconcile route TLS config:\n%s", diff)
 	}
 
-	assert.Empty(t, loaded.Spec.Host)
+	// Verify hostname
+	expectedHost := fmt.Sprintf("%s-%s-%s.apps.example.com", testArgoCDName, common.ApplicationSetControllerWebhookSuffix, testNamespace)
+	if diff := cmp.Diff(expectedHost, loaded.Spec.Host); diff != "" {
+		t.Fatalf("failed to reconcile route hostname:\n%s", diff)
+	}
 
+	// Verify port configuration
 	wantPort := &routev1.RoutePort{
 		TargetPort: intstr.FromString("webhook"),
 	}
 	if diff := cmp.Diff(wantPort, loaded.Spec.Port); diff != "" {
-		t.Fatalf("failed to reconcile route:\n%s", diff)
+		t.Fatalf("failed to reconcile route port:\n%s", diff)
 	}
 
+	// Verify annotations
 	if diff := cmp.Diff("my-annotation-value", loaded.Annotations["my-annotation-key"]); diff != "" {
-		t.Fatalf("failed to reconcile route:\n%s", diff)
+		t.Fatalf("failed to reconcile route annotations:\n%s", diff)
 	}
 
+	// Verify labels
 	if diff := cmp.Diff("my-label-value", loaded.Labels["my-label-key"]); diff != "" {
-		t.Fatalf("failed to reconcile route:\n%s", diff)
+		t.Fatalf("failed to reconcile route labels:\n%s", diff)
 	}
 
+	// Verify wildcard policy
 	if diff := cmp.Diff(wildcardPolicy, loaded.Spec.WildcardPolicy); diff != "" {
-		t.Fatalf("failed to reconcile route:\n%s", diff)
+		t.Fatalf("failed to reconcile route wildcard policy:\n%s", diff)
 	}
 }
 
@@ -492,6 +517,65 @@ func TestReconcileRouteForShorteningHostname(t *testing.T) {
 	}
 }
 
+func TestReconcileRouteForShorteningRoutename(t *testing.T) {
+	routeAPIFound = true
+	ctx := context.Background()
+	logf.SetLogger(ZapLogger(true))
+
+	// Use a long ArgoCD instance name to force truncation
+	longName := "this-is-a-very-long-argocd-instance-name-that-will-break-the-route-name-limit"
+	argoCD := makeArgoCD(func(a *argoproj.ArgoCD) {
+		a.Name = longName
+		a.Spec.ApplicationSet = &argoproj.ArgoCDApplicationSet{
+			WebhookServer: argoproj.WebhookServerSpec{
+				Route: argoproj.ArgoCDRouteSpec{
+					Enabled: true,
+				},
+			},
+		}
+	})
+
+	// Add a fake Ingress resource to satisfy the domain lookup
+	ingressConfig := &configv1.Ingress{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "cluster",
+		},
+		Spec: configv1.IngressSpec{
+			Domain: "apps.example.com",
+		},
+	}
+
+	resObjs := []client.Object{argoCD, ingressConfig}
+	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:      longName,
+			Namespace: testNamespace,
+		},
+	}
+
+	_, err := r.Reconcile(context.TODO(), req)
+	assert.NoError(t, err)
+
+	// The route name should be truncated to 63 chars
+	expectedRouteName := longName + "-" + common.ApplicationSetControllerWebhookSuffix
+	if len(expectedRouteName) > 63 {
+		expectedRouteName = expectedRouteName[:63]
+	}
+
+	loaded := &routev1.Route{}
+	err = r.Client.Get(ctx, types.NamespacedName{Name: expectedRouteName, Namespace: testNamespace}, loaded)
+	assert.NoError(t, err)
+	assert.LessOrEqual(t, len(loaded.Name), 63)
+}
+
 func TestReconcileRouteTLSConfig(t *testing.T) {
 	routeAPIFound = true
 	ctx := context.Background()

tests/ocp/1-007_verify_hostname_with_route/01-assert.yaml

@@ -0,0 +1,8 @@
+apiVersion: route.openshift.io/v1
+kind: Route
+metadata:
+  name: "argocd-long-name-for-route-testiiiiiiiiiiiiiiiiiiiiiiiing-appset"
+spec: {}
+status:
+  ingress:
+  - host: argocd-long-name-for-route-testiiiiiiiiiiiiiiiiiiiiiiiing-appset.apps.testing.lab.upshift.rdu2.redhat.com

tests/ocp/1-007_verify_hostname_with_route/01-install.yaml

@@ -0,0 +1,9 @@
+apiVersion: argoproj.io/v1beta1
+kind: ArgoCD
+metadata:
+  name: argocd-long-name-for-route-testiiiiiiiiiiiiiiiiiiiiiiiing
+spec:
+  applicationSet:
+    webhookServer:
+      route:
+        enabled: true

tests/ocp/1-007_verify_hostname_with_route/02-delete.yaml

@@ -0,0 +1,5 @@
+apiVersion: argoproj.io/v1beta1
+kind: ArgoCD
+metadata:
+  name: argocd-long-name-for-route-test-applicationset-controller-webho
+$delete: true