-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat: add merge support for casc defined system credentials #411
base: master
Are you sure you want to change the base?
Conversation
Enables support for merging casc defined credentials with existing credentials (i.e. manually created). fixes JENKINS-64079
c3e8d85
to
05fd220
Compare
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.
If I understand this correctly it means you can only ever add or update credentials via CasC. If you added one via CasC it can only be removed via the UI (as once persisted there is no way to tell the difference)?
Currently this is not possible since the casc system credentials will remove all non-casc defined credentials on restart.
That is kind of the point of CasC though, you are managing your instance via code - not via the UI?
String strategy = Util.fixEmptyAndTrim( | ||
System.getProperty("casc.credentials.merge.strategy", | ||
System.getenv("CASC_CREDENTIALS_MERGE_STRATEGY") | ||
)); |
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.
rather than put the strategy in an environment variable - is it not possible to manage it purely as code in the yaml file?
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'd go even further and define the default strategy value as replace
to make it explicit. Example yaml might look like:
- MergeStrategies
- CascCredentials: merge
- MergeStrategies
- CascCredentials: replace
I agree the behavior introduced is not in the true spirit of CasC being the source of config. If the merge strategy is configured then it would require manual removal of credentials that were added via CasC. In our Jenkins deployments we are trying to get teams to transition to vault credentials configured via CasC. Some credentials would be manually created while others are defined in CasC during this transition. We would accept the extra work of having to manually remove items no longer defined in CasC. Would something like a CasC specific CredentialStore implementation be a better solution? I'm thinking along the lines of something similar to KubernetesCredentialStore. This would mean:
credentials:
system:
domainCredentials:
- credentials:
- usernamePasswordCredential: {id: foo}
cascCredentials:
- credentials:
- usernamePasswordCredential: {id: bar} |
Enables support for merging casc defined credentials with existing credentials (i.e. manually created).
In some environments it is desirable to define some credentials declaratively while also be able to define credentials through the UI (which out otherwise be defined in plain text or in encoded format which could be easily transferable).
One such example is mixing vault credentials defined in casc config with those defined directly through Jenkins. In the below casc example it is assumed the the
vault-approle
credential was created through the Jenkins UI. Currently this is not possible since the casc system credentials will remove all non-casc defined credentials on restart.To enable merging behavior set the env var
CASC_CREDENTIALS_MERGE_STRATEGY=merge
or system property-Dcasc.credentials.merge.strategy=merge
fixes JENKINS-64079