feat: A new parent interface #3234

yuwenma · 2024-11-21T07:18:44Z

Change description

Fixes #

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-11-21T07:22:36Z

apis/common/parent/projectlocation.go

+
+var _ Parent = &ProjectAndLocationParent{}
+
+type ProjectAndLocationParent struct {


We can merge ProjectAndLocationParent and ProjectAndLocationRef, it makes this file shorter. But it adds additional complexity to the controller on error handling, and the order of the function calls to set the external.

I think we want Foo, FooRef and FooIdentity. I think this is FooIdentity, and ProjectAndLocationRef is FooRef

justinsb · 2024-11-21T13:23:08Z

apis/bigqueryconnection/v1beta1/connection_reference.go

-		}
-		if location != actualLocation {
-			return nil, fmt.Errorf("spec.location changed, expect %s, got %s", location, actualLocation)
+		if err := desiredParent.MatchActual(actualParent); err != nil {


Suggest just comparing String() of the ID

This MatchActual is more for KCC users.
My main concern about comparing String is that it cannot give users enough information to help them fix their bad config.
For example, the parent is build from spec.ProjectRef.External and spec.Location (for some other resources it may also have spec. FooRef.External spec.BarRef.External), it is unclear for users to know which field is misconfigured by just seeing something likeexpected format projects/<projectID>/location/<lcoation>, got ....

One improvements we can do is spec.parent.projectRef (I think we discussed this as well, but we have to make the API backward compatible)

Well I think I would make the message "Can't change object identity; existing identity is %q, new identity is %q" so it would be pretty easy to figure out what has changed, but ... I can send a PR on top of this - let's get this in!

justinsb · 2024-11-21T13:23:52Z

apis/bigqueryconnection/v1beta1/connection_reference.go

@@ -88,36 +87,13 @@ type BigQueryConnectionConnectionRef struct {
 	Namespace string `json:"namespace,omitempty"`
 }

-func parseExternal(external string) (string, string, string, error) {
+func ParseConnectionExternal(external string) (*parent.ProjectAndLocationParent, string, error) {


Suggest this should return (*ConnectionIdentity,error) (and not the parent split out)

Sure! (It used to be handy to split them because we don't have the identity. 😄 )

justinsb · 2024-11-21T13:24:22Z

apis/bigqueryconnection/v1beta1/connection_types.go

 // BigQueryConnectionConnectionSpec defines the desired state to connect BigQuery to external resources
 // +kcc:proto=google.cloud.bigquery.connection.v1.Connection
 type BigQueryConnectionConnectionSpec struct {
-	Parent `json:",inline"`
+	*parent.ProjectAndLocationRef `json:",inline"`


I thought you wanted to embed ConnectionIdentity here instead

justinsb · 2024-11-21T13:26:50Z

apis/common/parent/interface.go

+// ParentBuilder builds a Parent object from a ParentRef.
+// - ParentRef is the Config Connector API reference for identifying a resource's logical parent.
+// - The Parent object provides helper functions for parent-related logic in direct reconciliation.
+type ParentBuilder interface {


Maybe the krm object should implement ResolveIdentity(ctx context.Context, reader client.Reader) (Identity, error)

If we want Identity to be typed, it would have to be something like this:

type ObjectWithIdentity[T] interface { ResolveIdentity(ctx contetx.Context, reader client.Reader) (*T, error) }

justinsb · 2024-11-21T13:27:59Z

apis/common/parent/project.go

+// Builds a the ProjectParent from ProjectRef.
+// If `projectRef.external` is given, parse projectID from External, otherwise find the ConfigConnector project
+// according to `projectRef.name` and `projectRef.namespace`.
+func (p *ProjectRef) Build(ctx context.Context, reader client.Reader, othernamespace string, parent Parent) error {


I suspect we don't need this function. We need a function that resolves a ProjectRef to a ProjectIdentity, and ProjectIdentity should have a function that can return the ProjectID. It might be rename this function to map from ProjectRef -> ProjectIdentity.

We would have 3 concepts: Foo, FooRef and FooIdentity

FooIdentity is essentially the fully-qualified name, I don't know if we should name it FooLink or something like that instead, but I do think that just using these 3 concepts will simplify our code a lot.

justinsb · 2024-11-21T13:37:54Z

...sourcedefinition_bigqueryconnectionconnections.bigqueryconnection.cnrm.cloud.google.com.yaml

@@ -293,10 +289,6 @@ spec:
                    description: The `projectID` field of a project, when not managed
                      by Config Connector.
                    type: string
-                  kind:


Sadly we can't do this, this is a breaking API change. We can't break resources that specify kind, even though we (I) introduced it by mistake.

justinsb · 2024-11-21T13:38:45Z

pkg/controller/direct/bigqueryconnection/connection_controller.go

@@ -76,7 +75,7 @@ func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *u
 	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &obj); err != nil {
 		return nil, fmt.Errorf("error converting to %T: %w", obj, err)
 	}
-	connectionRef, err := krm.NewBigQueryConnectionConnectionRef(ctx, reader, obj)
+	connectionIdentity, err := krm.NewConnectionIdentity(ctx, reader, obj)


This is the one where maybe we could call obj.ResolveIdentity ... maybe :-)

justinsb · 2024-11-21T13:44:36Z

pkg/controller/direct/bigqueryconnection/connection_controller.go

 	req := &pb.CreateConnectionRequest{
-		Parent:     parent,
+		Parent:     a.id.Parent.String(),


I like this approach, but one thing we will have to be careful of is that we have a "partial ID" here, ResourceID is empty, the parent fields are set. There are going to be bugs because of this.

It might be easier to resolve the parent directly here, perhaps with a method on the embedded ProjectAndLocationID in BigQueryConnectionID

justinsb · 2024-11-21T13:46:46Z

pkg/controller/direct/bigqueryconnection/connection_controller.go

@@ -243,14 +223,14 @@ func (a *Adapter) Create(ctx context.Context, createOp *directbase.CreateOperati
 		return mapCtx.Err()
 	}

-	tokens := strings.Split(created.Name, "/")
-	parent, err = a.id.Parent()
+	_, name, err := krm.ParseConnectionExternal(created.Name)


Looking at this, I think it would read well if it was a method on BigQueryConnectionID, like this:

name := &BigQueryConnectionID{} if err := name.ParseSelfLink(created.Name); err != nil { ....

justinsb · 2024-11-21T13:49:29Z

pkg/controller/direct/bigqueryconnection/connection_controller.go

 	}
+	obj.Spec.ResourceID = &id


If we embed ID into Spec, I think these three lines will just be obj.Spec.ID = id

justinsb · 2024-11-22T23:20:58Z

apis/bigqueryconnection/v1beta1/connection_reference.go

-		}
-		if location != actualLocation {
-			return nil, fmt.Errorf("spec.location changed, expect %s, got %s", location, actualLocation)
+		if err := desiredParent.MatchActual(actualParent); err != nil {


Well I think I would make the message "Can't change object identity; existing identity is %q, new identity is %q" so it would be pretty easy to figure out what has changed, but ... I can send a PR on top of this - let's get this in!

justinsb · 2024-11-22T23:54:21Z

apis/bigqueryconnection/v1beta1/connection_reference.go

-		return nil, fmt.Errorf("cannot resolve project")
-	}
-	// Get location
-	location := obj.Spec.Location

 	// Get desired connection ID from spec
 	desiredID := direct.ValueOf(obj.Spec.ResourceID)


Do we need to fallback to name here?

justinsb · 2024-11-23T00:00:49Z

So I think two blockers, and everything else we can tackle in follow-up PRs:

I think we need a fallback to name?
I don't think you can remove the kind field

yuwenma · 2024-11-25T18:02:01Z

So I think two blockers, and everything else we can tackle in follow-up PRs:

I think we need a fallback to name?

I don't think you can remove the kind field

I reverted the changes to BQCC. So this PR only contains the interface and the implemented ProjectParent and ProjectAndLocationParent

justinsb · 2024-11-26T19:37:55Z

/approve
/lgtm

google-oss-prow · 2024-11-27T19:23: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

yuwenma commented Nov 21, 2024

View reviewed changes

yuwenma force-pushed the code-talks-parent branch from 08992fd to 816f435 Compare November 21, 2024 07:23

yuwenma assigned justinsb Nov 21, 2024

justinsb reviewed Nov 21, 2024

View reviewed changes

justinsb mentioned this pull request Nov 21, 2024

chore: pass in just enough args to ResolveProject #3233

Merged

2 tasks

justinsb reviewed Nov 23, 2024

View reviewed changes

yuwenma force-pushed the code-talks-parent branch from 816f435 to cfa3a8d Compare November 25, 2024 18:01

yuwenma force-pushed the code-talks-parent branch from cfa3a8d to a0937fe Compare November 26, 2024 18:03

justinsb approved these changes Nov 26, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Nov 26, 2024

google-oss-prow bot added the approved label Nov 26, 2024

yuwenma force-pushed the code-talks-parent branch from a0937fe to 9075f8a Compare November 26, 2024 19:39

google-oss-prow bot removed the lgtm label Nov 26, 2024

yuwenma force-pushed the code-talks-parent branch from 9075f8a to 1a3b58c Compare November 27, 2024 17:02

justinsb approved these changes Nov 27, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Nov 27, 2024

feat: A new parent interface

257a0e5

yuwenma force-pushed the code-talks-parent branch from 1a3b58c to 257a0e5 Compare November 27, 2024 17:32

google-oss-prow bot removed the lgtm label Nov 27, 2024

improve generate-crds

56046b9

justinsb approved these changes Nov 27, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Nov 27, 2024

google-oss-prow bot merged commit d192546 into GoogleCloudPlatform:master Nov 27, 2024
18 checks passed

yuwenma added this to the 1.126 milestone Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: A new parent interface #3234

feat: A new parent interface #3234

yuwenma commented Nov 21, 2024

yuwenma Nov 21, 2024

justinsb Nov 21, 2024

justinsb Nov 21, 2024

yuwenma Nov 21, 2024 •

edited

Loading

justinsb Nov 22, 2024

justinsb Nov 21, 2024

yuwenma Nov 21, 2024 •

edited

Loading

justinsb Nov 21, 2024

justinsb Nov 21, 2024

justinsb Nov 21, 2024

justinsb Nov 21, 2024

justinsb Nov 21, 2024

justinsb Nov 21, 2024

justinsb Nov 21, 2024

justinsb Nov 21, 2024

justinsb Nov 22, 2024

justinsb Nov 22, 2024

justinsb commented Nov 23, 2024

yuwenma commented Nov 25, 2024

justinsb commented Nov 26, 2024

google-oss-prow bot commented Nov 27, 2024


		var _ Parent = &ProjectAndLocationParent{}

		type ProjectAndLocationParent struct {

feat: A new parent interface #3234

feat: A new parent interface #3234

Conversation

yuwenma commented Nov 21, 2024

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

yuwenma Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenma Nov 21, 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

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

justinsb commented Nov 23, 2024

yuwenma commented Nov 25, 2024

justinsb commented Nov 26, 2024

google-oss-prow bot commented Nov 27, 2024

yuwenma Nov 21, 2024 •

edited

Loading

yuwenma Nov 21, 2024 •

edited

Loading