-
Notifications
You must be signed in to change notification settings - Fork 165
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(controller): allow GitHub App as authn method for image repos in ghcr #2391
Conversation
Signed-off-by: Dhouib-Mohamed <[email protected]>
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Dhouib-Mohamed <[email protected]>
8299d4a
to
38f3e0c
Compare
@Dhouib-Mohamed there's still about 81 more changed lines in this PR than I was expecting... |
I created the cache hit and the cache miss testes for both Git and Image because i maybe depends on the type of the credential since it is not just checking the inputs like secret and app id and it contains business logic |
@Dhouib-Mohamed if you look at how the cache keys are constructed, the repo type has no bearing on it. |
Signed-off-by: Dhouib-Mohamed <[email protected]>
b063d04
to
31c26d8
Compare
{ | ||
name: "cred type is git", | ||
credType: credentials.TypeGit, | ||
helper: &appCredentialHelper{}, | ||
assertions: func(t *testing.T, creds *credentials.Credentials, _ *cache.Cache, err error) { | ||
require.NoError(t, err) | ||
require.Nil(t, creds) | ||
}, | ||
}, | ||
{ | ||
name: "cred type is image", |
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.
These two new cases are still not needed. I know this may seem counter-intuitive, but remember this is white-box testing...
The previous case (cred type not supported) tests for an early return if the cred type is not supported. Subsequent cases only require the cred type to be a supported one so that we avoid that early return and move on to testing other outcomes. Again, with white-box testing, we have the luxury of knowing the implementation and we know that once we're past that potential early return, the cred type has no further bearing on the results.
Signed-off-by: Dhouib-Mohamed <[email protected]>
@Dhouib-Mohamed the changes look good. Thank you for your patience with the review process. Did you happen to test this end-to-end? i.e. Take it for a test drive? |
I tested a basic scenario (creating a credential from the ui) but i am not sure if i covered the necessary use cases |
@Dhouib-Mohamed this code doesn't really have anything to do with creating creds via the UI. This has to do with the back end retrieving temporary access tokens associated with a GitHub App. If I were testing this, I would write a small, standalone program that instantiates this credential helper and I would pass it a Kubernetes Secret I created myself, programmatically so test whether it returns the temporary token. If it does, I would copy that and test your ability to use that token to log into the Docker CLI and pull a private image from ghcr. |
Got it |
@krancour I have made the necessary tests and i have concluded that even with adding packages permissions to the Github app it still return a Not Authorized error when trying to interact with Github Packages ( images ). That's why i think that this PR should be closed for now. |
@Dhouib-Mohamed that discussion indeed looks like it doesn't work in our favor here. I'm going to a test a little as well and report back. One thing that I expect is a factor here -- and so I want to ask if you took it into account -- if the App has So:
As I said, I will experiment as well, but it is quite possible that you are correct and this isn't doable at the moment. |
@krancour So i tested on Git Repo level and on organization level and they both failed Do you want me to share the code i created to test this out to simplify your testing? |
@Dhouib-Mohamed I appreciate your thoroughness! I don't need the code... I think I can do that pretty quickly. It's the setup on the GitHub end of things that is the more time consuming part. 🤦♂️ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2391 +/- ##
==========================================
- Coverage 47.63% 47.61% -0.03%
==========================================
Files 244 244
Lines 17396 17394 -2
==========================================
- Hits 8287 8282 -5
- Misses 8698 8700 +2
- Partials 411 412 +1 ☔ View full report in Codecov by Sentry. |
@Dhouib-Mohamed thank you so much for the non-trivial amount of effort you put into this. I can independently confirm your findings. Despite all appearances (a package permissions option when configuring a GitHub App), an App installation token simply cannot enable access to a private ghcr.io package repository. I will close this PR for now, but it is my hope that someday this will work and this PR can be revived. |
Fixes #2383
Accepting credentials.TypeImage in the getCredentials condition
Adding Necessary tests linked to TypeImage to validate te changes