Resolve resource reference(initial PR) #2337

gemmahou · 2024-07-22T23:58:38Z

Change description

Rebased on #2352

Move resolve<resource> functions into apis/refs/ package.
Make GetResourceID, GetLocation helper functions.

TODO:
Draft a tool to auto-generate the resolve logic.

Note:
I only include ComputeNetworkRef and ComputeTargetHTTPProxyRef in this PR. More references will be required for ComputeForwardingRule.

Tests you have done

Run make ready-pr to ensure this PR is ready for review.
Perform necessary E2E testing for changed resources.

yuwenma · 2024-07-23T12:57:48Z

apis/refs/v1beta1/resourceref.go

I think we want to get rid of a generic resourceRef but using more dedicated kind. Any reasons to switch it back?

by reading the code, I think this is just a naming thing. Maybe move the functions to the computenetoworkref.go file

Removed generic resourceRef

yuwenma · 2024-07-23T13:03:48Z

apis/refs/v1beta1/util.go

Not a big fan of util.go. How about adapter.go (convert from the alpha resourceRef to beta Ref)? And move the GetResourceID and location to another file. And I think the GetResourceID and GetLocation shall not be under the ref dir eventually.

I moved GetResourceID GetLocation and GetProjectID(not resolve project) to helper.go(or maybe a better name), those methods can be reused.

yuwenma · 2024-07-23T13:05:36Z

apis/refs/v1beta1/resourceref.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func ComputeNetworkRef_ConvertToExternal(ctx context.Context, reader client.Reader, src client.Object, ref *v1alpha1.ResourceRef) (*v1alpha1.ResourceRef, error) {


I like this name! And I also think we can actually having our tools to auto-generate this functions when adding new resources from proto!

yuwenma · 2024-07-23T13:06:22Z

apis/refs/v1beta1/resourceref.go

by reading the code, I think this is just a naming thing. Maybe move the functions to the computenetoworkref.go file

justinsb · 2024-07-26T15:32:25Z

pkg/controller/direct/cloudbuild/workerpool_controller.go

@@ -90,7 +91,7 @@ func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *u
 	}

 	// Get GCP Project
-	projectRef, err := refs.ResolveProject(ctx, reader, obj, obj.Spec.ProjectRef)
+	projectRef, err := resolverefs.ResolveProjectRef(ctx, reader, obj, obj.Spec.ProjectRef)


So I moved the resolver functions into the API package recently. The intent was that we could then have the resolvers implement an interface, and make a lot of this generic.

WDYT - can we put the resolver functions into the refs package? (We could even just add a ResolveRef function to each ref in this PR?)

Regarding the placement of resolver functions, I don't have a strong preference. It could be api/refs/projectref.go or direct/refs/projectref.go. I think we should maintain consistency in the future. I'm open to moving the resolver functions to api/refs if that's preferable.

My main concern here is avoiding placing these functions in individual controller packages, for example direct/compute/networkref.go. Some references are used in multiple resources, like ComputeNetWork, which is referenced by CloudBuildWorkerPool and ComputeForwardingRule (and potentially others).

justinsb · 2024-07-26T15:34:37Z

pkg/controller/direct/resolverefs/helper.go

+	return location, nil
+}
+
+func GetProjectID(ctx context.Context, reader client.Reader, obj *unstructured.Unstructured) (string, error) {


So this was called something like "ResolveProjectID" as a hint that we might have to go chase down some references etc. I don't mind renaming it if it's confusing, but maybe add a comment to the function?

I reverted it back to ResolveProjectID, will address in another PR if needed.

justinsb · 2024-07-26T15:36:09Z

pkg/controller/direct/resolverefs/computetargethttpproxyref.go

+		if len(tokens) == 5 && tokens[0] == "projects" && tokens[2] == "global" && tokens[3] == "targetHttpProxies" {
+			return &ComputeTargetHTTPProxy{
+				Project:                  tokens[1],
+				ComputeTargetHTTPProxyID: tokens[4]}, nil


Looks like we should set Location == global here, so it round trips?

Thanks, fixed!

justinsb · 2024-07-26T15:38:19Z

pkg/controller/direct/resolverefs/projectref.go

+	ProjectID string
+}
+
+// ResolveProjectRef will resolve a ProjectRef to a Project, with the ProjectID.


TBD if this should actually just return a ProjectRef, to follow the pattern we seem to have built since. But no need to address in this PR (unless you want to)

I reverted it back, will address in another PR if needed.

gemmahou · 2024-07-29T20:33:39Z

Chatted with @yuwenma offline and reverted the function name change.

I will have a followup PR to resolve other refs(i.e. compute address, subnetwork etc), and a tool to auto-generate the resolver functions.

yuwenma

/lgtm

yuwenma · 2024-07-29T22:31:04Z

apis/refs/v1beta1/helper.go

@@ -30,3 +29,16 @@ func GetResourceID(u *unstructured.Unstructured) (string, error) {
 	}
 	return resourceID, nil
 }
+
+// TODO(yuhou): Location can be optional. Use provider default location when it's unset.
+func GetLocation(obj *unstructured.Unstructured) (string, error) {


If only called once, it is better to put the code inside ResolveTargetHTTPProxy or change this to a private function, in case it is misused by other functions (more context, I think more work needs to be done for location so it may not be ready to share yet)

Done! I think we'll eventually make GetLocation a helper function because it will be used in resolving more references. I agree that we need to do so when it's fully functional.

google-oss-prow · 2024-07-30T18:38:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb]

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

justinsb · 2024-07-30T18:39:01Z

Ooops ... didn't realize @yuwenma was looking at this also.

/hold

(for yuwen's reviews)

yuwenma · 2024-07-30T20:00:40Z

/hold cancel
Looks good! Thank you for the changes!

gemmahou assigned justinsb and acpana Jul 22, 2024

gemmahou force-pushed the resolve-ref branch from dc273f6 to 618ed6d Compare July 23, 2024 00:00

yuwenma reviewed Jul 23, 2024

View reviewed changes

gemmahou changed the title ~~feat: Resolve resource reference~~ WIP: feat: Resolve resource reference Jul 24, 2024

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

gemmahou force-pushed the resolve-ref branch 2 times, most recently from 0e8e619 to 009d76b Compare July 25, 2024 18:25

gemmahou mentioned this pull request Jul 25, 2024

fix: Update direct forwarding rule API to use v1beta1 refs #2352

Merged

2 tasks

resolve resourceRef

8a43c31

gemmahou force-pushed the resolve-ref branch from 009d76b to 40893a5 Compare July 25, 2024 22:47

gemmahou changed the title ~~WIP: feat: Resolve resource reference~~ feat: Resolve resource reference Jul 25, 2024

google-oss-prow bot removed the do-not-merge/work-in-progress label Jul 25, 2024

resolve refs and refactor code

6408587

gemmahou force-pushed the resolve-ref branch from 40893a5 to 6408587 Compare July 25, 2024 23:23

gemmahou mentioned this pull request Jul 26, 2024

feat: Direct controller for ComputeForwardingRule #2364

Merged

6 tasks

justinsb reviewed Jul 26, 2024

View reviewed changes

gemmahou force-pushed the resolve-ref branch 6 times, most recently from d69b507 to 48a7303 Compare July 29, 2024 20:27

gemmahou changed the title ~~feat: Resolve resource reference~~ Resolve resource reference(initial PR) Jul 29, 2024

yuwenma reviewed Jul 30, 2024

View reviewed changes

google-oss-prow bot assigned yuwenma Jul 30, 2024

google-oss-prow bot added the lgtm label Jul 30, 2024

move resolver functions back into api/refs

b184f64

gemmahou force-pushed the resolve-ref branch from 48a7303 to b184f64 Compare July 30, 2024 17:43

google-oss-prow bot removed the lgtm label Jul 30, 2024

justinsb approved these changes Jul 30, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Jul 30, 2024

google-oss-prow bot added the approved label Jul 30, 2024

google-oss-prow bot added the do-not-merge/hold label Jul 30, 2024

google-oss-prow bot removed the do-not-merge/hold label Jul 30, 2024

google-oss-prow bot merged commit 2b0f384 into GoogleCloudPlatform:master Jul 30, 2024
13 checks passed

gemmahou deleted the resolve-ref branch July 30, 2024 21:42

gemmahou mentioned this pull request Aug 7, 2024

feat: add kmskeyring ref #2438

Merged

yuwenma added this to the 1.121 milestone Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve resource reference(initial PR) #2337

Resolve resource reference(initial PR) #2337

gemmahou commented Jul 22, 2024 •

edited

Loading

yuwenma Jul 23, 2024

yuwenma Jul 23, 2024

gemmahou Jul 25, 2024

yuwenma Jul 23, 2024

gemmahou Jul 25, 2024 •

edited

Loading

yuwenma Jul 23, 2024

yuwenma Jul 23, 2024

justinsb Jul 26, 2024 •

edited

Loading

gemmahou Jul 26, 2024 •

edited

Loading

justinsb Jul 26, 2024

gemmahou Jul 26, 2024

justinsb Jul 26, 2024

gemmahou Jul 26, 2024

justinsb Jul 26, 2024

gemmahou Jul 26, 2024

gemmahou commented Jul 29, 2024

yuwenma left a comment

yuwenma Jul 29, 2024

gemmahou Jul 30, 2024

google-oss-prow bot commented Jul 30, 2024

justinsb commented Jul 30, 2024

yuwenma commented Jul 30, 2024

Resolve resource reference(initial PR) #2337

Resolve resource reference(initial PR) #2337

Conversation

gemmahou commented Jul 22, 2024 • edited Loading

Change description

Tests you have done

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

gemmahou Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

gemmahou Jul 26, 2024 • edited Loading

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

gemmahou commented Jul 29, 2024

yuwenma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-prow bot commented Jul 30, 2024

justinsb commented Jul 30, 2024

yuwenma commented Jul 30, 2024

gemmahou commented Jul 22, 2024 •

edited

Loading

gemmahou Jul 25, 2024 •

edited

Loading

justinsb Jul 26, 2024 •

edited

Loading

gemmahou Jul 26, 2024 •

edited

Loading