Change kube hairpin configuration settings and defaults #2417

juanluisvaladas · 2022-11-16T16:43:24Z

Description

Enable hairpin for kube-router by default

We agreed that the most logical decision was to enable hairpin by default. The old configuration doesn't reflect our real needs, for this reason we decided to deprecate the old configuration and add a new configuration. This commit handles all these details.

Also I spotted a bug related to reconciling, since I noticed it in one of the files which I spotted and it's just a one liner I slipped the commit in this PR.

Fixes #1953

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

How Has This Been Tested?

Manual test
Auto test added

Checklist:

juanluisvaladas · 2022-11-16T16:44:25Z

@hercherf @mtb-xt since you commented on #1953 I'd like your feedback as well please.

twz123

We should probably implement json.Unmarshaller for KubeRouter, now that we have a default which doesn't equal the zero value of its type.

pkg/apis/k0s.k0sproject.io/v1beta1/kuberouter.go

twz123 · 2022-11-17T08:06:32Z

pkg/component/controller/kuberouter.go

@@ -70,6 +71,27 @@ func (k *KubeRouter) Init(_ context.Context) error { return nil }
 // Stop no-op as nothing running
 func (k *KubeRouter) Stop() error { return nil }

+func getHairpinConfig(cfg *kubeRouterConfig, krc *v1beta1.KubeRouter) {


Maybe a better name this applyHairpinConfig, since this is not idempotently getting something, but updating the cfg in place.

mtb-xt · 2022-11-17T09:08:45Z

There was an edge case about the CNI config - if it was already generated, then activating hairpin mode in k0s won't update the CNI config.
If someone now creates the cluster with hairpin mode disabled, but then decides to enable it - it won't be updated as well.
I realize this is a rare scenario now it's enabled by default, but do we want to do anything about it or not?

k0s/pkg/component/controller/kuberouter.go

Line 232 in 78b4567

if [ ! -f /etc/cni/net.d/10-kuberouter.conflist ]; then

<- because of this check, if there is a CNI config present, it won't be touched.

juanluisvaladas · 2022-11-22T11:19:04Z

Having one field accepting multiple types breaks dynamic configuration because of kubernetes-sigs/controller-tools#735

I'm currently waiting for controller-manager people to give me the OK to start implementing this so in the meantime I'll put this on hold. If the proposed change wouldn't be accepted then we'll have to consider a strategy different than using the same field to accept multiple types.

juanluisvaladas · 2022-12-13T12:47:16Z

@jnummelin @twz123 I don't think I can achieve this even by patching the CRD manually.

The CRDs with IntOrString work because the porperties of the schema include the field x-kubernetes-int-or-string. The apiserver schema validation explictly skips some validations for IntOrString, so we cannot modify this.

I've tried to workaround this in many ways but I don't think it's actually possible. The only option that I see here would be adding x-kubernetes-preserve-unknown-fields to the KubeRouter struct and removing the HairpinMode field of the CRD but then this adds some additional problems in validation...

I see the following options:
1- Issue a new version of the clusterconfigs CRD including a new version v1beta2 in which the hairpinMode is a string. This is probably the best one.
2- Add a new field with a different name and deprecate HairpinMode, which isn't great because hairpinMode is the best name.
3- Allowing unknown fields and removing the field from the CRD, which brings issues with the validation.

twz123 · 2022-12-14T11:31:17Z

I would vote for option 2 (new field with a different name). This is what this PR initially aimed at, and it's a fair compromise compared to the alternatives. Introducing a new API version for this seems like a quite heavy thing to do. Maybe it's better to accumulate some changes over time and then issue a new API version that contains more improvements than just this one. Allowing unknown fields OTOH seems to me like removing a safety net for very little gain.

I didn't know that IntOrString was such a custom thing. Otherwise I wouldn't have brought it up as an example 🙈.

juanluisvaladas · 2022-12-14T22:19:36Z

I would vote for option 2 (new field with a different name).

And there is a big advantage that it's already implemented, I just had to go through the reflog :) and patch some little thing.

pkg/apis/k0s.k0sproject.io/v1beta1/kuberouter.go

twz123 · 2022-12-15T07:35:38Z

pkg/apis/k0s.k0sproject.io/v1beta1/kuberouter.go

-	// Activate Hairpin Mode (allow a Pod behind a Service to communicate to its own ClusterIP:Port)
-	HairpinMode bool `json:"hairpinMode"`
+	// Admits three values: "enabled" enables it globaly, "allowed" allows but services must be annotated explictly and "disabled"
+	Hairpin Hairpin `json:"hairpin"`


Worth mentioning the default value here as well.

Suggested change

Hairpin Hairpin `json:"hairpin"`

// Defaults to "Enabled".

// +kubebuilder:default=Enabled

Hairpin Hairpin `json:"hairpin"`

The kubebuilder default hint is still missing. But that's a purely cosmetic thing...

docs/configuration.md

pkg/apis/k0s.k0sproject.io/v1beta1/kuberouter.go

juanluisvaladas · 2022-12-15T09:55:35Z

@twz123 I addressed everything and I also changed the table in docs/configuration.md. Apparently I didn´t find the latest version if the reflog and there were a couple things missing even if the code was working as expected.

I went file by file and I believe everything is good now.

twz123 · 2022-12-15T10:44:37Z

pkg/apis/k0s.k0sproject.io/v1beta1/kuberouter.go

-	// Activate Hairpin Mode (allow a Pod behind a Service to communicate to its own ClusterIP:Port)
-	HairpinMode bool `json:"hairpinMode"`
+	// Admits three values: "enabled" enables it globaly, "allowed" allows but services must be annotated explictly and "disabled"
+	Hairpin Hairpin `json:"hairpin"`


The kubebuilder default hint is still missing. But that's a purely cosmetic thing...

We agreed that the most logical decision was to enable hairpin by default. The old configuration doesn't reflect our real needs, for this reason we decided to deprecate the old configuration and add a new configuration. This commit handles all these details. Fixes k0sproject#1953 Co-authored-by: Tom Wieczorek <twz123@users.noreply.github.com> Signed-off-by: Juan Luis de Sousa-Valadas Castaño <jvaladas@mirantis.com>

juanluisvaladas · 2022-12-15T11:01:11Z

Oh sorry I missed that one. That's added now.

juanluisvaladas requested a review from twz123 November 16, 2022 16:43

juanluisvaladas requested a review from a team as a code owner November 16, 2022 16:43

juanluisvaladas requested a review from s0j November 16, 2022 16:43

juanluisvaladas force-pushed the change-kube-hairpin branch from 58a9707 to 78b4567 Compare November 16, 2022 16:45

twz123 reviewed Nov 17, 2022

View reviewed changes

juanluisvaladas force-pushed the change-kube-hairpin branch from e0a62d5 to d005baf Compare November 18, 2022 16:29

juanluisvaladas changed the title ~~Change kube hairpin configuration settings and defaults~~ Change kube hairpin configuration settings and defaults [DO NOT REVIEW DOING NASTY STUFF WITH REFLOG RIGHT NOW] Dec 14, 2022

juanluisvaladas force-pushed the change-kube-hairpin branch 3 times, most recently from de5eeae to d0756a2 Compare December 14, 2022 22:14

juanluisvaladas changed the title ~~Change kube hairpin configuration settings and defaults [DO NOT REVIEW DOING NASTY STUFF WITH REFLOG RIGHT NOW]~~ Change kube hairpin configuration settings and defaults Dec 14, 2022

twz123 reviewed Dec 15, 2022

View reviewed changes

juanluisvaladas force-pushed the change-kube-hairpin branch from d0756a2 to e7f4ffb Compare December 15, 2022 09:52

twz123 previously approved these changes Dec 15, 2022

View reviewed changes

juanluisvaladas dismissed twz123’s stale review via 9800423 December 15, 2022 11:00

juanluisvaladas force-pushed the change-kube-hairpin branch from e7f4ffb to 9800423 Compare December 15, 2022 11:00

twz123 approved these changes Dec 15, 2022

View reviewed changes

juanluisvaladas merged commit 84775c0 into k0sproject:main Dec 15, 2022

twz123 mentioned this pull request Feb 20, 2024

Remove some outdated bullet points from kube-router docs #4086

Merged

16 tasks

k0s-bot mentioned this pull request Feb 20, 2024

[Backport release-1.29] Remove some outdated bullet points from kube-router docs #4088

Merged

twz123 mentioned this pull request Feb 20, 2024

[Backport release-1.28] Remove some outdated bullet points from kube-router docs #4089

Merged

This was referenced Feb 20, 2024

[Backport release-1.27] Remove some outdated bullet points from kube-router docs #4094

Merged

[Backport release-1.26] Remove some outdated bullet points from kube-router docs #4100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change kube hairpin configuration settings and defaults #2417

Change kube hairpin configuration settings and defaults #2417

juanluisvaladas commented Nov 16, 2022

juanluisvaladas commented Nov 16, 2022

twz123 left a comment

twz123 Nov 17, 2022

mtb-xt commented Nov 17, 2022

juanluisvaladas commented Nov 22, 2022

juanluisvaladas commented Dec 13, 2022 •

edited

Loading

twz123 commented Dec 14, 2022

juanluisvaladas commented Dec 14, 2022

twz123 Dec 15, 2022

twz123 Dec 15, 2022

juanluisvaladas commented Dec 15, 2022

twz123 Dec 15, 2022

juanluisvaladas commented Dec 15, 2022

Change kube hairpin configuration settings and defaults #2417

Change kube hairpin configuration settings and defaults #2417

Conversation

juanluisvaladas commented Nov 16, 2022

Description

Type of change

How Has This Been Tested?

Checklist:

juanluisvaladas commented Nov 16, 2022

twz123 left a comment

Choose a reason for hiding this comment

twz123 Nov 17, 2022

Choose a reason for hiding this comment

mtb-xt commented Nov 17, 2022

juanluisvaladas commented Nov 22, 2022

juanluisvaladas commented Dec 13, 2022 • edited Loading

twz123 commented Dec 14, 2022

juanluisvaladas commented Dec 14, 2022

twz123 Dec 15, 2022

Choose a reason for hiding this comment

twz123 Dec 15, 2022

Choose a reason for hiding this comment

juanluisvaladas commented Dec 15, 2022

twz123 Dec 15, 2022

Choose a reason for hiding this comment

juanluisvaladas commented Dec 15, 2022

juanluisvaladas commented Dec 13, 2022 •

edited

Loading