civo / terraform-provider-civo

Terraform Civo provider
https://www.civo.com
Mozilla Public License 2.0
71 stars 56 forks source link

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

Closed fernando-villalba closed 1 month ago

fernando-villalba commented 2 months ago

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

"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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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

Praveen005 commented 1 month ago

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 commented 1 month ago

Hi @uzaxirr,

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

Thank you!

uzaxirr commented 1 month ago

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 commented 1 month ago

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
uzaxirr commented 1 month ago

@Praveen005 please go ahead and raise a PR