[WIP] Unexport 'status' field in K8s.Resource #1403

maqiuyujoyce · 2024-03-20T01:17:53Z

Change description

Unexported status field in K8s.Resource and updated the marshal/unmarshal logic to ensure the unexported status field is still passed along.

Tests you have done

[N/A] Run make ready-pr to ensure this PR is ready for review.
Perform necessary E2E testing for changed resources.

go test -timeout 1200s -v -tags=integration ./pkg/controller/dynamic/ -test.run TestCreateNoChangeUpdateDelete -run-tests pubsubtopic

google-oss-prow · 2024-03-20T01:18:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from maqiuyujoyce. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

justinsb · 2024-03-25T13:33:07Z

FYI, I don't think there's any reason to abandon this. Using methods is generally much better at preventing errors than directly accessing fields.

justinsb · 2024-03-25T13:38:20Z

pkg/k8s/resource_test.go

@@ -83,147 +83,147 @@ func TestResource_IsResourceIDConfigured(t *testing.T) {
 	}
 }

-func TestResource_IsSpecOrStatusUpdateRequired(t *testing.T) {


What broke here?

The testdata construction is tricky with unexported field status. Added additional fields to make it work.

justinsb · 2024-03-25T13:40:02Z

pkg/krmtotf/fetchlivestate_test.go

@@ -30,666 +30,768 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )

