Panic with an invalid AWS OCI URL for HelmRepository

[adrien-f]

Hi 👋

While playing around, I noticed this panic:

E0913 12:40:20.412216       1 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
goroutine 523 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x4f08a0?, 0x3fd92a0})
	k8s.io/[email protected]/pkg/util/runtime/runtime.go:75 +0x99
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:110 +0xb9
panic({0x4f08a0, 0x3fd92a0})
	runtime/panic.go:838 +0x207
github.com/fluxcd/source-controller/internal/helm/registry.OIDCAdaptHelper({0x0?, 0x0?})
	github.com/fluxcd/source-controller/internal/helm/registry/auth.go:78 +0x22
github.com/fluxcd/source-controller/controllers.oidcAuthFromAdapter({0xe23c10, 0xc0009da420}, {0xc0009e6600, 0x32}, {0xc000d9b09c, 0x3})
	github.com/fluxcd/source-controller/controllers/helmrepository_controller_oci.go:387 +0x134
github.com/fluxcd/source-controller/controllers.(*HelmChartReconciler).buildFromHelmRepository(0xc0004fb380, {0xe23c48, 0xc029d00c60}, 0xc000d9c480, 0xc029d30000, 0xc0009da300)
	github.com/fluxcd/source-controller/controllers/helmchart_controller.go:520 +0xb35
github.com/fluxcd/source-controller/controllers.(*HelmChartReconciler).reconcileSource(0xc0004fb380, {0xe23c48, 0xc029d00c60}, 0xc000d9c480, 0xc0009da300)
	github.com/fluxcd/source-controller/controllers/helmchart_controller.go:439 +0x767
github.com/fluxcd/source-controller/controllers.(*HelmChartReconciler).reconcile(0xe36380?, {0xe23c48, 0xc029d00c60}, 0xc000d9c480, {0xc029d47c48, 0x3, 0x0?})
	github.com/fluxcd/source-controller/controllers/helmchart_controller.go:277 +0x1a4
github.com/fluxcd/source-controller/controllers.(*HelmChartReconciler).Reconcile(0xc0004fb380, {0xe23c48, 0xc029d00c60}, {{{0xc000e28a10?, 0x10?}, {0xc000594c78?, 0x21604c7?}}})
	github.com/fluxcd/source-controller/controllers/helmchart_controller.go:255 +0x55c
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0xe23ba0?, {0xe23c48?, 0xc029d00c60?}, {{{0xc000e28a10?, 0x7775a0?}, {0xc000594c78?, 0x21559f4?}}})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:121 +0xc8
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc000b4d0e0, {0xe23ba0, 0xc0007cfa80}, {0x5bd3a0?, 0xc00007e600?})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:320 +0x33c
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc000b4d0e0, {0xe23ba0, 0xc0007cfa80})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:273 +0x1d9
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:234 +0x85
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:230 +0x325
{"level":"error","ts":"2022-09-13T12:40:20.412Z","msg":"Reconciler error","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","helmChart":{"name":"flux-system-ms-test-afi","namespace":"flux-system"},"namespace":"flux-system","name":"flux-system-ms-test","reconcileID":"ab21fff2-de54-4187-ae2d-db775d4ecfaf","error":"panic: runtime error: invalid memory address or nil pointer dereference [recovered]"}

This can be replicated with v0.29.0 and the following HelmRepository:

apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: HelmRepository
metadata:
  name: xxx
  namespace: flux-system
spec:
  interval: 10m
  provider: aws
  type: oci
  url: oci://<account>.dkr.ecr.<region>.amazonaws.com

The URL doesn't match the regex used in pkg/oci:

https://github.com/fluxcd/pkg/blob/dbad05cf95b380c6f619a9bf76dc755c6ff6e3cc/oci/auth/aws/auth.go#L36-L47

Which results in returning oci.ProviderGeneric which is handled by returning nil, nil later

This is obviously an invalid URL but the panic doesn't help to find the error, which can be handled here:

source-controller/controllers/helmrepository_controller_oci.go

Lines 378 to 386 in 9e853a9

	// oidcAuthFromAdapter generates the OIDC credential authenticator based on the specified cloud provider.
	func oidcAuthFromAdapter(ctx context.Context, url, provider string) (helmreg.LoginOption, error) {
	auth, err := oidcAuth(ctx, url, provider)
	if err != nil {
	return nil, err
	}
	return registry.OIDCAdaptHelper(auth)
	}

L380, auth can be nil and is not checked. I'm up for a fix but which behavior would you like to have? Something like this maybe?

	if auth == nil {
		return nil, fmt.Errorf("could not validate OCI provider %s with URL %s", provider, url)
	}

Or update the behavior in pkg/oci instead to raise an error?

[souleb]

thanks for raising this @adrien-f. I think just adding the check is the way to go.

[makkes]

@adrien-f would you like to submit a PR fixing the issue?

[stefanprodan]

I think we could return the error in the oidcAuth function, under the switch as default.

[adrien-f]

I think we could return the error in the oidcAuth function, under the switch as default.

This actually happens a bit later in login.NewManager().Login() in the ImageRegistryProvider(image, ref) swich here, if we want to catch this edge case before, we'd have to regex-match before and that's what this method does already.

I've attempted a PR in #897 where we simply handle if the resulting authenticator is nil 🤗

[makkes]

I'd prefer for login.NewManager().Login() to actually return either a non-nil error or a non-nil Authenticator but never both. However, I understand that OCIRepositoryReconciler currently relies on these semantics so I'm fine just fixing the immediate bug for now.