Credentials changes for v2.0.0 #1052

jrhouston · 2020-11-02T20:54:07Z

Description

Removes the load_config_file attribute and drops implicit support for KUBECONFIG.

Acceptance tests

~~Have you added an acceptance test for the functionality being added?~~
Have you run the acceptance tests on this branch?

Release Note

Release note for CHANGELOG:

Remove `load_config_file` attribute from provider block
Remove explicit support for KUBECONFIG
Remove config_path default to ~/.kube/config
Add config_paths attribute to provider block

References

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

jrhouston · 2020-11-13T21:57:49Z

kubernetes/provider.go

+	if v, ok := d.Get("config_paths").([]string); ok && len(v) > 0 {
+		configPaths = v
+	} else if v := os.Getenv("KUBE_CONFIG_PATHS"); v != "" {
+		// NOTE we have to do this here because the schema


There's an open issue for this hashicorp/terraform-plugin-sdk#142

kubernetes/provider.go

jrhouston · 2020-11-20T15:36:14Z

kubernetes/provider_test.go

-	}
-	if err := os.Unsetenv("KUBE_LOAD_CONFIG_FILE"); err != nil {
-		t.Fatalf("Error unsetting env var KUBE_LOAD_CONFIG_FILE: %s", err)
+	envVars := map[string]string{


Mopped this up because it was tedious. 🧹

Nice, I actually did the same thing here, but I removed the whole section. I found it wasn't being used, or at least, it had no visible effect when I removed it. That's ok though, I'll remove this from my PR and test again once yours merges.

It seems to be used in this one test that doesn't actually run as part of the acceptance tests: https://github.com/hashicorp/terraform-provider-kubernetes/blob/master/kubernetes/provider_test.go#L70

Yeah, I saw that it didn't run during acceptance tests and that nothing was calling it... but I think I understand its purpose now. Thanks for showing me. I was misinterpreting it as a "TestProvider" configure function. But it actually tests the "provider configure" code, which does serve a purpose. So that's worth keeping.

dak1n1 · 2020-11-20T16:42:28Z

kubernetes/provider.go

+	} else if v := os.Getenv("KUBE_CONFIG_PATHS"); v != "" {
+		// NOTE we have to do this here because the schema
+		// does not yet allow you to set a default for a TypeList
+		configPaths = strings.Split(v, ":")


Splitting the string by : will work on Linux and MacOS, but Windows uses a semi-colon as the delimiter for its PATH. Maybe os.PathListSeparator would be better to use here than :?

Good call – I didn't know about os.PathListSeparator. I just had a poke at the kubectl code and it uses filepath.SplitList, which uses os.PathListSeparator underneath so I'm going to update to use that.

Just realized I haven't owned a Windows machine in about 10 years 😅.

dak1n1 · 2020-11-20T18:46:38Z

Would it be possible to add an error message to let users know why the provider can no longer find their KUBECONFIG? When I run the provider using this branch, I get these errors, neither of which tell me I need to set KUBE_CONFIG_PATH:

kubernetes_pod.main: Creating...

Error: Post "http://localhost/api/v1/namespaces/default/pods": dial tcp [::1]:80: connect: connection refused

$ make testacc

=== RUN   TestAccKubernetesDataSourcePod_basic
    provider_test.go:157: File config (KUBE_CTX_AUTH_INFO and KUBE_CTX_CLUSTER) or static configuration(KUBE_HOST, KUBE_USER, KUBE_PASSWORD, KUBE_CLUSTER_CA_CERT_DATA) or (KUBE_HOST, KUBE_CLIENT_CERT_DATA, KUBE_CLIENT_KEY_DATA, KUBE_CLUSTER_CA_CERT_DATA) must be set for acceptance tests
--- FAIL: TestAccKubernetesDataSourcePod_basic (0.00s)

It looks like we might need an additional check for the configuration during acceptance tests:

$ KUBE_CTX_CLUSTER=minikube KUBE_CTX_AUTH_INFO=minikube TESTARGS="-run 'Pod_basic'" make testacc


=== RUN   TestAccKubernetesDataSourcePod_basic
    data_source_kubernetes_pod_test.go:15: Step 1/2 error: Error running apply:
        Error: Post "http://localhost/api/v1/namespaces/default/pods": dial tcp [::1]:80: connect: connection refused


--- FAIL: TestAccKubernetesDataSourcePod_basic (1.22s)
=== RUN   TestAccKubernetesPod_basic
    resource_kubernetes_pod_test.go:28: Step 1/3 error: Error running apply:
        Warning: Interpolation-only expressions are deprecated

          on config052965964/terraform_plugin_test.tf line 64, in resource "kubernetes_pod" "test":
          64:             name     = "${kubernetes_secret.test.metadata.0.name}"

        Terraform 0.11 and earlier required all non-constant expressions to be
        provided via interpolation syntax, but this pattern is now deprecated. To
        silence this warning, remove the "${ sequence from the start and the }"
        sequence from the end of this expression, leaving just the inner expression.

        Template interpolation syntax is still used to construct strings from
        expressions when the template includes multiple interpolation sequences or a
        mixture of literal strings and interpolations. This deprecation applies only
        to templates that consist entirely of a single interpolation sequence.

        (and 4 more similar warnings elsewhere)


        Error: Post "http://localhost/api/v1/namespaces/default/configmaps": dial tcp [::1]:80: connect: connection refused



        Error: Post "http://localhost/api/v1/namespaces/default/secrets": dial tcp [::1]:80: connect: connection refused



        Error: Post "http://localhost/api/v1/namespaces/default/configmaps": dial tcp [::1]:80: connect: connection refused



        Error: Post "http://localhost/api/v1/namespaces/default/secrets": dial tcp [::1]:80: connect: connection refused


--- FAIL: TestAccKubernetesPod_basic (1.29s)
FAIL

website/docs/index.html.markdown

dak1n1 · 2020-11-20T19:08:37Z

kubernetes/provider.go

+				Type:        schema.TypeString,
+				Optional:    true,
+				DefaultFunc: schema.EnvDefaultFunc("KUBE_CONFIG_PATH", ""),
+				Description: "Path to the kube config file. Can be set with KUBE_CONFIG_PATH.",


This is great! Could we also add ConflictsWith = []string{"config_paths"}? And the equivalent for config_path, so that the SDK lets a user know if they've specified both? Otherwise we're selecting one of the two at random, and the user might find it unexpected. In my testing, it will yield this result:

$ terraform validate Error: ConflictsWith "config_paths": conflicts with config_path Error: ConflictsWith "config_path": conflicts with config_paths

However, if specified as environment variables, it doesn't give the user any error message:

$ KUBE_CONFIG_PATH=$KUBECONFIG KUBE_CONFIG_PATHS=$KUBECONFIG terraform init $ KUBE_CONFIG_PATH=$KUBECONFIG KUBE_CONFIG_PATHS=$KUBECONFIG terraform validate Success! The configuration is valid. $ KUBE_CONFIG_PATH=$KUBECONFIG KUBE_CONFIG_PATHS=$KUBECONFIG terraform apply --auto-approve kubernetes_pod.main: Creating... kubernetes_pod.main: Creation complete after 6s [id=default/test-pod] Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

That validation difference between the environment variables and the equivalent in HCL might be an SDK bug.

EDIT: I see, this is the same bug you linked in this PR. It affects setting the default, which affects ConflictsWith.

Co-authored-by: Stef Forrester <[email protected]>

jrhouston · 2020-11-23T20:25:43Z

Would it be possible to add an error message to let users know why the provider can no longer find their KUBECONFIG? When I run the provider using this branch, I get these errors, neither of which tell me I need to set KUBE_CONFIG_PATH:

So this looks to be because of this conditional: https://github.com/hashicorp/terraform-provider-kubernetes/blob/master/kubernetes/provider.go#L244
From looking into the linked issues it seems that this is fixed in 0.14 but will still be an issue for previous versions so we'll need to leave this in for now.

We could explicitly check the attributes supplied to the provider block but the tricky thing here is, to know for sure what's going on, we have to check:

They haven't supplied any credentials attributes, AND
They're not running inside a cluster, which is the default behaviour for the client if you don't supply any credentials.

If we also add in_cluster_config an explicit attribute too, then we could just look at the supplied attributes and then tell them that they need to specify at least one of these things. What do you think?

cc @alexsomesan

kubernetes/provider.go

Check kubeconfig files exist at configure time

dak1n1 · 2020-12-01T20:28:11Z

The error message for this is super nice! I did some testing with it today and it was a smooth user experience. So this is looking really good so far.

I tested a new scenario that might need some work: the ability to detect when one cluster has gone away. Here's the scenario:

I have two clusters. By default, the provider will apply all my kubernetes configurations to the first cluster listed in config_paths. But what happens when that cluster goes away? I imagine the expected behavior is that the provider will use the next cluster in config_paths. However, instead, it fails with this message:

kubernetes_pod.test: Creating...

Error: Post "http://localhost/api/v1/namespaces/default/pods": dial tcp [::1]:80: connect: connection refused

Here's my provider block:

provider "kubernetes" {
  config_paths = [
    "/home/dakini/.kube/second",
    "/home/dakini/.kube/config",
  ]
}

When I deleted the minikube cluster named second, the provider probably should try next config in line. However, this PR is really good as it is, so we could add this functionality once this one merges. I'll leave that decision to you.

alexsomesan

Finally we get rid of load_config_file... Nicely done!

I've had a look around and I found a spot around the InCluster stuff that I think might cause some trouble. Left some comments in there.

kubernetes/provider.go

alexsomesan · 2020-12-02T21:33:35Z

kubernetes/provider.go

+		"token",
+		"exec",
+	}
+	for _, a := range atLeastOneOf {


I'm not sure I understand this correctly, but it looks like having just "token" or "client_certificate" set would count as valid configuration. Not sure if I'm missing something else.

This could be confusing if not covering all combinations that make a valid configuration.

Also (not 100% sure) but it might be falsely triggered by progressive apply situations. Can we rule those out?

The intent of this code is not to cover all valid possible configurations, it is to rule out that:

The user has left the provider block is empty

Terraform is running inside a cluster.

The purpose of this is so we can tell the user to explicitly configure instead of generating the confusing "could not connect" error at apply time when the client actually gets initialized. Once we fix this we should be able to generate more meaningful errors that come from client go, as it will warn you about these problems.

We can use ConflictsWith and RequiredWith to validate that the right combination of attributes is being used together, but I would propose doing that in another pull request.

Sounds good. I didn't catch those nuances while reading through it the first time. Let's try this approach out and see how it improves things. Looks like hashicorp/terraform#24055 is confirmed fixed so we're clear to move in that regard.
I agree with doing things incrementally. Let's re-evaluate after this is released.

alexsomesan · 2020-12-02T21:51:32Z

kubernetes/provider.go

-				}
-				log.Printf("[DEBUG] Using overidden context: %#v", overrides.Context)
+			if _, err := os.Stat(path); err != nil {
+				return nil, fmt.Errorf("could not open kubeconfig %q: %v", p, err)


Would this fail to configure the provider if a single file is missing?
IMO, we should consider all entries in the list in a best effort to find valid credentials.

Also, in case of multiple files present it could be argued that a valid CurrentContext should be present to eliminate ambiguities about which cluster would be chosen to connect to. Otherwise we might end up applying changes to an undesirable destination.

The behaviour of kubectl when using a KUBECONFIG with multiple paths is not to error if one of the paths does not exist. That being said I think if we're erring on the side of explicitness by not supporting KUBECONFIG anymore (and allowing this to be set as a list in the provider block) then we should also be more explicit here and tell the user that one of the paths they specified does not exist rather than silently skipping it.

Sounds good then. Let's go with this!
We might need to keep an eye out for corner cases where this list would be set but not all entries might be actually present in the file system (yet). It's a stretch and quite unlikely, but we've seen funny things before.

What do you think about requiring a CurrentContext to be set when this list has more than one entry? Just as a thought experiment for now. Doesn't need to be part of this PR.

jrhouston · 2020-12-18T04:19:08Z

When I deleted the minikube cluster named second, the provider probably should try next config in line. However, this PR is really good as it is, so we could add this functionality once this one merges. I'll leave that decision to you.

@dak1n1 Did you confirm this is what's supposed to happen when you use kubectl? We just pass along the list of paths to client-go so I'm not sure how we would infer this. In any case I agree we should solve for that in another PR.

alexsomesan

All good on my side. This looks like it's going to improve quality of life for everyone.
Excited to try it out!

alexsomesan · 2020-12-19T01:12:23Z

kubernetes/provider.go

+		"token",
+		"exec",
+	}
+	for _, a := range atLeastOneOf {


Sounds good. I didn't catch those nuances while reading through it the first time. Let's try this approach out and see how it improves things. Looks like hashicorp/terraform#24055 is confirmed fixed so we're clear to move in that regard.
I agree with doing things incrementally. Let's re-evaluate after this is released.

alexsomesan · 2020-12-19T01:15:29Z

kubernetes/provider.go

-				}
-				log.Printf("[DEBUG] Using overidden context: %#v", overrides.Context)
+			if _, err := os.Stat(path); err != nil {
+				return nil, fmt.Errorf("could not open kubeconfig %q: %v", p, err)


Sounds good then. Let's go with this!
We might need to keep an eye out for corner cases where this list would be set but not all entries might be actually present in the file system (yet). It's a stretch and quite unlikely, but we've seen funny things before.

What do you think about requiring a CurrentContext to be set when this list has more than one entry? Just as a thought experiment for now. Doesn't need to be part of this PR.

ghost · 2021-01-21T05:10:18Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

ghost added the size/XXL label Nov 2, 2020

Remove load_config_file and support for KUBECONFIG environment variable

3be7d12

jrhouston force-pushed the v2-credentials-changes branch from 153f7d0 to 3be7d12 Compare November 13, 2020 20:43

ghost added size/M documentation and removed size/XXL labels Nov 13, 2020

jrhouston force-pushed the v2-credentials-changes branch from bc45567 to 9602d64 Compare November 13, 2020 21:56

ghost added size/L and removed size/M labels Nov 13, 2020

jrhouston commented Nov 13, 2020

View reviewed changes

kubernetes/provider.go Show resolved Hide resolved

jrhouston marked this pull request as ready for review November 13, 2020 22:02

jrhouston requested a review from a team November 13, 2020 22:02

Change config_path -> config_paths

5a16a65

jrhouston force-pushed the v2-credentials-changes branch 2 times, most recently from 940af6a to 7cc1c12 Compare November 20, 2020 15:12

jrhouston requested review from a team and removed request for a team November 20, 2020 15:22

jrhouston force-pushed the v2-credentials-changes branch from 7cc1c12 to af0af77 Compare November 20, 2020 15:32

jrhouston commented Nov 20, 2020

View reviewed changes

jrhouston force-pushed the v2-credentials-changes branch from af0af77 to 58883a6 Compare November 20, 2020 15:40

Support config_path and config_paths

f82e5f2

jrhouston force-pushed the v2-credentials-changes branch from 58883a6 to f82e5f2 Compare November 20, 2020 16:36

dak1n1 reviewed Nov 20, 2020

View reviewed changes

Use system path separator

70c644e

dak1n1 reviewed Nov 20, 2020

View reviewed changes

website/docs/index.html.markdown Outdated Show resolved Hide resolved

dak1n1 reviewed Nov 20, 2020

View reviewed changes

Update website/docs/index.html.markdown

d7516bc

Co-authored-by: Stef Forrester <[email protected]>

jrhouston added 2 commits November 23, 2020 15:29

Add ConflictsWith for config_path

04fdfac

Generate an error when the user does not supply any configuration

b0bfaf8

ghost added size/XL and removed size/L labels Nov 25, 2020

jrhouston commented Nov 25, 2020

View reviewed changes

kubernetes/provider.go Show resolved Hide resolved

Add better error message when provider is not configured

975f492

Check kubeconfig files exist at configure time

jrhouston requested a review from dak1n1 November 30, 2020 21:38

jrhouston mentioned this pull request Dec 1, 2020

Add config_paths to kubernetes block in provider config hashicorp/terraform-provider-helm#636

Merged

dak1n1 approved these changes Dec 1, 2020

View reviewed changes

alexsomesan requested changes Dec 2, 2020

View reviewed changes

alexsomesan mentioned this pull request Dec 4, 2020

Defer to lazy client initialization to prevent dialing localhost #1078

Open

2 tasks

Add check for api token path

75377d4

jrhouston requested a review from a team December 18, 2020 04:23

alexsomesan approved these changes Dec 19, 2020

View reviewed changes

jrhouston added 2 commits December 21, 2020 12:20

Defer checking valid configuration to avoid progressive apply issue

b9bcdd2

Fix crash in kubernetes_job waiter

f16553e

jrhouston merged commit a763455 into master Dec 21, 2020

jrhouston deleted the v2-credentials-changes branch December 21, 2020 17:48

ghost locked as resolved and limited conversation to collaborators Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Credentials changes for v2.0.0 #1052

Credentials changes for v2.0.0 #1052

jrhouston commented Nov 2, 2020 •

edited

Loading

jrhouston Nov 13, 2020

jrhouston Nov 20, 2020

dak1n1 Nov 20, 2020

jrhouston Nov 20, 2020

dak1n1 Nov 20, 2020

dak1n1 Nov 20, 2020

jrhouston Nov 20, 2020

dak1n1 commented Nov 20, 2020

dak1n1 Nov 20, 2020 •

edited

Loading

jrhouston commented Nov 23, 2020

dak1n1 commented Dec 1, 2020

alexsomesan left a comment

alexsomesan Dec 2, 2020

jrhouston Dec 18, 2020 •

edited

Loading

alexsomesan Dec 19, 2020 •

edited

Loading

alexsomesan Dec 2, 2020

jrhouston Dec 18, 2020

alexsomesan Dec 19, 2020

jrhouston commented Dec 18, 2020

alexsomesan left a comment

alexsomesan Dec 19, 2020 •

edited

Loading

alexsomesan Dec 19, 2020

ghost commented Jan 21, 2021

Credentials changes for v2.0.0 #1052

Credentials changes for v2.0.0 #1052

Conversation

jrhouston commented Nov 2, 2020 • edited Loading

Description

Acceptance tests

Release Note

References

Community Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dak1n1 commented Nov 20, 2020

dak1n1 Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

jrhouston commented Nov 23, 2020

dak1n1 commented Dec 1, 2020

alexsomesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhouston Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

alexsomesan Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhouston commented Dec 18, 2020

alexsomesan left a comment

Choose a reason for hiding this comment

alexsomesan Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 21, 2021

jrhouston commented Nov 2, 2020 •

edited

Loading

dak1n1 Nov 20, 2020 •

edited

Loading

jrhouston Dec 18, 2020 •

edited

Loading

alexsomesan Dec 19, 2020 •

edited

Loading

alexsomesan Dec 19, 2020 •

edited

Loading