Migrate Certificate Manager DNSAuthorization API #2561

anhdle-sso · 2024-08-22T23:40:18Z

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.

anhdle-sso · 2024-08-23T18:06:30Z

/assign @justinsb

Hamzawy63 · 2024-08-29T09:01:04Z

@anhdle-sso

Hi Anh,

I work Certificate Manager team and happened to see this pull request by accident. Just curious, could you provide me more context about this pull request? Is it part of the recent efforts to improve the config connector (https://github.com/GoogleCloudPlatform/k8s-config-connector?tab=readme-ov-file#-coming-soon-a-new-way-to-write-the-controller-) ?

Also, is the defined schema for this resource based on the the latest version that the config connector had? or will it be up-to-date with the API ? Are we no longer based on the Terraform Provider Google beta version?

I'm asking because the latest version we had in KCC didn't include the recently-added Enum field Type (https://cloud.google.com/certificate-manager/docs/reference/certificate-manager/rest/v1/projects.locations.dnsAuthorizations#type) and I wonder whether it will be supported after the migration or we will still need service team intervention to support it?

apis/certificatemanager/v1beta1/certificatemanagerdnsauthorization_types.go

yuwenma · 2024-08-29T18:08:11Z

@Hamzawy63
Yes, it is part of the recent "Direct resource" effort.
Right, we will support the new fields as long as

they are added before this Direct resource migration.
they show up in the google go client library that your team owns.

yuwenma · 2024-09-04T18:45:00Z

dev/tasks/generate-crds

-excluded_resources=("computeforwardingrule")
+excluded_resources=("computeforwardingrule" "certificatemanagerdnsauthorization")



So for a beta resource, we suggest not excluding the CRD generation using excluded_resources, because it gives the PR reviewing no clue on whether the outcome is correct or not (also it would be very error prone for the developers).

Instead, you can add the cnrm.cloud.google.com/dcl2crd: "true” in the type go tag. (example), and comment out those new fields that do not exist in the TF-based resources according to the outcome from dev/task/generate-crds. Guide here

yuwenma

Looks good! One comment about avoid using the excluded_resources flag so that we can compare whether this PR is backward compatible since the resource is already in Beta.

maqiuyujoyce

LGTM with a few non-blocking nits.

apis/certificatemanager/v1beta1/certificatemanagerdnsauthorization_types.go

apis/certificatemanager/v1beta1/types.generated.go

apis/certificatemanager/v1beta1/certificatemanagerdnsauthorization_types.go

anhdle-sso · 2024-09-04T22:03:27Z

Looks good! One comment about avoid using the excluded_resources flag so that we can compare whether this PR is backward compatible since the resource is already in Beta.

The current CRD also has v1alpha1 information. if I do not exclude this resource; the v1alpha1 part will be deleted which might cause issue for customer who is using v1alpha1.

yuwenma · 2024-09-04T22:29:42Z

Looks good! One comment about avoid using the excluded_resources flag so that we can compare whether this PR is backward compatible since the resource is already in Beta.

The current CRD also has v1alpha1 information. if I do not exclude this resource; the v1alpha1 part will be deleted which might cause issue for customer who is using v1alpha1.

you can include the alpha resource in the direct types. here's the related section in the guide.

anhdle-sso · 2024-09-04T23:32:30Z

Looks good! One comment about avoid using the excluded_resources flag so that we can compare whether this PR is backward compatible since the resource is already in Beta.

done. added alpha resource and regen crds

maqiuyujoyce · 2024-09-05T00:17:43Z

...definition_certificatemanagerdnsauthorizations.certificatemanager.cnrm.cloud.google.com.yaml

@@ -308,9 +341,3 @@ spec:
    storage: true


Could you try to:

Install the old CRD and create a CertificateManagerDNSAuthorization resource.

Upgrade to the new CRD.

Verify whether the upgrade works and whether the existing CertificateManagerDNSAuthorization successfully reconciled based on the new schema?

The reason I asked here is that we've never switched the storage version from v1alpha1 to v1beta1 so we probably want to be extra careful.

I tested by following these steps:

Checkout to master

Apply the existing crd kubectl apply -f config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_certificatemanagerdnsauthorizations.certificatemanager.cnrm.cloud.google.com.yaml; make deploy-controller && kubectl delete pods --namespace cnrm-system --all

Apply sample DNS Authorization kubectl apply -f config/samples/resources/certificatemanagerdnsauthorization/certificatamanager_v1beta1_certificatemanagerdnsauthorization.yaml

Check into my branch with new change and run step 2 again with an extra log line to confirm that the new controller is deployed.

Wait a bit and confirm that the resource is still good.

Make a change in the resource and apply again to force reconciliation.

The resource is updated and ready.

Awesome, thank you!

maqiuyujoyce · 2024-09-09T20:40:26Z

/lgtm though you'll probably want to figure out why the validation still fails.

yuwenma

/lgtm
/approve

Awesome! Thanks!

google-oss-prow · 2024-09-09T21:31:01Z

[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

yuwenma · 2024-09-09T21:32:09Z

apis/certificatemanager/v1alpha1/certificatemanagerdnsauthorization_types.go

+// +kubebuilder:printcolumn:name="Ready",JSONPath=".status.conditions[?(@.type=='Ready')].status",type="string",description="When 'True', the most recent reconcile of the resource succeeded"
+// +kubebuilder:printcolumn:name="Status",JSONPath=".status.conditions[?(@.type=='Ready')].reason",type="string",description="The reason for the value in 'Ready'"
+// +kubebuilder:printcolumn:name="Status Age",JSONPath=".status.conditions[?(@.type=='Ready')].lastTransitionTime",type="date",description="The last transition time for the value in 'Status'"
+// +kubebuilder:metadata:labels="cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=alpha";"cnrm.cloud.google.com/tf2crd=true"


The validation test seems to be wrong. It cannot deal with multi-versions correctly. You can remove the cnrm.cloud.google.com/stability-level here or update the validation check.

google-oss-prow bot assigned justinsb Aug 23, 2024

anhdle-sso unassigned justinsb Aug 23, 2024

anhdle-sso force-pushed the migrate_api_dns_auth branch from 20a5df5 to 572ebe0 Compare August 23, 2024 22:05

anhdle-sso requested review from yuwenma and justinsb August 26, 2024 17:21

yuwenma reviewed Aug 29, 2024

View reviewed changes

apis/certificatemanager/v1beta1/certificatemanagerdnsauthorization_types.go Show resolved Hide resolved

apis/certificatemanager/v1beta1/certificatemanagerdnsauthorization_types.go Show resolved Hide resolved

anhdle-sso force-pushed the migrate_api_dns_auth branch 2 times, most recently from 7a80250 to 2af3156 Compare September 3, 2024 20:20

anhdle-sso requested a review from maqiuyujoyce September 4, 2024 17:31

yuwenma reviewed Sep 4, 2024

View reviewed changes

maqiuyujoyce reviewed Sep 4, 2024

View reviewed changes

anhdle-sso closed this Sep 4, 2024

anhdle-sso reopened this Sep 4, 2024

anhdle-sso force-pushed the migrate_api_dns_auth branch 2 times, most recently from 685bb42 to 14aa100 Compare September 4, 2024 23:31

maqiuyujoyce reviewed Sep 5, 2024

View reviewed changes

anhdle-sso added 7 commits September 6, 2024 17:34

Migrate CertificateManagerDNSAuthorization API

c0af1e9

Add kubebuilder immutability validation

436225f

Generate types

057ad41

make ready-pr

9c85b97

Add kcc proto tag and regen

20fdf32

add required tag

5031b01

Add DNS Authorization v1alpha1 and regen crds

c2b00cf

make ready-pr

b7f1353

anhdle-sso force-pushed the migrate_api_dns_auth branch from 14aa100 to b7f1353 Compare September 6, 2024 17:37

yuwenma reviewed Sep 9, 2024

View reviewed changes

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

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

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

yuwenma added this to the 1.123 milestone Sep 9, 2024

yuwenma reviewed Sep 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Certificate Manager DNSAuthorization API #2561

Migrate Certificate Manager DNSAuthorization API #2561

anhdle-sso commented Aug 22, 2024 •

edited

Loading

anhdle-sso commented Aug 23, 2024

Hamzawy63 commented Aug 29, 2024 •

edited

Loading

yuwenma commented Aug 29, 2024 •

edited

Loading

yuwenma Sep 4, 2024

yuwenma left a comment

maqiuyujoyce left a comment

anhdle-sso commented Sep 4, 2024

yuwenma commented Sep 4, 2024 •

edited

Loading

anhdle-sso commented Sep 4, 2024

maqiuyujoyce Sep 5, 2024

anhdle-sso Sep 5, 2024

maqiuyujoyce Sep 9, 2024

maqiuyujoyce commented Sep 9, 2024

yuwenma left a comment

google-oss-prow bot commented Sep 9, 2024

yuwenma Sep 9, 2024 •

edited

Loading

		excluded_resources=("computeforwardingrule")
		excluded_resources=("computeforwardingrule" "certificatemanagerdnsauthorization")

Migrate Certificate Manager DNSAuthorization API #2561

Are you sure you want to change the base?

Migrate Certificate Manager DNSAuthorization API #2561

Conversation

anhdle-sso commented Aug 22, 2024 • edited Loading

Change description

Tests you have done

anhdle-sso commented Aug 23, 2024

Hamzawy63 commented Aug 29, 2024 • edited Loading

yuwenma commented Aug 29, 2024 • edited Loading

yuwenma Sep 4, 2024

Choose a reason for hiding this comment

yuwenma left a comment

Choose a reason for hiding this comment

maqiuyujoyce left a comment

Choose a reason for hiding this comment

anhdle-sso commented Sep 4, 2024

yuwenma commented Sep 4, 2024 • edited Loading

anhdle-sso commented Sep 4, 2024

maqiuyujoyce Sep 5, 2024

Choose a reason for hiding this comment

anhdle-sso Sep 5, 2024

Choose a reason for hiding this comment

maqiuyujoyce Sep 9, 2024

Choose a reason for hiding this comment

maqiuyujoyce commented Sep 9, 2024

yuwenma left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Sep 9, 2024

yuwenma Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

anhdle-sso commented Aug 22, 2024 •

edited

Loading

Hamzawy63 commented Aug 29, 2024 •

edited

Loading

yuwenma commented Aug 29, 2024 •

edited

Loading

yuwenma commented Sep 4, 2024 •

edited

Loading

yuwenma Sep 9, 2024 •

edited

Loading