Add possibility to set username for the git-resolver#9543
Add possibility to set username for the git-resolver#9543MarcusElevait wants to merge 2 commits intotektoncd:mainfrom
Conversation
… git resolver Prior to this commit, there was no possibility to configure a username for the git resolver, neither for clone nor for api. Added the possibility to set the username via the token secret.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/kind feature |
aThorp96
left a comment
There was a problem hiding this comment.
Thanks for the contribution @MarcusElevait!
I have a couple minor suggestions and two requests. The only blocking requests are to either remove or use the global git resolver config for the username field (I don't think it's necessary, personally, but not opposed to keeping it), since it's currently unused outside of tests, and to modify the test TestResolve to check that the username is propagated to the clientfunc and git request, respectively.
| | `revision` | Git revision to checkout a file from. This can be commit SHA (SHA-1 or SHA-256), branch or tag. | `aeb957601cf41c012be462827053a21a420befca` `main` `v0.38.2` | | ||
| | `gitToken` | An optional secret name in the `PipelineRun` namespace to fetch the token from when doing opration with the `git clone`. When empty it will use anonymous cloning. | `secret-gitauth-token` | | ||
| | `gitTokenKey` | An optional key in the token secret name in the `PipelineRun` namespace to fetch the token from when using the `git clone`. Defaults to `token`. | `token` | | ||
| | `username` | An optional key in the token secret name in the `PipelineRun` namespace to fetch the username from. Defaults to `username`. | | |
There was a problem hiding this comment.
Please include an example value:
| | `username` | An optional key in the token secret name in the `PipelineRun` namespace to fetch the username from. Defaults to `username`. | | | |
| | `username` | An optional key in the token secret name in the `PipelineRun` namespace to fetch the username from. Defaults to `username`. | `username` | |
| | `gitToken` | An optional secret name in the `PipelineRun` namespace to fetch the token from when doing opration with the `git clone`. When empty it will use anonymous cloning. | `secret-gitauth-token` | | ||
| | `gitTokenKey` | An optional key in the token secret name in the `PipelineRun` namespace to fetch the token from when using the `git clone`. Defaults to `token`. | `token` | | ||
| | `revision` | Git revision to checkout a file from. This can be commit SHA (SHA-1 or SHA-256), branch or tag. | `aeb957601cf41c012be462827053a21a420befca` `main` `v0.38.2` | |
There was a problem hiding this comment.
Thanks for cleaning up this whitespace!
| | `default-revision` | The default git revision to use if none is specified | `main` | | ||
| | `fetch-timeout` | The maximum time any single git clone resolution may take. **Note**: a global maximum timeout of 1 minute is currently enforced on _all_ resolution requests. | `1m`, `2s`, `700ms` | | ||
| | `default-url` | The default git repository URL to use for anonymous cloning if none is specified. | `https://github.com/tektoncd/catalog.git` | | ||
| | `scm-type` | The SCM provider type. Required if using the authenticated API with `org` and `repo`. | `github`, `gitlab`, `gitea`, `bitbucketcloud`, `bitbucketserver` | |
There was a problem hiding this comment.
I think the whitespace alignment for this section is now misaligned with this row
| # my-secret-token should be created in the namespace where the | ||
| # pipelinerun is created and contain a GitHub personal access | ||
| # token in the token key of the secret. | ||
| # token in the token key of the secret and a username depending on the git provider. |
There was a problem hiding this comment.
Nitpick, rewording this so it's clear "depending on the git provider" refers specifically to the username requirement
| # token in the token key of the secret and a username depending on the git provider. | |
| # token in the token key of the secret and, if required by the git provider, a username. |
| // APISecretKeyKey is the config map key for the containing the token within the token secret | ||
| APISecretKeyKey = "api-token-secret-key" | ||
| // APIUsernameSecretKey is the config map key containing the username within the token secret | ||
| APIUsernameSecretKey = "api-username-secret-key" |
There was a problem hiding this comment.
This appears to be unused outside of the tests
| token, ok := secret.Data[apiSecret.tokenKey] | ||
| if !ok { | ||
| err := fmt.Errorf("cannot get API token, key %s not found in secret %s in namespace %s", apiSecret.key, apiSecret.name, apiSecret.ns) | ||
| err := fmt.Errorf("cannot get API token, key %s not found in secret %s in namespace %s", apiSecret.tokenKey, apiSecret.name, apiSecret.ns) |
There was a problem hiding this comment.
Might need a codeQL exception here, this is a safe change flagged by regex. Renaming the variable would work too, since I don't like the idea of disabling this check on a log line
| if tc.config[tc.configIdentifer+APIUsernameSecretKey] != "" { | ||
| secretUsernameKey = tc.config[tc.configIdentifer+APIUsernameSecretKey] | ||
| } |
There was a problem hiding this comment.
Nitpick, since empty string is also the zero value we can just do this:
| if tc.config[tc.configIdentifer+APIUsernameSecretKey] != "" { | |
| secretUsernameKey = tc.config[tc.configIdentifer+APIUsernameSecretKey] | |
| } | |
| secretUsernameKey = tc.config[tc.configIdentifer+APIUsernameSecretKey] |
There was a problem hiding this comment.
Though this is the only place we reference the username key in the configmap so might not be needed at all
There was a problem hiding this comment.
It looks like this adds support for testing that the usernameKey is allowed and ensures the testcase initializes the usernamekey when necessary, however it doesn't appear that the test if the username was given to the git provider or API. When the code in ResolveApiGit and ResolveGitClone that uses the username key is removed, the test still passes
| type authCredentials struct { | ||
| token []byte | ||
| username []byte | ||
| } | ||
|
|
||
| func (g *GitResolver) getAPIToken(ctx context.Context, apiSecret *secretCacheKey, key string) ([]byte, error) { | ||
| func (g *GitResolver) getAuthenticationCredentials(ctx context.Context, apiSecret *secretCacheKey, key string) (*authCredentials, error) { |
There was a problem hiding this comment.
I appreciate the use of types here
… git resolver
Changes
When using the git resolver with a private Bitbucket cloud repository, you need to consider two things:
Currently for the git clone resolver the username is always set to "git". For the rest api there is no username set at all.
With my changes you can define a username within the secret of the token, which is then used in the respective resolver.
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