Skip to content
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

feat(listens) support multiple addresses and IPv6 #986

Merged
merged 4 commits into from
Jan 19, 2024
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jan 18, 2024

What this PR does / why we need it:

Configure default IPv6 listens for Kong listeners by default. The controller defaults are unchanged as the controller settings do not support multiple listen addresses. Note that this diverges from upstream: kong.conf.default does not bind to IPv6.

Replace the hidden .address service setting with a new hidden .addresses setting. The new setting is a list whereas the old setting was a single string.

Honor address overrides for the admin listen. It previously forced you to the default 0.0.0.0 address or 127.0.0.1 when disabled.

Which issue this PR fixes

Fix #847.

Relevant to Kong/kubernetes-ingress-controller#5138.

Special notes for your reviewer:

See listen_tests.txt for example settings using
values.yaml.txt

Umbrella chart shenanigans mean we can't really check this in ingress easily. I stuffed the modified kong chart into
ingress.tar.gz. It requires a nightly to actually work because of kic#5138:

helm install ana /tmp/symingress --set controller.ingressController.image.repository="kong/nightly-ingress-controller" --set controller.ingressController.image.tag="nightly" --set controller.ingressController.image.effectiveSemver=3.1.0

The old address field was never documented and AFAIK there shouldn't be much reason to use it (containers in Kubernetes shouldn't have reason to listen on anything other than all addresses or localhost only). The change to addresses is technically breaking but I'm considering it not so because it was effectively a developer-only setting.

Checklist

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • New or modified sections of values.yaml are documented in the README.md
  • Commits follow the Kong commit message guidelines

@rainest rainest marked this pull request as ready for review January 18, 2024 01:39
@rainest rainest requested a review from a team as a code owner January 18, 2024 01:39
czeslavo
czeslavo previously approved these changes Jan 18, 2024
@czeslavo
Copy link
Contributor

Was making it an opt-in setting (bind IPv6 only if explicitly asked for) an option? I'm not sure if that's a good direction to diverge from the defaults Kong uses.

@pmalek
Copy link
Member

pmalek commented Jan 18, 2024

Was making it an opt-in setting (bind IPv6 only if explicitly asked for) an option? I'm not sure if that's a good direction to diverge from the defaults Kong uses.

I share the same concern. Do we really want to flip this default for all users? Will that work on all machines, e.g. what if I have IPv6 disabled? Is that a noop for me then?

Would it be possible to make this a proper chart value, accepting a list with the default having a single entry: 0.0.0.0. Add a comment in values.yaml saying that if IPv6 is desired then add [::] to that list. I believe the implementation in the templates would basically remain the same but it would allow users to configure that. WDYT?

Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes just to block until we discuss making it configurable. Feel free to override if that's resolved

@rainest
Copy link
Contributor Author

rainest commented Jan 19, 2024

tl;dr

  • I think we should avoid yet one (or N=Service count) more config setting you have to set just to get a working install if we can. If we can support IPv6-only clusters out of the box without you needing to toggle on "IPv6 mode" explicitly, we should.
  • My intuition is that not many installs would need to override defaults to explicitly disable an address family the cluster supports. If we get reports that people actually do need that, we can add the settings in later, but adding them preemptively probably just gets us a setting nobody will actually use.
  • Because some nuances around how Services work, this effectively only matters for IPv6-only clusters. Dual-stack clusters still default to IPv4 only in practice.
  • There are no obvious technical issues with using the new defaults on dual stack or IPv4-only clusters. The container environment always supports dual stack. Its IPv6 address goes unused if the cluster isn't set up to use it.
  • Discovery does unfortunately have a duplicated update bug with dual stack, though the Service nuances mean that doesn't show up in practice as-is.

I'd like to avoid requiring a toggle to support something that should arguably work out of the box if it's available in the environment.

Unfortunately absent ongoing user surveys we don't have a good way to float new features, so turning it on and seeing if it causes issues for someone is the less than ideal way to see if we need a toggle.

Even with one I'd say it should default to IPv6 enabled, since it's only relevant if the wider environment supports it. If you have a dual-stack cluster but have somehow broken your networking such that only IPv4 actually works, you should probably disable IPv6 at the environment level rather than expecting applications to not try using IPv6.

The controller lacking support for multiple listens de facto still requires changing defaults for v6-only clusters, but ideally we address that in the future.

I don't want to formally expose addresses ever (AFAIK there shouldn't ever be a reason to bind to explicit addresses instead of wildcard/wildcard local insisde the container), but we could expose https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services and infer the appropriate listens from the enabled families.

