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

Updated looksLikeUUID to only match hyphenated UUIDs (Fixes #461) #693

Merged
merged 3 commits into from Nov 7, 2019

Conversation

@neufeldtech
Copy link

neufeldtech commented Nov 7, 2019

Cluster names that happened to be 32 characters were getting treated as UUIDs which causes issue #461.

Based on The tests, I'm hypothesizing that the DO api is assuming UUID's to always be represented in the hyphenated form (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx).

This PR updates the looksLikeUUID() function to use a strict regex match rather than uuid.Parse()'s more lenient checking, which should address the issue in #461 .

Copy link
Contributor

bentranter left a comment

This looks great, thanks a lot @neufeldtech!

@bentranter

This comment has been minimized.

Copy link
Contributor

bentranter commented Nov 7, 2019

Going to give the rest of the team some time in case they'd like to review as well, but I'll merge this before the end of the day.

@@ -1679,8 +1679,8 @@ func nodeByName(name string, nodes []*godo.KubernetesNode) (*godo.KubernetesNode
}

func looksLikeUUID(str string) bool {
_, err := uuid.Parse(str)
return err == nil
r := regexp.MustCompile("^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-4[a-fA-F0-9]{3}-[8|9|aA|bB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}$")

This comment has been minimized.

Copy link
@zachgersh

zachgersh Nov 7, 2019

Contributor

excellent job - one bit, could we have this 4[a-fA-F0-9]{3} not hardcoded to version 4 of the standard? maybe [1-4] would be better here?

This comment has been minimized.

Copy link
@bentranter

bentranter Nov 7, 2019

Contributor

To follow up on this, we're not sure if across all of our different teams we're only using UUIDv4s here, or if we're using a combination of UUIDv1s and UUIDv4s. Allowing all versions as valid prevents us from accidentally blocking valid UUIDs.

This comment has been minimized.

Copy link
@neufeldtech

neufeldtech Nov 7, 2019

Author

Updated to support versions 1 through 4

@bentranter bentranter dismissed zachgersh’s stale review Nov 7, 2019

PR looks good now

Copy link
Contributor

bentranter left a comment

Looks good, thanks again @neufeldtech!

@bentranter bentranter merged commit 301fad2 into digitalocean:master Nov 7, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@neufeldtech neufeldtech deleted the neufeldtech:461 branch Nov 7, 2019
@MattIPv4

This comment has been minimized.

Copy link
Member

MattIPv4 commented Nov 8, 2019

Hey, @neufeldtech - Thanks a ton for this sweet PR! 😄

Would you please shoot me an email when you get a chance?
mcowley at digitalocean dot com 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.