azurestack: Modify destroy code to handle public dns record deletion #5095
Conversation
|
/cc @patrickdillon |
|
/test e2e-azure |
rna-afk
force-pushed
the
azurestack_destroy_dns_record
branch
from
d8456e3
to
8303357
Compare
|
/test e2e-azure |
There was a problem hiding this comment.
This is in good shape. I don't love deleting based on the name but I don't think we have many other options. Adding a check against tag/metadata will provide some more piece of mind, but as I said we can follow that up later if time does not allow doing it now.
To merge this can you please address the two comments about squashing commit and deleting the comment. Then we can deal with the other work subsequently.
pkg/destroy/azure/azure.go
Outdated
| return err | ||
| } | ||
| for _, record := range recordPages.Values() { | ||
| if t := to.String(record.Name); t == privateDNSRecordName || t == publicDNSRecordName { |
There was a problem hiding this comment.
I wonder if in addition we can check based on a tag/metadata. I see that there is support for creating with a tag/metadata.
So would the client support a check to see if the record has the tag with key/value:
"kubernetes.io_cluster.${var.cluster_id}" = "owned"
I don't believe we can locate the DNS record by this tag, which would be the ideal scenario (then we would not need to add new values to the metadata).
This can also be followed up in a subsequent PR to add the tag in the terraform code & check the tag here.
There was a problem hiding this comment.
Actually if we can check on tags, then we wouldn't need/want to check the names. That is, we could just loop through all the records and see if they match the tag (not the name). But yeah that can be a subsequent PR once we confirm installer, ingress and any other dns records can be tagged.
There was a problem hiding this comment.
I'll check it out. Will update this PR.
There was a problem hiding this comment.
Are we adding the tags right now? I don't see them in the DNS records on the portal.
There was a problem hiding this comment.
No. We can add them in a subsequent pr.
rna-afk
force-pushed
the
azurestack_destroy_dns_record
branch
from
8303357
to
a0fc5b3
Compare
rna-afk
force-pushed
the
azurestack_destroy_dns_record
branch
from
a0fc5b3
to
3f946c0
Compare
rna-afk
force-pushed
the
azurestack_destroy_dns_record
branch
from
3f946c0
to
b9211ee
Compare
Modifying the Azure deletion code to check if the environment is Azure Stack Hub and if so, check and delete the A records in the public DNS that is relevant to the current cluster during cluster destroy.
Adding vendor files for Azure Stack Hub client support for DNS.
rna-afk
force-pushed
the
azurestack_destroy_dns_record
branch
from
b9211ee
to
4abfaaf
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@rna-afk: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Modifying the Azure deletion code to check if the environment is
Azure Stack Hub and if so, check and delete the A records in the
public DNS that is relevant to the current cluster during cluster
destroy.