Skip to content

Properly handle the trailing/ in namespace registration and advertisement#2270

Merged
jhiemstrawisc merged 3 commits into
PelicanPlatform:mainfrom
h2zh:prefix-validation-fix
May 12, 2025
Merged

Properly handle the trailing/ in namespace registration and advertisement#2270
jhiemstrawisc merged 3 commits into
PelicanPlatform:mainfrom
h2zh:prefix-validation-fix

Conversation

@h2zh

@h2zh h2zh commented Apr 29, 2025

Copy link
Copy Markdown
Contributor

We should allow user to set either /foo/bar or /foo/bar/ as the federationprefix, but fundamentally treat them as the same (without trailing slash).

This patch switches the order of prefix validation and prefix existence checking. It removes the trailing slash for the user input prefix, which prevents the error like this in the future.

Closes #1806

@h2zh h2zh added bug Something isn't working internal Internal code improvements, not user-facing registry Issue relating to the registry component labels Apr 29, 2025
@h2zh h2zh added this to the v7.17 milestone Apr 29, 2025
@jhiemstrawisc jhiemstrawisc self-requested a review April 29, 2025 21:51

@jhiemstrawisc jhiemstrawisc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Recording a few notes from the conversation we had in person for the record:

  1. This moved the error along a bit further so that registration succeeds, but there's still an issue with advertisement when the Origin exports a namespace with a trailing /. I think we should target a few extra spots. In server_utils/origin.go::validateFederationPrefix() we should add something to strip the trailing / from the Origin's internal representation of the export (with a warning log statement telling the admin that this is happening). However, there are still old origins that won't do this, so we should look at where the advertisement arrives at the Director and make sure to strip the trailing / there as well, before that ad is stored in the TTL cache and before the Director asks the registry for the namespace key(s) used to verify the registration.
  2. Let's add something in the e2e fed test package that tests the whole thing (the origin both registers and advertises successfully) to make sure the fixes in 1) did the trick.
  3. This one's a more recent pet peeve of mine that isn't something you introduced, but is in the functions you're touching here. I'm hoping you'll fix it 😉. In registry/registry.go::keySignChallengeCommit() we do something like:
if clientVerified && serverVerified {
    // big long section of indented code
} else {
    return
}

My brain doesn't like excessive indentation, so I think this would be a lot easier to read:

if !(clientVerified && serverVerified) {
    return
}

// big long section of code that's no longer indented

I think this style is referred to as guard clausing.

Comment thread registry/registry.go Outdated
@h2zh

h2zh commented May 8, 2025

Copy link
Copy Markdown
Contributor Author

For 1), I create a new function to remove the trailing '/' in Origin exports. Because prefix is just a local copy of the string passed in, reassigning it inside validateFederationPrefix won’t touch the original b.Exports[i].FederationPrefix.

@h2zh

h2zh commented May 9, 2025

Copy link
Copy Markdown
Contributor Author

For 2), after some digging, I think the origin test framework would be sufficient for this modification. While spinning up a new fed test is unnecessary - we can't rely on fed tests too much because each fed test increase the test time significantly (Tests within a package are running sequentially).

For 3), I'm happy to clear an eyesore 🧹🤪

@h2zh h2zh changed the title Properly handle the trailing/ in namespace registration Properly handle the trailing/ in namespace registration and advertisement May 9, 2025
h2zh added 2 commits May 9, 2025 21:02
- when the Origin exports a namespace with a trailing /
- when the advertisement arrives at the Director, before that ad is stored in the TTL cache and before the Director asks the registry for the namespace key(s) used to verify the registration
- add tests for the above
@h2zh h2zh force-pushed the prefix-validation-fix branch from fab1c88 to 8e52103 Compare May 9, 2025 21:15
@h2zh h2zh requested a review from jhiemstrawisc May 9, 2025 21:37

@jhiemstrawisc jhiemstrawisc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!!

@jhiemstrawisc jhiemstrawisc merged commit 6137efa into PelicanPlatform:main May 12, 2025
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working internal Internal code improvements, not user-facing registry Issue relating to the registry component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better handling of trailing / in federation prefixes

2 participants