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: add support for authenticated git clone #8537

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

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented Jan 28, 2025

Changes

Added support for authenticated git cloning using gitToken and gitTokenKey
parameters. Updated documentation to reflect the new parameters and their
usage. Modified resolver functions to handle authentication when cloning
repositories. Added tests to verify the functionality of authenticated git
cloning.

The differences between the two modes are:

  • The git clone method support anonymous cloning and authenticated cloning.
  • Depending of the Git provider git clone has a lower rate limit (if none)
    than the authenticated API.
  • The authenticated API supports private repositories and fetches only the file
    at the specified path rather than doing a full clone.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • Has Tests included if any functionality added or changed
  • pre-commit Passed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

An optional token can now be passed to the git clone method (using go-git library) to
bypass token limit when using the API.

/kind feature

@tekton-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 28, 2025
@tekton-robot tekton-robot requested review from dibyom and jerop January 28, 2025 16:01
@chmouel
Copy link
Member Author

chmouel commented Jan 28, 2025

/assign @vdemeester @aThorp96 @waveywaves

@tekton-robot
Copy link
Collaborator

@chmouel: GitHub didn't allow me to assign the following users: aThorp96.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @vdemeester @aThorp96 @waveywaves

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.

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/resolution/resolver/git/resolver.go 83.9% 84.4% 0.5

@@ -449,7 +479,7 @@ func ResolveAPIGit(ctx context.Context, params map[string]string, kubeclient kub
}, nil
}

func getAPIToken(ctx context.Context, apiSecret *secretCacheKey, kubeclient kubernetes.Interface, logger *zap.SugaredLogger, cache *cache.LRUExpireCache, ttl time.Duration, params map[string]string) ([]byte, error) {
func getAPIToken(ctx context.Context, apiSecret *secretCacheKey, kubeclient kubernetes.Interface, logger *zap.SugaredLogger, cache *cache.LRUExpireCache, ttl time.Duration, params map[string]string, key string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the gitToken also an APIToken? If not, since this now handles both the Git Token and the API Token, I wonder if it's helpful to rename the method name and local variables like (e.g. apiSecret) to be reflect this function becoming more generic.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure why not, both works tbh

pkg/resolution/resolver/git/resolver.go Outdated Show resolved Hide resolved
pkg/resolution/resolver/git/resolver.go Outdated Show resolved Hide resolved
docs/git-resolver.md Outdated Show resolved Hide resolved
pkg/resolution/resolver/git/resolver.go Outdated Show resolved Hide resolved
pkg/resolution/resolver/git/resolver.go Outdated Show resolved Hide resolved
docs/git-resolver.md Outdated Show resolved Hide resolved
pkg/resolution/resolver/git/resolver.go Show resolved Hide resolved
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from vdemeester after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

return nil, wrappedErr
}

secretVal, ok := secret.Data[apiSecret.key]
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)
logger.Info(err)
g.Logger.Info(err)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by an access to APISecretKey
flows to a logging call.
Copy link
Member Author

Choose a reason for hiding this comment

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

the log would only log the secret-name not the secret content (and nothing has changed in this patch compared to before) 🙃

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/remoteresolution/resolver/git/resolver.go 81.8% 82.4% 0.5
pkg/resolution/resolver/git/resolver.go 83.9% 84.5% 0.5

Added support for authenticated git cloning using `gitToken` and `gitTokenKey`
parameters. Updated documentation to reflect the new parameters and their
usage. Modified resolver functions to handle authentication when cloning
repositories. Added tests to verify the functionality of authenticated git
cloning.

The differences between the two modes are:

- The `git clone` method support anonymous cloning and authenticated cloning.
- Depending of the Git provider `git clone` has a lower rate limit (if none)
  than the authenticated API.
- The authenticated API supports private repositories and fetches only the file
  at the specified path rather than doing a full clone.

Signed-off-by: Chmouel Boudjnah <[email protected]>
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/remoteresolution/resolver/git/resolver.go 81.8% 82.4% 0.5
pkg/resolution/resolver/git/resolver.go 83.9% 84.5% 0.5

@chmouel
Copy link
Member Author

chmouel commented Jan 29, 2025

/test all

@chmouel chmouel marked this pull request as ready for review January 29, 2025 09:50
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 29, 2025
@tekton-robot
Copy link
Collaborator

The following Tekton test failed:

Test name Commit Details Required Rerun command
pull-tekton-pipeline-go-coverage-df 94ec9c2 link true /test pull-tekton-pipeline-go-coverage-df

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/remoteresolution/resolver/git/resolver.go 81.8% 82.4% 0.5
pkg/resolution/resolver/git/resolver.go 83.9% 84.5% 0.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

5 participants