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

Namespace registry admin #462

Merged
merged 10 commits into from
Dec 20, 2023

Conversation

turetske
Copy link
Collaborator

@turetske turetske commented Dec 6, 2023

This adds a requirement for admin approval before a cache can advertise to the director.

Currently there is no endpoint to approve caches, but the registry database can be altered via sqlite with the following commands after the cache has been created with pelican cache serve:

run sqlite

.open "/var/lib/pelican/registry.sqlite"
SELECT * FROM namespace;

There will be a list of origins and caches that looks like this:

id          prefix                pubkey             identity    admin_metadata         
----------  --------------------  -----------------  ----------  -----------------------
1           /caches/f1ec0afbfc09  <key_info>                     {"admin_approved":false}

Then run the following with 1 replaced by whatever id is in the id column of the cache you wish to approve

UPDATE namespace SET admin_metadata = '{"admin_approved":true}' WHERE id = 1;

@turetske turetske linked an issue Dec 6, 2023 that may be closed by this pull request
@turetske turetske added this to the v7.3.0 milestone Dec 6, 2023
namespace_registry/registry_db_test.go Outdated Show resolved Hide resolved
namespace_registry/registry_db_test.go Outdated Show resolved Hide resolved
namespace_registry/registry_db_test.go Outdated Show resolved Hide resolved
viper.Set("Registry.DbLocation", registry_db_dir)

err := InitializeDB()
assert.NoError(t, err, "error initializing registry database")
Copy link
Member

Choose a reason for hiding this comment

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

Not a request for change, but more my own curiosity -- across our tests, I've noticed we switch back and forth between require.NoError and assert.NoError. Have you found a good reason to pick one over the other?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, I looked this up as it seems to be personal preference. There is a minor difference. Basically, require will cause a test to quit and exit immediately, which may be what we want more often, but with go routines, it can cause some issues.

To quote someone else:
"However, if the test has goroutines, we need to be careful that the test waits for goroutines' completion. Otherwise, the test errors get buried/missed; leaving you wondering why a test case failed!"

namespace_registry/registry_db.go Outdated Show resolved Hide resolved
namespace_registry/registry_db.go Outdated Show resolved Hide resolved
namespace_registry/registry_db_test.go Outdated Show resolved Hide resolved
@turetske turetske force-pushed the namespace_registry_admin branch from b71062d to 5d57251 Compare December 8, 2023 18:47
@turetske turetske force-pushed the namespace_registry_admin branch from 5d57251 to 75deece Compare December 11, 2023 22:42
director/origin_api.go Outdated Show resolved Hide resolved
director/redirect.go Outdated Show resolved Hide resolved
director/redirect.go Outdated Show resolved Hide resolved
namespace_registry/registry.go Outdated Show resolved Hide resolved
namespace_registry/registry_db.go Outdated Show resolved Hide resolved
namespace_registry/registry.go Outdated Show resolved Hide resolved
namespace_registry/registry_db.go Outdated Show resolved Hide resolved
server_ui/advertise.go Outdated Show resolved Hide resolved
server_ui/register_namespace.go Outdated Show resolved Hide resolved
Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

Few small things and a rebase, and then I think we're good to go!

director/origin_api.go Outdated Show resolved Hide resolved
director/redirect.go Outdated Show resolved Hide resolved
director/redirect.go Outdated Show resolved Hide resolved
    -- Admin metadata now contains a json string representing admin approval
    -- Admin approval is set the false by default
    -- When verifying whether a cache can advertise to the director, the namespace_registry first checks to see if the cache has
       admin approval
    -- Fixed check of namespace existence for caches
    -- Refactored tests
    -- Fixed warning sent due to cache advertise failure due to lack of admin approval
…pace

-- Added a getNamespace endpoint for registry 2.0 that returns a namespace of the prefix if it exists
-- Added a metadata prefix to the wildcard auto-discovery to get around conflicts
-- changed all current access to 2.0 api (as opposed to 1.0, which we are still keeping for backwards compatability)
    -- Renamed boolean in dbGetJwks to be more accurate
    -- Switched out integers for actual http errors
    -- Created an adminApprovalError for use in cache endpoints
    -- Adjusted prefix header to be the same style as the rest of our pelican headers
    -- Added some clarifying comments
    -- Returns the name of the unapproved cache as part of the error message
@turetske turetske force-pushed the namespace_registry_admin branch from a546c47 to 8d30a3b Compare December 19, 2023 17:19
@haoming29 haoming29 mentioned this pull request Dec 20, 2023
director/origin_api.go Outdated Show resolved Hide resolved
@jhiemstrawisc jhiemstrawisc merged commit fed42c2 into PelicanPlatform:main Dec 20, 2023
9 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.

Require Admin approval for new caches
2 participants