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

Bug: azurerm_dns_aaaa_record - Normalize IPv6 addresses #5459

Merged
merged 9 commits into from
Jan 30, 2020

Conversation

phires
Copy link
Contributor

@phires phires commented Jan 21, 2020

This is a work in progress try to fix issue #5321 .
Sadly somehow the DiffSuppressFunc does not compare the correct set values with each other - and I have no idea why.
If adding a logline to the func I get something like this:

old 2607:f8b0:4005:800::1003, new 
old 2201:1234:1234::1, new 2201:1234:1234::1
old , new 2607:f8b0:4005:0800:0000:0000:0000:1003

Anyone any clues?

@ghost ghost added the size/M label Jan 21, 2020
@katbyte katbyte changed the title [WIP] Fix Issue 5321 (compressed IPv6 DNS record) [WIP] azurerm_dns_aaaa_record - suppress compressed IPv6 DNS record differences Jan 21, 2020
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.

Hi @phires,

Thanks for giving this a shot. I'm not sure we need the CompressIPv6Address function, a suppress should do the trick. Could we add an acctest with an IP address exhibing the error? once thats done we can dig into this further. thanks!

azurerm/helpers/azure/network.go Outdated Show resolved Hide resolved
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{
Type: schema.TypeString,
Copy link
Collaborator

Choose a reason for hiding this comment

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

And we should add a validate.IPv6Address

@katbyte
Copy link
Collaborator

katbyte commented Jan 23, 2020

👋 @phires, Is this reaady to be looked at outside the failing travis test?

@katbyte
Copy link
Collaborator

katbyte commented Jan 23, 2020

Pretty sure the validation function will allow the compressed form in, so it won't really help

@phires
Copy link
Contributor Author

phires commented Jan 24, 2020

wave @phires, Is this reaady to be looked at outside the failing travis test?

I've removed the unused import (shame on me), would be great to have a look onto this, why the DiffSuppressFunc is not working correctly.

@ghost ghost removed the waiting-response label Jan 24, 2020
@katbyte
Copy link
Collaborator

katbyte commented Jan 25, 2020

Hey @phires, i believe this is because we are trying to diff suppress on a set and they behave differently then a list. I think what we'll need to do is setup a custom hash function an a state migration to switch to the new hashs.

@WodansSon
Copy link
Collaborator

@phires, I believe @katbyte is totally correct in her assessment of what is happening here. I was playing around with this too and added some debug spew to your code just to see what the values were when entering and exiting your function and what I saw is consistent with what @katbyte is saying:

*** = Enter function
--- = Exit function

*** Str Old: 2607:f8b0:4005:800::1003 :: Str New:  ***
--- Old: 2607:f8b0:4005:800::1003 :: New: <nil> :: Equal: false ---

*** Str Old:  :: Str New: 2607:f8b0:4005:0800:0000:0000:0000:1003 ***
--- Old: <nil> :: New: 2607:f8b0:4005:800::1003 :: Equal: false ---

@phires
Copy link
Contributor Author

phires commented Jan 27, 2020

@phires, I believe @katbyte is totally correct in her assessment of what is happening here. I was playing around with this too and added some debug spew to your code just to see what the values were when entering and exiting your function and what I saw is consistent with what @katbyte is saying:

*** = Enter function
--- = Exit function

*** Str Old: 2607:f8b0:4005:800::1003 :: Str New:  ***
--- Old: 2607:f8b0:4005:800::1003 :: New: <nil> :: Equal: false ---

*** Str Old:  :: Str New: 2607:f8b0:4005:0800:0000:0000:0000:1003 ***
--- Old: <nil> :: New: 2607:f8b0:4005:800::1003 :: Equal: false ---

Yes, see my original PR text, I had added a similar func already ;)

So, a custom hash function it is. Any pointers where to start?

@WodansSon
Copy link
Collaborator

WodansSon commented Jan 27, 2020

@phires sure, I played around with this a bit and here is what I did...

In the ./azurerm/helpers/azure directory I created a file called ipv6.go. I then wrote the below code for that file:

package azure

import (
	"net"

	"github.com/hashicorp/terraform-plugin-sdk/helper/hashcode"
)

func NormalizeIPv6Address(ipv6 interface{}) string {
	return net.ParseIP(ipv6.(string)).String()
}

func HashIPv6Address(ipv6 interface{}) int {
	return hashcode.String(NormalizeIPv6Address(ipv6))
}

Then in the resource I updated the "records" schema Set function to be:

Set:           azure.HashIPv6Address,

I also removed the DiffSuppressFunc and removed the github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/suppress package import declaration.

Finally, I modified the expand function:

Orig code:
ipv6 := v.(string)

New code:
ipv6 := azure.NormalizeIPv6Address(v)

and the flatten function:

Orig code:
results = append(results, *record.Ipv6Address)

New code:
results = append(results, azure.NormalizeIPv6Address(*record.Ipv6Address))

That's it... I think that should work, now all we need to do is to move the test cases and update them to use the new normalize functions and I think we are gold. 😃

@WodansSon
Copy link
Collaborator

@phires doing it this way I don't think we will need the state migration either, but play with it and see how it goes.

@WodansSon
Copy link
Collaborator

👋 @phires just curious, did you want to make the changes to your code or are you ok with me pushing to your branch? Thanks again for the PR!

@ghost ghost added size/L and removed size/M labels Jan 29, 2020
@phires
Copy link
Contributor Author

phires commented Jan 29, 2020

Hi, I've implemented the changes and also build some tests for the new funcs.

@ghost ghost removed the waiting-response label Jan 29, 2020
@WodansSon WodansSon changed the title [WIP] azurerm_dns_aaaa_record - suppress compressed IPv6 DNS record differences Bug: azurerm_dns_aaaa_record - Normalize IPv6 addresses Jan 30, 2020
@WodansSon WodansSon added this to the v1.43.0 milestone Jan 30, 2020
@WodansSon WodansSon self-assigned this Jan 30, 2020
Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@phires thanks much for pushing those changes and adding the test cases. LGTM! 🚀

@WodansSon
Copy link
Collaborator

image

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.

Thanks for this @phires, LGTM 👍

@katbyte katbyte merged commit 4ee08e6 into hashicorp:master Jan 30, 2020
katbyte added a commit that referenced this pull request Jan 30, 2020
@ghost
Copy link

ghost commented Feb 4, 2020

This has been released in version 1.43.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.43.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants