Restore support for env-based API server discovery

[olix0r]

Current and expected behavior

In Linkerd, we have a report that dd0b258 breaks discovery for some configurations of Azure clusters. I'm not 100% clear on the details, but it sounds like they can't rely on the default kubernetes service.

Given that client-go still honors these environment variables, this manifests as only the Rust controllers being unable to communicate with the API server.

Possible solution

In the interest of compatibility (i.e., with client-go's behavior), I think it would be best to restore support for the environment variables.

If we really don't want to continue to support the environment variables, we should probably engage client-go about removing support for the environment variables so that there's really no expectation that they can be used.

Additional context

No response

Environment

...

Configuration and features

...

Affected crates

kube-client

Would you like to work on fixing this bug?

maybe

[olix0r]

From recent AKS release notes https://github.com/Azure/AKS/releases/tag/2022-07-17:

The annotation kubernetes.azure.com/set-kube-service-host-fqdn can now be added to pods to set the KUBERNETES_SERVICE_HOST variable to the domain name of the API server instead of the in-cluster service IP. This is useful in cases where the cluster egress is via a layer 7 firewall, like Azure Firewall with Application Rules.

AKS clusters with egress firewalls basically require the use of the environment variables for discovery :(

[nightkr]

Seems like the important section is https://docs.microsoft.com/en-us/azure/aks/limit-egress-traffic#required-outbound-network-rules-and-fqdns-for-aks-clusters. This seems like a pretty weird way to handle things on their end, but I can see other use cases for letting users override the address. And as you say, it's probably worth at least trying to stay aligned with client-go to minimize confusion.

I'd be +1 on reverting #876, but waiting for @clux's input since he seemed to have pretty strong opinions on the topic.

[olix0r]

I opened an issue on AKS Azure/AKS#3183, but that's probably a longer conversation.

I'd be willing to get a PR together if we can make it into the next kube release :)

[clux]

Well, the problem is that the old method also has problems, so not sure there's many great solutions here for us.

• AWS had problems with it using IPV6 In-cluster didn't format correct kube_host_port for IPv6 cluster #874
• rustls does not work in-cluster without explicit dns names rustls cannot reach a cluster through ip #153 (so while it does not work out-of-cluster without workarounds, it is load bearing in-cluster)

And as long as client-go has this TODO, it's not super tempting to switch our default way back to what is also broken.

What about some way to opt-into to old behaviour like some KUBE_LEGACY_CLUSTER_IP evar for AKS?

[nightkr]

The AWS thing looks like an error on our side, Rust's IpAddr also seems to accept "bare" IPv6 addresses. I think a reasonable heurestic would be to apply [{host}]:{port} if host.contains(':'), otherwise stick to {host}:{port}.

The rustls situation is sadder, but outside of our control (and an opt-in feature).

[olix0r]

@clux Thanks. This is helpful context.

What about some way to opt-into to old behaviour like some KUBE_LEGACY_CLUSTER_IP evar for AKS?

Something like this is probably a good middle-ground. In Linkerd, we could support an install flag that enables this.

Alternatively, we could do something more clever (but maybe too clever): only honor KUBERNETES_SERVICE_HOST when it is not an IP. E.g.

fn kube_dns() -> String {
    if let Ok(h) = std::env::var("KUBERNETES_SERVICE_HOST") {
        if !h.is_empty() && h.parse::<std::net::IpAddr>().is_err() {
            return h
        }
    }
    "kubernetes.default.svc".to_string()
}

[nightkr]

Alternatively, we could do something more clever (but maybe too clever): only honor KUBERNETES_SERVICE_HOST when it is not an IP. E.g.

This feels waaaay too weird and magical IMO.

Maybe if we emit a loud warning when ignoring the address, but that would give 99% of users a warning for a non-issue that they have no control over.

[olix0r]

I also opened kubernetes/kubernetes#112263 to discuss changing client-go's behavior.

[clux]

too magical

Yeah, same. Given this is legacy behaviour, only affects a smaller number of users, and can be reasonably opted in by Linkerd, it is preferable for us to have it be an explicit opt-in in our codebase about what this is meant to do - and that it is not intended to stick around (unless client-go does a 180 at some point).

Logging a message for using non-standard Kubernetes behaviour when activating this code path is probably a good idea as well, but it can be less loud when it's opt-in.

(The IPv6 issue could also be fixed, but when we tried that fix it, we ran into cert validation issues, so making it opt-in avoids us spending time on that.)

[clux]

Those upstream issues are great. Thanks a lot!

[olix0r]

#1001 adds a separate config constructor; but kubernetes/kubernetes#112263 (comment) suggests that client-go's behavior may be "correct." If that's the case, #1001 at least fixes host/port formatting for IPv6; and we should probably try to fix the webpki problem in Rustls...

https://github.com/rustls/webpki/ is a recent fork of briansmith/webpki that will be maintained by the rustls folks. We may have a path forward to fix IP validation in that repo. But, from our experience in Linkerd, there are more than enough quirks in rustls that make it a poor choice for supporting a wide variety of cluster configurations.

[olix0r]

Looking at some of the officially supported kubernetes client libraries, all appear to use the environment variables and none use the in-cluster DNS

• https://github.com/kubernetes-client/javascript/blob/98414841064cbc322b617538a34bf0f5e31a5122/src/config.ts#L194-L219
• https://github.com/kubernetes-client/python/blob/16935036252be0fdf164f8ce624bd71eb8d35e89/kubernetes/base/config/incluster_config.py#L68-L70
• https://github.com/kubernetes-client/c/blob/2bb4393cc827e4c105fe57ded59e787bc3a4fbf0/kubernetes/config/incluster_config.c#L44-L67
• https://github.com/kubernetes-client/csharp/blob/f0d15cb39ef377370b219f0aa5f860764f0cc12a/src/KubernetesClient/KubernetesClientConfiguration.InCluster.cs#L51-L52
• https://github.com/kubernetes-client/haskell/blob/e848bc0c5c8d4fe4d2c9e8555331c219814c8ec9/kubernetes-client/src/Kubernetes/Client/Config.hs#L97-L99
• https://github.com/kubernetes-client/java/blob/73ae39a97dfdec9936944c23015edc1ab6742589/util/src/main/java/io/kubernetes/client/util/ClientBuilder.java#L242-L253
• https://github.com/kubernetes-client/ruby/blob/33861097e2eab9954b3f07f6898e5e8199199731/kubernetes/src/kubernetes/config/incluster_config.rb#L22-L46

I think this means that we really can't rely on the in-cluster DNS name.