Fix controller generating cilium manifests with registry mirror #7170

cxbrowne1207 · 2023-12-13T07:31:11Z

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 Helm client in the controller. Instead of depending directly on the Helm executable client, we inject a HelmFactory to the cilium templater. We can then use the HelmFactory to create an instance of a HelmClient configured for the registry mirror of the cluster.

Testing (if applicable):

Created a new airgap upgrade from latest minor release e2e test for Docker which will test this flow during kindless upgrades. This is only necessarily "airgapped" before the upgrade process.
Test manually by creating a docker cluster in a simulated airgapped environment and upgrading from latest minor release where cilium is updated.
Ran a couple of e2e test with the following scenarios generally testing the change:
- Upgrade from latest minor release (CLI and FLC)
- Creating Management cluster, creating workload cluster and upgrading workload cluster
- Create Management cluster, creating workload cluster with API, and upgrading workload cluster through FLC

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.

eks-distro-bot · 2023-12-13T07:31:16Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cxbrowne1207 · 2023-12-13T09:16:32Z

/test all

codecov · 2023-12-13T09:21:32Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (1d5a8a4) 71.58% compared to head (49e7146) 71.65%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
pkg/executables/helm.go	70.37%	8 Missing ⚠️
pkg/dependencies/factory.go	83.78%	5 Missing and 1 partial ⚠️
cmd/eksctl-anywhere/cmd/deprecated_importimages.go	0.00%	3 Missing ⚠️
pkg/networking/cilium/templater.go	72.72%	2 Missing and 1 partial ⚠️
cmd/eksctl-anywhere/cmd/downloadimages.go	0.00%	2 Missing ⚠️
cmd/eksctl-anywhere/cmd/import_images.go	0.00%	2 Missing ⚠️
pkg/curatedpackages/customregistry.go	0.00%	2 Missing ⚠️
pkg/executables/builder.go	33.33%	2 Missing ⚠️
cmd/eksctl-anywhere/cmd/common.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7170      +/-   ##
==========================================
+ Coverage   71.58%   71.65%   +0.07%     
==========================================
  Files         545      548       +3     
  Lines       42362    42494     +132     
==========================================
+ Hits        30324    30451     +127     
- Misses      10345    10349       +4     
- Partials     1693     1694       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cxbrowne1207 · 2023-12-13T14:26:08Z

/test eks-anywhere-presubmit

pkg/cluster/helm.go

pkg/config/registry.go

pkg/config/registry_test.go

pkg/dependencies/factory.go

pkg/networking/cilium/templater.go

test/e2e/airgap.go

test/e2e/docker_test.go

pkg/config/registry.go

cxbrowne1207 · 2023-12-14T16:34:59Z

/test eks-anywhere-presubmit

controllers/factory.go

pkg/executables/helm.go

pkg/helm/envclient_factory.go

pkg/helm/factory.go

pkg/networking/cilium/templater.go

…om management cluster

…id of client under pkg/cluster/helm

…implement helm executable builder

pkg/dependencies/factory.go

pkg/helm/factory.go

g-gaston

/lgtm

g-gaston · 2023-12-15T21:05:52Z

/lgtm
/chery-pick release-0.18
/chery-pick release-0.17

cxbrowne1207 · 2023-12-15T21:21:17Z

/approve

eks-distro-bot · 2023-12-15T21:21:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cxbrowne1207

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cxbrowne1207]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cxbrowne1207 · 2023-12-15T21:32:50Z

/cherry-pick release-0.18

eks-distro-pr-bot · 2023-12-15T21:33:26Z

@cxbrowne1207: #7170 failed to apply on top of branch "release-0.18":

Applying: generate helm from a factory to load registry mirror configuration from management cluster
Using index info to reconstruct a base tree...
M	Makefile
M	controllers/factory.go
M	pkg/dependencies/factory.go
M	pkg/networking/cilium/templater.go
M	pkg/networking/cilium/templater_test.go
M	test/e2e/docker_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/docker_test.go
Auto-merging pkg/networking/cilium/templater_test.go
CONFLICT (content): Merge conflict in pkg/networking/cilium/templater_test.go
Auto-merging pkg/networking/cilium/templater.go
Auto-merging pkg/dependencies/factory.go
Auto-merging controllers/factory.go
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 generate helm from a factory to load registry mirror configuration from management cluster
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.18
/cherry-pick release-0.17

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cxbrowne1207 · 2023-12-15T21:37:01Z

/cherry-pick release-0.17

eks-distro-pr-bot · 2023-12-15T21:37:38Z

@cxbrowne1207: #7170 failed to apply on top of branch "release-0.17":

