-
Notifications
You must be signed in to change notification settings - Fork 70
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
garethjevans
wants to merge
1
commit into
jenkinsci:master
Choose a base branch
from
garethjevans:alternative-credential-id
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
apiVersion: v1 | ||
kind: Secret | ||
metadata: | ||
# this is the jenkins id. | ||
name: "another-test-usernamepass" | ||
labels: | ||
# so we know what type it is. | ||
"jenkins.io/credentials-type": "usernamePassword" | ||
# override the credential id | ||
"jenkins.io/credentials-id": "CUSTOM_ID_OF_CREDENTIAL" | ||
annotations: | ||
# description - can not be a label as spaces are not allowed | ||
"jenkins.io/credentials-description" : "credentials from Kubernetes" | ||
type: Opaque | ||
stringData: | ||
username: myUsername | ||
password: 'Pa$$word' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What stops 2 credentials with the same label value?
What stops one with the same value as another secrets name?
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.
At the moment, is there anything stopping someone from creating a Secret with the same name as an existing credential?
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.
we could store a mapping of Secret name -> Credential ID and warn/error if there are conflicts / or changes that need to be made?
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 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.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.
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)...
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'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).
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.
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
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 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.
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.
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).
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.
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.