`Config`: New `incluster` and `incluster_dns` constructors #1001

olix0r · 2022-09-07T22:50:07Z

This change restores the default behavior of reading the
KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT environment
variables, matching the official Kubernetes client libraries' behavior.
This behavior only applies when the rustls-tls feature is not enabled.

When the rustls-tls feature is enabled, incluster configurations
reference the DNS name kubernetes.default.svc.

Related to #1003 kubernetes/kubernetes#112263
Closes #1000

Signed-off-by: Oliver Gould ver@buoyant.io

The `Config::from_cluster_env` constructor is misleadingly named: it doesn't use the environment, it uses the default cluster configurations. This change deprecates the `Config::from_cluster_env` constructor in favor of `Config::load_in_cluster`. An additional constructor, `Config::load_in_cluster_from_legacy_env`, uses the `KUBERNETES_SERVICE_HOST` and `KUBERNETES_SERVICE_PORT` environment variables to match client-go's behavior. This changes does NOT alter the default inferred configuration in any way. It simply allows users to opt-in to using the old behavior. Related to kubernetes/kubernetes#112263 Closes kube-rs#1000 Signed-off-by: Oliver Gould <ver@buoyant.io>

.devcontainer/devcontainer.json

kube-client/src/config/mod.rs

Signed-off-by: Oliver Gould <ver@buoyant.io>

olix0r · 2022-09-08T15:29:02Z

Given kubernetes/kubernetes#112263 (comment), it looks like the env-based configuration isn't legacy at all. It's the only officially supported means to connect to the Kubernetes API server from within the cluster. The documentation is a lie!

With this in mind, I think we should update this PR to:

Stop referring to the environment approach as legacy;
Make the environment-based approach the default behavior;
Provide an alternate constructor that uses the DNS name and ignores the environment.

Yes, this means that rustls won't be usable within the cluster. That will need to be clearly documented. We should followup in rustls/webpki to address the deficiencies.

clux · 2022-09-08T19:01:00Z

With this in mind, I think we should update this PR to:

Stop referring to the environment approach as legacy;

Make the environment-based approach the default behavior;

Provide an alternate constructor that uses the DNS name and ignores the environment.

Oh, dear. That's unfortunate. But I agree with this assessment in light of of this :(

Add `Config::from_cluster_dns` to support the current behavior. Signed-off-by: Oliver Gould <ver@buoyant.io>

codecov-commenter · 2022-09-09T00:56:35Z

Codecov Report

Merging #1001 (727d809) into master (b757859) will increase coverage by 0.03%.
The diff coverage is 65.95%.

@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
+ Coverage   72.17%   72.21%   +0.03%     
==========================================
  Files          64       64              
  Lines        4532     4574      +42     
==========================================
+ Hits         3271     3303      +32     
- Misses       1261     1271      +10

Impacted Files	Coverage Δ
kube-client/src/config/mod.rs	`45.55% <0.00%> (-3.26%)`	⬇️
kube-client/src/config/incluster_config.rs	`71.11% <83.78%> (+71.11%)`	⬆️

Signed-off-by: Oliver Gould <ver@buoyant.io>

kube-client/src/config/mod.rs

When `rustls-tls` is enabled, the `kubernetes.default.svc` DNS name is used. Otherwise, the `KUBERNETES_SERVICE_{HOST,PORT}` environment variables are used. Signed-off-by: Oliver Gould <ver@buoyant.io>

olix0r · 2022-09-09T03:32:50Z

I took it a step further and made the behavior dependent on the TLS feature that's enabled. This allows rustls to work in most clusters. If rustls-tls isn't enabled, then we use the environment variables. This seems likely to work in the largest number of clusters.

olix0r · 2022-09-09T03:35:19Z

@clux I updated this to expose a single Config::incluster that produces an incluster configuration based on the TLS feature that is enabled. I can restore Config::from_cluster_env if we want to maintain compatibility, but it seems inaccurate to keep that name when we may not use the env.

kube-client/src/config/mod.rs

kube-client/src/config/incluster_config.rs

clux · 2022-09-09T17:10:14Z

Thanks a lot. The name change looks justified to me - and this is already a breaking change so might as well make it right now. Just left a few minor nits on docs and tests.

* Make `Config::incluster_env` and `Config::incluster_dns` public regardless of what features are enabled. * Restrict visibility for `pub` helpers that are not actually publicly exported. Signed-off-by: Oliver Gould <ver@buoyant.io>

Signed-off-by: Oliver Gould <ver@buoyant.io>

clux · 2022-09-09T19:03:55Z

This looks all good to me, happy to merge if you are happy with it!

olix0r · 2022-09-09T19:35:15Z

@clux Thanks. Looks good to merge!

The kube `Config` constructors have been renamed in version 0.75.0: kube-rs/kube#1001.

olix0r force-pushed the ver/client-env-hack branch from 25ae3f6 to 4e3743e Compare September 7, 2022 22:54

