Skip to content
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

resource azurerm_dns_zone: Add option to re-use host_name with dns_zone with soa_record creation #22312

Conversation

fredericfran-gds
Copy link
Contributor

When a DNS zone is created in Azure, there is a default SOA record which is created.

In this provider, a dns_zone with soa_record is done by:

  1. creating the dns_zone
  2. overwriting the soa_record of the zone with the data supplied in the soa_record config block

The soa_record block makes it mandatory to add the host_name argument when it is specified. This is NOT desired in most cases since the host_name is the MNAME SOA field which is the name of the primary nameserver and is set by Azure.

This commit attempts to fix this issue by:

  1. allowing the host_name to be omitted when soa_record is specified
  2. the host_name value will be derived from the original SOA record when the zone is created.

This issue is partly a bug in Azure API since it is is mentioned in the documentation that it should not be possible to update the 'host' property of the SOA record.

@fredericfran-gds fredericfran-gds changed the title Add option to re-use host_name with dns_zone with soa_record creation resource azurerm_dns_zone: Add option to re-use host_name with dns_zone with soa_record creation Jun 29, 2023
Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR!
I have left some additional mostly minor comments that once addressed this should be good to merge 👍

internal/services/dns/dns_zone_resource.go Outdated Show resolved Hide resolved
internal/services/dns/dns_zone_resource.go Outdated Show resolved Hide resolved
internal/services/dns/dns_zone_resource.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR!
I have left some additional mostly minor comments that once addressed this should be good to merge 👍

internal/services/dns/dns_zone_resource.go Outdated Show resolved Hide resolved
internal/services/dns/dns_zone_resource.go Outdated Show resolved Hide resolved
internal/services/dns/dns_zone_resource.go Outdated Show resolved Hide resolved
internal/services/dns/dns_zone_resource.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating this! I've left some minor comment, once they are resolved, then it should be good to go 👍

Besides, there are two CI errors that want to be fixed.

internal/services/dns/dns_zone_resource.go Outdated Show resolved Hide resolved
@fredericfran-gds

This comment was marked as off-topic.

@jackofallops jackofallops self-assigned this Jul 7, 2023
fredericfran-gds and others added 5 commits July 7, 2023 15:57
When a DNS zone is created in Azure, there is a default SOA record which is
created.

In this provider, a dns_zone with soa_record is done by:
1. creating the dns_zone
2. overwriting the soa_record of the zone with the data supplied in
   the soa_record config block

The `soa_record` block makes it mandatory to add the host_name argument
when it is specified. This is NOT desired in most cases since the host_name
is the MNAME SOA field which is the name of the primary nameserver and is
set by Azure.

This commit attempts to fix this issue by:
1. allowing the `host_name` to be omitted when `soa_record` is specified
2. the host_name value will be derived from the original SOA record when
   the zone is created.

This issue is partly a bug in Azure API since it is is mentioned in the
[documentation](https://learn.microsoft.com/en-us/azure/dns/dns-zones-records#soa-records)
that it should not be possible to update the 'host' property of the SOA record.
1. get soaRecord only rather than whole zone read
2. return Set(id) to original position
3. update documentation
4. update tests
@jackofallops jackofallops force-pushed the fredericfran-gds/re-use_nameserver-soa branch from 6f89fe6 to dcc654d Compare July 7, 2023 13:59
@github-actions github-actions bot added size/M and removed size/S labels Jul 7, 2023
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fredericfran-gds
I hope you don't mind, but while reviewing I noticed a few other things that needed some work / changes so I've made those and I'm running the tests now. As soon as that's done I'll get another member of the team to quickly check my changes too and we should be able to get this in.

Thanks!

@fredericfran-gds
Copy link
Contributor Author

Hi @fredericfran-gds I hope you don't mind, but while reviewing I noticed a few other things that needed some work / changes so I've made those and I'm running the tests now. As soon as that's done I'll get another member of the team to quickly check my changes too and we should be able to get this in.

Thanks!

Hi, no problem at all. Thanks for fixing, we wanted to be conservative and still allow someone to modify the host/host_name because strangely the API allows that when the documentation says no. Could be an issue with Azure API.

@jackofallops
Copy link
Member

Hi, no problem at all. Thanks for fixing, we wanted to be conservative and still allow someone to modify the host/host_name because strangely the API allows that when the documentation says no. Could be an issue with Azure API.

I've taken the approach of maintaining the value if specified for now, but we'll deprecate and remove the ability to set it in the next major version, since it would be a breaking change from a user configuration point of view.

The tests are all passing, so someone else on the team should be able to quickly re-review and this will hopefully ship in the next release.

image

@jackofallops jackofallops removed their assignment Jul 7, 2023
@jackofallops jackofallops requested a review from a team July 7, 2023 14:27
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🧬

@katbyte katbyte merged commit c3645f5 into hashicorp:main Jul 7, 2023
15 checks passed
@github-actions github-actions bot added this to the v3.65.0 milestone Jul 7, 2023
katbyte added a commit that referenced this pull request Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants