GCE: Delete cluster will also delete the DNS entries created by kubernetes #8250

mccare · 2020-01-02T18:35:51Z

Kops delete cluster will now also delete the GCE DNS entries.

GCE DNS deletion is done by creating a Change object with resource record deletions/additions for one zone. So the implementation is using the GroupDeleter and the GroupKey.

For deletion the zone.name was needed and was not present in the dns.ResourceRecordSet. So I passed it along as the name of the ResourceRecord to be deleted.

Any hints or pointers on how to write tests for this change very much appreciated. Works in my use case (example cluster on GCE).

k8s-ci-robot · 2020-01-02T18:35:59Z

Hi @mccare. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mccare · 2020-01-02T18:36:20Z

/assign @geojaz

geojaz · 2020-01-03T04:38:12Z

/ok-to-test

geojaz

I left comments- this looks like we're moving in a useful direction, but I think it brings up a bunch of questions that I'm not yet sure how to answer...

geojaz · 2020-01-03T04:41:27Z

pkg/resources/gce/gce.go

+
+	zoneResponse, err := d.gceCloud.NameService().ManagedZones.List(d.gceCloud.Project()).Do()
+	if err != nil {
+		return nil, fmt.Errorf("error getting GCE DNS zones %v", err)


This feels copypasta'd from AWS. I would prefer to s/getting GCE DNS zones/listing Cloud DNS zones (GCP Cloud DNS zones?) or similar.

I checked the aws implementation, and I was wrong about copypasta, but my comment stands :)

Not quite sure what you mean, the naming of NameService() to CloudDNS()?

Sorry, this was a nit. I would update the Error to error listing Cloud DNS zones.

pkg/resources/gce/gce.go

geojaz · 2020-01-03T04:51:05Z

pkg/resources/gce/gce.go

