From 54f85881487074ca78cc766632fbc96f4f6e8728 Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Mon, 9 Dec 2024 16:17:22 +0100 Subject: [PATCH 1/4] fix: jaas integration test --- .github/workflows/test_integration_jaas.yaml | 18 ++++++++++++------ .../provider/resource_kubernetes_cloud_test.go | 6 ++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test_integration_jaas.yaml b/.github/workflows/test_integration_jaas.yaml index 876ce036..6473a4cb 100644 --- a/.github/workflows/test_integration_jaas.yaml +++ b/.github/workflows/test_integration_jaas.yaml @@ -28,7 +28,7 @@ jobs: # Ensure project builds before running test build: name: Build-JAAS - runs-on: ubuntu-latest + runs-on: [self-hosted, jammy, x64] timeout-minutes: 5 steps: - uses: actions/checkout@v4 @@ -41,7 +41,7 @@ jobs: test: name: Integration-JAAS needs: build - runs-on: ubuntu-latest + runs-on: [self-hosted, jammy, x64] strategy: fail-fast: false timeout-minutes: 60 @@ -55,6 +55,10 @@ jobs: with: terraform_version: "1.9.*" terraform_wrapper: false + - name: Install docker compose plugin + run: | + for pkg in docker.io docker-doc docker-compose docker-compose-v2 podman-docker containerd runc; do sudo apt-get remove -y $pkg; done + sudo snap install docker --channel latest/stable # Starting JAAS will start the JIMM controller and dependencies and create a Juju controller on LXD and connect it to JIMM. - name: Setup JAAS uses: canonical/jimm/.github/actions/test-server@v3 @@ -68,11 +72,13 @@ jobs: sudo snap install microk8s --channel=1.28-strict/stable sudo usermod -a -G snap_microk8s $USER sudo chown -R $USER ~/.kube - sudo microk8s.enable dns storage - sudo microk8s.enable dns local-storage + sudo microk8s.enable dns + sudo microk8s.enable storage + sudo microk8s.enable hostpath-storage sudo -g snap_microk8s -E microk8s status --wait-ready --timeout=600 + sudo microk8s.config view | tee /home/$USER/microk8s-config.yaml echo "MICROK8S_CONFIG<> $GITHUB_ENV - sudo microk8s.config view >> $GITHUB_ENV + echo "$(cat /home/${USER}/microk8s-config.yaml)" >> $GITHUB_ENV echo "EOF" >> $GITHUB_ENV - name: Create additional networks when testing with LXD run: | @@ -97,5 +103,5 @@ jobs: - env: TF_ACC: "1" TEST_CLOUD: "lxd" - run: go test -parallel 10 -timeout 40m -v -cover ./internal/provider/ + run: go test -parallel 1 -timeout 40m -v -cover ./internal/provider/ timeout-minutes: 40 diff --git a/internal/provider/resource_kubernetes_cloud_test.go b/internal/provider/resource_kubernetes_cloud_test.go index aa88115a..5a6aafc1 100644 --- a/internal/provider/resource_kubernetes_cloud_test.go +++ b/internal/provider/resource_kubernetes_cloud_test.go @@ -14,6 +14,12 @@ import ( ) func TestAcc_ResourceKubernetesCloud(t *testing.T) { + // Note (alesstimec): Skipping this test, because the default + // hosted cloud tf provider adds is "other", which cannot + // be parsed by JIMM - it needs a valid cloud/region to determine + // which controller to add the cloud to. + SkipJAAS(t) + // TODO: This test is not adding model as a resource, which is required. // The reason in the race that we (potentially) have in the Juju side. // Once the problem is fixed (https://bugs.launchpad.net/juju/+bug/2084448), From ee0178ac61dcee34a3726334d1fa40725c0f1bf1 Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Mon, 16 Dec 2024 08:15:51 +0100 Subject: [PATCH 2/4] fix: kubernetes cloud resource --- .github/workflows/test_integration.yml | 6 +- .github/workflows/test_integration_jaas.yaml | 7 ++- docs/resources/kubernetes_cloud.md | 4 +- .../provider/resource_kubernetes_cloud.go | 63 +++++++++++++++++-- .../resource_kubernetes_cloud_test.go | 59 ++++++++++++++--- 5 files changed, 117 insertions(+), 22 deletions(-) diff --git a/.github/workflows/test_integration.yml b/.github/workflows/test_integration.yml index f75fca2a..4ce78a55 100644 --- a/.github/workflows/test_integration.yml +++ b/.github/workflows/test_integration.yml @@ -103,7 +103,7 @@ jobs: TF_ACC: "1" TEST_CLOUD: ${{ matrix.action-operator.cloud }} run: go test -parallel 1 -timeout 60m -v -cover ./internal/provider/ - timeout-minutes: 40 + timeout-minutes: 60 # Run acceptance tests in a matrix with Terraform CLI versions add-machine-test: @@ -189,5 +189,5 @@ jobs: echo "Running the test" cd ./internal/provider/ - go test ./... -timeout 30m -v -test.run TestAcc_ResourceMachine_AddMachine - timeout-minutes: 40 + go test ./... -timeout 60m -v -test.run TestAcc_ResourceMachine_AddMachine + timeout-minutes: 60 diff --git a/.github/workflows/test_integration_jaas.yaml b/.github/workflows/test_integration_jaas.yaml index 6473a4cb..f22d746f 100644 --- a/.github/workflows/test_integration_jaas.yaml +++ b/.github/workflows/test_integration_jaas.yaml @@ -28,7 +28,7 @@ jobs: # Ensure project builds before running test build: name: Build-JAAS - runs-on: [self-hosted, jammy, x64] + runs-on: ubuntu-latest timeout-minutes: 5 steps: - uses: actions/checkout@v4 @@ -103,5 +103,6 @@ jobs: - env: TF_ACC: "1" TEST_CLOUD: "lxd" - run: go test -parallel 1 -timeout 40m -v -cover ./internal/provider/ - timeout-minutes: 40 + run: go test -parallel 1 -timeout 60m -v -cover ./internal/provider/ + timeout-minutes: 60 + diff --git a/docs/resources/kubernetes_cloud.md b/docs/resources/kubernetes_cloud.md index 1ec088c6..4612851f 100644 --- a/docs/resources/kubernetes_cloud.md +++ b/docs/resources/kubernetes_cloud.md @@ -37,8 +37,8 @@ resource "juju_model" "my-model" { ### Optional - `kubernetes_config` (String, Sensitive) The kubernetes config file path for the cloud. Cloud credentials will be added to the Juju controller for you. -- `parent_cloud_name` (String) The parent cloud name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. -- `parent_cloud_region` (String) The parent cloud region name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. +- `parent_cloud_name` (String) The parent cloud name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller. +- `parent_cloud_region` (String) The parent cloud region name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller. ### Read-Only diff --git a/internal/provider/resource_kubernetes_cloud.go b/internal/provider/resource_kubernetes_cloud.go index d7a5858a..1f49cb32 100644 --- a/internal/provider/resource_kubernetes_cloud.go +++ b/internal/provider/resource_kubernetes_cloud.go @@ -20,6 +20,7 @@ import ( // Ensure provider defined types fully satisfy framework interfaces. var _ resource.Resource = &kubernetesCloudResource{} var _ resource.ResourceWithConfigure = &kubernetesCloudResource{} +var _ resource.ResourceWithConfigValidators = &kubernetesCloudResource{} func NewKubernetesCloudResource() resource.Resource { return &kubernetesCloudResource{} @@ -62,6 +63,13 @@ func (r *kubernetesCloudResource) Configure(ctx context.Context, req resource.Co r.subCtx = tflog.NewSubsystem(ctx, LogResourceKubernetesCloud) } +// ConfigValidators returns a list of functions which will all be performed during validation. +func (r *kubernetesCloudResource) ConfigValidators(context.Context) []resource.ConfigValidator { + return []resource.ConfigValidator{ + &kuberenetesCloudJAASValidator{r.client}, + } +} + // Metadata returns the metadata for the kubernetes cloud resource. func (r *kubernetesCloudResource) Metadata(_ context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse) { resp.TypeName = req.ProviderTypeName + "_kubernetes_cloud" @@ -92,11 +100,11 @@ func (r *kubernetesCloudResource) Schema(_ context.Context, req resource.SchemaR Sensitive: true, }, "parent_cloud_name": schema.StringAttribute{ - Description: "The parent cloud name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform.", + Description: "The parent cloud name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller.", Optional: true, }, "parent_cloud_region": schema.StringAttribute{ - Description: "The parent cloud region name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform.", + Description: "The parent cloud region name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller.", Optional: true, }, "id": schema.StringAttribute{ @@ -128,8 +136,10 @@ func (r *kubernetesCloudResource) Create(ctx context.Context, req resource.Creat // Create the kubernetes cloud. cloudCredentialName, err := r.client.Clouds.CreateKubernetesCloud( &juju.CreateKubernetesCloudInput{ - Name: plan.CloudName.ValueString(), - KubernetesConfig: plan.KubernetesConfig.ValueString(), + Name: plan.CloudName.ValueString(), + KubernetesConfig: plan.KubernetesConfig.ValueString(), + ParentCloudName: plan.ParentCloudName.ValueString(), + ParentCloudRegion: plan.ParentCloudRegion.ValueString(), }, ) if err != nil { @@ -201,8 +211,10 @@ func (r *kubernetesCloudResource) Update(ctx context.Context, req resource.Updat // Update the kubernetes cloud. err := r.client.Clouds.UpdateKubernetesCloud( juju.UpdateKubernetesCloudInput{ - Name: plan.CloudName.ValueString(), - KubernetesConfig: plan.KubernetesConfig.ValueString(), + Name: plan.CloudName.ValueString(), + KubernetesConfig: plan.KubernetesConfig.ValueString(), + ParentCloudName: plan.ParentCloudName.ValueString(), + ParentCloudRegion: plan.ParentCloudRegion.ValueString(), }, ) if err != nil { @@ -250,6 +262,45 @@ func (r *kubernetesCloudResource) trace(msg string, additionalFields ...map[stri tflog.SubsystemTrace(r.subCtx, LogResourceKubernetesCloud, msg, additionalFields...) } +type kuberenetesCloudJAASValidator struct { + client *juju.Client +} + +// Description implements the Description method of the resource.ConfigValidator interface. +func (v *kuberenetesCloudJAASValidator) Description(ctx context.Context) string { + return v.MarkdownDescription(ctx) +} + +// MarkdownDescription implements the MarkdownDescription method of the resource.ConfigValidator interface. +func (v *kuberenetesCloudJAASValidator) MarkdownDescription(_ context.Context) string { + return "Enforces that this resource can only be used with JAAS" +} + +// ValidateResource implements the ValidateResource method of the resource.ConfigValidator interface. +func (v *kuberenetesCloudJAASValidator) ValidateResource(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) { + if v.client == nil { + return + } + + if !v.client.IsJAAS() { + return + } + + var data kubernetesCloudResourceModel + resp.Diagnostics.Append(req.Config.Get(ctx, &data)...) + if resp.Diagnostics.HasError() { + return + } + + if data.ParentCloudName.ValueString() == "" { + resp.Diagnostics.AddError("Plan Error", "parent_cloud_name must be specified when applying to a JAAS controller") + } + + if data.ParentCloudRegion.ValueString() == "" { + resp.Diagnostics.AddError("Plan Error", "parent_cloud_region must be specified when applying to a JAAS controller") + } +} + func newKubernetesCloudID(kubernetesCloudName string, cloudCredentialName string) string { return fmt.Sprintf("%s:%s", kubernetesCloudName, cloudCredentialName) } diff --git a/internal/provider/resource_kubernetes_cloud_test.go b/internal/provider/resource_kubernetes_cloud_test.go index 5a6aafc1..e82d96aa 100644 --- a/internal/provider/resource_kubernetes_cloud_test.go +++ b/internal/provider/resource_kubernetes_cloud_test.go @@ -5,6 +5,7 @@ package provider import ( "os" + "regexp" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" @@ -14,12 +15,6 @@ import ( ) func TestAcc_ResourceKubernetesCloud(t *testing.T) { - // Note (alesstimec): Skipping this test, because the default - // hosted cloud tf provider adds is "other", which cannot - // be parsed by JIMM - it needs a valid cloud/region to determine - // which controller to add the cloud to. - SkipJAAS(t) - // TODO: This test is not adding model as a resource, which is required. // The reason in the race that we (potentially) have in the Juju side. // Once the problem is fixed (https://bugs.launchpad.net/juju/+bug/2084448), @@ -30,12 +25,17 @@ func TestAcc_ResourceKubernetesCloud(t *testing.T) { cloudName := acctest.RandomWithPrefix("tf-test-k8scloud") cloudConfig := os.Getenv("MICROK8S_CONFIG") + jaasTest := false + if _, ok := os.LookupEnv("IS_JAAS"); ok { + jaasTest = true + } + resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, ProtoV6ProviderFactories: frameworkProviderFactories, Steps: []resource.TestStep{ { - Config: testAccResourceKubernetesCloudWithoutModel(cloudName, cloudConfig), + Config: testAccResourceKubernetesCloudWithoutModel(cloudName, cloudConfig, jaasTest), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("juju_kubernetes_cloud."+cloudName, "name", cloudName), ), @@ -44,13 +44,56 @@ func TestAcc_ResourceKubernetesCloud(t *testing.T) { }) } -func testAccResourceKubernetesCloudWithoutModel(cloudName string, config string) string { +func TestAcc_ResourceKubernetesCloudWithJAASIncompleteConfig(t *testing.T) { + OnlyTestAgainstJAAS(t) + // TODO: This test is not adding model as a resource, which is required. + // The reason in the race that we (potentially) have in the Juju side. + // Once the problem is fixed (https://bugs.launchpad.net/juju/+bug/2084448), + // we should add the model as a resource. + if testingCloud != LXDCloudTesting { + t.Skip(t.Name() + " only runs with LXD") + } + cloudName := acctest.RandomWithPrefix("tf-test-k8scloud") + cloudConfig := os.Getenv("MICROK8S_CONFIG") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: frameworkProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccResourceKubernetesCloudWithoutParentCloud(cloudName, cloudConfig), + ExpectError: regexp.MustCompile("parent_cloud_region must be specified when applying to a JAAS controller"), + }, + }, + }) +} + +func testAccResourceKubernetesCloudWithoutModel(cloudName string, config string, jaasTest bool) string { return internaltesting.GetStringFromTemplateWithData( "testAccResourceSecret", ` resource "juju_kubernetes_cloud" "{{.CloudName}}" { name = "{{.CloudName}}" kubernetes_config = file("~/microk8s-config.yaml") + {{ if .JAASTest }} + parent_cloud_name = "lxd" + parent_cloud_region = "localhost" + {{ end }} +} +`, internaltesting.TemplateData{ + "CloudName": cloudName, + "Config": config, + "JAASTest": jaasTest, + }) +} + +func testAccResourceKubernetesCloudWithoutParentCloud(cloudName string, config string) string { + return internaltesting.GetStringFromTemplateWithData( + "testAccResourceSecret", + ` +resource "juju_kubernetes_cloud" "{{.CloudName}}" { + name = "{{.CloudName}}" + kubernetes_config = "test config" } `, internaltesting.TemplateData{ "CloudName": cloudName, From 7974ba0b15f267673a6a8160533ebe03d76bffd0 Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Tue, 17 Dec 2024 14:58:17 +0100 Subject: [PATCH 3/4] fix: fixed wording in internal/provider/resource_kubernetes_cloud.go Co-authored-by: Kian Parvin <46668016+kian99@users.noreply.github.com> --- .github/workflows/test_integration.yml | 2 ++ docs/resources/kubernetes_cloud.md | 4 ++-- internal/provider/resource_kubernetes_cloud.go | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test_integration.yml b/.github/workflows/test_integration.yml index 4ce78a55..be60e173 100644 --- a/.github/workflows/test_integration.yml +++ b/.github/workflows/test_integration.yml @@ -45,8 +45,10 @@ jobs: action-operator: - { lxd-channel: "5.21/stable", cloud: "lxd", cloud-channel: "5.21", juju: "2.9" } - { lxd-channel: "5.21/stable", cloud: "lxd", cloud-channel: "5.21", juju: "3" } + - { lxd-channel: "latest/stable", cloud: "lxd", cloud-channel: "latest", juju: "3" } - { lxd-channel: "5.21/stable", cloud: "microk8s", cloud-channel: "1.28-strict", juju: "3.1" } - { lxd-channel: "5.21/stable", cloud: "microk8s", cloud-channel: "1.28-strict", juju: "3" } + - { lxd-channel: "latest/stable", cloud: "microk8s", cloud-channel: "1.32-strict", juju: "3" } timeout-minutes: 60 steps: - uses: actions/checkout@v4 diff --git a/docs/resources/kubernetes_cloud.md b/docs/resources/kubernetes_cloud.md index 4612851f..4d4829d1 100644 --- a/docs/resources/kubernetes_cloud.md +++ b/docs/resources/kubernetes_cloud.md @@ -37,8 +37,8 @@ resource "juju_model" "my-model" { ### Optional - `kubernetes_config` (String, Sensitive) The kubernetes config file path for the cloud. Cloud credentials will be added to the Juju controller for you. -- `parent_cloud_name` (String) The parent cloud name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller. -- `parent_cloud_region` (String) The parent cloud region name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller. +- `parent_cloud_name` (String) The parent cloud name, for adding a k8s cluster from an existing cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller. +- `parent_cloud_region` (String) The parent cloud region name, for adding a k8s cluster from an existing cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller. ### Read-Only diff --git a/internal/provider/resource_kubernetes_cloud.go b/internal/provider/resource_kubernetes_cloud.go index 1f49cb32..bfc5a56f 100644 --- a/internal/provider/resource_kubernetes_cloud.go +++ b/internal/provider/resource_kubernetes_cloud.go @@ -100,11 +100,11 @@ func (r *kubernetesCloudResource) Schema(_ context.Context, req resource.SchemaR Sensitive: true, }, "parent_cloud_name": schema.StringAttribute{ - Description: "The parent cloud name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller.", + Description: "The parent cloud name, for adding a k8s cluster from an existing cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller.", Optional: true, }, "parent_cloud_region": schema.StringAttribute{ - Description: "The parent cloud region name in case adding k8s cluster from existed cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller.", + Description: "The parent cloud region name, for adding a k8s cluster from an existing cloud. Changing this value will cause the cloud to be destroyed and recreated by terraform. *Note* that this value must be set when running against a JAAS controller.", Optional: true, }, "id": schema.StringAttribute{ From 84e395482064693fa7975b450eb39ee6e384bd3e Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Wed, 18 Dec 2024 16:11:48 +0100 Subject: [PATCH 4/4] fix: use of secret URI instead of ID The `secret_id` field of the `juju_secret_resource` should hold the secret URI instead of the ID so that other resources can use it without having to manually prefix it with `secret:`. --- docs/resources/access_secret.md | 2 +- docs/resources/secret.md | 2 +- internal/juju/secrets.go | 36 ++++++++++----------- internal/juju/secrets_test.go | 20 ++++++------ internal/provider/data_source_secrets.go | 4 +-- internal/provider/resource_access_secret.go | 20 ++++++------ internal/provider/resource_secret.go | 21 ++++++------ internal/provider/resource_secret_test.go | 35 ++++++++++++++++++++ 8 files changed, 88 insertions(+), 52 deletions(-) diff --git a/docs/resources/access_secret.md b/docs/resources/access_secret.md index 56e9678b..ae4deae6 100644 --- a/docs/resources/access_secret.md +++ b/docs/resources/access_secret.md @@ -19,7 +19,7 @@ A resource that represents a Juju secret access. - `applications` (List of String) The list of applications to which the secret is granted. - `model` (String) The model in which the secret belongs. -- `secret_id` (String) The ID of the secret. E.g. coj8mulh8b41e8nv6p90 +- `secret_id` (String) The ID of the secret. E.g. secret:coj8mulh8b41e8nv6p90 ### Read-Only diff --git a/docs/resources/secret.md b/docs/resources/secret.md index 524f6043..a7c0ff27 100644 --- a/docs/resources/secret.md +++ b/docs/resources/secret.md @@ -49,7 +49,7 @@ resource "juju_application" "my-application" { ### Read-Only - `id` (String) The ID of the secret. Used for terraform import. -- `secret_id` (String) The ID of the secret. E.g. coj8mulh8b41e8nv6p90 +- `secret_id` (String) The ID of the secret. E.g. secret:coj8mulh8b41e8nv6p90 ## Import diff --git a/internal/juju/secrets.go b/internal/juju/secrets.go index 99c1ac2e..32b3057e 100644 --- a/internal/juju/secrets.go +++ b/internal/juju/secrets.go @@ -50,18 +50,18 @@ type CreateSecretInput struct { } type CreateSecretOutput struct { - SecretId string + SecretURI string } type ReadSecretInput struct { - SecretId string + SecretURI string ModelName string Name *string Revision *int } type ReadSecretOutput struct { - SecretId string + SecretURI string Name string Value map[string]string Applications []string @@ -69,7 +69,7 @@ type ReadSecretOutput struct { } type UpdateSecretInput struct { - SecretId string + SecretURI string ModelName string Name *string Value *map[string]string @@ -78,12 +78,12 @@ type UpdateSecretInput struct { } type DeleteSecretInput struct { - SecretId string + SecretURI string ModelName string } type GrantRevokeAccessSecretInput struct { - SecretId string + SecretURI string ModelName string Applications []string } @@ -126,16 +126,16 @@ func (c *secretsClient) CreateSecret(input *CreateSecretInput) (CreateSecretOutp encodedValue[k] = base64.StdEncoding.EncodeToString([]byte(v)) } - secretId, err := secretAPIClient.CreateSecret(input.Name, input.Info, encodedValue) + secretURIString, err := secretAPIClient.CreateSecret(input.Name, input.Info, encodedValue) if err != nil { return CreateSecretOutput{}, typedError(err) } - secretURI, err := coresecrets.ParseURI(secretId) + secretURI, err := coresecrets.ParseURI(secretURIString) if err != nil { return CreateSecretOutput{}, typedError(err) } return CreateSecretOutput{ - SecretId: secretURI.ID, + SecretURI: secretURI.String(), }, nil } @@ -150,8 +150,8 @@ func (c *secretsClient) ReadSecret(input *ReadSecretInput) (ReadSecretOutput, er secretAPIClient := c.getSecretAPIClient(conn) var secretURI *coresecrets.URI - if input.SecretId != "" { - secretURI, err = coresecrets.ParseURI(input.SecretId) + if input.SecretURI != "" { + secretURI, err = coresecrets.ParseURI(input.SecretURI) if err != nil { return ReadSecretOutput{}, err } @@ -169,7 +169,7 @@ func (c *secretsClient) ReadSecret(input *ReadSecretInput) (ReadSecretOutput, er return ReadSecretOutput{}, typedError(err) } if len(results) < 1 { - return ReadSecretOutput{}, &secretNotFoundError{secretId: input.SecretId} + return ReadSecretOutput{}, &secretNotFoundError{secretId: input.SecretURI} } if results[0].Error != "" { return ReadSecretOutput{}, errors.New(results[0].Error) @@ -185,7 +185,7 @@ func (c *secretsClient) ReadSecret(input *ReadSecretInput) (ReadSecretOutput, er applications := getApplicationsFromAccessInfo(results[0].Access) return ReadSecretOutput{ - SecretId: results[0].Metadata.URI.ID, + SecretURI: results[0].Metadata.URI.String(), Name: results[0].Metadata.Label, Value: decodedValue, Applications: applications, @@ -204,7 +204,7 @@ func (c *secretsClient) UpdateSecret(input *UpdateSecretInput) error { secretAPIClient := c.getSecretAPIClient(conn) // Specify by ID or Name - if input.SecretId == "" && input.Name == nil { + if input.SecretURI == "" && input.Name == nil { return errors.New("must specify either secret ID or name") } @@ -228,9 +228,9 @@ func (c *secretsClient) UpdateSecret(input *UpdateSecretInput) error { value = map[string]string{} } - if input.SecretId != "" { + if input.SecretURI != "" { // Specify by ID - secretURI, err := coresecrets.ParseURI(input.SecretId) + secretURI, err := coresecrets.ParseURI(input.SecretURI) if err != nil { return err } @@ -262,7 +262,7 @@ func (c *secretsClient) DeleteSecret(input *DeleteSecretInput) error { } secretAPIClient := c.getSecretAPIClient(conn) - secretURI, err := coresecrets.ParseURI(input.SecretId) + secretURI, err := coresecrets.ParseURI(input.SecretURI) if err != nil { return err } @@ -285,7 +285,7 @@ func (c *secretsClient) UpdateAccessSecret(input *GrantRevokeAccessSecretInput, secretAPIClient := c.getSecretAPIClient(conn) - secretURI, err := coresecrets.ParseURI(input.SecretId) + secretURI, err := coresecrets.ParseURI(input.SecretURI) if err != nil { return err } diff --git a/internal/juju/secrets_test.go b/internal/juju/secrets_test.go index 2a8a3782..32de315b 100644 --- a/internal/juju/secrets_test.go +++ b/internal/juju/secrets_test.go @@ -66,7 +66,7 @@ func (s *SecretSuite) TestCreateSecret() { s.Require().NoError(err) s.Assert().NotNil(output) - s.Assert().Equal(secretURI.ID, output.SecretId) + s.Assert().Equal(secretURI.String(), output.SecretURI) } func (s *SecretSuite) TestCreateSecretError() { @@ -130,7 +130,7 @@ func (s *SecretSuite) TestReadSecret() { client := s.getSecretsClient() output, err := client.ReadSecret(&ReadSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, Name: &secretName, Revision: &secretRevision, @@ -162,7 +162,7 @@ func (s *SecretSuite) TestReadSecretError() { client := s.getSecretsClient() output, err := client.ReadSecret(&ReadSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, }) s.Require().Error(err) @@ -192,7 +192,7 @@ func (s *SecretSuite) TestUpdateSecretWithRenaming() { client := s.getSecretsClient() err = client.UpdateSecret(&UpdateSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, Name: &newSecretName, Value: &decodedValue, @@ -221,7 +221,7 @@ func (s *SecretSuite) TestUpdateSecretWithRenaming() { // read secret and check if value is updated output, err := client.ReadSecret(&ReadSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, }) s.Require().NoError(err) @@ -249,7 +249,7 @@ func (s *SecretSuite) TestUpdateSecret() { client := s.getSecretsClient() err = client.UpdateSecret(&UpdateSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, Value: &decodedValue, AutoPrune: &autoPrune, @@ -278,7 +278,7 @@ func (s *SecretSuite) TestUpdateSecret() { // read secret and check if secret info is updated output, err := client.ReadSecret(&ReadSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, }) s.Require().NoError(err) @@ -299,7 +299,7 @@ func (s *SecretSuite) TestDeleteSecret() { client := s.getSecretsClient() err = client.DeleteSecret(&DeleteSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, }) s.Assert().NoError(err) @@ -320,14 +320,14 @@ func (s *SecretSuite) TestUpdateAccessSecret() { client := s.getSecretsClient() err = client.UpdateAccessSecret(&GrantRevokeAccessSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, Applications: applications, }, GrantAccess) s.Require().NoError(err) err = client.UpdateAccessSecret(&GrantRevokeAccessSecretInput{ - SecretId: secretId, + SecretURI: secretId, ModelName: *s.testModelName, Applications: applications, }, RevokeAccess) diff --git a/internal/provider/data_source_secrets.go b/internal/provider/data_source_secrets.go index 3dd61c01..23789df6 100644 --- a/internal/provider/data_source_secrets.go +++ b/internal/provider/data_source_secrets.go @@ -112,7 +112,7 @@ func (d *secretDataSource) Read(ctx context.Context, req datasource.ReadRequest, if data.SecretId.ValueString() == "" { readSecretInput.Name = data.Name.ValueStringPointer() } else { - readSecretInput.SecretId = data.SecretId.ValueString() + readSecretInput.SecretURI = data.SecretId.ValueString() } readSecretOutput, err := d.client.Secrets.ReadSecret(&readSecretInput) @@ -122,7 +122,7 @@ func (d *secretDataSource) Read(ctx context.Context, req datasource.ReadRequest, } d.trace(fmt.Sprintf("read secret data source %q", data.SecretId)) - data.SecretId = types.StringValue(readSecretOutput.SecretId) + data.SecretId = types.StringValue(readSecretOutput.SecretURI) // Save state into Terraform state resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) diff --git a/internal/provider/resource_access_secret.go b/internal/provider/resource_access_secret.go index 045211cd..c46eb34d 100644 --- a/internal/provider/resource_access_secret.go +++ b/internal/provider/resource_access_secret.go @@ -79,8 +79,8 @@ func (s *accessSecretResource) ImportState(ctx context.Context, req resource.Imp // Save the secret access details into the Terraform state state := accessSecretResourceModel{ Model: types.StringValue(modelName), - SecretId: types.StringValue(readSecretOutput.SecretId), - ID: types.StringValue(newSecretID(modelName, readSecretOutput.SecretId)), + SecretId: types.StringValue(readSecretOutput.SecretURI), + ID: types.StringValue(newSecretID(modelName, readSecretOutput.SecretURI)), } // Save the secret details into the Terraform state @@ -114,7 +114,7 @@ func (s *accessSecretResource) Schema(_ context.Context, req resource.SchemaRequ }, }, "secret_id": schema.StringAttribute{ - Description: "The ID of the secret. E.g. coj8mulh8b41e8nv6p90", + Description: "The ID of the secret. E.g. secret:coj8mulh8b41e8nv6p90", Required: true, PlanModifiers: []planmodifier.String{ stringplanmodifier.RequiresReplace(), @@ -177,7 +177,7 @@ func (s *accessSecretResource) Create(ctx context.Context, req resource.CreateRe err := s.client.Secrets.UpdateAccessSecret(&juju.GrantRevokeAccessSecretInput{ ModelName: plan.Model.ValueString(), - SecretId: plan.SecretId.ValueString(), + SecretURI: plan.SecretId.ValueString(), Applications: applications, }, juju.GrantAccess) if err != nil { @@ -209,7 +209,7 @@ func (s *accessSecretResource) Read(ctx context.Context, req resource.ReadReques } readSecretOutput, err := s.client.Secrets.ReadSecret(&juju.ReadSecretInput{ - SecretId: state.SecretId.ValueString(), + SecretURI: state.SecretId.ValueString(), ModelName: state.Model.ValueString(), }) if err != nil { @@ -225,7 +225,7 @@ func (s *accessSecretResource) Read(ctx context.Context, req resource.ReadReques } state.Applications = secretApplications - state.ID = types.StringValue(newSecretID(state.Model.ValueString(), readSecretOutput.SecretId)) + state.ID = types.StringValue(newSecretID(state.Model.ValueString(), readSecretOutput.SecretURI)) // Save state into Terraform state resp.Diagnostics.Append(resp.State.Set(ctx, &state)...) @@ -256,7 +256,7 @@ func (s *accessSecretResource) Update(ctx context.Context, req resource.UpdateRe var updatedAccessSecretInput juju.GrantRevokeAccessSecretInput updatedAccessSecretInput.ModelName = state.Model.ValueString() - updatedAccessSecretInput.SecretId = state.SecretId.ValueString() + updatedAccessSecretInput.SecretURI = state.SecretId.ValueString() if plan.Applications.Equal(state.Applications) { s.trace(fmt.Sprintf("no updates to secret access %q", state.SecretId)) @@ -297,7 +297,7 @@ func (s *accessSecretResource) Update(ctx context.Context, req resource.UpdateRe if !applicationsToGrant.IsEmpty() { err := s.client.Secrets.UpdateAccessSecret(&juju.GrantRevokeAccessSecretInput{ ModelName: state.Model.ValueString(), - SecretId: state.SecretId.ValueString(), + SecretURI: state.SecretId.ValueString(), Applications: applicationsToGrant.Values(), }, juju.GrantAccess) if err != nil { @@ -310,7 +310,7 @@ func (s *accessSecretResource) Update(ctx context.Context, req resource.UpdateRe if !applicationsToRevoke.IsEmpty() { err := s.client.Secrets.UpdateAccessSecret(&juju.GrantRevokeAccessSecretInput{ ModelName: state.Model.ValueString(), - SecretId: state.SecretId.ValueString(), + SecretURI: state.SecretId.ValueString(), Applications: applicationsToRevoke.Values(), }, juju.RevokeAccess) if err != nil { @@ -349,7 +349,7 @@ func (s *accessSecretResource) Delete(ctx context.Context, req resource.DeleteRe err := s.client.Secrets.UpdateAccessSecret(&juju.GrantRevokeAccessSecretInput{ ModelName: state.Model.ValueString(), - SecretId: state.SecretId.ValueString(), + SecretURI: state.SecretId.ValueString(), Applications: applications, }, juju.RevokeAccess) if err != nil { diff --git a/internal/provider/resource_secret.go b/internal/provider/resource_secret.go index 7e64121f..b30d58e0 100644 --- a/internal/provider/resource_secret.go +++ b/internal/provider/resource_secret.go @@ -84,7 +84,7 @@ func (s *secretResource) ImportState(ctx context.Context, req resource.ImportSta state := secretResourceModel{ Model: types.StringValue(modelName), Name: types.StringValue(readSecretOutput.Name), - SecretId: types.StringValue(readSecretOutput.SecretId), + SecretId: types.StringValue(readSecretOutput.SecretURI), } if readSecretOutput.Info != "" { @@ -131,7 +131,7 @@ func (s *secretResource) Schema(_ context.Context, req resource.SchemaRequest, r Sensitive: true, }, "secret_id": schema.StringAttribute{ - Description: "The ID of the secret. E.g. coj8mulh8b41e8nv6p90", + Description: "The ID of the secret. E.g. secret:coj8mulh8b41e8nv6p90", Computed: true, PlanModifiers: []planmodifier.String{ stringplanmodifier.UseStateForUnknown(), @@ -204,7 +204,7 @@ func (s *secretResource) Create(ctx context.Context, req resource.CreateRequest, return } - plan.SecretId = types.StringValue(createSecretOutput.SecretId) + plan.SecretId = types.StringValue(createSecretOutput.SecretURI) plan.ID = types.StringValue(newSecretID(plan.Model.ValueString(), plan.SecretId.ValueString())) s.trace(fmt.Sprintf("saving secret resource %q", plan.SecretId.ValueString()), map[string]interface{}{ @@ -212,8 +212,9 @@ func (s *secretResource) Create(ctx context.Context, req resource.CreateRequest, "name": plan.Name.ValueString(), "model": plan.Model.ValueString(), "info": plan.Info.ValueString(), - "values": plan.Value.String(), - "id": plan.ID.ValueString(), + // note (alesstimec): we should not be logging secret values! + //"values": plan.Value.String(), + "id": plan.ID.ValueString(), }) // Save plan into Terraform state resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) @@ -240,7 +241,7 @@ func (s *secretResource) Read(ctx context.Context, req resource.ReadRequest, res s.trace(fmt.Sprintf("reading secret resource %q", state.SecretId)) readSecretOutput, err := s.client.Secrets.ReadSecret(&juju.ReadSecretInput{ - SecretId: state.SecretId.ValueString(), + SecretURI: state.SecretId.ValueString(), ModelName: state.Model.ValueString(), }) if err != nil { @@ -255,7 +256,7 @@ func (s *secretResource) Read(ctx context.Context, req resource.ReadRequest, res if !state.Info.IsNull() { state.Info = types.StringValue(readSecretOutput.Info) } - state.ID = types.StringValue(newSecretID(state.Model.ValueString(), readSecretOutput.SecretId)) + state.ID = types.StringValue(newSecretID(state.Model.ValueString(), readSecretOutput.SecretURI)) secretValue, errDiag := types.MapValueFrom(ctx, types.StringType, readSecretOutput.Value) resp.Diagnostics.Append(errDiag...) @@ -297,7 +298,7 @@ func (s *secretResource) Update(ctx context.Context, req resource.UpdateRequest, var updatedSecretInput juju.UpdateSecretInput updatedSecretInput.ModelName = state.Model.ValueString() - updatedSecretInput.SecretId = state.SecretId.ValueString() + updatedSecretInput.SecretURI = state.SecretId.ValueString() // Check if the secret name has changed if !plan.Name.Equal(state.Name) { @@ -362,7 +363,7 @@ func (s *secretResource) Delete(ctx context.Context, req resource.DeleteRequest, err := s.client.Secrets.DeleteSecret(&juju.DeleteSecretInput{ ModelName: state.Model.ValueString(), - SecretId: state.SecretId.ValueString(), + SecretURI: state.SecretId.ValueString(), }) if err != nil { resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to delete secret, got error: %s", err)) @@ -380,5 +381,5 @@ func (s *secretResource) trace(msg string, additionalFields ...map[string]interf } func newSecretID(model, secret string) string { - return fmt.Sprintf("%s:%s", model, secret) + return fmt.Sprintf("%s:%s", model, strings.TrimPrefix(secret, "secret:")) } diff --git a/internal/provider/resource_secret_test.go b/internal/provider/resource_secret_test.go index 5bd72b42..52130c83 100644 --- a/internal/provider/resource_secret_test.go +++ b/internal/provider/resource_secret_test.go @@ -6,10 +6,14 @@ package provider import ( "fmt" "os" + "regexp" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/knownvalue" + "github.com/hashicorp/terraform-plugin-testing/statecheck" + "github.com/hashicorp/terraform-plugin-testing/tfjsonpath" internaltesting "github.com/juju/terraform-provider-juju/internal/testing" ) @@ -86,6 +90,37 @@ func TestAcc_ResourceSecret_CreateWithInfo(t *testing.T) { }) } +func TestAcc_ResourceSecret_CheckSecretID(t *testing.T) { + agentVersion := os.Getenv(TestJujuAgentVersion) + if agentVersion == "" { + t.Errorf("%s is not set", TestJujuAgentVersion) + } else if internaltesting.CompareVersions(agentVersion, "3.3.0") < 0 { + t.Skipf("%s is not set or is below 3.3.0", TestJujuAgentVersion) + } + + modelName := acctest.RandomWithPrefix("tf-test-model") + secretName := "tf-test-secret" + secretValue := map[string]string{ + "key1": "value1", + "key2": "value2", + } + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: frameworkProviderFactories, + Steps: []resource.TestStep{{ + Config: testAccResourceSecret(modelName, secretName, secretValue, ""), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue( + "juju_secret."+secretName, + tfjsonpath.New("secret_id"), + knownvalue.StringRegexp(regexp.MustCompile("secret:.*")), + ), + }, + }}, + }) +} + func TestAcc_ResourceSecret_CreateWithNoInfo(t *testing.T) { agentVersion := os.Getenv(TestJujuAgentVersion) if agentVersion == "" {