Add `ConflictsWith` to provider authentication config #1141

dak1n1 · 2021-02-03T00:49:23Z

Description

In the past, we relied on Kubernetes client-go to determine the order of precedence when given a list of mutually-exclusive parameters for authenticating to the Kubernetes cluster. This lead to some confusion, since client-go's decision could override even an explicit provider configuration. For example, if a token were specified in the Kubernetes provider config, and the environment variable KUBE_CONFIG_PATH was present, client-go would silently decide to use the KUBE_CONFIG_PATH to authenticate to the cluster.

In order to create a more explicit configuration, and more predictable behavior, this PR adds ConflictsWith to specific provider authentication attributes, effectively disallowing the use of conflicting methods of authentication, such as config_path and token. The provider now generates an error to tell the user that these options are mutually exclusive.

Acceptance tests

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

Output from acceptance testing:

$ TESTARGS="-run 'TestAccKubernetesProviderConfig'" make testacc
=== RUN   TestAccKubernetesProviderConfig_config_path
--- PASS: TestAccKubernetesProviderConfig_config_path (3.68s)
=== RUN   TestAccKubernetesProviderConfig_config_paths
--- PASS: TestAccKubernetesProviderConfig_config_paths (2.90s)
=== RUN   TestAccKubernetesProviderConfig_config_paths_env
--- PASS: TestAccKubernetesProviderConfig_config_paths_env (2.33s)
=== RUN   TestAccKubernetesProviderConfig_config_paths_env_wantError
--- PASS: TestAccKubernetesProviderConfig_config_paths_env_wantError (0.43s)
=== RUN   TestAccKubernetesProviderConfig_host_env_wantError
--- PASS: TestAccKubernetesProviderConfig_host_env_wantError (0.55s)
=== RUN   TestAccKubernetesProviderConfig_host
--- PASS: TestAccKubernetesProviderConfig_host (8.68s)
PASS

Release Note

Release note for CHANGELOG:

Display an error when the provider is configured with mutually-exclusive authentication options.

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

dak1n1 · 2021-02-19T23:50:15Z

_examples/eks/kubernetes-config/main.tf

-    command     = "aws"
-  }
-}
-


I moved the provider blocks into the root module, since that's best practice.

dak1n1 · 2021-02-19T23:57:06Z

_examples/gke/kubernetes-config/main.tf

-  token = data.google_client_config.default.access_token
-  cluster_ca_certificate = base64decode(var.cluster_ca_cert)
-}
-


I moved this provider config into the root module.

dak1n1 · 2021-02-19T23:58:24Z

_examples/gke/gke-cluster/main.tf

-  # This can be set statically, if preferred. See docs for details.
-  # https://registry.terraform.io/providers/hashicorp/google/latest/docs/guides/provider_reference#full-reference
-}
-


This provider config was moved into the root module.

dak1n1 · 2021-02-20T00:05:59Z

kubernetes/provider.go

+	if err := checkConfigurationValid(k.configData); err != nil {
+		return nil, err
+	}
+


I did a bunch of Progressive Apply testing and I wasn't able to find a difference between having this check enabled vs disabled.

Did you do your testing across different versions of Terraform and can you share your testing strategy? I'm still pretty uncertain about reliably reproducing the illusive progressive apply issue.

You can look at the issue that caused us to revert this change in the Helm provider for context: hashicorp/terraform-provider-helm#647

dak1n1 · 2021-02-20T00:10:40Z

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 = filepath.SplitList(v)


The SDK now seems to allow us to set a default for TypeList! 🎉 I didn't see the feature added, but it worked for me when I defined my own function of type schema.EnvDefaultFunc.

dak1n1 · 2021-02-20T00:12:38Z

kubernetes/provider.go

@@ -334,16 +434,16 @@ func initializeConfiguration(d *schema.ResourceData) (*restclient.Config, error)
 	}

 	// Overriding with static configuration
