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

Conversation

Rizwana777
Copy link
Contributor

@Rizwana777 Rizwana777 commented Jan 2, 2024

What does this PR do / why we need it:
Fix the failing of webhook route in argocd due to long route name

JIRA link:
https://issues.redhat.com/browse/GITOPSRVCE-794

controllers/argocd/route.go Outdated Show resolved Hide resolved
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this a bit more, hostnames are composed of a set of labels, like:
console-openshift-console.apps.rosa.v8wja-bzjbp-jw8.j2js.p3.openshiftapps.com

with each component part of the hostname being a single label:

  • console-openshift-console
  • apps
  • rosa
  • v8wja-bzjbp-jw8
  • j2js
  • p3
  • openshiftapps.com

So the rule appears to be that a hostname cannot be longer than 255 characters, and an individual label (constitute part of a hostname) cannot be longer than 63 characters.
(source)

So I think our shortening algorithm should be:

  • 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 > 255, then shorten the FIRST label until the host name is < 255
    • 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)

For example:

For example, this:
myhostnameaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.redhat.com
would become:
myhostnameaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.redhat.com

For example, this:
myhostnametwentysixletteraaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.redhat.com
would become: myhostnametwentysixletteraaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.redhat.com

WDYT?

@Rizwana777 Rizwana777 force-pushed the gitopsrvc-issue-794 branch 5 times, most recently from b1f9deb to 1d9bab8 Compare January 12, 2024 11:01
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the algorithm as described still needs to be implemented, and the E2E tests are failing.

controllers/argocd/route.go Outdated Show resolved Hide resolved
controllers/argocd/route.go Show resolved Hide resolved
controllers/argocd/route.go Show resolved Hide resolved
controllers/argocd/route_test.go Outdated Show resolved Hide resolved
@Rizwana777 Rizwana777 force-pushed the gitopsrvc-issue-794 branch 2 times, most recently from 8755d16 to fbfbe00 Compare January 24, 2024 10:18
@Rizwana777
Copy link
Contributor Author

@jgwest Apologies, I misunderstood the algorithm and now I have updated the logic, please take a look and also I am checking the overall hostname > 253 because entire hostname (including the delimiting dots but not a trailing dot) has a maximum of 253 ASCII characters(https://stackoverflow.com/questions/3523028/valid-characters-of-a-hostname)

controllers/argocd/route.go Outdated Show resolved Hide resolved
@Rizwana777 Rizwana777 force-pushed the gitopsrvc-issue-794 branch 2 times, most recently from 1d174c2 to b353338 Compare January 31, 2024 08:15
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more tiny request then good to go.

controllers/argocd/route.go Show resolved Hide resolved
@Rizwana777 Rizwana777 force-pushed the gitopsrvc-issue-794 branch 2 times, most recently from 9e61565 to 18027c4 Compare February 5, 2024 07:04
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @Rizwana777!

controllers/argocd/route.go Outdated Show resolved Hide resolved
@Rizwana777 Rizwana777 force-pushed the gitopsrvc-issue-794 branch 8 times, most recently from 12b0d24 to 0e19ab1 Compare February 8, 2024 12:05
@Rizwana777 Rizwana777 force-pushed the gitopsrvc-issue-794 branch 6 times, most recently from 5a125d6 to 136a553 Compare February 12, 2024 05:05
Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
@Rizwana777 Rizwana777 force-pushed the gitopsrvc-issue-794 branch 12 times, most recently from 5577dc3 to 27e717e Compare February 12, 2024 06:49
Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
@Rizwana777 Rizwana777 force-pushed the gitopsrvc-issue-794 branch 2 times, most recently from 53a9800 to d265d41 Compare February 14, 2024 04:51
Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
Copy link
Collaborator

@jaideepr97 jaideepr97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @Rizwana777

@jaideepr97 jaideepr97 merged commit e33ec74 into argoproj-labs:master Feb 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants