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

Add image overrides for CAPI/CAPRKE2 images #860

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

furkatgofurov7
Copy link
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@furkatgofurov7 furkatgofurov7 requested a review from a team as a code owner November 19, 2024 13:01
images:
- name: "cluster-api"
repository: "https://github.com/rancher-sandbox/cluster-api"
tag: "v1.7.7"
Copy link
Contributor

@hardys hardys Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I figured out why the CoreProvider override is not enough in my environment - we're following the airgapped docs and creating configmaps from the upstream manifests, we need to update the docs (and our edge airgap resources chart) to align with the forks instead

# kubectl get cm -n capi-system core-cluster-api-v1.7.7 -o yaml | grep image:
            image: registry.k8s.io/cluster-api/cluster-api-controller:v1.7.7
# kubectl get deployment -n capi-system -o yaml | grep image:
          image: registry.k8s.io/cluster-api/cluster-api-controller:v1.7.7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it seems that CM is created even in the non-airgapped scenario so my analysis above may be incorrect, I still think we need to align the airgapped docs though?

Copy link
Contributor

@Danil-Grigorev Danil-Grigorev Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please check if the configMap with overrides is mounted into CAPI operator pod? If it is not, then this will not effective (meaning clusterctl-config.yaml configMap). May happen if capi operator is not installed as a dependency of turtles within chart settings.

You don’t need to update your generated air-gap configmaps if you only need an image to point to the registry override we are specifying here, upstream manifests will work just fine. This will be performed by regular image overrides functionality implemented upstream (with a fix recently, so it should work as long as CM is mounted) and applied to all versions of manifests, even the air-gapped ones you have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it seems that CM is created even in the non-airgapped scenario so my analysis above may be incorrect, I still think we need to align the airgapped docs though?

rancher/turtles-docs#177

Copy link
Contributor

@Danil-Grigorev Danil-Grigorev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should specify an override to registry image, as displayed in example kubernetes-sigs/cluster-api-operator#616 (comment)

Specifying an override tag will need a change to updatecli procedure, and may forcefully override the tag user specifies on CAPIProvider, so it should not be set here.

internal/controllers/clusterctl/config.yaml Outdated Show resolved Hide resolved
internal/controllers/clusterctl/config.yaml Outdated Show resolved Hide resolved
internal/controllers/clusterctl/config.yaml Outdated Show resolved Hide resolved
internal/controllers/clusterctl/config.yaml Outdated Show resolved Hide resolved
internal/controllers/clusterctl/config.yaml Outdated Show resolved Hide resolved
internal/controllers/clusterctl/config.yaml Outdated Show resolved Hide resolved
@furkatgofurov7 furkatgofurov7 force-pushed the use-capi-caprke2-image-overrides branch 5 times, most recently from 09bd702 to 8dc93e8 Compare November 26, 2024 17:59
@furkatgofurov7 furkatgofurov7 force-pushed the use-capi-caprke2-image-overrides branch from 8dc93e8 to 0e92bda Compare November 26, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants