Add spec.api.onlyBindToAddress configuration #3824

pschichtel · 2023-12-18T01:41:17Z

I picked up #1038 and rebased it onto main. Smoke tests pass, but not entirely clear what was left here.

Should a new bindAddress option be created and change documentation for the existing address option, or should the existing address option's behavior be changed to match the existing documentation?

The later I can see causing some issues with existing setups, bindAddress is probably the safest route. Currently the default for bindAddress is "0.0.0.0", which is the default kube-api-server uses if the --bind-address flag isn't used.

Issue Fixes #957

What this PR Includes Adds the ability to define a bindAddress for the kube-api-server.

twz123

Thanks for picking this up.

I'd double-check if there are some bad interactions with node-local load balancing? I imagine the LB config needs to pick up the correct bind addresses as well?

We should think about some integration test, too, I suppose.

docs/configuration.md

pkg/apis/k0s/v1beta1/api.go

pkg/component/controller/apiserver.go

pkg/apis/k0s/v1beta1/api.go

twz123 · 2023-12-19T11:53:43Z

Okay, some things to proceed with:

We need an integration test. Need to come up with a scenario for that, i.e. what is to be tested.
We need to double-check if this plays nicely with nllb and other things. Do we need to update the docs on e.g. external load balancing? How would this affect k0sctl, if at all?

pschichtel · 2023-12-23T00:56:42Z

We need an integration test. Need to come up with a scenario for that, i.e. what is to be tested.

I'm not very familiar with the code base yet, can you point me to examples for integration tests?

We need to double-check if this plays nicely with nllb and other things.

I'd have to look into how nllb works exactly. Depending on how the upstream connection is made its configuration probably needs adjustments.

Do we need to update the docs on e.g. external load balancing?

I guess a disclaimer like "if you change the bind address make sure to align your external load balancer" or so. I feel like people changing bind addresses are probably aware of the impact. But that's just my opinion.

How would this affect k0sctl, if at all?

k0sctl would need to interact with the API server on the correct IP, wouldn't it?

twz123 · 2024-01-11T16:59:31Z

Sorry for getting back to you so late @pschichtel. I'll bluntly blame the holiday season for that. 😅

We need an integration test. Need to come up with a scenario for that, i.e. what is to be tested.

I'm not very familiar with the code base yet, can you point me to examples for integration tests?

We need to double-check if this plays nicely with nllb and other things.

I'd have to look into how nllb works exactly. Depending on how the upstream connection is made its configuration probably needs adjustments.

We could try to start with the existing nllb inttest, copy that over into a separate directory and start hacking on the new one. You can probably delete all the cluster-restart stuff in there, and just rely on the cluster bootstrapping part. You need to add the new directory name to inttest/Makefile.variables. Then you should be able to do something like make check-yolo (or whatever the name is you choose.)

Might make sense to simply try to overwrite the bind addresses of every component that you can get hold of and see what happens to the cluster.

Do we need to update the docs on e.g. external load balancing?

I guess a disclaimer like "if you change the bind address make sure to align your external load balancer" or so. I feel like people changing bind addresses are probably aware of the impact. But that's just my opinion.

Sounds good enough for the start.

How would this affect k0sctl, if at all?

k0sctl would need to interact with the API server on the correct IP, wouldn't it?

Yes. Would it need to look at the k0s config to figure this out? Or would it "just work"? Maybe @kke can tell.

pschichtel · 2024-01-14T18:39:25Z

Sorry for getting back to you so late @pschichtel. I'll bluntly blame the holiday season for that. 😅

No worries, I don't have any rush with this one.

We could try to start with the existing nllb inttest, copy that over into a separate directory and start hacking on the new one. You can probably delete all the cluster-restart stuff in there, and just rely on the cluster bootstrapping part. You need to add the new directory name to inttest/Makefile.variables. Then you should be able to do something like make check-yolo (or whatever the name is you choose.)

Might make sense to simply try to overwrite the bind addresses of every component that you can get hold of and see what happens to the cluster.

Makes sense, I'll have a go at that.

twz123 · 2024-02-19T16:19:01Z

(The linter error will go away after a rebase. See #3991 for details.)

pschichtel · 2024-02-19T16:42:39Z

yeah I plan to work on this again towards the end of this week

andycandy-de · 2024-03-22T23:11:18Z

Any idea when this feature will be available? I also have multiple IPs and only want to bind the k0scontroller to a specific IP.

pschichtel · 2024-03-25T15:53:58Z

@andycandy-de sadly this had to be pushed back a little in my prio list, but I still intent to eventuelly finish this up. I think some of the recent k0s/k8s releases also added additional components, which probably needs/deserves some investigation. I'm currently unable to work on it until mid of April, so no sooner than that.

pkg/apis/k0s/v1beta1/api.go

pkg/apis/k0s/v1beta1/api_test.go

twz123

Thanks for your dedication and perseverance, and sorry for the very late feedback. Looks good. Just some small fixes to use net.JoinHostPort. I'd like to get some quick feedback from other maintainers on the name of the config knob, 👍 otherwise.

twz123 · 2024-08-29T14:02:08Z

docs/configuration.md

+| Element             | Description                                                                                                                                                                                                                  |
+|---------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `address`           | Local address on which to bind an API. Also serves as one of the addresses pushed on the k0s create service certificate on the API. Defaults to first non-local address found on the node.                                   |
+| `onlyBindToAddress` | Whether to bind only to the IP address given by the `address` option, instead of binding to all addresses.                                                                                                                   |


Would be great to add some details as to why this knob exists, and what may be the consequences if set to true. Basically some excerpt of #3824 (comment).

I improved it I think, please have a look.

pkg/component/controller/apiserver.go

cmd/controller/certificates.go

docs/configuration.md

twz123 · 2024-08-29T14:20:09Z

docs/configuration.md

@@ -52,8 +52,10 @@ metadata:
 spec:
  api:
    address: 192.168.68.104
+    onlyBindToAddress: false


As it defaults to false and it's omitempty, the k0s kubeconfig create command won't include that field.

Suggested change

onlyBindToAddress: false

twz123 · 2024-08-29T14:20:53Z

docs/configuration.md

+| Element             | Description                                                                                                                                                                                                                  |
+|---------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `address`           | Local address on which to bind an API. Also serves as one of the addresses pushed on the k0s create service certificate on the API. Defaults to first non-local address found on the node.                                   |
+| `onlyBindToAddress` | Whether to bind only to the IP address given by the `address` option, instead of binding to all addresses.                                                                                                                   |


I wonder if we could just keep bindAddress instead of onlyBindToAddress and let the user decide which address to bind to instead of forcing it to be either the advertised address or all interfaces. This may be a niche requirement, though. I cannot really judge. Maybe some folks with more network expertise have an opinion? OTOH, the address field will be defaulted by k0s's address discovery if omitted. There wouldn't be an easy way to let users use the auto-detected address as the bind address, then.

Anyway, I'm fine with the onlyBindToAddress logic as well, but wanted to at least ask about it.

IMO bindAddress makes more sense and is probably more common in general. At least I remember seeing such knobs on other things quite often. 😄

Agreed. However, what about

There wouldn't be an easy way to let users use the auto-detected address as the bind address, then.

hmm, what's wrong in the current default behaviour of autodetecting the publish address and binding to all interfaces?

IMO bindAddress makes more sense and is probably more common in general. At least I remember seeing such knobs on other things quite often.

bind-address is a common term yes, but rarely any software asks you to correctly configure several addresses for different purposes, that may or may not interact with each other. That's why I intended to reduce the bind-address parameter to what makes sense. I think the current documentation of address is misleading, given nothing actually binds to that address, it's just the internal address published to other components.

hmm, what's wrong in the current default behaviour of autodetecting the publish address and binding to all interfaces?

The original issue #957 describe where this PR comes from: Multi-homed kubernetes hosts. In my case specially, my k0s hosts was also my edge router, so one of the interfaces the API server bound to was my internet uplink. With proper firewall rules in place no big deal, but it carries some risk. My setup since changed (for the better).

There wouldn't be an easy way to let users use the auto-detected address as the bind address, then.

if the user configured bindToAddressOnly: true without actually configuring the address, then it would just use the auto-detected address, right? (probably worth documenting too)

As I said in #3824 (comment), address and bindAddress are strongly related. Configuring them incorrectly will break the cluster. You can't bind to an address that's not internally announced (unless NAT between components is supported, which would be ridiculous), so you have 2 options: bind to all interfaces and announce one of the addresses or bind to the address you announce. I can't see any other valid configuration (again: assuming NAT is not a thing here).

Also... I'd assume (I don't have numbers on it) fairly few people actually have a need for this. Few kubernetes hosts are multi-homed and of those most are fine with binding all interfaces.

twz123 · 2024-08-29T14:24:12Z

docs/configuration.md

+| Element             | Description                                                                                                                                                                                                                  |
+|---------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `address`           | Local address on which to bind an API. Also serves as one of the addresses pushed on the k0s create service certificate on the API. Defaults to first non-local address found on the node.                                   |
+| `onlyBindToAddress` | Whether to bind only to the IP address given by the `address` option, instead of binding to all addresses.                                                                                                                   |


An alternative name could be bindToAllInterfaces which defaults to true. This would have to be implemented as a bool pointer though. onlyBindToAddress is a bit harder for me to understand.

I read in other PRs that you generally prefer defaulting to false, so I'd assume other booleans are also like that. Not sure this is worth breaking the consistency, especially since this is more of a niche option.

bindExclusivelyToAddress, bindExclusive, bind(To)Address(Only), addressBind?

Somehow onlyBindToAddress sounds clunky to me.

Of course address (advertise) + bindAddress: <address> (bind) would be the best, I think there's an issue for that (#1880).

Of course address (advertise) + bindAddress: <address> (bind) would be the best, I think there's an issue for that (#1880).

Still unsure if we care about the autodetection aspects. We'd loose those with address + bindAddress. Personally, I'm okay with the boolean flag (whatever name it will have in the end). We can revisit that whenever we get around to #4822.

Somehow onlyBindToAddress sounds clunky to me.

I agree, I'm not super happy with the name, but at least it's descriptive.

Of course address (advertise) + bindAddress:
(bind) would be the best

I actually don't agree here. I think bindAddress is not an option that should be offered, because the additional flexibility brings no benefit, only the risk that people misconfigure it.

pschichtel · 2024-08-29T20:42:25Z

Great. I'm currently on the go, but can probably clean this up on Sunday.

github-actions · 2024-08-31T17:53:08Z

This pull request has merge conflicts that need to be resolved.

pkg/apis/k0s/v1beta1/api.go

github-actions · 2024-09-06T13:03:08Z

This pull request has merge conflicts that need to be resolved.

pschichtel · 2024-09-10T09:35:43Z

Is that failing job just flakyness? It seems to exceeded a deadline unrelated to my change, right?

twz123 · 2024-09-10T20:55:35Z

Yep, the inttests are quite a bit flaky, unfortunately.

Signed-off-by: Alex Hutchins <gakio12@gmail.com> Signed-off-by: gakio12 <gakio12@gmail.com> Signed-off-by: Phillip Schichtel <phillip@schich.tel> # Conflicts: # cmd/controller/certificates.go

Signed-off-by: Phillip Schichtel <phillip@schich.tel>

twz123

Thank you!

mback2k · 2024-10-10T19:49:25Z

k0sctl will need to be tweaked to support this as it currently tries to validate the API server coming up this way:

executing `curl -kso /dev/null --connect-timeout 20 -w \"%{http_code}\" \"https://localhost:6443/version\"`"

and then fails this way:

INFO ==> Running phase: Upgrade controllers 
INFO [ssh] ...: starting upgrade   
INFO [ssh] ...: waiting for the k0s service to start 
INFO * Running clean-up for phase: Acquire exclusive host lock 
INFO * Running clean-up for phase: Upgrade controllers 
INFO ==> Apply failed

twz123 · 2024-10-11T06:21:01Z

@mback2k Mind creating a k0sctl issue for that?

pschichtel requested a review from a team as a code owner December 18, 2023 01:41

pschichtel requested review from makhov and jnummelin December 18, 2023 01:41

pschichtel mentioned this pull request Dec 18, 2023

Add spec.api.bindAddress configuration #1038

Closed

pschichtel force-pushed the feature/bind-address branch from 0e18077 to da52a28 Compare December 18, 2023 01:45

twz123 reviewed Dec 18, 2023

View reviewed changes

pschichtel force-pushed the feature/bind-address branch 7 times, most recently from 5d4f908 to 86eeb7b Compare December 18, 2023 21:31

pschichtel force-pushed the feature/bind-address branch from 86eeb7b to a27e9fd Compare December 23, 2023 00:58

pschichtel force-pushed the feature/bind-address branch from a27e9fd to 81c1516 Compare January 6, 2024 01:05

michaelbeaumont mentioned this pull request Jan 11, 2024

Add config option to set bind address for all components. #1880

Closed

16 tasks

pschichtel force-pushed the feature/bind-address branch from 81c1516 to d5c491e Compare January 29, 2024 02:15

pschichtel force-pushed the feature/bind-address branch from d5c491e to 22ddd5f Compare February 27, 2024 22:45

pschichtel force-pushed the feature/bind-address branch from 22ddd5f to 241f2e4 Compare April 14, 2024 09:25

twz123 reviewed Apr 16, 2024

View reviewed changes

pkg/apis/k0s/v1beta1/api.go Outdated Show resolved Hide resolved

pkg/apis/k0s/v1beta1/api_test.go Outdated Show resolved Hide resolved

twz123 reviewed Apr 16, 2024

View reviewed changes

pkg/apis/k0s/v1beta1/api_test.go Outdated Show resolved Hide resolved

pschichtel force-pushed the feature/bind-address branch from 241f2e4 to 42a1627 Compare April 16, 2024 19:50

pschichtel force-pushed the feature/bind-address branch from b374c5f to fce9505 Compare August 13, 2024 22:05

