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.

Sign up for GitHub

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

Jump to bottom

Fix perpetual diff in claim_ref #1227

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Fix perpetual diff in claim_ref #1227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions kubernetes/resource_kubernetes_persistent_volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all PVCs are namespaced, I figured we should put some guardrails in here to catch cases where users accidentally leave it blank. In my testing, the Kubernetes API allows the ClaimRef to be blank, but it will never bind until the correct namespace is specified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good reasoning.

I wasn't sure if namespace is actually required in the Kubernetes schema or not, so I checked in the schema reference docs. It's indeed not required. However, I noticed that claimRef is actually defined as type ObjectReference. That type has a bunch of other attributes that are missing in our schema (were also missing in the original PR).
I wonder if we should take this opportunity to backfill those as well so that claimRef actually follows the official API schema.
I also missed them when reviewing the original PR.

Complete set of attributes of claimRef described here: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.21/#objectreference-v1-core

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I noticed all those extra attributes coming up inside the PV during testing, and in the API docs. However, we only need name and namespace for this feature. If there is a need for the other attributes later, like if users need to use an ObjectReference outside of a PV, I think it makes sense to add these things as people ask for it. Then we can evaluate the usefulness of each feature that users are asking for, and prioritize accordingly. So far it seems we've implemented all the useful types without creating an ObjectReference (we have Secret, CronJob, ServiceAccount). I imagine it would involve some refactoring to go back and change how those work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good then. I'll document the missing ObjectReference fields in a separate issue and we can get back to them when there is demand.

"name": {
Type: schema.TypeString,
Expand Down
5 changes: 0 additions & 5 deletions kubernetes/resource_kubernetes_persistent_volume_claim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,11 +716,6 @@ resource "kubernetes_persistent_volume" "test" {
}
}
}
lifecycle {
ignore_changes = [
spec[0].claim_ref,
]
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section was causing the test to pass, when it shouldn't have. It masked the perpetual diff.

resource "kubernetes_persistent_volume_claim" "test" {
wait_until_bound = true
Expand Down
144 changes: 124 additions & 20 deletions kubernetes/resource_kubernetes_persistent_volume_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,29 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestAccKubernetesPersistentVolume_minimal(t *testing.T) {
var conf api.PersistentVolume
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_minimal tests are a type I started adding a few months back. The goal with these is just to provision the bare minimum amount of infrastructure, with as few options set as possible, which should work in local testing and on cloud environments. These kinds of tests will tell you if the basic resource provisioning is working properly, and if the default values for the resource are working properly.

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)
Expand Down Expand Up @@ -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{
Expand All @@ -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),
),
},
},
Expand Down Expand Up @@ -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"
Expand All @@ -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 {
Expand Down
19 changes: 15 additions & 4 deletions kubernetes/structure_persistent_volume_spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed the variable name here to be consistent with the rest of the file.

Name: o["name"].(string),
Namespace: o["namespace"].(string),
Expand Down
40 changes: 40 additions & 0 deletions kubernetes/structure_persistent_volume_spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
11 changes: 0 additions & 11 deletions kubernetes/structures_container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved because it's only used in the volume schema.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically v1.ObjectReference is used in a bunch of other resources (listed here: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.21/#objectreference-v1-core).

We might want to keep this and think about consolidating all those use-cases to re-use the same schema snippet and flatten / expand code (not in this PR, of course).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I did see that in the API docs, but in our codebase it's only used for the persistent volume schema, so I thought it made sense to keep it in that file until it's used more broadly. We may never use it outside of the persistent volume schema. But even if we do, the containers schema isn't quite the right place for it either, since it wouldn't be used in containers.

32 changes: 0 additions & 32 deletions kubernetes/structures_container_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}