-
Notifications
You must be signed in to change notification settings - Fork 264
feat: add GAR Helm chart support to credential providers #4637
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
base: main
Are you sure you want to change the base?
feat: add GAR Helm chart support to credential providers #4637
Conversation
- Add TypeHelm support to GAR ServiceAccountKeyProvider and WorkloadIdentityFederationProvider - Update regex patterns to handle oci:// prefix for GAR Helm charts Fixes "Unauthenticated request" errors when Kargo warehouses discover Helm charts from Google Artifact Registry. Signed-off-by: Filip Nowak <filip@airspace-intelligence.com>
Add documentation for authenticating with Google Artifact Registry OCI Helm chart repositories, including credential setup and warehouse subscription examples. Signed-off-by: Filip Nowak <filip@airspace-intelligence.com>
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Is there anything else I have to do on my side? @hiddeco |
For OCI Helm chart repositories (like Google Artifact Registry), use the `oci://` prefix and omit the `name` field: | ||
|
||
```yaml | ||
spec: | ||
subscriptions: | ||
- chart: | ||
repoURL: oci://us-central1-docker.pkg.dev/my-project/my-helm-charts/my-chart | ||
semverConstraint: ^1.0.0 | ||
``` | ||
|
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 think this was covered already on lines 634 - 636.
For OCI Helm chart repositories (like Google Artifact Registry), use the `oci://` prefix and omit the `name` field: | |
```yaml | |
spec: | |
subscriptions: | |
- chart: | |
repoURL: oci://us-central1-docker.pkg.dev/my-project/my-helm-charts/my-chart | |
semverConstraint: ^1.0.0 | |
``` |
## Working with Private Repositories | ||
|
||
Frequently, `Warehouse`s require access to private repositories, in which case | ||
appropriate credentials must be made available in some form. The many available | ||
authentication options are covered in detail on the | ||
[Managing Credentials](../50-security/30-managing-credentials.md) page. | ||
|
||
|
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 why this line was added, but most markdown linters do not like double line breaks.
@@ -532,6 +532,21 @@ stringData: | |||
repoURL: <artifact registry url> |
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 may be worth replacing or augmenting this with an example, since image repo URLs do not have leading oci://
while chart repo URLs do.
kargo.akuity.io/cred-type: helm | ||
stringData: | ||
gcpServiceAccountKey: <base64-encoded service account key> | ||
repoURL: <oci helm chart repository url> |
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.
Same as here.
@@ -532,6 +532,21 @@ stringData: | |||
repoURL: <artifact registry url> | |||
``` | |||
|
|||
For OCI Helm chart repositories, the `Secret` should be labeled with `helm` instead: |
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.
This feels like an "afterthought." I'd suggest that we rewind all the way to line 520 and introduce the two examples like:
To use this option, your `Secret` should take the following form:
* For a container image repository:
```yaml
< example goes here>
```
* For an OCI Helm chart repository:
```yaml
< example goes here>
```
@@ -37,7 +38,13 @@ func NewServiceAccountKeyProvider() credentials.Provider { | |||
} | |||
|
|||
func (p *ServiceAccountKeyProvider) Supports(credType credentials.Type, repoURL string, data map[string][]byte) bool { | |||
if credType != credentials.TypeImage || data == nil || data[serviceAccountKeyKey] == nil { | |||
if (credType != credentials.TypeImage && credType != credentials.TypeHelm) || |
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.
Nit: This feels easier to comprehend, although this isn't a hill I'd die on. Up to you.
if (credType != credentials.TypeImage && credType != credentials.TypeHelm) || | |
if !(credType == credentials.TypeImage || credType == credentials.TypeHelm) || |
fakeGCRRepoURL = "gcr.io/my-project/my-repo" | ||
fakeGARRepoURL = "us-central1-docker.pkg.dev/my-project/my-repo" | ||
fakeGCROCIRepoURL = "oci://gcr.io/my-project/my-repo" | ||
fakeGAROCIRepoURL = "oci://us-central1-docker.pkg.dev/my-project/my-repo" |
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.
All of these are OCI. Helm charts are just more in your face about the fact because, for whatever reason, Helm chose to use a leading oci://
with URLs for OCI repos. Kargo just follows their lead, because if that's what Helm users are accustomed to, who are we to argue?
I would suggest naming these this way for accuracy and clarity:
fakeGCRRepoURL = "gcr.io/my-project/my-repo" | |
fakeGARRepoURL = "us-central1-docker.pkg.dev/my-project/my-repo" | |
fakeGCROCIRepoURL = "oci://gcr.io/my-project/my-repo" | |
fakeGAROCIRepoURL = "oci://us-central1-docker.pkg.dev/my-project/my-repo" | |
fakeGCRImageRepoURL = "gcr.io/my-project/my-repo" | |
fakeGARImageRepoURL = "us-central1-docker.pkg.dev/my-project/my-repo" | |
fakeGCRChartRepoURL = "oci://gcr.io/my-project/my-repo" | |
fakeGARChartRepoURL = "oci://us-central1-docker.pkg.dev/my-project/my-repo" |
gcrURLRegex = regexp.MustCompile(`^(?:oci://)?(?:.+\.)?gcr\.io/`) // Legacy | ||
garURLRegex = regexp.MustCompile(`^(?:oci://)?(.+-docker\.pkg\.dev/)`) |
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.
The scope of this PR creeps too much if we want to fix this now, so I'd like to just leave this note here for myself.
gcrURLRegex = regexp.MustCompile(`^(?:oci://)?(?:.+\.)?gcr\.io/`) // Legacy | |
garURLRegex = regexp.MustCompile(`^(?:oci://)?(.+-docker\.pkg\.dev/)`) | |
// TODO(krancour): Repo URLs are not currently normalized prior to credential | |
// providers being invoked. When that is fixed, the optional leading `oci://` | |
// can be removed from these regular expressions. | |
gcrURLRegex = regexp.MustCompile(`^(?:oci://)?(?:.+\.)?gcr\.io/`) // Legacy | |
garURLRegex = regexp.MustCompile(`^(?:oci://)?(.+-docker\.pkg\.dev/)`) |
fakeProjectID = "test-project" | ||
fakeGCRRepoURL = "gcr.io/my-project/my-repo" | ||
fakeGARRepoURL = "us-central1-docker.pkg.dev/my-project/my-repo" | ||
fakeGCROCIRepoURL = "oci://gcr.io/my-project/my-repo" | ||
fakeGAROCIRepoURL = "oci://us-central1-docker.pkg.dev/my-project/my-repo" |
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.
Same as here.
Thank you for this @filip-aipl. I left some minor, easily-addressable comments. This all checks out end-to-end. As soon as this is cleaned up, I'll approve. Thanks again! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4637 +/- ##
==========================================
+ Coverage 54.16% 54.17% +0.01%
==========================================
Files 395 395
Lines 33896 33904 +8
==========================================
+ Hits 18359 18367 +8
Misses 14608 14608
Partials 929 929 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixes #4801
Summary
Adds Google Artifact Registry (GAR) Helm chart support to existing credential providers.
Changes
credentials.TypeHelm
support in GAR ServiceAccountKeyProvider and WorkloadIdentityFederationProvideroci://
prefix for GAR/GCR Helm chart URLsProblem Solved
Fixes "Unauthenticated request" errors when Kargo warehouses attempt to discover Helm charts from Google Artifact Registry repositories.
Testing