Applying: generate helm from a factory to load registry mirror configuration from management cluster
Using index info to reconstruct a base tree...
M	Makefile
M	controllers/cluster_controller.go
M	controllers/factory.go
M	pkg/dependencies/factory.go
M	pkg/dependencies/factory_test.go
M	pkg/networking/cilium/templater.go
M	pkg/networking/cilium/templater_test.go
M	test/e2e/airgap.go
M	test/e2e/docker_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/docker_test.go
Auto-merging test/e2e/airgap.go
Auto-merging pkg/networking/cilium/templater_test.go
CONFLICT (content): Merge conflict in pkg/networking/cilium/templater_test.go
Auto-merging pkg/networking/cilium/templater.go
Auto-merging pkg/dependencies/factory_test.go
Auto-merging pkg/dependencies/factory.go
Auto-merging controllers/factory.go
Auto-merging controllers/cluster_controller.go
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 generate helm from a factory to load registry mirror configuration from management cluster
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.17

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…7170) * generate helm from a factory to load registry mirror configuration from management cluster * move helmfactory to helm package (no other changes) * decoupled executables from helm factory, renamed helm tyoes and got rid of client under pkg/cluster/helm * changes to other files due to change executable builder signature to implement helm executable builder * add helm env client factory implementation for cilium.HelmClientFactory * remove helm.ClientFactory dependency on ops it doesn't need * rename GetClientForCluster -> Get and addressed other PR comments * move WithHelmClientFactory to controller factory and build cilium Templater there and added comments * add generated files * dropping registry client and executableclient interfaces in favor of just client * re-build client only when registry changes * fix linting error * moved WithEnv implementaton details back to executable helm * add new helm executeable unit test * always return new client in helm client factory and add comment to ProxyConfig * refactored ExecutableBuilder -> ClientBuilder * extract code from poluted pkg/helm/factory.go file into client.go and config.go * remove helmClient from struct. not needed to store * referencing implementation instead of interface in factory dependencies * change joinEnv to mergeMap