-func TestWithFieldsPresetForRead(t *testing.T) {


Same q: something broke in this test?

Nope, see the answer above.

justinsb · 2024-03-25T13:41:53Z

pkg/krmtotf/resource.go

+func getObservedStateFromStatus(status map[string]interface{}) map[string]interface{} {
+	observedState, exists, _ := unstructured.NestedMap(status, k8s.ObservedStateFieldName)
+	if !exists {
+		observedState = make(map[string]interface{})


Probably safer to just return nil, lest someone try to write to it

justinsb · 2024-03-25T13:45:15Z

pkg/krmtotf/tftokrm.go

-			status["observedState"] = observedState
+		observedFields := resolveObservedFields(resource, krmState)
+		if len(observedFields) > 0 {
+			// Merge the observed fields into the observed state.


Should we merge or just build it anew?

Just to clarify, what is "build it anew" in this context? Creating a new map?

Observed fields are also part of the observed state but manually configured for spec fields in service mappings (#1023, #1059) so I think merging should be the right behavior. Glad to adjust if you are suggesting a simpler implementation.

maqiuyujoyce · 2024-03-26T04:10:23Z

FYI, I don't think there's any reason to abandon this. Using methods is generally much better at preventing errors than directly accessing fields.

In order to make it work E2E, marshaling/unmarshaling logic for k8s.Resource, krmtotf.Resource, dcl.Resource, and unstructured.Unstructured need to be updated. This impacts every single resource in admission and reconciliation, and also in the import/export tool.

I'm pretty confident about #1353 as the scope of it is quite limited (TF-based resources only, status field only, will likely only impact new resources), and we have decent unit tests, mock tests, and integration tests to cover most use cases. But for this PR, the scope of the change is way larger, and I'm quite concerned about merging the PR before we have healthy integration tests with good coverage.

I'm proposing we don't include it for 1.115 milestone and only merge the refactored code after we have the existing integration test back to healthy / set up a new integration test flow with good coverage.

justinsb · 2024-03-27T01:34:44Z

I propose we get the observedStatus work into 1.116.

Then we can cut the 1.115 branch this week (and give it a few days to stabilize). Big change there will be AlloyDB, it looks like.

This does presuppose that we are able to cut 1.114.1, I'm working on that now.

marshaling/unmarshaling logic for k8s.Resource, krmtotf.Resource, dcl.Resource, and unstructured.Unstructured need to be updated.

What does this look like? Have you tried implementing json.Unmarshaler / Json.Marshaler? The trick is normally to assemble whatever you want the object to look like (often in a map[string]any), and then call json.Marshal on it.

Anyway, I popped this back into the 1.116 milestone, but mostly because I hadn't seen your comments when going through. I don't mind how we tackle the safety from a code-perspective, we could add tests as I started doing in #1438

maqiuyujoyce · 2024-03-28T23:00:23Z

What does this look like?

Here is one example: https://github.com/GoogleCloudPlatform/k8s-config-connector/pull/1403/files#diff-565e4cccca460065f11c5e41024f882b8ea7a8cb9eeff6f0e0959b96b75dfd63R414

Have you tried implementing json.Unmarshaler / Json.Marshaler?

In our codebase, a lot of the marshaling/unmarshaling logic are done by calling util.Marshal() (code here:https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/pkg/util/util.go#L22). I did a search, and found it is called at 45 places, and not every time it is used to unmarshall to a k8s.Resource.

I tried to add special logic to handle k8s.Resource in util.Marshal(), but it gives me a circular dependency issue. Also the raw object is not always a map, but can be any inherited type of k8s.Resource. I consider this task a quick win, so worked around it by manually identifying all the places where k8s.Resource is the target type to unmarshall.

I'm okay with merge the change as is and let our internal integration tests to verify whether the change makes sense.

maqiuyujoyce mentioned this pull request Mar 20, 2024

Move computed fields to status.observedState for new CRDs #1353

Merged

2 tasks

justinsb reviewed Mar 25, 2024

View reviewed changes

justinsb added this to the 1.115 milestone Mar 26, 2024

maqiuyujoyce force-pushed the 202403-unexport-resource-status branch from be40617 to 9956a8a Compare March 26, 2024 03:49

maqiuyujoyce changed the title ~~[To Be Abandoned] Unexport k8s.Resource.Status to verify if there is anything missing in #1353~~ [To Be Abandoned?] Unexport 'status' field in K8s.Resource Mar 26, 2024

Unexport 'status' field in K8s.Resource

bec99c8

maqiuyujoyce force-pushed the 202403-unexport-resource-status branch from 9956a8a to bec99c8 Compare March 26, 2024 03:58

Add nil check when marshaling status as unstructured

cc1148e

maqiuyujoyce mentioned this pull request Mar 26, 2024

Return nil when observedState is not found in status #1430

Merged

1 task

maqiuyujoyce changed the title ~~[To Be Abandoned?] Unexport 'status' field in K8s.Resource~~ [WIP] Unexport 'status' field in K8s.Resource Mar 26, 2024

google-oss-prow bot added the do-not-merge/work-in-progress label Mar 26, 2024

maqiuyujoyce modified the milestones: 1.115, Future Mar 26, 2024

justinsb modified the milestones: Future, 1.116 Mar 27, 2024

justinsb modified the milestones: 1.116, 1.118 Apr 24, 2024

justinsb modified the milestones: 1.118, 1.119 May 15, 2024

yuwenma modified the milestones: 1.119, Future Jun 17, 2024

maqiuyujoyce closed this Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Unexport 'status' field in K8s.Resource #1403

[WIP] Unexport 'status' field in K8s.Resource #1403

maqiuyujoyce commented Mar 20, 2024 •

edited

Loading

google-oss-prow bot commented Mar 20, 2024

justinsb commented Mar 25, 2024

justinsb Mar 25, 2024

maqiuyujoyce Mar 26, 2024

justinsb Mar 25, 2024

maqiuyujoyce Mar 26, 2024

justinsb Mar 25, 2024

maqiuyujoyce Mar 26, 2024

justinsb Mar 25, 2024

maqiuyujoyce Mar 26, 2024

maqiuyujoyce commented Mar 26, 2024 •

edited

Loading

justinsb commented Mar 27, 2024 •

edited

Loading

maqiuyujoyce commented Mar 28, 2024

[WIP] Unexport 'status' field in K8s.Resource #1403

[WIP] Unexport 'status' field in K8s.Resource #1403

Conversation

maqiuyujoyce commented Mar 20, 2024 • edited Loading

Change description

Tests you have done

google-oss-prow bot commented Mar 20, 2024

justinsb commented Mar 25, 2024

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

maqiuyujoyce commented Mar 26, 2024 • edited Loading

justinsb commented Mar 27, 2024 • edited Loading

maqiuyujoyce commented Mar 28, 2024

maqiuyujoyce commented Mar 20, 2024 •

edited

Loading

maqiuyujoyce commented Mar 26, 2024 •

edited

Loading

justinsb commented Mar 27, 2024 •

edited

Loading