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

added new function uuidv5 #21244

Closed
wants to merge 0 commits into from
Closed

Conversation

lscheidler
Copy link
Contributor

it would be nice, if this function could be added, so it can be used in different use cases.

My use-case for this function is, that it will be used in aws_route53_record resources, so that the sub-domain isn't easily guessed, but anyone knowing the dns name can convert it back to plain text.

resource "aws_route53_record" "client-name-or-domain" {
  zone_id = "${aws_route53_zone.primary.zone_id}"
  name    = "${uuidv5("dns", "client-name-or-domain")}.example.com"
  type    = "A"
  ttl     = "300"
  records = ["${aws_eip.lb.public_ip}"]
}

Rebased pull request #21197 to master branch.

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this, @lscheidler!

This looks great to me, and I'd love to merge it once the initial v0.12 release is closed and we've re-opened the master branch for new features.

I left a note inline about a possible additional feature here. No worries if you don't want to attack that right now, since it can be added later with no backward compatibility issue, but I think it would round off this functionality nicely by being open to third-party-allocated namespaces.

case args[0].AsString() == "x500":
namespace = uuidv5.NamespaceX500
default:
return cty.UnknownVal(cty.String), fmt.Errorf("uuidv5() doesn't support namespace %s", args[0].AsString())
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the namespace portion of a v5 UUID is itself a UUID, with the four you handled above as well-known ones. With that in mind, perhaps we should also allow namespace to be given as a literal uuid (requiring the same format that uuid and uuidv5 would return) and use it verbatim if so? That way anyone who is working with a non-standard namespace they allocated themselves (or with a future standard namespace Terraform doesn't support yet) could still use it with this function.

I do like the idea of having short, readable aliases for the standard ones above, though: makes the intent much clearer to the reader in those cases than having the first argument be a literal uuid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this little extension and will looking into it. Thanks for Your input.

@lscheidler
Copy link
Contributor Author

Added support for custom namespaces.

@lscheidler
Copy link
Contributor Author

Hi @apparentlymart, any updates regarding this pull request?

@apparentlymart
Copy link
Contributor

Hi @lscheidler!

Your question is timely since we're just starting to get caught up on the PRs that we had to put on hold during the v0.12 finish up.

Unfortunately I seem to have inadvertently closed this PR but I am about to merge it to master with just some edits to the documentation page. Sorry for the accidental close.

@apparentlymart
Copy link
Contributor

Merged as aa07806. Thanks, @lscheidler!

@ghost
Copy link

ghost commented Jul 25, 2019

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.

@ghost ghost locked and limited conversation to collaborators Jul 25, 2019
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.

2 participants