Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants