-
Notifications
You must be signed in to change notification settings - Fork 370
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.
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
Add spec.api.onlyBindToAddress configuration #3824
Conversation
0e18077
to
da52a28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5d4f908
to
86eeb7b
Compare
Okay, some things to proceed with:
|
I'm not very familiar with the code base yet, can you point me to examples for integration tests?
I'd have to look into how nllb works exactly. Depending on how the upstream connection is made its configuration probably needs adjustments.
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.
k0sctl would need to interact with the API server on the correct IP, wouldn't it? |
86eeb7b
to
a27e9fd
Compare
a27e9fd
to
81c1516
Compare
Sorry for getting back to you so late @pschichtel. I'll bluntly blame the holiday season for that. 😅
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 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.
Sounds good enough for the start.
Yes. Would it need to look at the k0s config to figure this out? Or would it "just work"? Maybe @kke can tell. |
No worries, I don't have any rush with this one.
Makes sense, I'll have a go at that. |
81c1516
to
d5c491e
Compare
(The linter error will go away after a rebase. See #3991 for details.) |
yeah I plan to work on this again towards the end of this week |
d5c491e
to
22ddd5f
Compare
Any idea when this feature will be available? I also have multiple IPs and only want to bind the k0scontroller to a specific IP. |
@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. |
22ddd5f
to
241f2e4
Compare
241f2e4
to
42a1627
Compare
b374c5f
to
fce9505
Compare
fce9505
to
a30115d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
docs/configuration.md
Outdated
| 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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I improved it I think, please have a look.
docs/configuration.md
Outdated
@@ -52,8 +52,10 @@ metadata: | |||
spec: | |||
api: | |||
address: 192.168.68.104 | |||
onlyBindToAddress: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it defaults to false
and it's omitempty
, the k0s kubeconfig create
command won't include that field.
onlyBindToAddress: false |
docs/configuration.md
Outdated
| 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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO bindAddress
makes more sense and is probably more common in general. At least I remember seeing such knobs on other things quite often. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. However, what about
There wouldn't be an easy way to let users use the auto-detected address as the bind address, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, what's wrong in the current default behaviour of autodetecting the publish address and binding to all interfaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
docs/configuration.md
Outdated
| 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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Great. I'm currently on the go, but can probably clean this up on Sunday. |
This pull request has merge conflicts that need to be resolved. |
a30115d
to
96fb12d
Compare
96fb12d
to
bfb1952
Compare
This pull request has merge conflicts that need to be resolved. |
bfb1952
to
4a92e86
Compare
142dd33
to
2f1d5b5
Compare
Is that failing job just flakyness? It seems to exceeded a deadline unrelated to my change, right? |
Yep, the inttests are quite a bit flaky, unfortunately. |
Signed-off-by: Alex Hutchins <[email protected]> Signed-off-by: gakio12 <[email protected]> Signed-off-by: Phillip Schichtel <[email protected]> # Conflicts: # cmd/controller/certificates.go
Signed-off-by: Phillip Schichtel <[email protected]>
Signed-off-by: Phillip Schichtel <[email protected]>
2f1d5b5
to
a759281
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
and then fails this way:
|
@mback2k Mind creating a k0sctl issue for that? |
I picked up #1038 and rebased it onto main. Smoke tests pass, but not entirely clear what was left here.
From #1038: