Skip to content

Refactor Cilium CNI installation #12101

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Refactor Cilium CNI installation #12101

wants to merge 8 commits into from

Conversation

tico88612
Copy link
Member

@tico88612 tico88612 commented Apr 1, 2025 •
edited
Loading

What type of PR is this?

/kind design
/kind feature

What this PR does / why we need it:

We would deprecate the old template installation, and using the Cilium CLI will be better.

Which issue(s) this PR fixes:

Fixes #12049
Related #11487

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Cilium CNI installation replaces Jinja template with Cilium CLI
[action required] `cilium_agent_custom_args` and `cilium_operator_custom_args` are deprecated, please use `cilium_agent_extra_args` and `cilium_operator_extra_args`.
[action required] `cilium_identity_allocation_mode` default change to `crd`.
[action required] `cilium_enable_host_legacy_routing` default change to `false`.

@k8s-ci-robot k8s-ci-robot added kind/design Categorizes issue or PR as related to design. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 1, 2025
@tico88612
Copy link
Member Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 1, 2025
@tico88612
Copy link
Member Author

For the extended cilium test.

/label ci-extended

@k8s-ci-robot k8s-ci-robot added the ci-extended Run additional tests label Apr 1, 2025
@tico88612 tico88612 force-pushed the refactor/cilium-install branch 2 times, most recently from d5433be to 775aa12 Compare April 1, 2025 13:51
@tico88612
Copy link
Member Author

/label tide/merge-method-merge

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. label Apr 1, 2025
@tico88612 tico88612 force-pushed the refactor/cilium-install branch 5 times, most recently from 58e644b to c281a50 Compare April 3, 2025 13:34
@tico88612 tico88612 changed the title [WIP] Refactor Cilium CNI installation Refactor Cilium CNI installation Apr 3, 2025
@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 3, 2025
@tico88612
Copy link
Member Author

/retest-failed

1 similar comment
@tico88612
Copy link
Member Author

/retest-failed

@VannTen
Copy link
Contributor

VannTen commented Apr 8, 2025

I didn't review thoroughly yet, I'll see if I can find the time.
Since this is a relatively big change (right ?) do we want to hold this one until after 2.28, to have more testing time in master before a release ?

@tico88612
Copy link
Member Author

@VannTen Selfishly, I hope it will be released on 2.28 (I'm not sure if Cilium 1.15 is compatible with Kubernetes 1.32), but if it requires a lot of testing (it's too close to the K8s 1.33 release), I agree with your idea.

@VannTen
Copy link
Contributor

VannTen commented Apr 8, 2025 via email

@tico88612 tico88612 mentioned this pull request Apr 8, 2025
Open
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2025
@tico88612 tico88612 force-pushed the refactor/cilium-install branch from c281a50 to 6acbcbe Compare April 9, 2025 13:34
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2025
@RaulButuc
Copy link
Contributor

@VannTen Selfishly, I hope it will be released on 2.28 (I'm not sure if Cilium 1.15 is compatible with Kubernetes 1.32), but if it requires a lot of testing (it's too close to the K8s 1.33 release), I agree with your idea.

Definitely not compatible, no. Had to manually upgrade cilium to v1.16.6 and then v1.17.2 to be able to move to K8s v1.30+ (currently on v1.32.3).
I would also vote for releasing in 2.28.

RaulButuc
RaulButuc suggested changes Apr 10, 2025
View reviewed changes
Copy link
Contributor

@RaulButuc RaulButuc left a comment

Choose a reason for hiding this comment

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

lgtm overall, just believe v1.17.2 should be used instead as it contains a few bug fixes

@tico88612 tico88612 force-pushed the refactor/cilium-install branch from 6acbcbe to 7674e5c Compare April 10, 2025 08:33
@tico88612 tico88612 requested a review from RaulButuc April 10, 2025 08:35
RaulButuc
RaulButuc approved these changes Apr 11, 2025
View reviewed changes
Copy link
Contributor

@RaulButuc RaulButuc left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

@RaulButuc: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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-sigs/prow repository.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RaulButuc, tico88612
Once this PR has been reviewed and has the lgtm label, please assign liupeng0518 for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tico88612 tico88612 mentioned this pull request Apr 11, 2025
Open
@koobfoo koobfoo mentioned this pull request Apr 15, 2025
Open
@snowhanse
Copy link

How does airgap install work with this refactoring?

@tico88612
Copy link
Member Author

@snowhanse I don't have any air-gap environment to test. AFAIK, you just need to make sure that your air gap environment has access to the Cilium CLI and images.

Jinja Template puts a heavy burden on the maintainers, the version is not up-to-date (not even aligned upstream), and especially now that the number of project maintainers is very lack, there is no way to help with updates.

@snowhanse
Copy link

@tico88612 I get that, and understand the motivation.

I'm just wondering as we remove all the references to images like
https://github.com/kubernetes-sigs/kubespray/pull/12101/files#diff-172661997c97994bba466dbf0df5e18234f5475a25d52f5557a45b3e1dd7f29cL38
image: "{{ cilium_operator_image_repo }}:{{ cilium_operator_image_tag }}"

but as I didn't see them reflected in the values file, did not know how we would be able to perform offline install.
I see they have appeared in the meantime.
Thank you.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2025
@tico88612
Upgrade outdated cilium_min_version_required
b27cffb 
Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
@tico88612
Remove old cilium templates install
2f0b75b 
Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
@tico88612
Use cilium-cli install Cilium
98f5625 
Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
@tico88612
Change kube_owner to root for cilium CI test
18ba935 
Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
@tico88612
Change cilium_kube_proxy_replacement to true for CI tests
552c7c0 
Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
@tico88612
Upgrade cilium version to 1.17.2
fb074e6 
Signed-off-by: ChengHao Yang
<17496418+tico88612@users.noreply.github.com>
@tico88612
Upgrade Cilium related images
2288bb9 
Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
@tico88612
Cilium values use image variables
a2b4c2b 
Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
@tico88612 tico88612 force-pushed the refactor/cilium-install branch from cebe4e9 to a2b4c2b Compare April 30, 2025 07:36
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaulButuc RaulButuc RaulButuc approved these changes

@ErikJiang ErikJiang Awaiting requested review from ErikJiang

@VannTen VannTen Awaiting requested review from VannTen

Assignees
No one assigned
Labels
ci-extended Run additional tests cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cilium CNI version bump
5 participants
@tico88612 @VannTen @RaulButuc @k8s-ci-robot @snowhanse