-	if v, ok := d.GetOk("insecure"); ok {
+	if v, ok := d.GetOk("insecure"); ok && v != "" {


These checks are just meant to ensure the various configuration attributes are not initialized with any default/empty values. For example, if the provider thinks we've set host = "", it won't let us set config_path = "~/.kube/config" because of the ConflictsWith.

jrhouston

Thanks for doing this @dak1n1 – this mostly looks great, especially all the tests you've added and config examples you've tidied up.

I added a few questions about the conflicts with stuff, and there seems to be a problem with sourcing the KUBE_CONFIG_PATHS environment variable.

jrhouston · 2021-03-17T17:07:56Z

kubernetes/provider.go

 				Optional:    true,
 				Description: "A list of paths to kube config files. Can be set with KUBE_CONFIG_PATHS environment variable.",
+				// config_paths conflicts with every attribute except for "insecure", since all of these options will be read from the kubeconfig.


It seems like the insecure option can actually be part of the kubeconfig file but is called insecure-skip-tls-verify https://github.com/kubernetes/kubernetes/blob/f4a156eb29d51b73f52d69ab4c5f96e440eebc41/staging/src/k8s.io/client-go/pkg/apis/clientauthentication/v1beta1/types.go#L86

Is there a good reason to also have it here when using config_path in that case?

Nope, my mistake! I thought it was a command-line override that works anywhere. I'll have to fix that. But I'm going to hold off doing anything with this PR until we have that meeting next week.

Just to give you a status update: I'll fix the things you mentioned as soon as I can get the needed functionality into the SDK. (Since we don't want to enable ConflictsWith in "fatal error" mode, the SDK work will be needed before this PR can progress).

jrhouston · 2021-03-17T17:11:22Z

kubernetes/provider.go

+				Optional:      true,
+				DefaultFunc:   schema.EnvDefaultFunc("KUBE_CTX", nil),
+				Description:   "Context to choose from the kube config file. ",
+				ConflictsWith: []string{"exec", "token", "client_certificate", "client_key", "username", "password"},


Wouldn't this also conflict with cluster_ca_certificate since it's just for when using a kubeconfig file?

jrhouston · 2021-03-17T17:17:51Z

kubernetes/provider.go

+// configPathsEnv fetches the value of the environment variable KUBE_CONFIG_PATHS, if defined.
+func configPathsEnv() (interface{}, error) {
+	value, exists := os.LookupEnv("KUBE_CONFIG_PATHS")
+	if exists {


Just a nit but because there's a bunch of stuff inside this conditional i would invert it so that if !exists returns early so the rest of the logic is just the body of the function.

jrhouston · 2021-03-17T17:35:19Z

kubernetes/provider.go

 			},
 			"config_paths": {
 				Type:        schema.TypeList,
 				Elem:        &schema.Schema{Type: schema.TypeString},
+				DefaultFunc: configPathsEnv,


This doesn't actually seem to be working for me on your branch:

When I use KUBE_CONFIG_PATH it works fine:

KUBE_CONFIG_PATH=~/.kube/config terraform plan ✘ 1 conflictswith ✱ ◼ kubernetes_config_map.testy: Refreshing state... [id=default/testo] No changes. Infrastructure is up-to-date. That Terraform did not detect any differences between your configuration and the remote system(s). As a result, there are no actions to take.

But when I try to use KUBE_CONFIG_PATHS:

KUBE_CONFIG_PATHS=~/.kube/config terraform plan ✘ 1 conflictswith ✱ ◼ kubernetes_config_map.testy: Refreshing state... [id=default/testo] ╷ │ Error: Get "http://localhost/api/v1/namespaces/default/configmaps/testo": dial tcp [::1]:80: connect: connection refused │ │ ╵

However if I set config_paths in the config it works fine. So I suspect something isn't right with the default function here.

Nice, thanks for testing! I'll have to check this out.

jrhouston · 2021-03-17T17:40:41Z

kubernetes/provider.go

+	if err := checkConfigurationValid(k.configData); err != nil {
+		return nil, err
+	}
+


Did you do your testing across different versions of Terraform and can you share your testing strategy? I'm still pretty uncertain about reliably reproducing the illusive progressive apply issue.

You can look at the issue that caused us to revert this change in the Helm provider for context: hashicorp/terraform-provider-helm#647

jrhouston · 2021-03-17T17:49:40Z

kubernetes/provider_test.go

+	})
+}
+
+func TestAccKubernetesProviderConfig_config_paths_env(t *testing.T) {


Given that I couldn't get KUBE_CONFIG_PATHS to work locally as mentioned above, I'm confused by how this test passes 🤯

Ah, I see this probably passes because the validation on the provider block doesn't error but terraform breaks gives an error at apply time because the list is empty.

I added some logging to the provider block where I can see that configPathsEnv is being called, but when I do a d.Get("config_paths") in the providerConfigure function it comes up empty. 😢

jrhouston · 2021-03-17T17:59:18Z

kubernetes/provider_test.go

+			},
+			{
+				Config: testAccKubernetesProviderConfig(
+					providerConfig_host("https://test-host") +


I found the nesting of functions here a bit confusing to read – I think I would have preferred just one big string with the provider block in it so I can see exactly what is being tested. I see you are trying not to repeat yourself here and avoid copypasta though.

dak1n1 · 2021-04-21T21:39:31Z

There's a PR that I tested out today, which gives us the conflictsWith warning level messages. It's working, but it will take some time for it to be reviewed and released. Probably sometime in June. hashicorp/terraform-plugin-sdk#745

In order to create a more explicit configuration, and more predictable behavior, this PR adds ConflictsWith to specific provider authentication attributes, effectively disallowing the use of conflicting methods of authentication, such as config_path and token. The provider now generates an error to tell the user that these options are mutually exclusive.

hashicorp-cla · 2022-03-12T17:19:38Z

All committers have signed the CLA.

ghost added size/L size/XL and removed size/L labels Feb 3, 2021

This was referenced Feb 6, 2021

Add updated examples for GKE and EKS #1115

Merged

v2.0.1 Authentication failures with token retrieved via aws_eks_cluster_auth #1131

Closed

Alternative to load_config_file = false? #1162

Closed

dak1n1 force-pushed the conflictswith branch from 242f7b1 to 9b2e03a Compare February 16, 2021 23:05

dak1n1 mentioned this pull request Feb 17, 2021

AtLeastOneOf doesn't work for Optional attributes (references self) hashicorp/terraform-plugin-sdk#705

Closed

dak1n1 force-pushed the conflictswith branch from 6dca5d8 to c30a64f Compare February 19, 2021 23:44

dak1n1 marked this pull request as ready for review February 19, 2021 23:44

dak1n1 requested a review from jrhouston February 19, 2021 23:48

dak1n1 commented Feb 19, 2021

View reviewed changes

dak1n1 force-pushed the conflictswith branch from c30a64f to e1ad2e2 Compare February 19, 2021 23:54

dak1n1 commented Feb 19, 2021

View reviewed changes

dak1n1 force-pushed the conflictswith branch 2 times, most recently from 002dc63 to 72a16df Compare February 20, 2021 00:00

dak1n1 commented Feb 20, 2021

View reviewed changes

dak1n1 linked an issue Feb 25, 2021 that may be closed by this pull request

Provider allows mutually-exclusive configuration options #1179

Open

dak1n1 force-pushed the conflictswith branch from d5b9d54 to c706a16 Compare February 26, 2021 19:53

ghost added size/XXL and removed size/XL labels Feb 26, 2021

dak1n1 mentioned this pull request Feb 26, 2021

Getting x509-certificate-signed-by-unknown-authority #1154

Closed

dak1n1 mentioned this pull request Mar 12, 2021

Replace EKS test-infra with example #1192

Merged

2 tasks

dak1n1 force-pushed the conflictswith branch from c706a16 to c53b836 Compare March 14, 2021 17:07

jrhouston requested changes Mar 17, 2021

View reviewed changes

dak1n1 mentioned this pull request Mar 18, 2021

ConflictsWith deprecation warning hashicorp/terraform-plugin-sdk#727

Open

Base automatically changed from master to main March 23, 2021 15:53

dak1n1 mentioned this pull request Mar 30, 2021

Error during EKS Creation: Post "http://localhost/api/v1/namespaces/kube-system/configmaps": dial tcp 127.0.0.1:80: connect: connection refused terraform-aws-modules/terraform-aws-eks#1280

Closed

dak1n1 force-pushed the conflictswith branch from 2f0e503 to d8c5d0b Compare April 2, 2021 22:08

ghost added size/XL and removed size/XXL labels Apr 2, 2021

dak1n1 mentioned this pull request Apr 3, 2021

Terraform.io to EKS "Error: Kubernetes cluster unreachable" hashicorp/terraform-provider-helm#400

Closed

dak1n1 added 6 commits April 21, 2021 14:50

Update examples used for testing ConflictsWith

ff0db58

Fix function name

7651347

Remove lockfile

ce198a4

run test lint fix

211009c

Add conflictswith warning level message

415a860

dak1n1 force-pushed the conflictswith branch from d8c5d0b to 415a860 Compare April 21, 2021 21:54

Use new type for ConditionsMode

0ef58a6

dak1n1 mentioned this pull request Jun 3, 2021

Provider doesn't properly setup when only given config_context value without config_path #1274

Closed

dak1n1 mentioned this pull request Jul 2, 2021

AKS - dial tcp [::1]:80: connect: connection refused on all plans modifying the azurerm_kubernetes_cluster resource #1307

Closed

Merge branch 'main' into conflictswith

ce5d98b

BBBmau requested a review from a team as a code owner April 11, 2023 16:01

BBBmau mentioned this pull request May 3, 2023

add conflictsWith to provider schema #2084

Merged

2 tasks

jrhouston closed this Jun 6, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ConflictsWith` to provider authentication config #1141

Add `ConflictsWith` to provider authentication config #1141

dak1n1 commented Feb 3, 2021 •

edited

Loading

dak1n1 Feb 19, 2021

dak1n1 Feb 19, 2021

dak1n1 Feb 19, 2021

dak1n1 Feb 20, 2021

jrhouston Mar 17, 2021

dak1n1 Feb 20, 2021

dak1n1 Feb 20, 2021

jrhouston left a comment

jrhouston Mar 17, 2021

dak1n1 Mar 17, 2021

dak1n1 Apr 2, 2021

jrhouston Mar 17, 2021

jrhouston Mar 17, 2021

jrhouston Mar 17, 2021

dak1n1 Mar 17, 2021

jrhouston Mar 17, 2021

jrhouston Mar 17, 2021

jrhouston Mar 17, 2021

jrhouston Mar 17, 2021

dak1n1 commented Apr 21, 2021

hashicorp-cla commented Mar 12, 2022 •

edited

Loading

Add ConflictsWith to provider authentication config #1141

Add ConflictsWith to provider authentication config #1141

Conversation

dak1n1 commented Feb 3, 2021 • 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

jrhouston left a 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

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 Apr 21, 2021

hashicorp-cla commented Mar 12, 2022 • edited Loading

Add `ConflictsWith` to provider authentication config #1141

Add `ConflictsWith` to provider authentication config #1141

dak1n1 commented Feb 3, 2021 •

edited

Loading

hashicorp-cla commented Mar 12, 2022 •

edited

Loading