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: allow credential id to be specified #53

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

Conversation

garethjevans
Copy link
Contributor

Allows the credential id to be set from a value that is not the secret name, this is useful when you want a credential called SOME_VALUE_LIKE_THIS that cannot be used as a Secret name.

The label to be used is jenkins.io/credentials-id

Fixes JENKINS-62360

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master 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

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.

Hi Gareth.

Thankyou for the PR but this suffers the same issues as #27.

Credential IDs need to be unique and immutable, moving from the name to an annotation violates that and causes issues because of it

In other words this is probably not the fix you want, rather you may want to look at a general purpose credential alias plugin that allows you to alias IDs, this could be used then until you migrate the job configuration. (I had thought about providing that in this plugin as a configMap but that would not help fixing credentials before a migration to k8s / k8s credentials, so is probably better off as a separate plugin)

if (labels != null) {
String overrideId = labels.get(JENKINS_IO_CREDENTIALS_ID_LABEL);
if (overrideId != null) {
return overrideId;
Copy link
Member

Choose a reason for hiding this comment

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

What stops 2 credentials with the same label value?
What stops one with the same value as another secrets name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, is there anything stopping someone from creating a Secret with the same name as an existing credential?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could store a mapping of Secret name -> Credential ID and warn/error if there are conflicts / or changes that need to be made?

Copy link
Member

Choose a reason for hiding this comment

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

if you mean an ID "foo" in k8s and using the stock credentials provider then no - but you can not do that twice in the same provider.
When you do manage to do it it is always deterministic, you get the Credential (Secret) from the credential provider with the highest ordinal (and if there are multiple with the same ordinal it is backed by a list so deterministic until you restart, and then it is mostly stable as it comes from the plugin scout order).

With this approach, if one secret got updated it would be used until the other secret got updated, causing potential randomness. I guess k8s list uses some stable ordering but if not, just reconnecting to the API server after a connection will potentially cause changes.

Copy link
Member

Choose a reason for hiding this comment

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

we could store a mapping of Secret name -> Credential ID and warn/error if there are conflicts / or changes that need to be made?

We could, but at the end of the day I think credential aliasing would be much more useful. At the end of the day - the IDs are not needed to interact with any external system - so this should only ever be an interim solution (e.g. migration) and as such not a core part of the plugin (you may want to start the migration before you move to k8s)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I agree, from the Jenkins docs on https://www.jenkins.io/doc/book/using/using-credentials/ it states:

"You can use upper- or lower-case letters for the credential ID, as well as any valid separator character. However, for the benefit of all users on your Jenkins instance, it is best to use a single and consistent convention for specifying credential IDs."

It should be possible to do this without the need for an additional plugin (which doesn't yet exist).

Copy link
Member

@jtnord jtnord Jun 1, 2021

Choose a reason for hiding this comment

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

That talks about the stock credentials provider only. Other credentials provider (such as cyberark) and k8s may well have their own further restrictions. I will file a PR to get that page fixed :)

update -> jenkins-infra/jenkins.io#4390

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have no intention of merging this, could you mark the JIRA issues as Won't Fix to make sure that this PR is not opened for the third time.

Choose a reason for hiding this comment

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

What a pity to refuse such feature for the reason of potential Bad usage by guys who have overall administrative rights.

Except if it compromise plugin stability or worse Jenkins instance, I see no reason to reject it.

I use a lot JasC and if I misconfigure my Jenkins instance. It is my owb ans entire fault. Features (i.e. Plugins) may limit misbehavior/configuration but must offer freedom in usage (at least for power user).

You made technical design decision that already affect usability and there is a simple solution to leverage it with low-risk cost (manageable and not affecting stability).

As I havent took a look at how it works not sure if label (use for identification and selection) is more appropriate than annotation (use for information).

Dont take my words too tough, english isnt my mother tongue. But I highly want you to reconsider your position regarding naming "freedom" (over just aliasing).

Copy link
Member

Choose a reason for hiding this comment

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

As I said in the Jira there is no need to support this in this plugin.
If you want to be able to migrate credentials then that can be done in a different plugin that supports credential aliasing and can work across any credential provider.

I'm not saying there is not a need for that, but I am saying I do not see a need to be able to specify a non DNS corformant id in this pkugin., The only reason anyone has put forward for that so far is migrating.

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