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

Simplify auto-generated pac-gitauth secret URL #1311

Merged
merged 1 commit into from
Jun 26, 2023
Merged

Simplify auto-generated pac-gitauth secret URL #1311

merged 1 commit into from
Jun 26, 2023

Conversation

Bibz87
Copy link
Contributor

@Bibz87 Bibz87 commented Jun 7, 2023

Changes

Changed GitLab's payload parser to get repository URL from project.get_http_url instead of project.web_url.

Fixes #1307

Submitter Checklist

  • ♽ Run make test lint before submitting a PR (ie: with pre-commit, no need to waste CPU cycle on CI
  • 📖 If you are adding a user facing feature or make a change of the behavior, please verify that you have documented it
  • 🧪 100% coverage is not a target but most of the time we would rather have a unit test if you make a code change.
  • 🎁 If that's something that is possible to do please ensure to check if we can add a e2e test.
  • 🔎 If there is a flakiness in the CI tests then don't necessary ignore it, better get the flakyness fixed before merging or if that's not possible there is a good reason to bypass it. (token rate limitation may be a good reason to skip).

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 7, 2023

In GitLab's installation instructions, the manual Repository creation example has been updated to include .git in the URL. However, I wasn't able to find the proper place for tkn pac create repo to add .git to a GitLab repository URL if it's missing.

@chmouel any idea of where that could be done? Thanks!

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 7, 2023

/retest

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 7, 2023

Not sure if I broke something with e2e tests, though...

@chmouel
Copy link
Member

chmouel commented Jun 8, 2023

/ok-to-test

@pipelines-as-code
Copy link

pipelines-as-code bot commented Jun 8, 2023

Golang test coverage difference report

Coverage unchanged. 🥈

Package report
package                                                                            before    after    delta
-------                                                                           -------  -------  -------
pkg/acl                                                                           100.00%  100.00%         
pkg/action                                                                         76.19%   76.19%         
pkg/adapter                                                                        72.41%   72.41%         
pkg/apis/features                                                                 100.00%  100.00%         
pkg/cli/info                                                                       88.23%   88.23%         
pkg/cli/prompt                                                                     74.46%   74.46%         
pkg/cli/status                                                                     95.23%   95.23%         
pkg/cli/webhook                                                                    59.36%   59.36%         
pkg/cmd/tknpac/bootstrap                                                            5.72%    5.72%         
pkg/cmd/tknpac/completion                                                          50.00%   50.00%         
pkg/cmd/tknpac/create                                                              43.36%   43.36%         
pkg/cmd/tknpac/describe                                                            46.31%   46.31%         
pkg/cmd/tknpac/generate                                                            62.20%   62.20%         
pkg/cmd/tknpac/info                                                                62.50%   62.50%         
pkg/cmd/tknpac/list                                                                46.47%   46.47%         
pkg/cmd/tknpac/resolve                                                             74.67%   74.67%         
pkg/cmd/tknpac/webhook                                                             52.47%   52.47%         
pkg/consoleui                                                                      84.12%   84.12%         
pkg/customparams                                                                   92.64%   92.64%         
pkg/events                                                                         73.33%   73.33%         
pkg/formatting                                                                     98.73%   98.73%         
pkg/git                                                                            84.84%   84.84%         
pkg/hub                                                                            90.62%   90.62%         
pkg/kubeinteraction                                                                52.50%   52.50%         
pkg/kubeinteraction/status                                                         77.27%   77.27%         
pkg/matcher                                                                        86.47%   86.47%         
pkg/params/clients                                                                 14.86%   14.86%         
pkg/params/settings                                                                79.48%   79.48%         
pkg/pipelineascode                                                                 80.95%   80.95%         
pkg/provider                                                                       76.19%   76.19%         
pkg/provider/bitbucketcloud                                                        87.16%   87.16%         
pkg/provider/bitbucketserver                                                       88.32%   88.32%         
pkg/provider/gitea                                                                 32.66%   32.66%         
pkg/provider/gitea/structs                                                         22.68%   22.68%         
pkg/provider/github                                                                83.03%   83.03%         
pkg/provider/github/app                                                            78.33%   78.33%         
pkg/provider/gitlab                                                                86.49%   86.49%         
pkg/random                                                                        100.00%  100.00%         
pkg/reconciler                                                                     46.10%   46.10%         
pkg/resolve                                                                        87.93%   87.93%         
pkg/secrets                                                                        93.02%   93.02%         
pkg/sort                                                                           51.20%   51.20%         
pkg/sync                                                                           91.13%   91.13%         
pkg/templates                                                                     100.00%  100.00%         
pkg/webhook                                                                        22.22%   22.22%         
                                                                          total:   67.23%   67.23%   +0.00%

@chmouel
Copy link
Member

chmouel commented Jun 8, 2023

#1313 should fix the doc generation error, you should be able to rebase to get the fix

@chmouel
Copy link
Member

chmouel commented Jun 8, 2023

for the linter error you can use this make target to get things fixed (as long you have markdownlint binary installed)

% make fix-markdownlint
Fixing markdown files...
Markdown is clean ✨

@chmouel
Copy link
Member

chmouel commented Jun 8, 2023

/ok-to-test

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 8, 2023

Thanks!

Any idea on how to handle tkn pac create repo?

@chmouel
Copy link
Member

chmouel commented Jun 8, 2023

give me a bit to think a bit about it, i don't think we need to force users using the .git url

we had that same issue for bitbucketcloud and i think if you set the CloneURL field properly in the parse payload it would do the right thing properly. ie see this comment

https://github.com/openshift-pipelines/pipelines-as-code/blob/main/pkg/secrets/basic_auth.go#L31

i am not totally sure tho so need to give it a try

@chmouel
Copy link
Member

chmouel commented Jun 8, 2023

(for e2e failing, there is some flakes going on with the generated pipelienrun that is not realted to your commit)

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 8, 2023

Is there any reason for the generated git authentication secret to use the full repository URL instead of just the host? That would fix the issue while having no impact on the Repository CRD.

-[credential "https://gitlab.example.com/my-project"]
+[credential "https://gitlab.example.com"]
   helper = store

@chmouel
Copy link
Member

chmouel commented Jun 8, 2023

that's a good point i don't think there is a reason, did you look if it's support greedy operator like start so we would not have to care about .git or not ?

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 8, 2023

What do you mean by "support greedy operator"? I don't understand that part.

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 20, 2023

@chmouel if you're talking about git's context matching behaviour, then according to git's documentation, git will match a context to a config section if the context is a subset of the configured section. e.g. https://example.com/foo.git would match [credential "https://example.com"]. That means that setting the config file to GitLab's base URL instead of being project-specific would let git match any project from that GitLab instance. As these credentials are tightly coupled to PipelineRuns that kind of change shouldn't have an impact on security, as long as given credentials are scoped properly within GitLab.

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 20, 2023

Also, from git's docs:

If the "pattern" URL [from the config section] does include a path component, then this too must match exactly: the context https://example.com/bar/baz.git will match a config entry for https://example.com/bar/baz.git (in addition to matching the config entry for https://example.com) but will not match a config entry for https://example.com/bar.

So having a config section that uses a GitLab group URL wouldn't match properly, either.

@chmouel
Copy link
Member

chmouel commented Jun 21, 2023

thanks for linking the documentation, yes that sounds good to me. we can just make that change in the git secret and that would work... This would be very useful as well on githup app and our granulated token scoping.

the existing e2e test on gitea for private repo should test that i think so no need to add one.

(and sorry for the long time to answer on this)

@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 21, 2023

Excellent, I'll get on that right away.

Thanks!

@Bibz87 Bibz87 marked this pull request as ready for review June 21, 2023 15:56
@chmouel
Copy link
Member

chmouel commented Jun 21, 2023

/retest

@chmouel
Copy link
Member

chmouel commented Jun 23, 2023

this look good to me,

any chance you can modify https://pipelinesascode.com/docs/guide/privaterepo/ and add a note that we are adding the URL of the git provider host (i.e https://github.com or https://gitlab.com) as url in gitconfig

and if you can finally squash all commits as one so we are good to merge

@Bibz87 Bibz87 changed the title Fix GitLab repository URL parsing Simplify auto-generated pac-gitauth secret URL Jun 23, 2023
@Bibz87
Copy link
Contributor Author

Bibz87 commented Jun 23, 2023

this look good to me,

any chance you can modify https://pipelinesascode.com/docs/guide/privaterepo/ and add a note that we are adding the URL of the git provider host (i.e https://github.com or https://gitlab.com) as url in gitconfig

and if you can finally squash all commits as one so we are good to merge

Done!

Squash-merging from Pull Requests can also be enforced via project settings. :)

@chmouel
Copy link
Member

chmouel commented Jun 26, 2023

/ok-to-test

Squash-merging from Pull Requests can also be enforced via project settings. :)

yeah sometime it's okay to have multiple feature in a PR (like having a refactoring commit and then a feature/bugfix commit) if clearly identified, admitely i can just hit the dropdown of squashing commit on demand if needed as well but i probably going to forget it :)

@@ -18,6 +18,12 @@ This secret contains a [Git Config](https://git-scm.com/docs/git-config) file:
file: .git-credentials, which includes the https URL using the token obtained
from the GitHub application or secret attached to the repo CR.

{{< hint info >}} For compatibility, the [Git
Copy link
Member

Choose a reason for hiding this comment

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

i don't think we need multiple hints which is close to the other one and the info is a bit confusing for the end user... but no worries let's merge this and i'll rephrase that thereafter.

image

@chmouel chmouel merged commit 0c393f1 into openshift-pipelines:main Jun 26, 2023
4 checks passed
@Bibz87 Bibz87 deleted the fix/gitlab-repo-url branch June 26, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Unable to clone private GitLab repository with auto-generated pac-gitauth secret
2 participants