+				continue
+			}
+
+			if !strings.HasSuffix(record.Name, "."+d.clusterName+".") {


I think you're missing some interpolation here?

Regardless, the logic feels a bit arbitrary to me. If I'm reading this correctly, you're basically trying to figure out if Cloud DNS records are records that we care about (and thus would need to delete). Would it be useful to pull out the logic to figure out if a given record is one that we should be cleaning up after into a helper?

One major thing that I think we're not yet considering, but was eluded to in the comments of the faux-original implementation (commented out) is the concept of "shared" ownership; ie: assuming there is a record in cloud dns that we think we/kops should own, do we really own it? In this case, i'm not trying to poke holes in your implementation, i'm trying to figure out how to clean up after ourselves... I don't have an answer or suggestion right now.

Basically the implementation was taken from the AWS implementation.

I've refactored some:

pulled out the method if a DNS name is kops managed into the clusterDiscoveryGCE (Best would be if this would be a general method for all providers )

Only api, api.internal and bastion. will be deleted.

Master-public-name which can be set at the command line will not be deleted. I would need access to the Cloudspec which is not accessible from here

etcd seems obsolete since the implementation of etcd-manager (see buildPrecreateDNSHostnames in dns.go)

I've readded the check for gossip host name, that was in the previous comments. As far as I understood the previous comments, they were mentioning shared zones. The records we delete are created by kubernetes. So IMHO they can't be really shared (only one A record per name allowed) and can be deleted.

This is not to say that the code may not delete a record e.g. in a private only zone with the same DNS name as the public zone. But from my tests I cannot create a cluster where there is a private a public zone with the same DNS name.

This is not to say that the code may not delete a record e.g. in a private only zone with the same DNS name as the public zone. But from my tests I cannot create a cluster where there is a private a public zone with the same DNS name.

I think we can probably not worry about shared ownership of Cloud DNS records, but I think it's possible to end up in a place where you have a shared Zone. Whether or not kops supports that right now for gce is an open question.

For zones, would it be possible to add a label like we have in many AWS resources? I was thinking that if a zone was specified that already existed, we could add label: "kubernetes.io/cluster/" + clusterName = "shared". Then we could skip deletion of that zone.

This seems like a great place for some tests to make sure that we're cleaning up when we need to and not deleting shared resources, but I'm not sure which route to follow without doing some more thinking. @rifelpet Do you have any suggestions?

Using labels sounds optimal. Indeed for AWS resources we dont want kops to manage, kops supports detecting a shared label/tag and if it is present, it wont delete the resource. In AWS it is mostly around networking (VPC, subnets, etc) but I think it makes sense to use it for DNS zones as well.

changes addressed

justinsb · 2020-04-07T14:22:45Z

pkg/resources/gce/gce.go

+
+	for _, record := range r {
+		r := record.Obj.(*clouddns.ResourceRecordSet)
+		zoneName = record.Name


Using the groupKey would be great here

justinsb · 2020-04-07T14:23:27Z

pkg/resources/gce/gce.go

+
+			if d.isKopsManagedDNSName(record.Name) {
+				resource := resources.Resource{
+					Name:         zone.Name,


I think this is shown to the user, so it might be nice to keep this as record.Name

justinsb · 2020-04-07T14:24:17Z

Thanks @mccare this looks great. I had one idea about using the group key for the zone name, but I'm going to experiment in a follow-on PR (and will cc you if it works!).

/approve
/lgtm

k8s-ci-robot · 2020-04-07T14:24:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, mccare

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

Fixes build problems caused by concurrent changes

rifelpet · 2020-04-07T15:03:27Z

/lgtm

Use the GroupKey to pass the zone name, meaning the name can be the user-facing value. Follow-on to kubernetes#8250

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 2, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 2, 2020

k8s-ci-robot requested review from joshbranham and robinpercy January 2, 2020 18:36

k8s-ci-robot assigned geojaz Jan 2, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 3, 2020

geojaz previously requested changes Jan 3, 2020

View reviewed changes

mccare force-pushed the gce-delete-dns-entries-for-cluster-delete branch 5 times, most recently from 2f9a26b to 932be2b Compare January 3, 2020 12:26

mccare requested a review from geojaz January 3, 2020 12:43

mccare force-pushed the gce-delete-dns-entries-for-cluster-delete branch from 932be2b to 8f9e455 Compare January 3, 2020 15:56

mccare force-pushed the gce-delete-dns-entries-for-cluster-delete branch from 8f9e455 to 956d4b5 Compare January 14, 2020 18:52

justinsb reviewed Apr 7, 2020

View reviewed changes

k8s-ci-robot assigned justinsb Apr 7, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2020

mccare and others added 2 commits April 7, 2020 10:37

kops delete will also delete the DNS entries for GCE

eb3bb41

Use dns.NewService instead of dns.New

765fa36

Fixes build problems caused by concurrent changes

justinsb force-pushed the gce-delete-dns-entries-for-cluster-delete branch from 956d4b5 to 765fa36 Compare April 7, 2020 14:37

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2020

k8s-ci-robot assigned rifelpet Apr 7, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2020

k8s-ci-robot merged commit f14ce65 into kubernetes:master Apr 7, 2020

k8s-ci-robot added this to the v1.18 milestone Apr 7, 2020

justinsb added a commit to justinsb/kops that referenced this pull request Apr 8, 2020

GCE DNS Cleanup: Print record name

be1a03a

Use the GroupKey to pass the zone name, meaning the name can be the user-facing value. Follow-on to kubernetes#8250

justinsb mentioned this pull request Apr 8, 2020

GCE DNS Cleanup: Print record name #8869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCE: Delete cluster will also delete the DNS entries created by kubernetes #8250

GCE: Delete cluster will also delete the DNS entries created by kubernetes #8250

mccare commented Jan 2, 2020

k8s-ci-robot commented Jan 2, 2020

mccare commented Jan 2, 2020

geojaz commented Jan 3, 2020

geojaz left a comment

geojaz Jan 3, 2020

geojaz Jan 3, 2020

mccare Jan 3, 2020

geojaz Mar 10, 2020

geojaz Jan 3, 2020

geojaz Jan 3, 2020

mccare Jan 3, 2020 •

edited

Loading

geojaz Mar 10, 2020

rifelpet Mar 10, 2020

justinsb Apr 7, 2020

justinsb Apr 7, 2020

justinsb commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

rifelpet commented Apr 7, 2020

GCE: Delete cluster will also delete the DNS entries created by kubernetes #8250

GCE: Delete cluster will also delete the DNS entries created by kubernetes #8250

Conversation

mccare commented Jan 2, 2020

k8s-ci-robot commented Jan 2, 2020

mccare commented Jan 2, 2020

geojaz commented Jan 3, 2020

geojaz 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

mccare Jan 3, 2020 • 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

justinsb commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

rifelpet commented Apr 7, 2020

mccare Jan 3, 2020 •

edited

Loading