From 54f85881487074ca78cc766632fbc96f4f6e8728 Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Mon, 9 Dec 2024 16:17:22 +0100 Subject: [PATCH 1/3] 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/3] 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 d344dfaf1cca7a500ff5e8024651b274b3f8b971 Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Tue, 17 Dec 2024 14:58:17 +0100 Subject: [PATCH 3/3] fix: fixed wording in internal/provider/resource_kubernetes_cloud.go Co-authored-by: Kian Parvin <46668016+kian99@users.noreply.github.com> --- docs/resources/kubernetes_cloud.md | 4 ++-- internal/provider/resource_kubernetes_cloud.go | 12 ++++-------- internal/provider/resource_kubernetes_cloud_test.go | 12 +++++------- 3 files changed, 11 insertions(+), 17 deletions(-) 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..7de37ccb 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{ @@ -273,7 +273,7 @@ func (v *kuberenetesCloudJAASValidator) Description(ctx context.Context) string // 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" + return "Enforces that the parent_cloud_name is specified when applying to a JAAS controller." } // ValidateResource implements the ValidateResource method of the resource.ConfigValidator interface. @@ -293,11 +293,7 @@ func (v *kuberenetesCloudJAASValidator) ValidateResource(ctx context.Context, re } 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") + resp.Diagnostics.AddError("Plan Error", "Field `parent_cloud_name` must be specified when applying to a JAAS controller.") } } diff --git a/internal/provider/resource_kubernetes_cloud_test.go b/internal/provider/resource_kubernetes_cloud_test.go index e82d96aa..e908e20f 100644 --- a/internal/provider/resource_kubernetes_cloud_test.go +++ b/internal/provider/resource_kubernetes_cloud_test.go @@ -59,12 +59,10 @@ func TestAcc_ResourceKubernetesCloudWithJAASIncompleteConfig(t *testing.T) { 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"), - }, - }, + Steps: []resource.TestStep{{ + Config: testAccResourceKubernetesCloudWithoutParentCloudName(cloudName, cloudConfig), + ExpectError: regexp.MustCompile("Field `parent_cloud_name` must be specified when applying to a JAAS.*"), + }}, }) } @@ -87,7 +85,7 @@ resource "juju_kubernetes_cloud" "{{.CloudName}}" { }) } -func testAccResourceKubernetesCloudWithoutParentCloud(cloudName string, config string) string { +func testAccResourceKubernetesCloudWithoutParentCloudName(cloudName string, config string) string { return internaltesting.GetStringFromTemplateWithData( "testAccResourceSecret", `