-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: DNS record deletion issue in MAAS provider #196
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hamatuni. Thank you very much both for the bug report and the PR. Let's take a look of what is missing to get it merged:
- Please take a look of a pseudo-code that I consider a 100% fix for the bug.
- Please sign the Canonical CLA so that our CI check is happy. You can start from this page: https://ubuntu.com/legal/contributors
Apart from the tl;dr version of the proposed solution (suggested pseudocode), here is a most complete description of the actual issue. When MAAS creates a DNS A/AAAA record, under the hood it creates a MAAS DNS resource which is linked to the IP of that record. MAAS is also designed in a way that inside its DNS resources can contain 1 or more IP addresses and 1 or more DNS records (all records except A/AAAA, which are handled with the IP addresses).
When a DNS resource is created with 1 or more IPs, MAAS will also create in its database IPAddress objects for each IP, which are displayed as user reserved. Those IPAddress objects can be shared between many MAAS DNS resources. In your example, you have 2 MAAS DNS resource entities that are linked with the same IPAddress entity (8.8.8.8
). When a DNS resource is deleted by MAAS, all its IPAdress related entities are left intact. To overcome this MAAS limitation (maybe bug, maybe design choice, needs investigation), the provider, after deleting a DNS resource, in case of A/AAAA, it iterates all its related IPAddresses and it tries to delete them too. In other words, it tries not to leave orphan IPAddress objects. However, it is doing it blindly, so it will also remove IPAddresses which are currently also used by other DNS resources (8.8.8.8
). In addition, if a DNS resource has only a link to that IPAddress which is deleted, the DNS resource is deleted too. That is your second DNS resource. I think that with a solution like the proposed pseudocode, the provider will be able to handle appropriately the potential orphan IPAddress objects without causing issues with other DNS resources.
Please give it a try and if you are satisfied with it, embed it in your PR and let me know to review. 🍺
@@ -188,12 +188,13 @@ func resourceDnsRecordDelete(ctx context.Context, d *schema.ResourceData, meta i | |||
if err != nil { | |||
return diag.FromErr(err) | |||
} | |||
if err := client.DNSResource.Delete(id); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ "github.com/hashicorp/go-set"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
@@ -191,7 +192,23 @@ func resourceDnsRecordDelete(ctx context.Context, d *schema.ResourceData, meta i
if err := client.DNSResource.Delete(id); err != nil {
return diag.FromErr(err)
}
+ allDNSResources, err := client.DNSResources.Get()
+ if err != nil {
+ return diag.FromErr(err)
+ }
+ allOtherDNSResourcesIPAddresses := set.New[string](0)
+ for _, dnsResource := range allDNSResources {
+ if dnsResource.ID == id {
+ continue
+ }
+ for _, ipAddress := range dnsResource.IPAddresses {
+ allOtherDNSResourcesIPAddresses.Insert(ipAddress.IP.String())
+ }
+ }
for _, ipAddress := range dnsResource.IPAddresses {
+ if allOtherDNSResourcesIPAddresses.Contains(ipAddress.IP.String()) {
+ continue
+ }
if err := client.IPAddresses.Release(&entity.IPAddressesParams{IP: ipAddress.IP.String()}); err != nil {
return diag.FromErr(err)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one will avoid deleting an IP address of the under deletion DNS Resource if that IP address is in use by at least another DNS Resource.
This pull request addresses an issue in the MAAS Terraform provider where deleting a DNS record inadvertently removes other DNS records pointing to the same IP address. The changes ensure that only the specified DNS record is deleted, preventing the unintended removal of other records.
Changes Made
Conditional IP Address Check:
Added a check to ensure the DNS resource's IP address matches the IP address of the record to be deleted. This ensures only the correct record is targeted for deletion.
Avoid IP Address Release:
Modified the deletion logic to avoid releasing the IP address. This prevents the removal of other records that share the same IP address.
fix - #195