[1.26 feature] Add flannelOpts flag and deprecate only server flannel options #6557

manuelbuil · 2022-11-24T15:18:38Z

Signed-off-by: Manuel Buil mbuil@suse.com

Proposed Changes

Combine the only-server flannel options:

backend (in 1.27)
ipv6-masq
external-ip

into one cli.StringSlice (flannel-opts) and deprecate those as single flags

Types of Changes

Breaking API change

Verification

Deploy with --flannel-opts, for example --flannel-opts=[backend=wireguard, ipv6-masq]

Testing

Linked Issues

#6556

User-Facing Change

Deprecate flannel-backend, flannel-ipv6-masq, flannel-external-ip flags and replace them with flannel-opts flag

Further Comments

brandond · 2022-11-28T23:10:46Z

pkg/cli/cmds/server.go

@@ -208,20 +209,25 @@ var ServerFlags = []cli.Flag{
 	ClusterDomain,
 	cli.StringFlag{
 		Name:        "flannel-backend",
-		Usage:       "(networking) backend<=option1=val1,option2=val2> where backend is one of 'none', 'vxlan', 'ipsec', 'host-gw', 'wireguard-native', or 'wireguard' (deprecated)",
+		Usage:       "(deprecated) use --flannel-opts",


I wonder if we should add --disable-flannel or something?

Hiding the option to disable flannel even further, when it's a very common option for users that want to deploy their own CNI, seems like a step backwards from a UX perspective.

IIRC, disable-flannel was deprecated and removed. The way to do it now if using none as backend

brandond · 2022-11-28T23:12:20Z

pkg/cli/cmds/server.go

 		Destination: &ServerConfig.FlannelExternalIP,
 	},
+	cli.StringSliceFlag{
+		Name:  "flannel-opts",
+		Usage: "(networking) Flannel options: backend:<option>, ipv6-masq, external-ip. Backend is one of 'none', 'vxlan', 'ipsec', 'host-gw' or 'wireguard-native'",


We're going to drop ipsec as well, so lets just list the still-supported backends.

Suggested change

Usage: "(networking) Flannel options: backend:<option>, ipv6-masq, external-ip. Backend is one of 'none', 'vxlan', 'ipsec', 'host-gw' or 'wireguard-native'",

Usage: "(networking) Flannel options: backend=string, ipv6-masq=bool, external-ip=bool. backend is one of 'none', 'vxlan', 'host-gw' or 'wireguard-native'",

dereknola · 2022-11-29T17:08:41Z

I would argue for two separate flannel flags, flannel-backend as we have it now and flannel-opts with everything else.

manuelbuil · 2022-11-29T17:20:56Z

I would argue for two separate flannel flags, flannel-backend as we have it now and flannel-opts with everything else.

I don't have a strong opinion. Just remember that the reason to create this PR was because there were more than 2 flannel options in the server, which was observed as too many. If we remove flannel-backend from the PR, we are back to 2 flags, which makes me wonder if it makes sense to proceed :P

dereknola · 2022-12-01T20:54:43Z

I totally understand, I was one of the people pushing for the flannel-opts flag. But the flannel-backend is already a complex flag with lots of options, and deprecating it is going to be a nightmare. The new flannel flags were bools , so having a single flag to encapsulate all the bools makes sense to me.

dereknola · 2022-12-01T20:57:04Z

pkg/cli/cmds/server.go

 		Destination: &ServerConfig.FlannelIPv6Masq,
 	},
 	cli.BoolFlag{
 		Name:        "flannel-external-ip",
-		Usage:       "(networking) Use node external IP addresses for Flannel traffic",
+		Usage:       "(deprecated) use --flannel-opts",


We need to add logrus.Warn when these flags are used as per https://github.com/dereknola/k3s/blob/master/docs/adrs/deprecating-and-removing-flags.md

Thanks! I missed that

dereknola · 2022-12-01T20:57:06Z

pkg/cli/cmds/server.go

 		Destination: &ServerConfig.FlannelBackend,
 		Value:       "vxlan",
 	},
 	cli.BoolFlag{
 		Name:        "flannel-ipv6-masq",
-		Usage:       "(networking) Enable IPv6 masquerading for pod",
+		Usage:       "(deprecated) use --flannel-opts",


We need to add logrus.Warn when these flags are used as per https://github.com/dereknola/k3s/blob/master/docs/adrs/deprecating-and-removing-flags.md

Thanks! I missed that

manuelbuil · 2022-12-02T14:14:48Z

I totally understand, I was one of the people pushing for the flannel-opts flag. But the flannel-backend is already a complex flag with lots of options, and deprecating it is going to be a nightmare. The new flannel flags were bools , so having a single flag to encapsulate all the bools makes sense to me.

That's a good point, flannel-backend is too complicated. I think allowing to configure extra options through it (apart from the backend) was a mistake because we can do it using flannel-conf flag. I have noticed that Roberto already added a deprecation message for 1.27 ==> 26e9405, that's good. Then, maybe we can collapse flannel-backend inside flannel-opts when releasing 1.27

matttrach · 2022-12-02T19:23:34Z

pkg/agent/config/config.go

+		case strings.Contains(opt, "external-ip"):
+			externalIP = true
+		default:
+			return false, false, fmt.Errorf("wrong arg: %s passed in --flannel-opts. Correct args are backend, ipv6-masq and external-ip", opt)


nit: serial comma?

thanks! I did not know about the serial comma concept

matttrach · 2022-12-02T19:25:05Z

pkg/cli/cmds/server.go

@@ -208,20 +209,25 @@ var ServerFlags = []cli.Flag{
 	ClusterDomain,
 	cli.StringFlag{
 		Name:        "flannel-backend",
-		Usage:       "(networking) backend<=option1=val1,option2=val2> where backend is one of 'none', 'vxlan', 'ipsec', 'host-gw', 'wireguard-native', or 'wireguard' (deprecated)",
+		Usage:       "(networking) backend<=option1=val1,option2=val2> where backend is one of 'none', 'vxlan', 'ipsec', 'host-gw' or 'wireguard-native'",


nit: serial comma

manuelbuil · 2022-12-02T20:48:21Z

Message for my future self: update the docs!

brandond · 2022-12-05T21:09:57Z

It might be worth doing a full ADR for how we'd like to handle Flannel configuration in K3s. I get that the current set of flags doesn't scale well, but I feel like this could be a step backwards in terms of user experience.

It would be nice to capture:

What we currently allow configuring
How we currently allow configuring these items (can some be achieved multiple ways?)
How often do we think users use the current flags
Do users have any work-arounds for settings that we don't allow configuring?

manuelbuil · 2022-12-11T15:55:21Z

It might be worth doing a full ADR for how we'd like to handle Flannel configuration in K3s. I get that the current set of flags doesn't scale well, but I feel like this could be a step backwards in terms of user experience.

It would be nice to capture:

What we currently allow configuring

How we currently allow configuring these items (can some be achieved multiple ways?)

How often do we think users use the current flags

Do users have any work-arounds for settings that we don't allow configuring?

I can create an ADR tomorrow. The user related questions are relevant but not sure how we can answer that :P

Signed-off-by: Manuel Buil <mbuil@suse.com>

brandond · 2022-12-14T00:19:34Z

docs/adrs/flannel-options.md

+* Use it for potential new flannel server flags
+
+Downsides:
+* Users are used to those flags, specially `flannel-backend`, therefore it might be seen as a negative changei. That's why we should carefully follow the deprecation policy.


Suggested change

* Users are used to those flags, specially `flannel-backend`, therefore it might be seen as a negative changei. That's why we should carefully follow the deprecation policy.

* Users are used to those flags, specially `flannel-backend`, therefore it might be seen as a negative change. That's why we should carefully follow the deprecation policy.

brandond · 2022-12-14T00:22:14Z

docs/adrs/flannel-options.md

+
+## Proposal
+
+Starting in v1.26, introduce a new `flannel-opts` flag that includes flannel server options. The redundant flags are deprecated and removed in a few releases.


OK, so just to be clear we're only talking about making changes to the server flags - the agent flags that allow users to pass custom flannel and cni config files would be retained?

If our goal is to simplify the user's life, packing everything into one giant --flannel-opts that requires extensive documentation feels like a step backwards. Right now the flannel-backend flag is pretty self-explanatory; the only detail we need to get into for that is what backends are available and why someone would chose one over another.

I see your point. I think I'm going to close this PR because it does not bring much user benefit. If in the future, there are more flannel flags coming (unlikely), we can reconsider this. But I think we are complicating things for just two flags, so the benefit is small

manuelbuil requested a review from a team as a code owner November 24, 2022 15:18

brandond requested changes Nov 28, 2022

View reviewed changes

dereknola requested changes Dec 1, 2022

View reviewed changes

manuelbuil force-pushed the flannelopts branch 4 times, most recently from ad999ad to 90e5624 Compare December 2, 2022 18:39

manuelbuil requested review from brandond and dereknola December 2, 2022 18:40

matttrach reviewed Dec 2, 2022

View reviewed changes

manuelbuil force-pushed the flannelopts branch 2 times, most recently from 4cfcf90 to 817aad6 Compare December 5, 2022 17:26

manuelbuil requested a review from matttrach December 5, 2022 17:26

manuelbuil force-pushed the flannelopts branch from 817aad6 to 37a6dc6 Compare December 12, 2022 18:33

Add flannelOpt

f09b51c

Signed-off-by: Manuel Buil <mbuil@suse.com>

manuelbuil force-pushed the flannelopts branch from 37a6dc6 to f09b51c Compare December 13, 2022 15:11

brandond reviewed Dec 14, 2022

View reviewed changes

manuelbuil closed this Dec 15, 2022

brandond mentioned this pull request Feb 9, 2023

[WIP] Convert flannel bools to single stringslice #6897

Closed

manuelbuil deleted the flannelopts branch June 14, 2023 08:53

rdesaintleger mentioned this pull request Feb 14, 2025

Ipv4 no masq #11791

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.26 feature] Add flannelOpts flag and deprecate only server flannel options #6557

[1.26 feature] Add flannelOpts flag and deprecate only server flannel options #6557

manuelbuil commented Nov 24, 2022 •

edited

Loading

brandond Nov 28, 2022

manuelbuil Nov 29, 2022 •

edited

Loading

brandond Nov 28, 2022

dereknola commented Nov 29, 2022

manuelbuil commented Nov 29, 2022

dereknola commented Dec 1, 2022

dereknola Dec 1, 2022

manuelbuil Dec 2, 2022

dereknola Dec 1, 2022

manuelbuil Dec 2, 2022

manuelbuil commented Dec 2, 2022 •

edited

Loading

matttrach Dec 2, 2022

manuelbuil Dec 5, 2022

matttrach Dec 2, 2022

manuelbuil commented Dec 2, 2022

brandond commented Dec 5, 2022 •

edited

Loading

manuelbuil commented Dec 11, 2022

brandond Dec 14, 2022

brandond Dec 14, 2022 •

edited

Loading

manuelbuil Dec 15, 2022

	Usage: "(networking) Flannel options: backend:<option>, ipv6-masq, external-ip. Backend is one of 'none', 'vxlan', 'ipsec', 'host-gw' or 'wireguard-native'",
	Usage: "(networking) Flannel options: backend=string, ipv6-masq=bool, external-ip=bool. backend is one of 'none', 'vxlan', 'host-gw' or 'wireguard-native'",

	* Users are used to those flags, specially `flannel-backend`, therefore it might be seen as a negative changei. That's why we should carefully follow the deprecation policy.
	* Users are used to those flags, specially `flannel-backend`, therefore it might be seen as a negative change. That's why we should carefully follow the deprecation policy.


		## Proposal

		Starting in v1.26, introduce a new `flannel-opts` flag that includes flannel server options. The redundant flags are deprecated and removed in a few releases.

[1.26 feature] Add flannelOpts flag and deprecate only server flannel options #6557

[1.26 feature] Add flannelOpts flag and deprecate only server flannel options #6557

Conversation

manuelbuil commented Nov 24, 2022 • edited Loading

Proposed Changes

Types of Changes

Verification

Testing

Linked Issues

User-Facing Change

Further Comments

Choose a reason for hiding this comment

manuelbuil Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereknola commented Nov 29, 2022

manuelbuil commented Nov 29, 2022

dereknola commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelbuil commented Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelbuil commented Dec 2, 2022

brandond commented Dec 5, 2022 • edited Loading

manuelbuil commented Dec 11, 2022

Choose a reason for hiding this comment

brandond Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelbuil commented Nov 24, 2022 •

edited

Loading

manuelbuil Nov 29, 2022 •

edited

Loading

manuelbuil commented Dec 2, 2022 •

edited

Loading

brandond commented Dec 5, 2022 •

edited

Loading

brandond Dec 14, 2022 •

edited

Loading