-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Let the user pass a secret via a parameter for SCM API operations when using the git API resolver #7239
Let the user pass a secret via a parameter for SCM API operations when using the git API resolver #7239
Conversation
/kind feature |
/retest |
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.
/meow
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The following is the coverage report on the affected files.
|
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 used token and tokenKey to be simple as the params name, let me know if you have another name in mind (like token-secret-ref or something) i don't mind anything and can refactor it if needed
@@ -18,8 +18,10 @@ This Resolver responds to type `git`. | |||
| `url` | URL of the repo to fetch and clone anonymously. Either `url`, or `repo` (with `org`) must be specified, but not both. | `https://github.com/tektoncd/catalog.git` | | |||
| `repo` | The repository to find the resource in. Either `url`, or `repo` (with `org`) must be specified, but not both. | `pipeline`, `test-infra` | | |||
| `org` | The organization to find the repository in. Default can be set in [configuration](#configuration). | `tektoncd`, `kubernetes` | | |||
| `token` | An optional secret name in the `PipelineRun` namespace to fetch the token from. Defaults to empty, meaning it will try to use the configuration from the global configmap. | `secret-name`, (empty) | |
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.
not sure if those tables has been 'designed' to handle such long string, let me know if need to short this string a bit to match the formatting
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.
It should be rendered fine. https://tekton.dev/docs/pipelines/git-resolver/
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.
yes :) but I don't know if the rendering handle properly long lines...
(on another note is there a way to run the docs locally? i.e: with hugo or others?)
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.
@afrittoli ^^
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
a553e9e
to
1025878
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
1025878
to
3923413
Compare
The following is the coverage report on the affected files.
|
3923413
to
0b4994f
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
We were previously only allowing using a secret from the global configmap, which mean every users on the cluster would have to use the same git value (which could be a security issue). We now let the user use their own token on their own namespace with the optional "token" and (even more optional) "tokenKey" params. The fallback will still be the global configmap. Added a bit logic to make sure we are caching secrets only when user use the token from global configmap. Since we can't ensure for local secrets on namespace may be rotated often. Signed-off-by: Chmouel Boudjnah <[email protected]>
0b4994f
to
170ce5b
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
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.
/lgtm
🎉
We were previously only allowing using a secret from the global configmap, which mean every users on the cluster would have to use the same git value (which could be a security issue).
We now let the user use their own token on their own namespace with the optional "token" and (even more optional) "tokenKey" params.
The fallback will still be the global configmap.
Added a bit logic to make sure we are caching secrets only when user use the token from global configmap. Since we can't ensure for local secrets on namespace may be rotated often.
/kind feature
Changes
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes