-
Notifications
You must be signed in to change notification settings - Fork 32
Add test coverage for cache scitoken overrides using Xrootd.ScitokensConfig #3028
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
base: main
Are you sure you want to change the base?
Conversation
bbockelm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Xrootd.ScitokensConfig works for both caches and origins, meaning this functionality already exists. Let's focus on providing test coverage for this fact in cache mode. I like having both a unit test and an e2e test as is done in this PR.
I'd additionally like a new E2E test that:
- Sets up a full federation and pulls a file through the cache.
- Places the origin in downtime and removes corresponding namespace ads.
- Triggers a cache authz refresh. This can be done by overwriting the
Xrootd.ScitokensConfigfile with some new issuer covering an unrelated path. You can verify the refresh by seeing new data show up in the corresponding generated file. - Verify you can no longer access the data through the cache.
- Trigger another cache authz refresh, this time adding the authorization information explicitly for the test prefix.
- Verify you can now access the cached object, even with the origin offline.
This new test will demonstrate that the Xrootd.ScitokensConfig override is the only thing needed to access offline data in a cache.
|
|
||
| /*************************************************************** | ||
| * | ||
| * Copyright (C) 2025, Pelican Project, Morgridge Institute for Research |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 0bdf17e
docs/parameters.yaml
Outdated
| default: false | ||
| components: ["cache"] | ||
| --- | ||
| name: Cache.ScitokensUserOverride |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this name; ScitokensUserOverride implies this is done by the user (it's done by the administrator) and we tend to suffix files with Location. How about:
Cache.ScitokensConfigLocation
I would note that there's a parameter named Xrootd.ScitokensConfig marked as usable by the cache... is this already done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right - Xrootd.ScitokensConfig already provides this functionality for both cache and origin. Removed the new parameter and reverted to testing the existing functionality in commit 0bdf17e
docs/parameters.yaml
Outdated
| Any issuers defined in this file will be merged with the cache's auto-generated issuers. This is useful for | ||
| site-local caches that need to continue serving objects during origin downtime. | ||
|
|
||
| Note: This parameter is runtime reloadable. The cache's maintenance loop periodically regenerates the scitokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This note is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in commit 0bdf17e
|
|
||
| // Create a scitoken override file for the cache | ||
| overrideFile := filepath.Join(tmpDir, "cache-override.cfg") | ||
| require.NoError(t, param.Set(param.Cache_ScitokensUserOverride.GetName(), overrideFile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Xrootd.ScitokensConfig already implements this functionality, it's just not tested. Remove the new configuration code and test to see if the old knobs work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - removed the new configuration code and created a comprehensive E2E test using the existing Xrootd.ScitokensConfig functionality in commit 0bdf17e
xrootd/authorization.go
Outdated
|
|
||
| // If the cache is in site-local mode and a user override file is specified, | ||
| // load and merge the override issuers into the configuration | ||
| if param.Cache_EnableSiteLocalMode.GetBool() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can all be removed -- it looks like the existing Xrootd.ScitokensConfig suffices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed and removed in commit 0bdf17e
Added comprehensive E2E test as requested in commit 0bdf17e. The test:
The test demonstrates that Xrootd.ScitokensConfig override is sufficient for accessing offline data in a cache. |
This helps refresh information quickly in unit tests. Includes a "floor" to prevent this from happening too quickly in production.
If the director failed - but the user specified a cache to use - continue trying the request against that cache. Use the default token; in this case, it will not select the right token automatically. This allows a cache to be leveraged and Pelican to work while everything else in the federation is 100% offline.
Shows that data can be accessed at a cache even when the origin is offline!
8b9d74c to
996bf60
Compare
Adds comprehensive test coverage demonstrating that caches can serve cached objects during origin downtime using the existing
Xrootd.ScitokensConfigparameter.Changes
Test Coverage: Added
TestCacheScitokensConfigOverrideE2E test that demonstrates:Xrootd.ScitokensConfigwith an unrelated issuerKey Finding: The functionality to override scitoken configuration for caches already exists via
Xrootd.ScitokensConfig, which is marked as usable by both cache and origin components. No new parameters or implementation changes were needed.Example Configuration
Administrators can use the existing
Xrootd.ScitokensConfigparameter to provide override configurations:With
Xrootd.ScitokensConfig=/path/to/scitokens.cfg, the cache merges these issuers with auto-generated ones, enabling continued service when upstream origins are unreachable. The parameter is already runtime reloadable through the existing maintenance loop.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.