[Spike] Fix controller generating cilium manifests with registry mirror #7139
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Skipping CI for Draft Pull Request. |
eks-distro-bot
added
the
size/L
cxbrowne1207
force-pushed
the
fix-airgapped-cilium-controller-upgrade
branch
from
5494db2 to
fb6fcd0
Compare
| @@ -112,6 +127,20 @@ func WithPolicyAllowedNamespaces(namespaces []string) ManifestOpt { | |||
| } | |||
| } | |||
|
|
|||
| func (t *Templater) registryLogin(ctx context.Context, helm Helm, spec *cluster.Spec) error { | |||
There was a problem hiding this comment.
looks like this "t" is not used, it can be a function instead of a method of Templater
pkg/networking/cilium/templater.go
Outdated
| r := registrymirror.FromCluster(spec.Cluster) | ||
| helm := t.helmFactory.GetInstance(executables.WithRegistryMirror(r)) | ||
|
|
||
| if spec.Cluster.Spec.RegistryMirrorConfiguration != nil { |
There was a problem hiding this comment.
This if check is duplicated in registryLogin function
| } | ||
|
|
||
| func (f *HelmFactory) GetInstance(opts ...executables.HelmOpt) *executables.Helm { | ||
| f.mu.Lock() |
There was a problem hiding this comment.
Curious, do we really need a lock here?
| func (f *Factory) WithHelmFactory(opts ...executables.HelmOpt) *Factory { | ||
| f.WithExecutableBuilder() | ||
|
|
||
| f.buildSteps = append(f.buildSteps, func(ctx context.Context) error { |
There was a problem hiding this comment.
Overall, factory is not really a preferred abstract in golang. As we have adopted it, we should try not making it more complicated.
This method introduces something feels like "nested factory", which makes the abstract harder to understand. I believe we can achieve the target without this "helmFactory"
cxbrowne1207
changed the title
cxbrowne1207
changed the title
Issue #, if available:
Description of changes:
The CNI reconciler fails to generate cilium manifests in an airgapped environment because it tries to fetch the image from public.ecr.aws instead of the registry mirror. This is because the registry mirror for the helm executable reference in the cilium templater is never set when managing a cluster using FLC, so when generating the manifests helm , the logic that replaces the host in the image uri is skipped
This PR addresses the issue by enabling the controller to handle helm charts when reconciling the CNI is to construct the
Helminstance in the controller. Instead of depending directly on the Helm, we can inject a HelmFactory to the cilium templater. We can then use theHelmFactoryto create an instance ofHelmwith a registry mirror configuration if one is defined on theClusterobject.Testing (if applicable):
Documentation added/planned (if applicable):
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.