Fix perpetual diff in claim_ref #1227

dak1n1 · 2021-04-12T22:43:11Z

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.

Description

Acceptance tests

Have you added an acceptance test for the functionality being added?
Have you run the acceptance tests on this branch?

Output from acceptance testing:

go test -run TestFlattenObjectRef '-timeout=30s' '-parallel=4' /home/dakini/go/src/github.com/hashicorp/terraform-provider-kubernetes/kubernetes 
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   0.036s

=== RUN   TestAccKubernetesPersistentVolume_minimal
--- PASS: TestAccKubernetesPersistentVolume_minimal (2.53s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   2.578s


=== RUN   TestAccKubernetesPersistentVolume_hostpath_claimRef
--- PASS: TestAccKubernetesPersistentVolume_hostpath_claimRef (13.56s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   13.604s

Release Note

Release note for CHANGELOG:

Fix perpetual diff in claim_ref

References

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

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.

dak1n1 · 2021-04-12T22:44:47Z

kubernetes/resource_kubernetes_persistent_volume.go

 										Elem:        schema.TypeString,
-										Required:    true,
+										Optional:    true,
+										Default:     "default",
 									},


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.

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

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.

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.

dak1n1 · 2021-04-12T22:45:22Z

kubernetes/resource_kubernetes_persistent_volume_claim_test.go

-    ignore_changes = [
-      spec[0].claim_ref,
-    ]
-  }
 }


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

dak1n1 · 2021-04-12T22:47:32Z

kubernetes/structure_persistent_volume_spec.go

 	}
-	o := v[0].(map[string]interface{})
+	o := l[0].(map[string]interface{})
 	return &v1.ObjectReference{


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

dak1n1 · 2021-04-12T22:48:39Z

kubernetes/structures_container.go

-		att["namespace"] = in.Namespace
-	}
-	return []interface{}{att}
-}


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

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).

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.

dak1n1 · 2021-04-12T23:04:09Z

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


_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.

alexsomesan

Thanks for the nice resolution here, @dak1n1 !

I left some thoughts here and there, but most importantly I realised that the current schema for claimRef seems to be incomplete.

I think this might be a good opportunity to fix that too, before users start pointing it out.

alexsomesan · 2021-04-13T13:17:17Z

kubernetes/resource_kubernetes_persistent_volume.go

 										Elem:        schema.TypeString,
-										Required:    true,
+										Optional:    true,
+										Default:     "default",
 									},


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

alexsomesan · 2021-04-13T13:32:29Z

kubernetes/structures_container.go

-		att["namespace"] = in.Namespace
-	}
-	return []interface{}{att}
-}


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).

alexsomesan

Looks good. Thanks for sorting this out!

ghost · 2021-05-14T05:10:00Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

ghost added size/L labels Apr 12, 2021

dak1n1 commented Apr 12, 2021

View reviewed changes

dak1n1 requested a review from a team April 12, 2021 22:49

dak1n1 commented Apr 12, 2021

View reviewed changes

alexsomesan reviewed Apr 13, 2021

View reviewed changes

alexsomesan approved these changes Apr 13, 2021

View reviewed changes

dak1n1 merged commit f9aeea9 into hashicorp:main Apr 13, 2021

ghost locked as resolved and limited conversation to collaborators May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix perpetual diff in claim_ref #1227

Fix perpetual diff in claim_ref #1227

dak1n1 commented Apr 12, 2021

dak1n1 Apr 12, 2021

alexsomesan Apr 13, 2021

dak1n1 Apr 13, 2021

alexsomesan Apr 13, 2021

dak1n1 Apr 12, 2021

dak1n1 Apr 12, 2021

dak1n1 Apr 12, 2021

alexsomesan Apr 13, 2021

dak1n1 Apr 13, 2021

dak1n1 Apr 12, 2021

alexsomesan left a comment

alexsomesan Apr 13, 2021

alexsomesan Apr 13, 2021

alexsomesan left a comment

ghost commented May 14, 2021

Fix perpetual diff in claim_ref #1227

Fix perpetual diff in claim_ref #1227

Conversation

dak1n1 commented Apr 12, 2021

Description

Acceptance tests

Release Note

References

Community Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsomesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsomesan left a comment

Choose a reason for hiding this comment

ghost commented May 14, 2021