It is extra template logic and extra config burden (especially if we do it "properly" and let you set it per-Service--I'd probably ignore that and expose a single setting/require you use the same setting for every Service) though, so if we can live without it, we save one more setting that <1% of installs will actually use.

Will that work on all machines, e.g. what if I have IPv6 disabled?

Yeah. The only problem case is if you're using a single-stack listen for only the protocol your cluster doesn't support. More detail below, but from Kong's perspective its environment does support either address family. It can bind to IPv6 even if those container-internal addresses aren't reachable from outside. Having a link-local IPv6 address doesn't interfere with IPv4 traffic.

For many dual-stack clusters we actually won't use IPv6 at the cluster level anyway. Kubernetes has a an apparently hard-coded default Service configuration that defaults to single-stack only based on the first IP range you configure. If you configure IPv4 addresses first, you'll actually only use IPv4, at least for Services, unless we do expose the family/stack Service settings.

If you configure IPv6 first, you were presumably in a broken state similar to IPv6-only clusters, since the Service would effectively be IPv6-only and clients would only try to talk v6 and fail because there was no corresponding container listen. I assume these were probably uncommon in practice given the apparent absence of complaints about us not working on dual-stack clusters.

Since we're using Service Endpoints for discovery, you'll only get the address family the Service supports even if the Pod has both families assigned (AFAICT this holds for both normal and headless Services equally). Most installs will still use IPv4 only even on dual-stack clusters.

Testing discovery with a manually-configured dual stack admin Service does indicate that it's a bit boneheaded about this though. IIRC the code is just listing every endpoint and assuming every address it finds is a Pod (which is true in single stack), and the updates then fire twice. In a single proxy replica environment:

2024-01-19T00:11:53Z	debug	events	successfully applied Kong configuration to https://[fd00:10:244::10]:8444	{"v": 1, "type": "Normal", "object": {"kind":"Pod","namespace":"default","name":"ana-controller-6cd6c6dd9b-kptm9","apiVersion":"v1"}, "reason": "KongConfigurationSucceeded"}
2024-01-19T00:11:53Z	debug	events	successfully applied Kong configuration to https://10.244.0.16:8444	{"v": 1, "type": "Normal", "object": {"kind":"Pod","namespace":"default","name":"ana-controller-6cd6c6dd9b-kptm9","apiVersion":"v1"}, "reason": "KongConfigurationSucceeded"}

2024-01-19T00:11:56Z	debug	events	successfully applied Kong configuration to https://10.244.0.16:8444	{"v": 1, "type": "Normal", "object": {"kind":"Pod","namespace":"default","name":"ana-controller-6cd6c6dd9b-kptm9","apiVersion":"v1"}, "reason": "KongConfigurationSucceeded"}
2024-01-19T00:11:56Z	debug	events	successfully applied Kong configuration to https://[fd00:10:244::10]:8444	{"v": 1, "type": "Normal", "object": {"kind":"Pod","namespace":"default","name":"ana-controller-6cd6c6dd9b-kptm9","apiVersion":"v1"}, "reason": "KongConfigurationSucceeded"}

On an IPv4-only KIND instance, I'm using the (new) default dual-stack admin bind. The controller uses the IPv4 cluster IP because there's no v6 IP available and successfully talks to the admin API:

$ kubectl logs ana-gateway-llrq8 | grep admin_listen
2024/01/18 22:18:54 [debug] admin_listen = {"0.0.0.0:8444 http2 ssl","[::]:8444 http2 ssl"}

$ kubectl logs ana-controller-zxghl | grep 8444
...
2024-01-18T22:18:57Z	info	Successfully synced configuration to Kong	{"url": "https://10.244.0.12:8444", "update_strategy": "InMemory", "v": 0}

$ cat /tmp/v4.conf                             
kind: Cluster          
apiVersion: kind.x-k8s.io/v1alpha4
networking:
  ipFamily: ipv4
  apiServerAddress: 127.0.0.1

Going into more detail, TIL that containers do not inherit their host's network capabilities, which I guess makes sense. On the kubelet:

root@boop-control-plane:/# sysctl -A 2>/dev/null | grep net.ipv6.conf.default.disable_ipv6
net.ipv6.conf.default.disable_ipv6 = 1

versus in the proxy container:

kong@ana-gateway-llrq8:/$ sysctl -A 2>/dev/null | grep net.ipv6.conf.default.disable_ipv6  
net.ipv6.conf.default.disable_ipv6 = 0

Can't see a whole lot further in the container since we lack most of the standard Linux network tools (and I can't figure it out from /proc info) and NGINX isn't reporting low-level bind info on success, but I'm guessing that we still get a self-assigned v6 address and bind to that. For comparison, using an explicit address we don't have in the container definitely fails:

nginx: [emerg] bind() to 9.9.9.9:8999 failed (99: Cannot assign requested address)

In that sense it does probably make sense for upstream kong.conf.default to default to IPv4 only, since it may well end up being installed into a v4-only environment and would fail without overrides. In our case, however, we can rely on the container always supporting both and don't need to be as conservative.

@rainest rainest requested a review from pmalek January 19, 2024 00:23
pmalek
pmalek previously approved these changes Jan 19, 2024
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under impression that exposing this would be relatively simple hence my proposal. Basically, instead of "hardcoding" in templates, use a default that people will be able to change. Opt in or opt out (preferably the latter) but not hardcoded until it breaks things.

Given the investigation and results posted in this PR I'm fine with moving forward with this.

@rainest rainest dismissed stale reviews from pmalek and czeslavo via 5ed9691 January 19, 2024 20:23
Configure default IPv6 listens for Kong listeners by default. The
controller defaults are unchanged as the controller settings do not
support multiple listen addresses.

Replace the hidden .address service setting with a new hidden .addresses
setting. The new setting is a list whereas the old setting was a single
string.

Honor address overrides for the admin listen. It previously forced you
to the default 0.0.0.0 address or 127.0.0.1 when disabled.
@rainest
Copy link
Contributor Author

rainest commented Jan 19, 2024

Rebase to fix conflicts and added a release as well, since someone wanted the other thing that got merged.

@rainest rainest merged commit d8e5998 into main Jan 19, 2024
24 checks passed
@rainest rainest deleted the feat/v6-multiaddr branch January 19, 2024 21:14
@lel-amri
Copy link

Thanks for making IPv6 easier to use. I'm hitting an issue at work on an IPv4 only cluster. It seems that the "container environment" does not set any IPv6 address in our pods networks namespaces. AFAIK, the network setup (lo included) is handled by the CNI, which, in our case, is explicitly told to not set IPv6. @rainest do you have a quote from a specification that says "the container environment always supports dual stack"? If that's not the case, I'd consider this default configuration a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot choose admin listen address
4 participants