feat: add direct controller for BigQueryDataTransferConfig #2688

jingyih · 2024-09-12T20:29:48Z

jingyih

@yuwenma I added some notes on special handling for service generated ID.

jingyih · 2024-09-13T01:22:18Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	// Get ResourceID
+	resourceID := direct.ValueOf(obj.Spec.ResourceID)
+	if resourceID == "" {
+		serviceGeneratedID, err := parseServiceGeneratedIDFromExternalRef(obj)


resource ID is service generated. It could be "" prior to resource creation.

jingyih · 2024-09-13T01:22:57Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	if a.id == nil {
+		return false, nil
+	}
+	if a.id.transferConfigID == "" { // resource ID is not yet generated by the GCP service


special handling for finding resource prior to being created.

jingyih · 2024-09-13T01:23:16Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	if err != nil {
+		return fmt.Errorf("error converting %s to BigQueryDataTransferConfigIdentity: %w", created.Name, err)
+	}
+	a.id.transferConfigID = serviceGeneratedID


need to write the service generated ID back

jingyih · 2024-09-13T01:23:40Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	if mapCtx.Err() != nil {
+		return mapCtx.Err()
+	}
+	resource.Name = a.id.FullyQualifiedName() // need to pass service generated ID to GCP API


need to pass the service generated ID when updating the resource.

yuwenma · 2024-09-13T01:28:29Z

apis/bigquerydatatransfer/v1alpha1/groupversion_info.go

+
+	GroupVersionKind = schema.GroupVersionKind{
+		Group:   GroupVersion.Group,
+		Version: GroupVersion.Version,
+		Kind:    "BigQueryDataTransferConfig",
+	}


A better format could be adding
var BigQueryDataTransferConfigGVK = GroupVersion.WithKind("BigQueryDataTransferConfig") in the _types.go, which is what the latest template does.

Cool. I am updating the controller template in #2692, so that we don't have a undefined GroupVersionKind right after scaffolding the controller code.

yuwenma · 2024-09-13T01:29:46Z

dev/tasks/generate-crds

@@ -55,7 +55,7 @@ go run ./scripts/crd-tools reflow-descriptions --dir apis/config/crd/

 # excluded_resources are resources that are under development for a direct conversion
 # we don't modify the CRD just yet for those but will in the future
-excluded_resources=("bigquerydatatransferconfig" "securesourcemanagerinstance")


yuwenma · 2024-09-13T01:42:30Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	if obj.Spec.PubSubTopicRef != nil {
+		topic, err := refv1beta1.ResolvePubSubTopic(ctx, reader, obj, obj.Spec.PubSubTopicRef)
+		if err != nil {
+			return nil, err
+		}
+		obj.Spec.PubSubTopicRef.External = topic.String()
+	}
+
+	// Resolve BigQueryDataSet Ref
+	if obj.Spec.DatasetRef != nil {
+		dataset, err := refv1beta1.ResolveBigQueryDataset(ctx, reader, obj, obj.Spec.DatasetRef)
+		if err != nil {
+			return nil, err
+		}
+		obj.Spec.DatasetRef.External = dataset.String()
+	}
+
+	// Resolve KMSCryptoKey Ref
+	if obj.Spec.EncryptionConfiguration != nil {
+		kmsCryptoKeyRef, err := refv1beta1.ResolveKMSCryptoKeyRef(ctx, reader, obj, obj.Spec.EncryptionConfiguration.KmsKeyRef)
+		if err != nil {
+			return nil, err
+		}
+		obj.Spec.EncryptionConfiguration.KmsKeyRef = kmsCryptoKeyRef
+	}


I think we shall move the non-parent depedencies in the create and update calls.

Otherwise the presence of the dependency would block the target resource

IIUC, it mainly affects Find() call? In other words, if the dependencies are not present, Create() and Update() will be blocked anyway, but Find() should not be blocked.

It affects Delete. If a reference is missing, it cannot delete the object.

I don't think this is a blocker issue.

yuwenma · 2024-09-13T01:43:47Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	if a.id == nil {
+		return false, nil
+	}


Mind explaining what this means?

I will remove this block. I think it was from my previous iteration on how to handle the service generated ID. Thanks for spotting!

yuwenma · 2024-09-13T01:47:17Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	}
+
+	// Get ResourceID
+	resourceID := direct.ValueOf(obj.Spec.ResourceID)


Did you miss a case that metadata.name --> resourceID if spec.resourceID is not given? Or is that not a valid case here? I mean the case that KCC acquire a GCP resource that matches the metadata.name.

For resource with service generated ID, I am not sure how useful is metadata.name. It will be ignored during GCP resource creation, and later it cannot be used to match (acquire) the GCP resource. To acquire the GCP resource, we have to give GCP API the service generated ID (i.e., via setting spec.resourceID)

I mean KCC acquire a GCP resource: the BigQuery server creates a Transfer object called "xyz", user then specify KCC metadata.name xyz to acquire that pre-existing resource. Is the service generated ID a valid k8s name?

From my tests, the generated ID is something like "66e60359-0000-2d33-8bc7-c82add80addc". I think k8s name needs to start and end with an alphanumeric character?

Besides, when user try to acquire a GCP resource, I prefer them to explicitly set spec.ResourceID.

kk, looks like all BigQuery service-generated ID is all using the UUID format. Then I'm good with this.

yuwenma · 2024-09-13T01:50:38Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	var id *BigQueryDataTransferConfigIdentity
+	externalRef := direct.ValueOf(obj.Status.ExternalRef)
+	if externalRef == "" {
+		id = BuildID(projectID, location, resourceID)


So the problem here is that since we do not assign metadata.name to resourceID, if spec.resourceID is not given and the GCP object is not created (no status.externalRef), this will always pass in an empty resourceID, which fails the following logics.

yuwenma · 2024-09-13T01:52:28Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+			return nil, fmt.Errorf("BigQueryDataTransferConfig %s/%s has spec.location changed, expect %s, got %s",
+				u.GetNamespace(), u.GetName(), id.location, location)
+		}
+		if id.transferConfigID != resourceID {


I think this only works for acquire but not KCC-created. Because the transferConfigID is server generated and KCC does not write it back to spec.resourceID (and not metadata.name)

In the case of acquiring this resource, I believe we want users to specifically set spec.resourceID to the service generated ID? (as opposed to set metadata.Name to the service generated ID)

Mind elaborating a little bit more on why "we want users to specifically set spec.resourceID to the service generated ID"?

I was referring to
https://screenshot.googleplex.com/6Dcx5KjqfXvuexB

I think it makes sense to have users explicitly set spec.ResourceID to express the intend to "acquire a resource".

Okay, the code is good because the service-generated ID is UUID so it can't be placed in metadata.name

yuwenma · 2024-09-13T01:55:40Z

I feel like maybe we can merge the tool-generated code first, and handle the service-generated ID in a follow up PR, because the change needs to happen in multiple places and I'd like to use that as a coding example.

jingyih · 2024-09-16T22:37:14Z

Rebased to resolve a conflict.

Surface the "serviceAccountName" in BigQueryDataTransferConfig API

yuwenma · 2024-09-18T05:56:13Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	var opts []option.ClientOption
+	if m.config.UserAgent != "" {
+		opts = append(opts, option.WithUserAgent(m.config.UserAgent))
+	}
+	if m.config.HTTPClient != nil {
+		opts = append(opts, option.WithHTTPClient(m.config.HTTPClient))
+	}
+	if m.config.UserProjectOverride && m.config.BillingProject != "" {
+		opts = append(opts, option.WithQuotaProject(m.config.BillingProject))
+	}


Now we have a helper function for this. You can check the controller template

Cool! update the code.

yuwenma · 2024-09-18T05:58:29Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+	}
+
+	// Get ResourceID
+	resourceID := direct.ValueOf(obj.Spec.ResourceID)


I mean KCC acquire a GCP resource: the BigQuery server creates a Transfer object called "xyz", user then specify KCC metadata.name xyz to acquire that pre-existing resource. Is the service generated ID a valid k8s name?

yuwenma · 2024-09-18T06:00:26Z

pkg/controller/direct/bigquerydatatransfer/bigquerydatatransferconfig_controller.go

+			return nil, fmt.Errorf("BigQueryDataTransferConfig %s/%s has spec.location changed, expect %s, got %s",
+				u.GetNamespace(), u.GetName(), id.location, location)
+		}
+		if id.transferConfigID != resourceID {


Mind elaborating a little bit more on why "we want users to specifically set spec.resourceID to the service generated ID"?

yuwenma · 2024-09-19T02:10:51Z

/lgtm
/approve

google-oss-prow · 2024-09-19T02:11:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuwenma

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~~ [yuwenma]

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

doc: update release notes for #2688

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

jingyih force-pushed the BigQueryDataTransferConfig branch 2 times, most recently from 41af159 to 7fa396f Compare September 13, 2024 01:20

jingyih commented Sep 13, 2024

View reviewed changes

yuwenma reviewed Sep 13, 2024

View reviewed changes

jingyih force-pushed the BigQueryDataTransferConfig branch 3 times, most recently from 6d549e4 to da9025a Compare September 13, 2024 23:44

jingyih changed the title ~~WIP: add SciFi controller for BigQueryDataTransferConfig~~ feat: add SciFi controller for BigQueryDataTransferConfig Sep 13, 2024

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

jingyih force-pushed the BigQueryDataTransferConfig branch from da9025a to 0afa310 Compare September 16, 2024 08:57

jingyih added 3 commits September 16, 2024 22:36

feat: generate CRD for BigQueryDataTransferConfig

5c36af9

fix: guard against nil pointer in mappings

57701b9

chore: mark additional required fields in BigQueryDataTransferConfig

a2d2ea3

jingyih force-pushed the BigQueryDataTransferConfig branch from 0afa310 to c3c84d4 Compare September 16, 2024 22:36

jingyih added 7 commits September 17, 2024 01:38

feat: implement direct controller for BigQueryDataTransferConfig

3153023

test: add a basic test for BigQueryDataTransferConfig

9f7bb1a

chore: make ready-pr

31bbc17

feat: add serviceAccountName field in BigQueryDataTransferConfig

d681d5d

Surface the "serviceAccountName" in BigQueryDataTransferConfig API

test: add a test of scheuled query for BigQueryDataTransferConfig

bc4dd55

fix: userID is an output field in BigQueryDataTransferConfig

2be80be

chore: update test outputs

0f14fe0

jingyih force-pushed the BigQueryDataTransferConfig branch from c3c84d4 to 0f14fe0 Compare September 17, 2024 07:01

test: restrict custom normalizations to BigQueryDataTransferConfig

ad942bb

yuwenma reviewed Sep 18, 2024

View reviewed changes

chore: update bigquerydatatransfer controller to use latest template

f8daaa5

jingyih force-pushed the BigQueryDataTransferConfig branch from b8bf4dc to f8daaa5 Compare September 18, 2024 20:35

google-oss-prow bot assigned yuwenma Sep 19, 2024

google-oss-prow bot added the lgtm label Sep 19, 2024

google-oss-prow bot added the approved label Sep 19, 2024

google-oss-prow bot merged commit e6d725b into GoogleCloudPlatform:master Sep 19, 2024
16 checks passed

jingyih added a commit to jingyih/k8s-config-connector that referenced this pull request Sep 19, 2024

doc: update release notes for GoogleCloudPlatform#2688

5859d45

google-oss-prow bot added a commit that referenced this pull request Sep 19, 2024

Merge pull request #2741 from jingyih/BigQueryDataTransferConfig

060cb6d

doc: update release notes for #2688

jingyih changed the title ~~feat: add SciFi controller for BigQueryDataTransferConfig~~ feat: add direct controller for BigQueryDataTransferConfig Sep 27, 2024

nitashadhingra pushed a commit to nitashadhingra/k8s-config-connector that referenced this pull request Oct 10, 2024

doc: update release notes for GoogleCloudPlatform#2688

a12f351

yuwenma added this to the 1.124 milestone Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add direct controller for BigQueryDataTransferConfig #2688

feat: add direct controller for BigQueryDataTransferConfig #2688

jingyih commented Sep 12, 2024 •

edited

Loading

jingyih left a comment

jingyih Sep 13, 2024

jingyih Sep 13, 2024

jingyih Sep 13, 2024

jingyih Sep 13, 2024

yuwenma Sep 13, 2024

jingyih Sep 13, 2024

yuwenma Sep 13, 2024

yuwenma Sep 13, 2024

yuwenma Sep 13, 2024

jingyih Sep 13, 2024

yuwenma Sep 19, 2024

yuwenma Sep 19, 2024

yuwenma Sep 13, 2024

jingyih Sep 13, 2024

yuwenma Sep 13, 2024

jingyih Sep 13, 2024 •

edited

Loading

yuwenma Sep 18, 2024

jingyih Sep 18, 2024

yuwenma Sep 19, 2024

yuwenma Sep 13, 2024

yuwenma Sep 13, 2024

jingyih Sep 13, 2024

yuwenma Sep 18, 2024

jingyih Sep 18, 2024

yuwenma Sep 19, 2024

yuwenma commented Sep 13, 2024

jingyih commented Sep 16, 2024

yuwenma Sep 18, 2024

jingyih Sep 18, 2024

yuwenma Sep 18, 2024

yuwenma Sep 18, 2024

yuwenma commented Sep 19, 2024

google-oss-prow bot commented Sep 19, 2024

feat: add direct controller for BigQueryDataTransferConfig #2688

feat: add direct controller for BigQueryDataTransferConfig #2688

Conversation

jingyih commented Sep 12, 2024 • edited Loading

jingyih left a 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

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

jingyih Sep 13, 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

yuwenma commented Sep 13, 2024

jingyih commented Sep 16, 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

yuwenma commented Sep 19, 2024

google-oss-prow bot commented Sep 19, 2024

jingyih commented Sep 12, 2024 •

edited

Loading

jingyih Sep 13, 2024 •

edited

Loading