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

Fix SIGSEGV when resolving charts dependencies #827

Merged
merged 1 commit into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion internal/helm/chart/dependency_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ func NewDependencyManager(opts ...DependencyManagerOption) *DependencyManager {
func (dm *DependencyManager) Clear() error {
var errs []error
for _, v := range dm.downloaders {
errs = append(errs, v.Clear())
if v != nil {
errs = append(errs, v.Clear())
}
}
return errors.NewAggregate(errs)
}
Expand Down Expand Up @@ -257,6 +259,10 @@ func (dm *DependencyManager) resolveRepository(url string) (repo repository.Down
defer dm.mu.Unlock()

nUrl := repository.NormalizeURL(url)
err = repository.ValidateDepURL(nUrl)
if err != nil {
return
}
if _, ok := dm.downloaders[nUrl]; !ok {
if dm.getChartDownloaderCallback == nil {
err = fmt.Errorf("no chart repository for URL '%s'", nUrl)
Expand Down
9 changes: 9 additions & 0 deletions internal/helm/chart/dependency_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ func TestDependencyManager_Clear(t *testing.T) {
},
"with credentials": ociRepoWithCreds,
"without credentials": &repository.OCIChartRepository{},
"nil downloader": nil,
}

dm := NewDependencyManager(WithRepositories(downloaders))
Expand Down Expand Up @@ -428,6 +429,14 @@ func TestDependencyManager_addRemoteDependency(t *testing.T) {
},
wantErr: "no chart repository for URL",
},
{
name: "resolve aliased repository error",
downloaders: map[string]repository.Downloader{},
dep: &helmchart.Dependency{
Repository: "@fantastic-charts",
},
wantErr: "aliased repository dependency is not supported",
},
{
name: "strategic load error",
downloaders: map[string]repository.Downloader{
Expand Down
28 changes: 28 additions & 0 deletions internal/helm/repository/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,23 @@ limitations under the License.
package repository

import (
"fmt"
"strings"

helmreg "helm.sh/helm/v3/pkg/registry"
)

const (
alias = "@"
hiddeco marked this conversation as resolved.
Show resolved Hide resolved
)

var (
// errInvalidDepURL is returned when the dependency URL is not supported
errInvalidDepURL = fmt.Errorf("invalid dependency repository URL")
// errInvalidAliasedDep is returned when the dependency URL is an alias
errInvalidAliasedDep = fmt.Errorf("aliased repository dependency is not supported")
)

// NormalizeURL normalizes a ChartRepository URL by its scheme.
func NormalizeURL(repositoryURL string) string {
if repositoryURL == "" {
Expand All @@ -35,3 +47,19 @@ func NormalizeURL(repositoryURL string) string {
return strings.TrimRight(repositoryURL, "/") + "/"

}

// ValidateDepURL returns an error if the given depended repository URL declaration is not supported
// The reason for this is that the dependency manager will not be able to resolve the alias declaration
// e.g. repository: "@fantastic-charts"
func ValidateDepURL(repositoryURL string) error {
switch {
case strings.HasPrefix(repositoryURL, helmreg.OCIScheme):
return nil
case strings.HasPrefix(repositoryURL, "https://") || strings.HasPrefix(repositoryURL, "http://"):
return nil
case strings.HasPrefix(repositoryURL, alias):
return fmt.Errorf("%w: %s", errInvalidAliasedDep, repositoryURL)
default:
return fmt.Errorf("%w: %s", errInvalidDepURL, repositoryURL)
}
}