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

Allow defining credential IDs not conforming to DNS subdomain names. #62

Conversation

midu-git
Copy link

@midu-git midu-git commented Jun 10, 2022

Kubernetes secret names require to conform to DNS subdomain names
https://kubernetes.io/docs/concepts/configuration/secret/#restriction-names-data
Jenkins, however, doesn't introduce such a restriction
https://www.jenkins.io/doc/book/using/using-credentials/

  • 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

@jtnord
Copy link
Member

jtnord commented Jun 15, 2022

Hi midu-git

Thank you for taking your time to create this, however if you had searched the issues and existing PRs you would have found somethign for this that I have already rejected.
#27 #53 https://issues.jenkins.io/browse/JENKINS-62360

Credential IDs do not have to be meaningful - they are IDs and I strongly believe that you can create meaningful IDs that conform to the k8s standards, so adding this to the plugin is just adding extra code where it is not generally needed.

Where you do need this functionality is if you are attempting to migrate from storing credentials in one provider to another, and have lots of jobs or configuration that would need to be updated. However - this plugin does not have to care about that, A better approach is to create a new credential provider that supports aliases so you can alias a specific ID to one or more IDs so that something requesting blah would be served by a credential with the ID wibble, whilst at the same time keeping track of where and when it was used to enable the user to complete the migration and eventually remove this mapping.

@jtnord jtnord closed this Jun 15, 2022
@midu-git
Copy link
Author

Hi @jtnord.

Thank you for providing your opinion.

Unfortunately, I disagree on your argument. I agree that one should conform in using IDs that are "standardised". However, this is a concept connected to specific contexts. For environment variables, e.g., it is accepted to use SCREAMING_SNAKE_CASE. In context of Jenkins, credential IDs are not restricted to kebab-case, or even further to restrictions required for Kubernetes secret names. Although credential providers are free to impose further restrictions, I don't get why this should be further restricted in this specific case.
If your concern is keeping code simple, I don't understand then, why you allow a much more complicated mapping for username/password keys. The former can indeed be controlled, the credential ID not, as you have already pointed out for the case of migrations. Moreover, the increase in code complexity is negligible here.

Indeed, in our case it's a migration issue. However, as we provide Jenkins instances that are not fully under our control, I'm very unsure if the migration path described by you is realistic. Moreover, introducing an alias credential provider is more than an overhead. Our intention is to drop any additional credential providers, such as https://plugins.jenkins.io/aws-secrets-manager-credentials-provider, as we deploy Jenkins in multiple clouds and thus we want to be more cloud agnostic. This would introduce a step back for us.

Said that, can you provide an argument for the rejection? In particular, why is introducing a new credential provider a better solution?

@midu-git
Copy link
Author

@jtnord Thanks for the discussion.

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.

2 participants