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

[FEATURE] Make saving kubeconfig to state optional when creating a civo_kubernetes resource #296

Closed
fernando-villalba opened this issue Aug 1, 2024 · 8 comments · Fixed by #309

Comments

@fernando-villalba
Copy link
Contributor

fernando-villalba commented Aug 1, 2024

Description

When creating a civo_kubernetes resource the kubeconfig gets saved to state, this includes secret tokens that grant complete access to the cluster. This is potentially not very secure.

I can see many scenarios (perhaps most?) where a user may not want that at all. Users may want to just create a cluster and then fetch the configuration with the Civo CLI or Dashboard, etc.

Acceptance Criteria

  • Create a new boolean configuration input called write_kubeconfig or something like that. When true write kubeconfig to state, when false don't write it.
  • Ideally it would be good to default this to false, but it may cause backward compatibility issues. I say we default it to false, but output a message to the user saying that if they want to keep it to add the write_kubeconfig flag and set it to true. For example:

"Starting on version x kubeconfig is not written to state by default, if you wish to keep the kubeconfig configuration in state, please add the input write_kubeconfig and set it to true"

@Praveen005
Copy link
Contributor

Hi @uzaxirr

Hope you are doing fine.

I worked on this issue and would like to explain my approach for your review.

For this, I added a write_kubeconfig field like below:

Screenshot 2024-08-02 021220

and since the update in state of a resource after creation/modification happens in resourceKubernetesClusterRead, I added a check which assigns kubeconfig an empty string if write_kubeconfig is set to false.

Screenshot 2024-08-02 021429

And as evident below, It correctly hides the kubeconfig value.

Screenshot 2024-08-02 015812

Post these changes, resource creation/update/deletion goes through as usual. I tested it.

For the warning message I have a question, Should I add a validation function to check if the provider's version is <= v1.0.48, if it is, show users a warning, else don't ?

Regards!

@fernando-villalba
Copy link
Contributor Author

Great work as usual @Praveen005 to your last question, I say yes! That would be great. The rest looks pretty good to me. Make sure that changing write_kubeconfig is possible even after creation of the resource which looks like you have already.

I will let @uzaxirr weigh in on all the rest, see his thoughts.

@Praveen005
Copy link
Contributor

Thank you for the feedback @fernando-villalba. I'll proceed with raising the PR then.

@Praveen005
Copy link
Contributor

Praveen005 commented Aug 3, 2024

Hi @fernando-villalba,

I added the validation function for write_kubeconfig attribute and It correctly shows the warning message as evident below.

Screenshot 2024-08-03 153555

But I have a question, How will user know that the default behaviour has changed? This warning will flash only if write_kubeconfig attribute is specified in main.tf.

Initially, till the next version is released, should we flash this warning regardless of write_kubeconfig specified or not. If yes, should I return this warning from resourceKubernetesClusterCreate or maybe from resourceKubernetesClusterRead function?


As for updating the write_kubeconfig after creation, we need to add a writeKubeconfig field in KubernetesClusterConfig in civogo sdk:

https://github.com/civo/civogo/blob/22e0a732c195459152546dfd52ab854867b65f39/kubernetes.go#L133

@Praveen005
Copy link
Contributor

Hi @uzaxirr,

When you have a moment, could you please review this? Your input is needed to proceed.

Thank you!

@uzaxirr
Copy link
Member

uzaxirr commented Aug 6, 2024

hii @Praveen005

  1. No i dont think we need to flash the warning every now and then, we could just flash it only when required. But will update the docs
  2. Why do we need to add the write_kubeconfig in SDK i see no reason for that, it can be handled from terraform itself?

@Praveen005
Copy link
Contributor

Praveen005 commented Aug 6, 2024

Thank you for reviewing it @uzaxirr

For the sdk change part, actually I was getting the following error, every time I updated the configuration.

Screenshot 2024-08-06 193933

Debug log revealed that, we are passing an empty config. to UpdateKubernetesCluster() inside resourceKubernetesClusterUpdate(), which caused this error.

Had there been a field like writeKubeconfig, this possibly wouldn't have happened I believe. But this doesn't necessitate adding a field to KubernetesClusterConfig in the sdk, as you rightly pointed out.

Screenshot 2024-08-06 195619

As a workaround to ensure we don't send an empty config, I did the following:

Screenshot 2024-08-07 004019

I am not sure if this is the best way, but I couldn't think of another one. And this fixed the issue.


Furthermore in the ValidateWriteKubeconfig function, I extracted the provider version the user is using and compared it with the threshold version (the current 1.0.48), the warning is shown if the provider's version is <= 1.0.48

func ValidateWriteKubeconfig(v interface{}, path cty.Path) diag.Diagnostics {
	var versionInfo versionInfo
	diags := diag.Diagnostics{}

	cmd := exec.Command("terraform", "version", "-json")
	output, err := cmd.Output()
	if err != nil {
		log.Printf("[ERROR] error running terraform show: %v\n", err)
		return diags
	}

	err = json.Unmarshal(output, &versionInfo)
	if err != nil {
		log.Printf("[ERROR] error parsing JSON: %v\n", err)
		return diags
	}
	versionField := "registry.terraform.io/civo/civo"
	currentProviderVersion := versionInfo.ProviderSelections[versionField]
	thresholdProviderVersion := "1.0.48"

	v1, err := version.NewSemver(currentProviderVersion)
	if err != nil {
		log.Println("[ERROR] error parsing the given version")
		return diags
	}
	v2, err := version.NewVersion(thresholdProviderVersion)
	if err != nil {
		log.Println("[ERROR] error parsing the given version")
		return diags
	}

	if v1.LessThanOrEqual(v2) {
		diags = append(diags, diag.Diagnostic{
			Severity: diag.Warning,
			Summary:  "Default kubeconfig behavior changed",
			Detail:   "Starting from version 1.0.49, kubeconfig will no longer be written to the Terraform state by default for the civo_kubernetes resource. This change is made to enhance security by preventing sensitive information from being stored in state files. If you want to retain kubeconfig in your state file, please update your configuration by adding the `write_kubeconfig` parameter and setting it to `true`. Example configuration: `write_kubeconfig = true`.",
		})
	}
	return diags
}

I have a question, is the error handling done right here? If other errors like parsing error occurs, I am giving user the benefit of doubt and not showing the warning. Is it the right approach to take?

Screenshot 2024-08-06 232945

This is how the warning looks:

Screenshot 2024-08-06 233136

Post changes, the creation/update goes through as intended:

Screenshot 2024-08-07 001911

Kubeconfig hidden:

Screenshot 2024-08-07 001305

Updating resource:

Screenshot 2024-08-07 001856 Screenshot 2024-08-07 001824

Kubeconfig being shown in state:

Screenshot 2024-08-07 001730

Updating again:

Screenshot 2024-08-07 001926 Screenshot 2024-08-07 002032

fernando-villalba pushed a commit that referenced this issue Aug 7, 2024
@uzaxirr
Copy link
Member

uzaxirr commented Aug 7, 2024

@Praveen005 please go ahead and raise a PR

fernando-villalba pushed a commit that referenced this issue Aug 13, 2024
uzaxirr pushed a commit that referenced this issue Aug 13, 2024
* documentation for issue #296

Co-authored-by: Fernando Villalba <fernando@civo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants