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.

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

Support cilium ip-masq-agent configuration #8893

Merged
merged 2 commits into from
May 31, 2022
Merged

Support cilium ip-masq-agent configuration #8893

merged 2 commits into from
May 31, 2022

Conversation

mahjonp
Copy link
Contributor

@mahjonp mahjonp commented May 30, 2022 •
edited by oomichi
Loading

Signed-off-by: mahjonp [email protected]

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR adds cilium ip-masq-agent configuration support and also fixes #8889

Special notes for your reviewer:

The enable-ip-masq-agent flag should be set according to: cilium/cilium@040e788

Does this PR introduce a user-facing change?:

[cilium] Add the cilium ip-masq-agent configuration support

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 30, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 30, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @mahjonp!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @mahjonp. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 30, 2022
@k8s-ci-robot k8s-ci-robot requested review from jayonlau and oomichi May 30, 2022 04:25
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 30, 2022
@mahjonp mahjonp changed the title [WIP] fix deploy Cilium with eBPF-based Masquerading failed Support cilium ip-masq-agent configuration May 30, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2022
@mahjonp mahjonp force-pushed the fix_ip_masq_agent branch from 8259597 to b2bc25d Compare May 30, 2022 06:43
@liupeng0518
Copy link
Member

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 30, 2022
@cristicalin
Copy link
Contributor

Looks nice, thanks for this contribution @mahjonp

/ok-to-test
/approve

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 30, 2022
@cristicalin cristicalin added this to the 2.19 milestone May 30, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, mahjonp

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2022
@Thearas
Copy link
Contributor

Thearas commented May 30, 2022

Fix #8889.

mahjonp
mahjonp commented May 30, 2022
View reviewed changes
roles/network_plugin/cilium/templates/cilium/config.yml.j2
@@ -206,6 +206,9 @@ data:
{% endif %}
{% endif %}

# IP Masquerade Agent
enable-ip-masq-agent: "{{ cilium_ip_masq_agent_enable }}"
Copy link
Contributor Author

@mahjonp mahjonp May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I forget to set enable-ip-masq-agent before 😆

@oomichi
Copy link
Contributor

oomichi commented May 31, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2022
@k8s-ci-robot k8s-ci-robot merged commit c927da0 into kubernetes-sigs:master May 31, 2022
@oomichi oomichi mentioned this pull request May 31, 2022
Closed
@mahjonp mahjonp deleted the fix_ip_masq_agent branch June 1, 2022 03:13
@floryut floryut added kind/network Network section in the release note and removed kind/feature Categorizes issue or PR as related to a new feature. labels Jun 1, 2022
@we10710aa
Copy link

@mahjonp I think it should be

{% if cilium_ip_masq_agent_enable %}

instead of

{% if not cilium_ip_masq_agent_enable %}

if ip masq agent is not enabled, there will be no such configmap called ip-masq-agent

@oomichi Can you confirm on this one? I think this break v2.19 with cilium cni

@mahjonp
Copy link
Contributor Author

mahjonp commented Jun 6, 2022

@mahjonp I think it should be

{% if cilium_ip_masq_agent_enable %}

instead of

{% if not cilium_ip_masq_agent_enable %}

if ip masq agent is not enabled, there will be no such configmap called ip-masq-agent

@oomichi Can you confirm on this one? I think this break v2.19 with cilium cni

Oops, you are right, I will submit a bug fix ASAP

This was referenced Jun 6, 2022
Merged
Merged
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jul 1, 2023
@mahjonp @LuckySB
Support cilium ip-masq-agent configuration (kubernetes-sigs#8893)
8c4e3cd 
* fix deploy Cilium with eBPF-based Masquerading failed

Signed-off-by: mahjonp <[email protected]>

* forget to add the enable-ip-masq-agent flag

Signed-off-by: mahjonp <[email protected]>
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Oct 23, 2023
@mahjonp @LuckySB
Support cilium ip-masq-agent configuration (kubernetes-sigs#8893)
a2f9809 
* fix deploy Cilium with eBPF-based Masquerading failed

Signed-off-by: mahjonp <[email protected]>

* forget to add the enable-ip-masq-agent flag

Signed-off-by: mahjonp <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayonlau jayonlau Awaiting requested review from jayonlau

@oomichi oomichi Awaiting requested review from oomichi

Assignees

@oomichi oomichi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/network Network section in the release note lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
2.19
Development

Successfully merging this pull request may close these issues.

Deploy Cilium with eBPF-based Masquerading failed
8 participants
@mahjonp @k8s-ci-robot @liupeng0518 @cristicalin @Thearas @oomichi @we10710aa @floryut