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

option: Rename egress gateway flag to enable-ipv4-egress-gateway #17695

Merged
merged 1 commit into from Nov 2, 2021
Merged

option: Rename egress gateway flag to enable-ipv4-egress-gateway #17695

merged 1 commit into from Nov 2, 2021

Conversation

pchaigno
Copy link
Member

The new name should clarify that this flag only enables an IPv4 egress gateway feature; IPv6 is not supported yet. Once IPv6 is supported, users may not want to enable it even on dual-stack clusters because SNATing is generally less frequent in IPv6. To prepare for this future addition of enable-ipv6-egress-gateway, it's easier to rename now while the feature is still in beta.

@pchaigno pchaigno added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. upgrade-impact This PR has potential upgrade or downgrade impact. feature/egress-gateway Impacts the egress IP gateway feature. labels Oct 25, 2021
@pchaigno pchaigno requested a review from a team October 25, 2021 15:51
@pchaigno pchaigno requested review from a team as code owners October 25, 2021 15:51
@pchaigno pchaigno requested review from a team, joamaki and twpayne October 25, 2021 15:51
qmonnet
qmonnet approved these changes Oct 25, 2021
View reviewed changes
pkg/option/config.go Outdated Show resolved Hide resolved
@pchaigno pchaigno force-pushed the rename-egress-gateway-agent-flag branch from f0a2a7e to 25d614c Compare October 26, 2021 11:33
@pchaigno pchaigno requested a review from a team as a code owner October 26, 2021 11:33
@pchaigno pchaigno force-pushed the rename-egress-gateway-agent-flag branch from 25d614c to 1daf4e2 Compare October 26, 2021 11:34
jibi
jibi reviewed Oct 26, 2021
View reviewed changes
Copy link
Member

@jibi jibi left a comment •
edited

Choose a reason for hiding this comment

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

Even though this is a beta feature I think we should keep the old --enable-egress-gateway around and mark it for 1.12 deprecation in daemon/cmd/daemon_main.go:

flags.MarkDeprecated(option.EnableEgressGateway, "This flag is no longer available and will be removed in v1.12")

just to avoid leaving users that upgrade to 1.11 with a broken daemon config

qmonnet
qmonnet reviewed Oct 26, 2021
View reviewed changes
daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved
@pchaigno
Copy link
Member Author

Even though this is a beta feature I think we should keep the old --enable-egress-gateway around and mark it for 1.12 deprecation [...] just to avoid leaving users that upgrade to 1.11 with a broken daemon config

What's the point of having beta if we're going to take as many precautions when making changes as with a stable feature? For a user to break their setup on upgrade, they would have to (1) not read the upgrade guide and (2) not test a basic upgrade first (we'll error on unknown flag), all that while knowing they are using a beta feature.

@pchaigno
option: Rename egress gateway flag to enable-ipv4-egress-gateway
f50cc08 
The new name should clarify that this flag only enables an IPv4 egress
gateway feature; IPv6 is not supported yet. Once IPv6 is supported,
users may not want to enable it even on dual-stack clusters because
SNATing is generally less frequent in IPv6. To prepare for this future
addition of enable-ipv6-egress-gateway, it's easier to rename now while
the feature is still in beta.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the rename-egress-gateway-agent-flag branch from 1daf4e2 to f50cc08 Compare October 27, 2021 15:18
@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 1, 2021
@nebril nebril merged commit 5d6a12d into cilium:master Nov 2, 2021
@pchaigno pchaigno deleted the rename-egress-gateway-agent-flag branch November 2, 2021 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qmonnet qmonnet qmonnet approved these changes

@twpayne twpayne twpayne approved these changes

@jibi jibi jibi approved these changes

@joamaki joamaki joamaki approved these changes

@errordeveloper errordeveloper Awaiting requested review from errordeveloper errordeveloper was automatically assigned from cilium/helm

Assignees

@errordeveloper errordeveloper

Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. feature/egress-gateway Impacts the egress IP gateway feature. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. upgrade-impact This PR has potential upgrade or downgrade impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pchaigno @twpayne @jibi @joamaki @qmonnet @errordeveloper @nebril