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

feat: add merge support for casc defined system credentials #411

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cronik
Copy link

@cronik cronik commented Jan 24, 2023

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.

unclassified:
  hashicorpVault:
    configuration:
      vaultCredentialId: "vault-approle"
      vaultUrl: "https://myvault.mycorp.net"
credentials:
  system:
    domainCredentials:
      - credentials:
          - vaultUsernamePasswordCredentialImpl:
              description: "vault managed credential"
              id: "my-vault-cred"
              path: "kv/jenkins"
              scope: GLOBAL

To enable merging behavior set the env var CASC_CREDENTIALS_MERGE_STRATEGY=merge or system property -Dcasc.credentials.merge.strategy=merge

fixes JENKINS-64079

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Enables support for merging casc defined credentials with existing credentials
(i.e. manually created).

fixes JENKINS-64079
@cronik cronik force-pushed the feature/JENKINS-64079 branch from c3e8d85 to 05fd220 Compare February 18, 2023 19:33
@cronik cronik marked this pull request as ready for review February 18, 2023 19:46
@cronik cronik requested a review from a team as a code owner February 18, 2023 19:46
Copy link
Member

@jtnord jtnord left a 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?

Comment on lines +79 to +82
String strategy = Util.fixEmptyAndTrim(
System.getProperty("casc.credentials.merge.strategy",
System.getenv("CASC_CREDENTIALS_MERGE_STRATEGY")
));
Copy link
Member

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?

Copy link

@mikecirioli mikecirioli Apr 28, 2023

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

@cronik
Copy link
Author

cronik commented May 12, 2023

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?

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:

  • cascCredentials could not be added/removed/updated via the UI
  • Removed items in cascCredentials would be reflected without manual intervention
  • Credentials added via the UI would not be impacted since they would be managed under domainCredentials
credentials:
  system:
    domainCredentials:
      - credentials:
          - usernamePasswordCredential: {id: foo}
    cascCredentials: 
      - credentials:
          - usernamePasswordCredential: {id: bar}

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.

3 participants