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

Add switch to disable kubeconfig #36

Merged
merged 5 commits into from
Aug 3, 2017

Conversation

sergeohl
Copy link

The provider load by default the kubeconfig and merge it with the conf in Terraform.

We want do all the settings in terraform for that we don't want to include the conf local. The merge have some odd behaviour. So we want to have a way to disable it.

This pr add just a switch to disable the local config.

Exemple:

provider "kubernetes" {
    host                        = "${data.aws_ssm_parameter.k8s_url.value}"
    token                       = "${data.aws_ssm_parameter.jenkins_password.value}"
    insecure                    = true
    disable_local_config = true
}

@radeksimko
Copy link
Member

Hi @sergeohl
do you mind providing some details about this?

The merge have some odd behaviour.

I'm not necessarily against this change, but I'd like to know the reason - it sounds like we're adding a work around for a bug which should be fixed instead. 😉

@sergeohl
Copy link
Author

Hi @radeksimko

Yes I can explain. For exemple, I have kubeconfig with the configuration for my user. In my TF module I configure the provider with my CI user. When I launch terraforn command, the provider will take my kubeconfig and add the TF settings. All field empty in the TF and set in the kubeconfig will be there. Like certificate, username, password.

In function of result of merge, a apply will pass even if the CI user doesn't have the permission.
It is not a bug of the provider.

I think it is most in the side of k8s.io/kubernetes/pkg/client. If we give to the config several auth (token, usename,cert, ...) it have it's own order to choose which creds it have to use.

Personally in this kind of system (aka terrafom) I prefer it not use at all file in the home dir, expect if I said to use it.

@sergeohl
Copy link
Author

@radeksimko Is it a fair reason for you ?

@radeksimko
Copy link
Member

I'm not sure I fully understood the reasons - what you described in your last comment seems like an explanation of how it works, rather than justification of why it should work differently.

That said I can't find a strong reason to not include it as it doesn't affect users who wish to keep using the current behaviour, I was just hoping to see some detailed specific use cases or unexpected behaviours.

Either way, before we can merge this do you mind resolving conflicts?

I also wanted to discuss name of the option - positive flags usually work better for booleans, i.e. load_config_file defaulting to true instead of disable_local_config. It's easier to read in english.

Finally, as this is boolean (either false or true, never nil) we can significantly simplify the code to variable definition (2 lines) + conditional, e.g.

var cfg *restclient.Config
var err error
if d.Get("load_config_file").(bool) {
	cfg, err = tryLoadingConfigFile(d)
}

What do you think?

@sergeohl
Copy link
Author

sergeohl commented Aug 1, 2017

I changed the name of the variable as you asked and resolved the conflit.

For the reason, some settings in the k8s.io/kubernetes/pkg/client can be set together. Like a token with a auth certificate configuration. If in terraform you want to use token (to test locally your CI system) but for your cli usage of kubectl you have a configuration with certificate, the provider merge the config and terraform failed. You have to move your kubeconfig, launch your terraform command and get back your kubeconfig.

Some of our users had other problem like that with their minikube setup but I don't have the detail.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

This looks like it's nearly ready for shipping, I just left you two small comments.

@@ -97,4 +97,6 @@ The following arguments are supported:
* `config_context` - (Optional) Context to choose from the config file. Can be sourced from `KUBE_CTX`.
* `config_context_auth_info` - (Optional) Authentication info context of the kube config (name of the kubeconfig user, `--user` flag in `kubectl`). Can be sourced from `KUBE_CTX_AUTH_INFO`.
* `config_context_cluster` - (Optional) Cluster context of the kube config (name of the kubeconfig cluster, `--cluster` flag in `kubectl`). Can be sourced from `KUBE_CTX_CLUSTER`.
* `token` - (Optional) Token of your service account. Can be sourced from `KUBE_TOKEN`.
* `token` - (Optional) Token of your service account.
Copy link
Member

Choose a reason for hiding this comment

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

I think you removed a bit more than necessary here. 👀 🙂

Type: schema.TypeBool,
Optional: true,
DefaultFunc: schema.EnvDefaultFunc("KUBE_LOAD_CONFIG_FILE", true),
Description: "Do not load local kubeconfig.",
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind updating this description also?

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the patience and endurance. :)

@radeksimko radeksimko merged commit c25963e into hashicorp:master Aug 3, 2017
@ghost ghost locked and limited conversation to collaborators Apr 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants