install: Avoid using deprecated "tunnel" flag #1993
Conversation
The tunnel option is deprecated and will be removed in Cilium v1.15. This commit fixes the remaining uses I have found where the CI explicitly set the old `tunnel` flag. Note that the Cilium CLI also still sets the flag some times in our CI, this is addressed by cilium/cilium-cli#1993. Signed-off-by: Sebastian Wicki <[email protected]>
| @@ -50,7 +50,10 @@ import ( | |||
| const ( | |||
| configNameClusterID = "cluster-id" | |||
| configNameClusterName = "cluster-name" | |||
| configNameTunnel = "tunnel" | |||
|
|
|||
| configNameTunnelLegacy = "tunnel" | |||
There was a problem hiding this comment.
nit: Wondering if we should use Deprecated vs Legacy to indicate this feature will be axed in subsequent releases. Might make the intent clearer for devs and to grep when the time comes??
There was a problem hiding this comment.
The thing is that the feature is deprecated in Cilium, not Cilium CLI. Cilium CLI will have to support it even once it has been removed in Cilium, because Cilium CLI supports more than just one Cilium version. Thus my choice of using Legacy, which I think therefore fits better than Deprecated.
You however raise a good point - we probably should add a few comments saying that we can remove the logic once Cilium v1.14 is EOL (which will happen in about 2 years). Does that sound good to you?
| @@ -826,6 +829,18 @@ func (k *K8sClusterMesh) extractAccessInformation(ctx context.Context, client k8 | |||
| } | |||
| } | |||
|
|
|||
| tunnelProtocol := "" | |||
| if cm.Data[configNameRoutingMode] == "tunnel" { | |||
There was a problem hiding this comment.
nit: Should we check the software version vs relying solely on the config option. ie could surface that the flag is going to be deprecated back to the our users...
There was a problem hiding this comment.
I don't it makes much sense to make reading of the flag version dependent (when writing the flag, then absolutely, we should check the version and that's what the PR already does). At least at the moment, all released versions of Cilium understand the old version of the flag. Once the flag has been removed (which may or may not happen in v1.15), that might change, but we can only remove the flag after we've merged this PR and fixed our CI.
Surfacing the flag is planned (see the comment I've linked in the OP) - but it should be flagged during installation time. And maybe when running cilium status. But raising this while the user is trying to enable clustermesh (a separate task that has nothing to do with tunneling) I think is not the right thing to do, because at that point it's already "too late" and would be confusing the the user, because that's not what they are focusing on right now.
| @@ -523,7 +528,7 @@ func (k *K8sInstaller) generateConfigMap() (*corev1.ConfigMap, error) { | |||
| return nil, fmt.Errorf("--install-no-conntrack-iptables-rules cannot be enabled on Azure AKS") | |||
| } | |||
|
|
|||
| if cm.Data["tunnel"] != "disabled" { | |||
| if cm.Data["tunnel"] != "disabled" || cm.Data["routing-mode"] != "native" { | |||
| return nil, fmt.Errorf("--install-no-conntrack-iptables-rules requires tunneling to be disabled") | |||
There was a problem hiding this comment.
nit: Since we are checking 2 config params now might as well surface the issue to the user to avoid confusion.
There was a problem hiding this comment.
Again, adding validation to Helm is planned. But for this to work, we first need to fix our usages in CI, for which we first have to merge this PR.
maintainer-s-little-helper
bot
added
the
ready-to-merge
gandro
removed
the
ready-to-merge
The tunnel option is deprecated and will be removed in Cilium v1.15. This commit fixes the remaining uses I have found where the Cilium CLI still set the old `tunnel` flag unconditionally, which will lead to issues once the flag is no longer accepted [1]. The Cilium CLI now only uses the deprecated `tunnel` flag for Cilium versions 1.13 and older. When reading the ConfigMap (such as in the clustermesh code), we attempt to first parse the new values, before falling back on the old ones. [1] cilium/cilium#27841 (comment) Signed-off-by: Sebastian Wicki <[email protected]>
gandro
force-pushed
the
pr/gandro/use-tunnelprotocol
branch
from
25781a5 to
1d86d69
Compare
|
Pushed a few source code comments based on the feedback above. Once CI passes, this is ready to merge. |
maintainer-s-little-helper
bot
added
the
ready-to-merge
The tunnel option is deprecated and will be removed in Cilium v1.15. This commit fixes the remaining uses I have found where the CI explicitly set the old `tunnel` flag. Note that the Cilium CLI also still sets the flag some times in our CI, this is addressed by cilium/cilium-cli#1993. Signed-off-by: Sebastian Wicki <[email protected]>
The tunnel option is deprecated and will be removed in Cilium v1.15. This commit fixes the remaining uses I have found where the CI explicitly set the old `tunnel` flag. Note that the Cilium CLI also still sets the flag some times in our CI, this is addressed by cilium/cilium-cli#1993. Signed-off-by: Sebastian Wicki <[email protected]>
The tunnel option is deprecated and will be removed in Cilium v1.15. This commit fixes the remaining uses I have found where the Cilium CLI still set the old
tunnelflag unconditionally, which will lead to issues once the flag is no longer accepted [1]. The Cilium CLI now only uses the deprecatedtunnelflag for Cilium versions 1.13 and older.When reading the ConfigMap (such as in the clustermesh code), we attempt to first parse the new values, before falling back on the old ones.
[1] cilium/cilium#27841 (comment)