-
Notifications
You must be signed in to change notification settings - Fork 968
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
Refactor kubernetes client config initialization and fix in-cluster config #690
Conversation
kubernetes/provider.go
Outdated
if v, ok := d.GetOk("host"); ok { | ||
cfg.Host = v.(string) | ||
overrides.ClusterInfo.Server = v.(string) |
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.
This change causes a regression when passing a bare IP address instead of a URL, e.g. when using this:
provider "kubernetes" {
version = "1.10.1-dev3"
load_config_file = false
host = data.google_container_cluster.cluster0.endpoint
token = data.google_client_config.default.access_token
cluster_ca_certificate = base64decode(
data.google_container_cluster.cluster0.master_auth[0].cluster_ca_certificate,
)
}
I believe that's because overrides.ClusterInfo.Server
always expects a URL: https://github.com/kubernetes/client-go/blob/v12.0.0/tools/clientcmd/api/types.go#L68-L69
While cfg.Host
is more flexible: https://github.com/kubernetes/client-go/blob/v12.0.0/rest/config.go#L53-L57
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.
A work-around is to use this:
host = "https://${data.google_container_cluster.cluster0.endpoint}:443"
But I'm experimenting how to avoid this.
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.
Resolved.
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.
Not the proper way to do 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.
Alright, overrides
are processed too late to be taken into account by defaultServerUrlFor()
:
- https://github.com/kubernetes/client-go/blob/v12.0.0/tools/clientcmd/client_config.go#L176
- https://github.com/kubernetes/client-go/blob/v12.0.0/rest/config.go#L438-L439
- https://github.com/kubernetes/client-go/blob/v12.0.0/rest/url_utils.go#L85
- https://github.com/kubernetes/client-go/blob/v12.0.0/tools/clientcmd/client_config.go#L189
- https://github.com/kubernetes/client-go/blob/v12.0.0/tools/clientcmd/client_config.go#L211
So far, I've tested this successfully in the following situations: A subset of acceptance tests
Using kubeconfig against GKE 1.13.11-gke.5
Using access token against GKE 1.13.11-gke.14
In-cluster with minikube and kubernetes 1.16Cross-cluster with minikube and kubernetes 1.16 targeting minikube and kubernetes 1.15Configurations not tested yet
|
I take this one for a ride in CI, three major cloud platforms. |
@pdecat @alexsomesan what's the state of this PR? Anything I could help to move it forward? |
At this stage, I believe it mostly needs more testing and a complete review. |
…es to support both HTTP and HTTPS when a bare IP address is passed into the host provider field
6457242
to
bff578f
Compare
@alexsomesan anything we could do to help move this forward? |
I've tested this and it works as specified. I'm a bit on the fence whether we should fail when running in-cluster unless the On the other hand, I have one suggestion about the example based on a job. I think it should be more explicit about the fact that it's not illustrating the default in-cluster scenario and explain that external credentials are being injected as well as the external API being used. I have a feeling that in-cluster API is little known enough that this clarification will save confusion going forward. |
@houqp aiming to add this to the upcoming release. Likely on monday. |
Good news 🎉
What about adding explicit messages with
We can indeed expand a bit on the description of the job example. Maybe referencing some other existing doc on the matter could help, e.g.:
Feel free to add what you think is best, I'm not sure I'll have time to address this before Monday. |
Sounds good, @pdecat. I can take care of the changes. |
I've given it more thought and I think the examples included here all have their merits. I'd like to keep them as they are. I'll follow up with a simpler one for in-cluster in addition to this. |
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.
Tested manually and in CI. Works as expected.
This PR is an alternative to #686 that intends to: