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

Add tunnel mode config and egress gateway config params #14723

Merged
merged 3 commits into from
Feb 15, 2021
Merged

Add tunnel mode config and egress gateway config params #14723

merged 3 commits into from
Feb 15, 2021

Conversation

blzhao-0
Copy link
Contributor

@blzhao-0 blzhao-0 commented Jan 25, 2021 •
edited

This PR adds a tunnel mode arg to the init script of cilium devices so
that 'mode' and 'tunnel mode' is represented in separate params. It
also adds the config options for enabling egress gateway and makes
sure that tunnel is enabled when egress gateway is specified.

This change is part the egress NAT feature, Ref: #13575.

Signed-off-by: Bolun Zhao blzhao@google.com

Please ensure your pull request adheres to the following guidelines:

@blzhao-0 blzhao-0 requested a review from a team January 25, 2021 19:30
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 25, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 25, 2021
joestringer
joestringer requested changes Jan 25, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Hi @MasterZ40 , thanks for the PR.

The code change itself looks mostly straightforward, but I don't understand why. Would you mind providing a brief motivation in the commit message / PR description?

Please also review the rest of the PR description, everything below "Please ensure your pull request adheres to the following guidelines" is for you to complete or remove if it makes no sense. In particular the release note needs to be either set, or just removed (it's fine to remove in this case since the PR title describes the changes sufficiently).

One more specific change request below.

bpf/init.sh Show resolved Hide resolved
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Jan 25, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 25, 2021
@anfernee anfernee mentioned this pull request Jan 28, 2021
Closed
10 tasks
@blzhao-0 blzhao-0 force-pushed the mixedmode_up branch 2 times, most recently from 56bf38a to 58765c8 Compare January 29, 2021 01:54
pchaigno
pchaigno requested changes Jan 29, 2021
View reviewed changes
bpf/init.sh Outdated Show resolved Hide resolved
bpf/init.sh Show resolved Hide resolved
@pchaigno pchaigno added sig/loader Impacts the loading of BPF programs into the kernel. kind/cleanup This includes no functional changes. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Jan 29, 2021
@blzhao-0 blzhao-0 requested a review from a team February 1, 2021 19:33
@blzhao-0 blzhao-0 requested a review from a team as a code owner February 1, 2021 19:33
@blzhao-0 blzhao-0 requested a review from twpayne February 1, 2021 19:33
@anfernee
Copy link
Contributor

anfernee commented Feb 2, 2021

Do we need this commit: f4ecd64 ?

@pchaigno
Copy link
Member

pchaigno commented Feb 2, 2021

Do we need this commit: f4ecd64 ?

It's needed for the egress gateway, no? Might as well include it now.

@anfernee
Copy link
Contributor

anfernee commented Feb 2, 2021

Do we need this commit: f4ecd64 ?

It's needed for the egress gateway, no? Might as well include it now.

Yes. I'd like to hold this one for now for this refactor.

@blzhao-0
Copy link
Contributor Author

blzhao-0 commented Feb 2, 2021

Do we need this commit: f4ecd64 ?

It's needed for the egress gateway, no? Might as well include it now.

Yes. I'd like to hold this one for now for this refactor.

Synced with @anfernee we are holding this pr to only the refactor.

@blzhao-0 blzhao-0 changed the title Refactor of tunnel mode and tunnel specific configs Add tunnel mode config and egress gateway config params Feb 8, 2021
@blzhao-0
Copy link
Contributor Author

blzhao-0 commented Feb 8, 2021 •
edited

Do we need this commit: f4ecd64 ?

It's needed for the egress gateway, no? Might as well include it now.

Yes. I'd like to hold this one for now for this refactor.

Synced with @anfernee we are holding this pr to only the refactor.

Per further discussion with @anfernee, we decided to add back in the "config for egress gateway" commits with clearer descriptions of the options and their relationships.

@joestringer
Copy link
Member

@MasterZ40 will you update the PR description specifically to remove the "fixes" and release note sections since they don't make sense? The release note piece in particular will break release scripting if you do not either update or remove it.

@blzhao-0
Copy link
Contributor Author

blzhao-0 commented Feb 8, 2021

@MasterZ40 will you update the PR description specifically to remove the "fixes" and release note sections since they don't make sense? The release note piece in particular will break release scripting if you do not either update or remove it.

Updated PR description. Thanks!

@blzhao-0 blzhao-0 closed this Feb 8, 2021
@blzhao-0 blzhao-0 reopened this Feb 8, 2021
joestringer
joestringer approved these changes Feb 8, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment •
edited

Choose a reason for hiding this comment

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

The changes themselves look fine to me. You'll need to take into account MTU for mixed mode as well, otherwise endpoints may end up transmitting packets that cannot fit in the tunnel (I believe this is currently switching on tunnel mode enablement).

Then there's just the question of whether to merge PRs that contain partially-finished features. There has been some ongoing debate about this. I think previously the PR was just refactoring so it's clear we would just merge it, but with the latest changes to also start including the beginnings of the feature for #13575, it's less clear.

EDIT: I guess on the latter aspect, the ship has already departed. So I think the remaining item here is just the other codeowners.

@joestringer joestringer removed their assignment Feb 8, 2021
@maintainer-s-little-helper
Copy link

Commit acd9408776448f309f68cb27f6cba4c7c5032123 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 9, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 9, 2021
@blzhao-0
Refactor of tunnel mode and tunnel specific configs
ec52e28 
This change adds a tunnel mode arg to the init script of cilium devices so
that 'mode' and 'tunnel mode' is represented in seperate params. The
functioning should remain the same.

This change is part of the ground work for the egress NAT feature, which
requires this tunnel config information Ref: cilium#13575

Signed-off-by: Bolun Zhao <blzhao@google.com>
@blzhao-0
Add option enable-egress-gateway
77d5068 
Adds "enable-egress-gateway" to the config, with defualt value to be
false.

Signed-off-by: Bolun Zhao <blzhao@google.com>
@blzhao-0
Enable tunnel mode when egress gateway is configured
4e94bc9 
Sets the tunnel option when egress gateway is configured whilst direct
routing mode. Under this circumstance, the tunnel will be used to
redirect egress traffic to the gateway and it defaults to vxlan.

Signed-off-by: Bolun Zhao <blzhao@google.com>
@pchaigno
Copy link
Member

pchaigno commented Feb 9, 2021

test-me-please

@anfernee
Copy link
Contributor

ready to merge this one?

@pchaigno
Copy link
Member

Yes, I think so. Tests are green and we have two reviews. Not all team reviews are covered, but I think the changes are trivial.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 11, 2021
@joestringer joestringer moved this from Done to In progress in 1.10.0 Feb 12, 2021
@aanm aanm merged commit 0592e8f into cilium:master Feb 15, 2021
1.10.0 automation moved this from In progress to Done Feb 15, 2021
@anfernee anfernee mentioned this pull request Feb 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anfernee anfernee anfernee left review comments

@joestringer joestringer joestringer approved these changes

@pchaigno pchaigno pchaigno approved these changes

@borkmann borkmann Awaiting requested review from borkmann

@twpayne twpayne Awaiting requested review from twpayne

Assignees

@twpayne twpayne

@borkmann borkmann

Labels
kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. sig/loader Impacts the loading of BPF programs into the kernel.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@blzhao-0 @anfernee @pchaigno @joestringer @borkmann @twpayne @aanm