-
Notifications
You must be signed in to change notification settings - Fork 87
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
Switch Radius Helm chart pull from ACR to GHCR #7455
Changes from 7 commits
28c3731
b68b464
1878b92
9bad8e3
8efe737
be3c5fd
c8cf5c8
834e416
66a1a7e
b847900
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,15 +20,18 @@ import ( | |
_ "embed" | ||
"errors" | ||
"fmt" | ||
"net/http" | ||
"os" | ||
"path/filepath" | ||
"strings" | ||
"time" | ||
|
||
containerderrors "github.com/containerd/containerd/remotes/errors" | ||
helm "helm.sh/helm/v3/pkg/action" | ||
"helm.sh/helm/v3/pkg/chart" | ||
"helm.sh/helm/v3/pkg/chart/loader" | ||
"helm.sh/helm/v3/pkg/cli" | ||
"helm.sh/helm/v3/pkg/registry" | ||
"k8s.io/cli-runtime/pkg/genericclioptions" | ||
) | ||
|
||
|
@@ -79,7 +82,6 @@ func locateChartFile(dirPath string) (string, error) { | |
|
||
func helmChartFromContainerRegistry(version string, config *helm.Configuration, repoUrl string, releaseName string) (*chart.Chart, error) { | ||
pull := helm.NewPull() | ||
pull.RepoURL = repoUrl | ||
pull.Settings = &cli.EnvSettings{} | ||
pullopt := helm.WithConfig(config) | ||
pullopt(pull) | ||
|
@@ -101,8 +103,42 @@ func helmChartFromContainerRegistry(version string, config *helm.Configuration, | |
|
||
pull.DestDir = dir | ||
|
||
_, err = pull.Run(releaseName) | ||
var chartRef string | ||
|
||
if !registry.IsOCI(repoUrl) { | ||
// For non-OCI registries (like contour), we need to set the repo URL | ||
// to the registry URL. The chartRef is the release name. | ||
// ex. | ||
// pull.RepoURL = https://charts.bitnami.com/bitnami | ||
// pull.Run("contour") | ||
pull.RepoURL = repoUrl | ||
chartRef = releaseName | ||
} else { | ||
// For OCI registries (like radius), we will use the | ||
// repo URL + the releaseName as the chartRef. | ||
// pull.Run("oci://ghcr.io/radius-project/helm-chart/radius") | ||
chartRef = fmt.Sprintf("%s/%s", repoUrl, releaseName) | ||
|
||
// Since we are using an OCI registry, we need to set the registry client | ||
registryClient, err := registry.NewClient() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
pull.SetRegistryClient(registryClient) | ||
} | ||
|
||
_, err = pull.Run(chartRef) | ||
if err != nil { | ||
// Error handling for a specific case where credentials are stale. | ||
// This happens for ghcr in particular because ghcr does not use | ||
// subdomains - the scope of a login is all of ghcr.io. | ||
// https://github.com/helm/helm/issues/12584 | ||
err := extractHelmError(err) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return nil, fmt.Errorf("error downloading helm chart from the registry for version: %s, release name: %s. Error: %w", version, releaseName, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest using Basically it's helpful to know the difference between:
|
||
} | ||
|
||
|
@@ -136,3 +172,17 @@ func runUpgrade(upgradeClient *helm.Upgrade, releaseName string, helmChart *char | |
} | ||
return err | ||
} | ||
|
||
// extractHelmError is a helper function to extract a specific error | ||
// (403 unauthorized when downloading a helm chart from ghcr.io) from a chain of errors. | ||
// If the error is not found, it returns nil. | ||
func extractHelmError(err error) error { | ||
var errUnexpectedStatus containerderrors.ErrUnexpectedStatus | ||
if errors.As(err, &errUnexpectedStatus) { | ||
if errUnexpectedStatus.StatusCode == http.StatusForbidden && strings.Contains(errUnexpectedStatus.RequestURL, "ghcr.io") { | ||
return fmt.Errorf("recieved 403 unauthorized when downloading helm chart from the registry. you may want to perform a `docker logout ghcr.io` and re-try the command") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest using |
||
} | ||
} | ||
|
||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to also handle the case where the error is NOT an Every time we special case one type of error, we need to also handle the case where it's some other type of error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, nevermind. I see you did that above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest putting this line in here as well for clarity: https://github.com/radius-project/radius/pull/7455/files#diff-35b8f26ef00e63853d6a8303dfdb8755355d7cb6d48ba9449d5a04af62a723cbR142 |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package helm | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"net/http" | ||
"testing" | ||
|
||
containerderrors "github.com/containerd/containerd/remotes/errors" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestExtractHelmError(t *testing.T) { | ||
err := errors.New("error") | ||
extractedErr := extractHelmError(err) | ||
assert.Nil(t, extractedErr) | ||
|
||
err = fmt.Errorf("%w: wrapped error", errors.New("error")) | ||
extractedErr = extractHelmError(err) | ||
assert.Nil(t, extractedErr) | ||
|
||
err = fmt.Errorf("%w: wrapped error", containerderrors.ErrUnexpectedStatus{}) | ||
extractedErr = extractHelmError(err) | ||
assert.Nil(t, extractedErr) | ||
|
||
err = fmt.Errorf("%w: wrapped error", containerderrors.ErrUnexpectedStatus{StatusCode: http.StatusForbidden, RequestURL: "ghcr.io/myregistry"}) | ||
extractedErr = extractHelmError(err) | ||
assert.Equal(t, errors.New("recieved 403 unauthorized when downloading helm chart from the registry. you may want to perform a `docker logout ghcr.io` and re-try the command"), extractedErr) | ||
|
||
err = containerderrors.ErrUnexpectedStatus{StatusCode: http.StatusForbidden, RequestURL: "ghcr.io/myregistry"} | ||
extractedErr = extractHelmError(err) | ||
assert.Equal(t, errors.New("recieved 403 unauthorized when downloading helm chart from the registry. you may want to perform a `docker logout ghcr.io` and re-try the command"), extractedErr) | ||
} |
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.
Should we check if repoUrl is of this type
oci://ghcr.io/radius-project
?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 code should work for all oci registries if we need to add them in the future. So I don't think we need to hard code it. the
registry.IsOCI()
call should determine if it's an oci registry or not.