… (#7184) * generate helm from a factory to load registry mirror configuration from management cluster * move helmfactory to helm package (no other changes) * decoupled executables from helm factory, renamed helm tyoes and got rid of client under pkg/cluster/helm * changes to other files due to change executable builder signature to implement helm executable builder * add helm env client factory implementation for cilium.HelmClientFactory * remove helm.ClientFactory dependency on ops it doesn't need * rename GetClientForCluster -> Get and addressed other PR comments * move WithHelmClientFactory to controller factory and build cilium Templater there and added comments * add generated files * dropping registry client and executableclient interfaces in favor of just client * re-build client only when registry changes * fix linting error * moved WithEnv implementaton details back to executable helm * add new helm executeable unit test * always return new client in helm client factory and add comment to ProxyConfig * refactored ExecutableBuilder -> ClientBuilder * extract code from poluted pkg/helm/factory.go file into client.go and config.go * remove helmClient from struct. not needed to store * referencing implementation instead of interface in factory dependencies * change joinEnv to mergeMap

… (#7185) * generate helm from a factory to load registry mirror configuration from management cluster * move helmfactory to helm package (no other changes) * decoupled executables from helm factory, renamed helm tyoes and got rid of client under pkg/cluster/helm * changes to other files due to change executable builder signature to implement helm executable builder * add helm env client factory implementation for cilium.HelmClientFactory * remove helm.ClientFactory dependency on ops it doesn't need * rename GetClientForCluster -> Get and addressed other PR comments * move WithHelmClientFactory to controller factory and build cilium Templater there and added comments * add generated files * dropping registry client and executableclient interfaces in favor of just client * re-build client only when registry changes * fix linting error * moved WithEnv implementaton details back to executable helm * add new helm executeable unit test * always return new client in helm client factory and add comment to ProxyConfig * refactored ExecutableBuilder -> ClientBuilder * extract code from poluted pkg/helm/factory.go file into client.go and config.go * remove helmClient from struct. not needed to store * referencing implementation instead of interface in factory dependencies * change joinEnv to mergeMap

eks-distro-bot added the do-not-merge/work-in-progress label Dec 13, 2023

eks-distro-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 13, 2023

cxbrowne1207 force-pushed the fix-airgapped-cilium-controller-upgrade branch 3 times, most recently from 1b8aa61 to 6a9ada4 Compare December 13, 2023 09:16

cxbrowne1207 marked this pull request as ready for review December 13, 2023 09:25

eks-distro-bot removed the do-not-merge/work-in-progress label Dec 13, 2023

cxbrowne1207 requested a review from g-gaston December 13, 2023 14:52

g-gaston reviewed Dec 13, 2023

View reviewed changes

eks-distro-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 14, 2023

cxbrowne1207 force-pushed the fix-airgapped-cilium-controller-upgrade branch 2 times, most recently from 92b2414 to c1d332a Compare December 14, 2023 06:04

cxbrowne1207 force-pushed the fix-airgapped-cilium-controller-upgrade branch from 01bdf87 to 09d5618 Compare December 14, 2023 10:31

cxbrowne1207 requested a review from g-gaston December 14, 2023 16:18

g-gaston reviewed Dec 14, 2023

View reviewed changes

cxbrowne1207 added 5 commits December 14, 2023 20:31

generate helm from a factory to load registry mirror configuration fr…

43bb892

…om management cluster

move helmfactory to helm package (no other changes)

d117e1c

decoupled executables from helm factory, renamed helm tyoes and got r…

907e106

…id of client under pkg/cluster/helm

changes to other files due to change executable builder signature to …

cffec16

…implement helm executable builder

add helm env client factory implementation for cilium.HelmClientFactory

c22ae36

cxbrowne1207 force-pushed the fix-airgapped-cilium-controller-upgrade branch from f0bedd7 to a6086df Compare December 15, 2023 18:51

g-gaston reviewed Dec 15, 2023

View reviewed changes

pkg/dependencies/factory.go Outdated Show resolved Hide resolved

pkg/dependencies/factory.go Outdated Show resolved Hide resolved

pkg/helm/factory.go Outdated Show resolved Hide resolved

cxbrowne1207 added 2 commits December 15, 2023 19:51

remove helmClient from struct. not needed to store

7e49b0e

referencing implementation instead of interface in factory dependencies

abd3e14

cxbrowne1207 requested a review from g-gaston December 15, 2023 20:18

g-gaston reviewed Dec 15, 2023

View reviewed changes

eks-distro-bot assigned g-gaston Dec 15, 2023

eks-distro-bot added lgtm and removed lgtm labels Dec 15, 2023

change joinEnv to mergeMap

49e7146

cxbrowne1207 force-pushed the fix-airgapped-cilium-controller-upgrade branch from 89e48fd to 49e7146 Compare December 15, 2023 21:01

eks-distro-bot added the lgtm label Dec 15, 2023

eks-distro-bot added the approved label Dec 15, 2023

eks-distro-bot merged commit a319d43 into aws:main Dec 15, 2023

cxbrowne1207 mentioned this pull request Dec 15, 2023

[release-0.17] Fix controller generating cilium manifests with registry mirror (#7170) #7184

Merged

cxbrowne1207 mentioned this pull request Dec 15, 2023

[release-0.18] Fix controller generating cilium manifests with registry mirror (#7170) #7185

Merged

cxbrowne1207 deleted the fix-airgapped-cilium-controller-upgrade branch December 15, 2023 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix controller generating cilium manifests with registry mirror #7170

Fix controller generating cilium manifests with registry mirror #7170

cxbrowne1207 commented Dec 13, 2023 •

edited

Loading

eks-distro-bot commented Dec 13, 2023

cxbrowne1207 commented Dec 13, 2023

codecov bot commented Dec 13, 2023 •

edited

Loading

cxbrowne1207 commented Dec 13, 2023

cxbrowne1207 commented Dec 14, 2023

g-gaston left a comment

g-gaston commented Dec 15, 2023

cxbrowne1207 commented Dec 15, 2023

eks-distro-bot commented Dec 15, 2023

cxbrowne1207 commented Dec 15, 2023 •

edited

Loading

eks-distro-pr-bot commented Dec 15, 2023

cxbrowne1207 commented Dec 15, 2023

eks-distro-pr-bot commented Dec 15, 2023

Fix controller generating cilium manifests with registry mirror #7170

Fix controller generating cilium manifests with registry mirror #7170

Conversation

cxbrowne1207 commented Dec 13, 2023 • edited Loading

eks-distro-bot commented Dec 13, 2023

cxbrowne1207 commented Dec 13, 2023

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

cxbrowne1207 commented Dec 13, 2023

cxbrowne1207 commented Dec 14, 2023

g-gaston left a comment

Choose a reason for hiding this comment

g-gaston commented Dec 15, 2023

cxbrowne1207 commented Dec 15, 2023

eks-distro-bot commented Dec 15, 2023

cxbrowne1207 commented Dec 15, 2023 • edited Loading

eks-distro-pr-bot commented Dec 15, 2023

cxbrowne1207 commented Dec 15, 2023

eks-distro-pr-bot commented Dec 15, 2023

cxbrowne1207 commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

cxbrowne1207 commented Dec 15, 2023 •

edited

Loading