Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix webhook route fail in argocd due to long route name #1129

Merged
merged 3 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion controllers/argocd/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -419,10 +419,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{
Expand Down
71 changes: 71 additions & 0 deletions controllers/argocd/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -248,6 +255,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{
Expand Down Expand Up @@ -325,6 +339,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"),
}
Expand Down Expand Up @@ -392,3 +413,53 @@ func (r *ReconcileArgoCD) reconcileApplicationSetControllerWebhookRoute(cr *argo
}
return r.Client.Update(context.TODO(), route)
}

Rizwana777 marked this conversation as resolved.
Show resolved Hide resolved
// 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) {
Rizwana777 marked this conversation as resolved.
Show resolved Hide resolved
Rizwana777 marked this conversation as resolved.
Show resolved Hide resolved
if hostname == "" {
return "", nil
}

Rizwana777 marked this conversation as resolved.
Show resolved Hide resolved
// 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
}
85 changes: 85 additions & 0 deletions controllers/argocd/route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package argocd
import (
"context"
"fmt"
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -405,6 +406,90 @@ func TestReconcileRouteApplicationSetTls(t *testing.T) {
}
}

func TestReconcileRouteForShorteningHostname(t *testing.T) {
Rizwana777 marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
10 changes: 7 additions & 3 deletions controllers/argocd/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -1601,10 +1601,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
}
Loading