From f9aeea97345b251e211ac8b33a640d0389897fee Mon Sep 17 00:00:00 2001 From: Stef Forrester Date: Tue, 13 Apr 2021 11:55:46 -0700 Subject: [PATCH] Fix perpetual diff in claim_ref (#1227) The claim_ref attribute would perpetually show a diff if the PV was created without a claim_ref specified. This is because all PVs gain a claimRef once a PVC binds to it. (This creates the bi-directional link between PV and PVC, and it's done on the Kubernetes API side). To support this, `claim_ref` is now a Computed attribute, and it will update to show the claimRef once a PVC has bound to the PV, (only now it will do so without creating a diff in terraform). Also added extra tests to catch this case and moved claim_ref's flatteners/expanders/tests into the correct files. --- .../resource_kubernetes_persistent_volume.go | 6 +- ...kubernetes_persistent_volume_claim_test.go | 5 - ...ource_kubernetes_persistent_volume_test.go | 144 +++++++++++++++--- .../structure_persistent_volume_spec.go | 19 ++- .../structure_persistent_volume_spec_test.go | 40 +++++ kubernetes/structures_container.go | 11 -- kubernetes/structures_container_test.go | 32 ---- 7 files changed, 183 insertions(+), 74 deletions(-) create mode 100644 kubernetes/structure_persistent_volume_spec_test.go diff --git a/kubernetes/resource_kubernetes_persistent_volume.go b/kubernetes/resource_kubernetes_persistent_volume.go index 0a9012e894..4d7dc73476 100644 --- a/kubernetes/resource_kubernetes_persistent_volume.go +++ b/kubernetes/resource_kubernetes_persistent_volume.go @@ -118,14 +118,16 @@ func resourceKubernetesPersistentVolume() *schema.Resource { Type: schema.TypeList, Description: "A reference to the persistent volume claim details for statically managed PVs. More Info: https://kubernetes.io/docs/concepts/storage/persistent-volumes/#binding", Optional: true, + Computed: true, MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "namespace": { Type: schema.TypeString, - Description: "The namespace of the PersistentVolumeClaim", + Description: "The namespace of the PersistentVolumeClaim. Uses 'default' namespace if none is specified.", Elem: schema.TypeString, - Required: true, + Optional: true, + Default: "default", }, "name": { Type: schema.TypeString, diff --git a/kubernetes/resource_kubernetes_persistent_volume_claim_test.go b/kubernetes/resource_kubernetes_persistent_volume_claim_test.go index f70b9387b8..f613f8a762 100644 --- a/kubernetes/resource_kubernetes_persistent_volume_claim_test.go +++ b/kubernetes/resource_kubernetes_persistent_volume_claim_test.go @@ -716,11 +716,6 @@ resource "kubernetes_persistent_volume" "test" { } } } - lifecycle { - ignore_changes = [ - spec[0].claim_ref, - ] - } } resource "kubernetes_persistent_volume_claim" "test" { wait_until_bound = true diff --git a/kubernetes/resource_kubernetes_persistent_volume_test.go b/kubernetes/resource_kubernetes_persistent_volume_test.go index 244ffbb874..5b86356f9b 100644 --- a/kubernetes/resource_kubernetes_persistent_volume_test.go +++ b/kubernetes/resource_kubernetes_persistent_volume_test.go @@ -16,6 +16,29 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +func TestAccKubernetesPersistentVolume_minimal(t *testing.T) { + var conf api.PersistentVolume + randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum) + name := fmt.Sprintf("tf-acc-test-%s", randString) + + const resourceName = "kubernetes_persistent_volume.test" + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + IDRefreshName: resourceName, + ProviderFactories: testAccProviderFactories, + CheckDestroy: testAccCheckKubernetesPersistentVolumeDestroy, + Steps: []resource.TestStep{ + { + Config: testAccKubernetesPersistentVolumeConfig_hostPath_basic(name), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckKubernetesPersistentVolumeExists(resourceName, &conf), + resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.#", "0"), + ), + }, + }, + }) +} + func TestAccKubernetesPersistentVolume_azure_basic(t *testing.T) { var conf api.PersistentVolume randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum) @@ -923,12 +946,11 @@ func TestAccKubernetesPersistentVolume_regression(t *testing.T) { }) } -func TestAccKubernetesPersistentVolume_hostPath_claimRef(t *testing.T) { - var conf api.PersistentVolume +func TestAccKubernetesPersistentVolume_hostpath_claimRef(t *testing.T) { + var conf1, conf2 api.PersistentVolume + var conf3 api.PersistentVolumeClaim randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum) name := fmt.Sprintf("tf-acc-test-%s", randString) - claimNamespace := "default" - claimName := "expected-claim-name" const resourceName = "kubernetes_persistent_volume.test" resource.Test(t, resource.TestCase{ @@ -937,30 +959,32 @@ func TestAccKubernetesPersistentVolume_hostPath_claimRef(t *testing.T) { ProviderFactories: testAccProviderFactories, CheckDestroy: testAccCheckKubernetesPersistentVolumeDestroy, Steps: []resource.TestStep{ - // create a volume without a claimRef { - Config: testAccKubernetesPersistentVolumeConfig_hostPath_basic(name), + Config: testAccKubernetesPersistentVolumeConfig_hostPath_claimRef_noNamespace(name), Check: resource.ComposeAggregateTestCheckFunc( - testAccCheckKubernetesPersistentVolumeExists(resourceName, &conf), - resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.#", "0"), + testAccCheckKubernetesPersistentVolumeExists(resourceName, &conf1), + resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.#", "1"), + resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.0.name", name), + resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.0.namespace", "default"), ), }, - // set the claimRef and assert it's present { - Config: testAccKubernetesPersistentVolumeConfig_hostPath_claimRef(name, claimNamespace, claimName), + Config: testAccKubernetesPersistentVolumeConfig_hostPath_claimRef_withNamespace(name), Check: resource.ComposeAggregateTestCheckFunc( - testAccCheckKubernetesPersistentVolumeExists(resourceName, &conf), - resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.#", "1"), - resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.0.namespace", claimNamespace), - resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.0.name", claimName), + testAccCheckKubernetesPersistentVolumeExists(resourceName, &conf2), + resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.0.namespace", name), + resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.0.name", name), + testAccCheckKubernetesPersistentVolumeForceNew(&conf1, &conf2, false), ), }, - // unset the claimRef and assert it's absent { - Config: testAccKubernetesPersistentVolumeConfig_hostPath_basic(name), + Config: testAccKubernetesPersistentVolumeConfig_hostPath_claimRef_withPVC(name), Check: resource.ComposeAggregateTestCheckFunc( - testAccCheckKubernetesPersistentVolumeExists(resourceName, &conf), - resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.#", "0"), + testAccCheckKubernetesPersistentVolumeExists(resourceName, &conf2), + testAccCheckKubernetesPersistentVolumeClaimExists("kubernetes_persistent_volume_claim.test", &conf3), + resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.0.namespace", name), + resource.TestCheckResourceAttr(resourceName, "spec.0.claim_ref.0.name", name), + testAccCheckKubernetesPersistentVolumeForceNew(&conf1, &conf2, false), ), }, }, @@ -1920,11 +1944,71 @@ func testAccKubernetesPersistentVolumeConfig_hostPath_basic(name string) string }`, name) } -func testAccKubernetesPersistentVolumeConfig_hostPath_claimRef(name string, claimNamespace string, claimName string) string { +func testAccKubernetesPersistentVolumeConfig_hostPath_claimRef_noNamespace(name string) string { return fmt.Sprintf(`resource "kubernetes_persistent_volume" "test" { metadata { name = "%s" } + spec { + capacity = { + storage = "1Gi" + } + access_modes = ["ReadWriteMany"] + mount_options = ["foo"] + claim_ref { + name = "%s" + } + + persistent_volume_source { + host_path { + path = "/mnt/local-volume" + } + } + } +}`, name, name) +} + +func testAccKubernetesPersistentVolumeConfig_hostPath_claimRef_withNamespace(name string) string { + return fmt.Sprintf(`resource "kubernetes_namespace" "test" { + metadata { + name = "%s" + } +} +resource "kubernetes_persistent_volume" "test" { + metadata { + name = "%s" + } + spec { + capacity = { + storage = "1Gi" + } + access_modes = ["ReadWriteMany"] + mount_options = ["foo"] + claim_ref { + name = "%s" + namespace = kubernetes_namespace.test.metadata.0.name + } + + persistent_volume_source { + host_path { + path = "/mnt/local-volume" + } + } + } +}`, name, name, name) +} + +func testAccKubernetesPersistentVolumeConfig_hostPath_claimRef_withPVC(name string) string { + return fmt.Sprintf(`resource "kubernetes_namespace" "test" { + metadata { + name = "%s" + } +} + +resource "kubernetes_persistent_volume" "test" { + metadata { + name = "%s" + } spec { capacity = { storage = "1Gi" @@ -1942,7 +2026,27 @@ func testAccKubernetesPersistentVolumeConfig_hostPath_claimRef(name string, clai } } } -}`, name, claimName, claimNamespace) +} + +resource "kubernetes_persistent_volume_claim" "test" { + metadata { + name = "%s" + namespace = "%s" + } + + spec { + access_modes = ["ReadWriteOnce"] + + resources { + requests = { + storage = "1Gi" + } + } + + volume_name = kubernetes_persistent_volume.test.metadata.0.name + } +} +`, name, name, name, name, name, name) } func testAccKubernetesPersistentVolume_regression(provider, name, path, typ string) string { diff --git a/kubernetes/structure_persistent_volume_spec.go b/kubernetes/structure_persistent_volume_spec.go index 0b22b4148a..9f8c989740 100644 --- a/kubernetes/structure_persistent_volume_spec.go +++ b/kubernetes/structure_persistent_volume_spec.go @@ -403,6 +403,17 @@ func flattenPersistentVolumeSpec(in v1.PersistentVolumeSpec) []interface{} { return []interface{}{att} } +func flattenObjectRef(in *v1.ObjectReference) []interface{} { + att := make(map[string]interface{}) + if in.Name != "" { + att["name"] = in.Name + } + if in.Namespace != "" { + att["namespace"] = in.Namespace + } + return []interface{}{att} +} + func flattenPhotonPersistentDiskVolumeSource(in *v1.PhotonPersistentDiskVolumeSource) []interface{} { att := make(map[string]interface{}) att["pd_id"] = in.PdID @@ -1025,11 +1036,11 @@ func expandPersistentVolumeSpec(l []interface{}) (*v1.PersistentVolumeSpec, erro return obj, nil } -func expandClaimRef(v []interface{}) *v1.ObjectReference { - if len(v) == 0 || v[0] == nil { - return nil +func expandClaimRef(l []interface{}) *v1.ObjectReference { + if len(l) == 0 || l[0] == nil { + return &v1.ObjectReference{} } - o := v[0].(map[string]interface{}) + o := l[0].(map[string]interface{}) return &v1.ObjectReference{ Name: o["name"].(string), Namespace: o["namespace"].(string), diff --git a/kubernetes/structure_persistent_volume_spec_test.go b/kubernetes/structure_persistent_volume_spec_test.go new file mode 100644 index 0000000000..52dbaedef9 --- /dev/null +++ b/kubernetes/structure_persistent_volume_spec_test.go @@ -0,0 +1,40 @@ +package kubernetes + +import ( + "reflect" + "testing" + + v1 "k8s.io/api/core/v1" +) + +func TestFlattenObjectRef(t *testing.T) { + cases := []struct { + Input *v1.ObjectReference + ExpectedOutput []interface{} + }{ + { + &v1.ObjectReference{ + Name: "demo", + Namespace: "default", + }, + []interface{}{ + map[string]interface{}{ + "name": "demo", + "namespace": "default", + }, + }, + }, + { + &v1.ObjectReference{}, + []interface{}{map[string]interface{}{}}, + }, + } + + for _, tc := range cases { + output := flattenObjectRef(tc.Input) + if !reflect.DeepEqual(output, tc.ExpectedOutput) { + t.Fatalf("Unexpected output from flattener.\nExpected: %#v\nGiven: %#v", + tc.ExpectedOutput, output) + } + } +} diff --git a/kubernetes/structures_container.go b/kubernetes/structures_container.go index f7e7e975fe..d54641b84a 100644 --- a/kubernetes/structures_container.go +++ b/kubernetes/structures_container.go @@ -1028,14 +1028,3 @@ func expandContainerResourceRequirements(l []interface{}) (*v1.ResourceRequireme return obj, nil } - -func flattenObjectRef(in *v1.ObjectReference) []interface{} { - att := make(map[string]interface{}) - if in.Name != "" { - att["name"] = in.Name - } - if in.Namespace != "" { - att["namespace"] = in.Namespace - } - return []interface{}{att} -} diff --git a/kubernetes/structures_container_test.go b/kubernetes/structures_container_test.go index 6d4dc9af82..4b9fdb54a7 100644 --- a/kubernetes/structures_container_test.go +++ b/kubernetes/structures_container_test.go @@ -212,35 +212,3 @@ func TestExpandConfigMapKeyRef(t *testing.T) { } } } - -func TestFlattenObjectRef(t *testing.T) { - cases := []struct { - Input *v1.ObjectReference - ExpectedOutput []interface{} - }{ - { - &v1.ObjectReference{ - Name: "demo", - Namespace: "default", - }, - []interface{}{ - map[string]interface{}{ - "name": "demo", - "namespace": "default", - }, - }, - }, - { - &v1.ObjectReference{}, - []interface{}{map[string]interface{}{}}, - }, - } - - for _, tc := range cases { - output := flattenObjectRef(tc.Input) - if !reflect.DeepEqual(output, tc.ExpectedOutput) { - t.Fatalf("Unexpected output from flattener.\nExpected: %#v\nGiven: %#v", - tc.ExpectedOutput, output) - } - } -}