pschichtel force-pushed the feature/bind-address branch from fce9505 to a30115d Compare August 26, 2024 00:06

twz123 reviewed Aug 29, 2024

View reviewed changes

github-actions bot added the merge-conflict label Aug 31, 2024

pschichtel force-pushed the feature/bind-address branch from a30115d to 96fb12d Compare September 1, 2024 17:13

github-actions bot removed the merge-conflict label Sep 1, 2024

pschichtel force-pushed the feature/bind-address branch from 96fb12d to bfb1952 Compare September 1, 2024 17:24

jnummelin reviewed Sep 6, 2024

View reviewed changes

pkg/apis/k0s/v1beta1/api.go Outdated Show resolved Hide resolved

jnummelin added this to the 1.31 milestone Sep 6, 2024

twz123 changed the title ~~Add spec.api.bindAddress configuration~~ Add spec.api.onlyBindToAddress configuration Sep 6, 2024

github-actions bot added the merge-conflict label Sep 6, 2024

pschichtel force-pushed the feature/bind-address branch from bfb1952 to 4a92e86 Compare September 6, 2024 21:24

github-actions bot removed the merge-conflict label Sep 6, 2024

pschichtel force-pushed the feature/bind-address branch 2 times, most recently from 142dd33 to 2f1d5b5 Compare September 6, 2024 21:39

gakio12 and others added 3 commits September 15, 2024 20:58

Add spec.api.bindAddress configuration

548dc41

Signed-off-by: Alex Hutchins <gakio12@gmail.com> Signed-off-by: gakio12 <gakio12@gmail.com> Signed-off-by: Phillip Schichtel <phillip@schich.tel> # Conflicts: # cmd/controller/certificates.go

Add an integration test for spec.api.bindAddress

e799ec7

Signed-off-by: Phillip Schichtel <phillip@schich.tel>

replace BindAddress by Address and OnlyBindToAddress

a759281

Signed-off-by: Phillip Schichtel <phillip@schich.tel>

pschichtel force-pushed the feature/bind-address branch from 2f1d5b5 to a759281 Compare September 15, 2024 18:58

twz123 approved these changes Sep 17, 2024

View reviewed changes

twz123 merged commit d9473da into k0sproject:main Sep 17, 2024
88 checks passed

mback2k mentioned this pull request Oct 11, 2024

Missing support for spec.api.onlyBindToAddress configuration k0sproject/k0sctl#773

Closed

kke mentioned this pull request Oct 14, 2024

CPLB virtual address fall back for kube API URL k0sproject/k0sctl#772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add spec.api.onlyBindToAddress configuration #3824

Add spec.api.onlyBindToAddress configuration #3824

pschichtel commented Dec 18, 2023

twz123 left a comment

twz123 commented Dec 19, 2023

pschichtel commented Dec 23, 2023

twz123 commented Jan 11, 2024

pschichtel commented Jan 14, 2024

twz123 commented Feb 19, 2024

pschichtel commented Feb 19, 2024

andycandy-de commented Mar 22, 2024

pschichtel commented Mar 25, 2024

twz123 left a comment

twz123 Aug 29, 2024

pschichtel Sep 1, 2024

twz123 Aug 29, 2024

twz123 Aug 29, 2024

jnummelin Aug 29, 2024

twz123 Aug 29, 2024

jnummelin Aug 30, 2024

pschichtel Sep 1, 2024

twz123 Aug 29, 2024

pschichtel Sep 1, 2024

kke Sep 11, 2024

twz123 Sep 11, 2024

pschichtel Sep 11, 2024

pschichtel commented Aug 29, 2024

github-actions bot commented Aug 31, 2024

github-actions bot commented Sep 6, 2024

pschichtel commented Sep 10, 2024

twz123 commented Sep 10, 2024

twz123 left a comment

mback2k commented Oct 10, 2024

twz123 commented Oct 11, 2024

Add spec.api.onlyBindToAddress configuration #3824

Add spec.api.onlyBindToAddress configuration #3824

Conversation

pschichtel commented Dec 18, 2023

twz123 left a comment

Choose a reason for hiding this comment

twz123 commented Dec 19, 2023

pschichtel commented Dec 23, 2023

twz123 commented Jan 11, 2024

pschichtel commented Jan 14, 2024

twz123 commented Feb 19, 2024

pschichtel commented Feb 19, 2024

andycandy-de commented Mar 22, 2024

pschichtel commented Mar 25, 2024

twz123 left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschichtel commented Aug 29, 2024

github-actions bot commented Aug 31, 2024

github-actions bot commented Sep 6, 2024

pschichtel commented Sep 10, 2024

twz123 commented Sep 10, 2024

twz123 left a comment

Choose a reason for hiding this comment

mback2k commented Oct 10, 2024

twz123 commented Oct 11, 2024