hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io
Other
42.77k stars 9.56k forks source link

New Feature interpolationFuncBase64Md5 #19245

Closed TechyMatt closed 5 years ago

TechyMatt commented 6 years ago

Hi Y'all,

I've written a new function I need for a related Terraform AzureRM Provider issue #1990 and I've initially written it for Terraform v0.11.10. I assume at this point it'd make sense to do a PR against the latest v0.12 release and have it merged (hopefully) into that? Just want to confirm before I fully test as I know the way functions are written has changed.

Cheers!

apparentlymart commented 6 years ago

Hi @mb290! Thanks for starting this conversation.

You're right that the new lang package is the right place for new function implementations; the old ones in the config package will be deleted soon as part of our cleanup efforts.

With that said, I am a little concerned that we are gradually accumulating a whole matrix of different (base, hash function) pairs here, which will give us a lot of different combinations to document, test, and maintain.

While indeed there has been some precedent already for base64 hash functions, the sha256 one feels somewhat justified in that that hash function produces lots of bits and so he encoding is pretty verbose.

With older hash functions like MD5 and SHA1, hex encoding is the de-facto standard and what most people think of when they think if running these hash functions. I can see that Azure is bucking that trend in their wire protocol, presumably for a more compact representation on the wire.

With all of this said, I think I'd prefer here to have the provider expect a more conventional hex MD5 for this and do the transcode to Base64 internally. That way we can use the function that is already present and keep this quirk hidden inside the Azure provider.

What do you think? Is there a technical reason why that couldn't work?

TechyMatt commented 6 years ago

@apparentlymart I fully agree that this would be the easier solution and I spent a good amount of time trying to make it work. The challenge is that I couldn't find a clean way in Go to convert from an MD5 hex to binary and then encode it with base64.

In my opinion I'd like to keep the existing format of the additional function in TF core, whilst I agree accumulating this matrix is far from ideal, there is already the president set and do we want to start managing snowflakes independently in providers?

The other aspect is from a user perspective as well, trying to debug issues in the future would be a nightmare if someone is clearly putting in an md5 function into their resource but getting something that is far from md5 in the state. The hash is also very visible in the storage blob through the UI so again trying to match plan changes would be very difficult.

Happy to continue to hash this out (pardon the pun), in the mean time I have compiled TF locally with my solution so that I could move forward on my TFE workflow implementation of the desired file change functionality.

apparentlymart commented 6 years ago

Hi @mb290,

I think I'd say that it is indeed part of a provider's mission to translate between Terraform's lightweight coventions for representing data to the formats remote APIs use. For example, we also have an emerging convention that timestamps in Terraform are always RFC3339 format -- and that is what Terraform date/time functions will use -- and providers should translate to other formats where needed for the remote API to ensure that data can flow from resource to resource across providers without lots of noisy conversion code. The mapping from Terraform configuration constructs to the SDK's own types is a more obvious example of this role.

This will usually be a two-way conversion, and I think that is true in this case too. The provider would convert hex to base64 when doing Create/Update and would convert base64 back to hex when doing Read.

Since the base64 hash is visible in the UI and therefore familiar to some Azure users, I'd consider having the provider have two different arguments so both can be used, or having a Computed-only one that reflects back the base64 so it can be seen and correlated when needed.

Regarding the implementation: the conversion here isn't really hash-specific, since a hash result is fundamentally just an array of bytes of a particular length. The provider can just check that the input has the expected number of hex digits and then transcode the bytes directly. encoding/hex can get you from hex to bytes, and then encoding/base64 will get you from bytes to base64. These packages also both have the opposite operations for implementing Read.

katbyte commented 5 years ago

@mb290, closing this as we would like the functionality to be within the provider.

ghost commented 4 years ago

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.