kube: allow overriding client api server URL via ENV

[ChandonPierre]

In our environment, the endpoint for kubernetes.default.svc is in the RFC6598 address range. This conflicts with the range Tailscale uses for node address assignment; additionally, the proxy pod drops traffic to addresses in this range that are not from the tailscale interface:

iifname != "tailscale0*" ip saddr 100.64.0.0/10 counter packets 0 bytes 0 drop

Previously we worked around this by setting --netfilter-mode=off but as of #11238 this has become more challenging.

I would like to instead override the address containerboot uses to talk to the kubernetes api server and point it to something not in the RFC6598 range. Having tested this locally it does appear to resolve my issue (where containerboot cannot POST to kubernetes.default.svc)

Closes #11397

[irbekrm]

Hi @ChandonPierre thank you for the PR.

Agree that we should fix #11397, thank you for picking that up.

We should though, instead of introducing extra env var for kube api server address, allow for those values to be read from the default kube env vars KUBERNETES_SERVICE_HOST, KUBERNETES_SERVICE_PORT so that users with Kubernetes environments where the API server is not exposed on kubernetes.default.svc can benefit from the values being already populated by the service provider. We'll have introduce another env var, perhaps something like TS_KUBERNETES_READ_API_SERVER_ADDRESS_FROM_ENV and, if that's set, populate the API server URL from env var values here-ish. The operator can set TS_KUBERNETES_READ_API_SERVER_ADDRESS_FROM_ENV to true for its proxies as its own API server client already reads from the default env vars, but we probably don't want to change the defaults for containerboot.

I am not sure if this will fix your issue. Do you expect the API server's TLS cert to contain the address that you want to use?

[ChandonPierre]

I am not sure if this will fix your issue. Do you expect the API server's TLS cert to contain the address that you want to use?

It does, yes; the API server has the correct DNS SAN. I've built this branch and tested it in my environment.

We should though, instead of introducing extra env var for kube api server address, allow for those values to be read from the default kube env vars KUBERNETES_SERVICE_HOST, KUBERNETES_SERVICE_PORT

Yes, this makes more sense

We'll have introduce another env var, perhaps something like TS_KUBERNETES_READ_API_SERVER_ADDRESS_FROM_ENV

I'm less convinced on this one - KUBERNETES_SERVICE_HOST should always be defined, even if service links are disabled:

https://github.com/kubernetes/kubernetes/blob/a31030543c47aac36cf323b885cfb6d8b0a2435f/pkg/kubelet/kubelet_pods.go#L664-L675

On the surface, it doesn't seem to me like this would introduce breaking functionality? This appears to be how client-go handles in-cluster config:

https://github.com/kubernetes/client-go/blob/62f959700d559dd8a33c1f692cb34219cfef930f/tools/clientcmd/client_config.go#L644-L650

[irbekrm]

It should always be defined, but in practice, we have seen various non-standard environments and I am weary of breaking folks' custom configurations (i.e non-operator kube deployments).

[ChandonPierre]

@irbekrm updated in 5c3933f

[irbekrm]

Thanks @ChandonPierre I've left a couple comments to restructure the PR, let me know if that makes sense. Apart from that the change in general makes sense and thanks again for picking this up 👍🏼

[irbekrm]

Instead of doing this, can we have the caller to read the env var (in that case it would be the containerboot client here). We could add a new New(opts ClientOpts) function and deprecate the old one. ClientOpts could have a URL field.

In containerboot, we can read in the env var here-ish and resolve the URL before creating new client.

[ChandonPierre]

Instead of doing this, can we have the caller to read the env var (in that case it would be the containerboot client here). We could add a new New(opts ClientOpts) function and deprecate the old one. ClientOpts could have a URL field.

In containerboot, we can read in the env var here-ish and resolve the URL before creating new client.

Having thought about this abit, I can't really envision doing this in a way that's DRY. I do feel like some of this would be simpler using client-go.

Since there's already a code path for reading the host from env for unit-tests and we want to gate this functionality around a feature flag anyway, I feel that the simplest solution is to use SetURL for this as well: ab5fbed

[irbekrm]

I do feel like some of this would be simpler using client-go

It definitely would, the reason why we don't do that is to keep the size smaller (the container image is used not just on kube)

I feel that the simplest solution is to use SetURL for this as well: ab5fbed

There is another kube client that gets initiated when state storage is a Kubernetes Secret, here to read/update the Secret. Your PR now only updates the client that is initiated to check the Secret permissions, whilst the client for state store will still use the default URL. (This code is a little bit convoluted as it has grown naturally and at some point will need refactor, but right now we are low bandwidth).

[ChandonPierre]

I see; seems like tests are handled differently there

recycled the same approach in 8da0086; still not very DRY I'm afraid

[irbekrm]

Thanks for the updates @ChandonPierre .

still not very DRY I'm afraid

I think this is the best we can get without a bigger refactor of the state.

Happy for this one to go in just one more nit- can we change os.Getenv("TS_KUBERNETES_READ_API_SERVER_ADDRESS_FROM_ENV") != "" -> os.Getenv("TS_KUBERNETES_READ_API_SERVER_ADDRESS_FROM_ENV") == "true" or something like that to not have folks wonder what value needs to be used & how to unset?

[ChandonPierre]

I think this is the best we can get without a bigger refactor of the state.

Agreed.

Happy for this one to go in just one more nit- can we change os.Getenv("TS_KUBERNETES_READ_API_SERVER_ADDRESS_FROM_ENV") != "" -> os.Getenv("TS_KUBERNETES_READ_API_SERVER_ADDRESS_FROM_ENV") == "true" or something like that to not have folks wonder what value needs to be used & how to unset?

Done in 659e3fb + squashed commits

[irbekrm]

Looks good thanks!

There is the typo mentioned + could you please reference #11397 in the commit message (i.e Updates tailscale/tailscale#11397 like here f227083)? That will fix the issuebot failure. I am not sure why the DCO check is failing as your commit looks signed.

Apart from that, happy for this to go in, thank you for your patience working on this

[ChandonPierre]

Looks good thanks!

There is the typo mentioned + could you please reference #11397 in the commit message (i.e Updates tailscale/tailscale#11397 like here f227083)? That will fix the issuebot failure. I am not sure why the DCO check is failing as your commit looks signed.

Apart from that, happy for this to go in, thank you for your patience working on this

Updated :)

I double checked my profile settings, not sure why DCO is picking up the obfuscated address

[irbekrm]

Thanks again for working on this @ChandonPierre , glad we got this one in.

I've cut a dev Helm chart/container images, if that helps with your testing (chart version "1.67.63", app version "unstable-v1.67.63")

Additionally (as I remember you mostly needed this change for a workaround to use the operator in a cluster with a CGNAT Pod CIDR range?), we've had a couple more users reports on this and might look into whether there is anything else we can do to not drop packets received on proxy Pod IPs.

[ChandonPierre]

I've cut a dev Helm chart/container images, if that helps with your testing (chart version "1.67.63", app version "unstable-v1.67.63")

Thanks! We'll get this validated

Additionally (as I remember you mostly needed this change for a workaround to use the operator in a cluster with a CGNAT Pod CIDR range?), we've had a couple more users reports on this and might look into whether there is anything else we can do to not drop packets received on proxy Pod IPs.

Our environment is a bit unique in that the API server endpoint (and a few other control-plane services) exist in the CGNAT range. Happy to help with testing if it's helpful - feel free to shoot over an e-mail to coordinate.