-
Notifications
You must be signed in to change notification settings - Fork 0
Issue 124 #139
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
…ount for new dependencies
…xt rather than a canceled one
…lers can throw out the bytes if they don't need it
…or is non-nil but empty
…s not return an error. Also exported func so it is now GetCondorVaultTokenLocation
…ernal/vaultToken.GetCondorVaultTokenLocation instead
…d added new function workerTypeFromConfig
…th, and shortened up some variable names
… token from configuration
… store the token in a condor credd
…hat returns the valid worker types for which retries are available
…ions to use that func. Also added new iterator ValidTokenGetterWorkerTypes
…rned error so callers can check for it with errors.Is
…re vault tokens in a condor credd
…orker type in token-push code
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.
Pull request overview
This PR implements configurable token getter functionality to allow users to choose between using condor_vault_storer or direct htgettoken calls for obtaining tokens. This addresses Issue #124 by making the token retrieval method configurable rather than always requiring Condor involvement.
Key Changes:
- Introduced a new
GetTokenWorkerType to support direct token retrieval without Condor - Added worker-specific configuration options for token getter behavior (interactive mode, alternate token getter)
- Refactored WorkerType constants to use lowercase naming and added a Worker() method for cleaner dispatch
- Updated vendor dependencies including scitokens-go v0.3.3, lestrrat-go/jwx v1.2.30, and others
Reviewed changes
Copilot reviewed 36 out of 132 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/worker/workerType.go | Refactored WorkerType enum, renamed constants (e.g., GetKerberosTicketsWorkerType → GetKerberosTickets), added GetToken type and Worker() method for function dispatch |
| internal/worker/workerSpecificConfig.go | Added InteractiveTokenGetterOption and AlternateTokenGetterOption config options with supporting getters/setters |
| internal/worker/getTokenWorker.go | New worker implementation for direct token retrieval with TokenGetter interface and tokenGetterConfig implementation |
| internal/worker/condorVaultStorerWorker.go | Updated to support interactive token getter configuration option via getInteractiveTokenGetterOptionFromConfig |
| internal/worker/pushTokensWorker.go | Updated to handle nil credds slice and use vaultToken.GetCondorVaultTokenLocation |
| internal/vaultToken/vaultToken.go | Moved GetCondorVaultTokenLocation to be exported and improved error handling in user ID lookup |
| managedTokens.jsonnet, managedTokens.json | Added vaultServer field to configuration |
| libsonnet/experimentConfig.libsonnet | Added tokenGetterOverride to supported overrides list |
| go.mod | Updated dependencies: scitokens-go v0.3.3, jwx v1.2.30, goccy/go-json v0.10.3, and others |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@vitodb Feel free to ask me any questions you have about this. It's a pretty huge PR. |
…he option is not nil AND there was no error retrieving it
…nStorerAndGetter interface
General idea
This PR implements functionality to allow operators to choose between using condor_vault_storer or direct htgettoken calls for obtaining tokens. This addresses Issue #124 by making the token retrieval method configurable rather than always requiring Condor involvement.
Key Changes:
GetTokenWorkerType to support direct token retrieval without CondorStoreAndGetTokenandStoreAndGetTokenInteractiveWorkerTypes so that bothStoreAndGetTokenandGetTokensupport the use of an optionalinteractiveconfiguration option.Workerfunctions via theWorkerType.Worker()methodtoken-pushandrefresh-uids-from-ferryto use these changes(This General Idea summary was adapted from copilot summary)
Added functionality by package
internal/vaultToken
HtgettokenClient. Callers can usevaultToken.NewHtgettokenClientto get a newHtgettokenClient, and then useHtgettokenClient.GetTokento obtain a new bearer token and vault token. This is the interface tohtgettokenInteractiveTokenStorerandNonInteractiveTokenStorertypes, and instead created aVaultStorerClienttype that acts much likeHtgettokenClient. The methodVaultStorerClient.GetAndStoreTokenobtains the vault token and stores it in a condor credd. This is the interface tocondor_vault_storer.HtgettokenClientandVaultStorerClientuse a newcommandExecutorinterface to implement interactive and non-interactive running of thehtgettokenandcondor_vault_storercommands.internal/worker
StoreAndGetTokenInteractiveWorkerType intoStoreAndGetToken. AddedWorkerType.Workermethod to mapWorkerTypevalues to unexportedWorkerfunctions. This gives callers a SINGLE way to call upon a worker. For example, if a user wants to start aGetTokenworker, they would call the function returned byGetToken.Worker().InteractiveTokenGetterOptionandAlternateTokenGetterOptionthat callers may put into theConfigtype'sworkerSpecificConfigfield.ValidRetryWorkerTypesandValidTokenGetterWorkerTypesthat allow callers to see whichWorkerTypesactually support token-getting or retries respectively.WorkerSpecificConfigOption. The setters mainly make sure theConfig.workerSpecificConfigfield is properly initialized, and allow you to store any value there. The getters type-check and validate the stored values.workerfunc calledgetTokenWorker. This worker calls the various functions to obtain a vault token viahtgettoken(by using theinternal/vaultTokenpackage), and handles communication of status and errors with the caller. It supports the use of theWorkerSpecificConfigOptionInteractiveTokenGetterOption.TokenGetterto allow for specification of alternate token-getters for mocking or future pluggability.tokenGetterConfigimplementsTokenGetter, and runshtgettokenby creating a newvaultToken.HtgettokenClient, and calling itsGetTokenmethod, as described in the internal/vaultToken section above.WorkerfuncstoreAndGetTokenWorker, which also supports theWorkerSpecificConfigOptionInteractiveTokenGetterOption. The newtokenStorerAndGetterinterface is declared here for testability and pluggability, but in the current case, that interface is implicitly implemented byinternal/vaultToken.VaultStorerClient. I chose to structure it this way because there was a fair bit more information needed for the "helper" function and I didn't want to have too many arguments for the helper functionstoreAndGetTokensForSchedd.Workerfuncs, since they will be called via their correspondingWorkerType.Config.Scheddsfield tonil. This propagates to thepushTokensWorkerworkerand is now supported by that function.Configuration changes (libsonnet/experimentConfig.libsonnet)
tokenGetterOverridethat can be specified in an experiment configurationtoken-push
getTokenGetterOverrideFromConfigurationthat gets thetokenGetterOverridekey from the experiment-specific configuration and validates it. A valid value for that key must be a string that corresponds to the name of aworker.WorkerType, but with the first letter lower-cased. So to override the defaultStoreAndGetTokenWorkerTypewith aGetTokenWorkerType, we use the key-value pair:tokenGetterOverride: "getToken". If the string passes this validation check, we then make sure that the specified worker type supports getting tokens using the exportedworker.ValidTokenGetterWorkerTypesfunction. The returnworker.WorkerTypevalue if any checks fail is the default ofworker.StoreAndGetToken.worker.StoreAndGetTokenInteractiveas a validWorkerType, and added a new helper functionworkerTypeFromConfigthat takes a string and returns the correspondingWorkerTypeif the string is valid. This is used in the previous point for validation.worker.Workerfuncs, the funcstartServiceConfigWorkerForProcessingnow accepts aworker.WorkerTypewt, and callswt.Worker()to launch the worker. I also added some error handling in case a worker needs to be skipped (like theworker.StoreAndGetTokensWorkerTypeif we're not using condor at all).worker.StoreAndGetTokensworker func before, route each service'sworker.Configobject to either be run throughworker.GetToken(htgettoken, indicated by step (2a) in the comments), orworker.StoreAndGetTokens(condor_vault_storer, or step (2b)). For the former case, set theConfig'sScheddsfield toniland skip querying the condor collector or cache for schedds.refresh-uids-from-ferry
withKerberosJWTAuthshould use the newvaultToken.HtgettokenClientto get a bearer and vault token.Testing
After all of these changes, I ran the following tests to validate the change. All passed.
Closes #124.