olix0r commented Sep 7, 2022

View reviewed changes

.devcontainer/devcontainer.json Show resolved Hide resolved

kube-client/src/config/mod.rs Outdated Show resolved Hide resolved

constify "https" scheme to make accidental "http" harder

5ffdb5e

Signed-off-by: Oliver Gould <ver@buoyant.io>

olix0r mentioned this pull request Sep 7, 2022

Restore support for env-based API server discovery #1000

Closed

Restore Config::from_cluster_env naming

dd00e12

Add `Config::from_cluster_dns` to support the current behavior. Signed-off-by: Oliver Gould <ver@buoyant.io>

olix0r mentioned this pull request Sep 9, 2022

client: rustls is incompatible the env-based in-cluster config #1003

Closed

olix0r added 2 commits September 9, 2022 02:46

Disable the in-cluster rustls test

4a6aaf3

Signed-off-by: Oliver Gould <ver@buoyant.io>

fix typo

d272478

Signed-off-by: Oliver Gould <ver@buoyant.io>

olix0r commented Sep 9, 2022

View reviewed changes

kube-client/src/config/mod.rs Outdated Show resolved Hide resolved

client: Make discovery conditional on the TLS impl

bf4d07c

When `rustls-tls` is enabled, the `kubernetes.default.svc` DNS name is used. Otherwise, the `KUBERNETES_SERVICE_{HOST,PORT}` environment variables are used. Signed-off-by: Oliver Gould <ver@buoyant.io>

Merge branch 'master' into ver/client-env-hack

698d353

clux reviewed Sep 9, 2022

View reviewed changes

kube-client/src/config/mod.rs Outdated Show resolved Hide resolved

clux reviewed Sep 9, 2022

View reviewed changes

kube-client/src/config/incluster_config.rs Outdated Show resolved Hide resolved

clux reviewed Sep 9, 2022

View reviewed changes

kube-client/src/config/incluster_config.rs Show resolved Hide resolved

clux reviewed Sep 9, 2022

View reviewed changes

kube-client/src/config/incluster_config.rs Show resolved Hide resolved

clux added the changelog-change changelog change category for prs label Sep 9, 2022

clux added this to the 0.75.0 milestone Sep 9, 2022

olix0r added 3 commits September 9, 2022 17:50

Review feedback

be9da2b

* Make `Config::incluster_env` and `Config::incluster_dns` public regardless of what features are enabled. * Restrict visibility for `pub` helpers that are not actually publicly exported. Signed-off-by: Oliver Gould <ver@buoyant.io>

Add URI-formatting tests

7c9645c

Signed-off-by: Oliver Gould <ver@buoyant.io>

fmt

727d809

Signed-off-by: Oliver Gould <ver@buoyant.io>

clux approved these changes Sep 9, 2022

View reviewed changes

clux changed the title ~~client: Expose a Config constructor to support legacy configurations~~ Config: New incluster and incluster_dns constructors Sep 9, 2022

clux merged commit 79e4e09 into kube-rs:master Sep 9, 2022

olix0r deleted the ver/client-env-hack branch September 9, 2022 23:15

olix0r mentioned this pull request Sep 9, 2022

KUBERNETES_SERVICE_HOST env variable not being used to communicate with API-server linkerd/linkerd2#9339

Closed

teskje added a commit to MaterializeInc/materialize that referenced this pull request Nov 10, 2022

Adjust to renamed kube config constructor

d17c907

The kube `Config` constructors have been renamed in version 0.75.0: kube-rs/kube#1001.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Config`: New `incluster` and `incluster_dns` constructors #1001

`Config`: New `incluster` and `incluster_dns` constructors #1001

olix0r commented Sep 7, 2022 •

edited

olix0r commented Sep 8, 2022 •

edited

clux commented Sep 8, 2022

codecov-commenter commented Sep 9, 2022 •

edited

olix0r commented Sep 9, 2022

olix0r commented Sep 9, 2022

clux commented Sep 9, 2022

clux commented Sep 9, 2022

olix0r commented Sep 9, 2022

Config: New incluster and incluster_dns constructors #1001

Config: New incluster and incluster_dns constructors #1001

Conversation

olix0r commented Sep 7, 2022 • edited

olix0r commented Sep 8, 2022 • edited

clux commented Sep 8, 2022

codecov-commenter commented Sep 9, 2022 • edited

Codecov Report

olix0r commented Sep 9, 2022

olix0r commented Sep 9, 2022

clux commented Sep 9, 2022

clux commented Sep 9, 2022

olix0r commented Sep 9, 2022

`Config`: New `incluster` and `incluster_dns` constructors #1001

`Config`: New `incluster` and `incluster_dns` constructors #1001

olix0r commented Sep 7, 2022 •

edited

olix0r commented Sep 8, 2022 •

edited

codecov-commenter commented Sep 9, 2022 •

edited