ManageIQ / floe

Floe is a runner for Amazon States Language workflows
Apache License 2.0
0 stars 6 forks source link

Pod names might overload the 63 character limit #133

Closed Fryguy closed 1 year ago

Fryguy commented 1 year ago

Extracted from https://github.com/ManageIQ/floe/pull/132#discussion_r1358348735

I think there's technically a prior bug here since the pod_name can only be 63 characters, and it's possible to have an image name longer than (63 - 2 - 4 - 36 =) 21 characters. Before this change, we could afford 26 characters, which is still pretty small.

I don't think we need a full UUID either - thinking we might be able to use, say, 8 or 12 random hex bytes.

SecureRandom.uuid is overly large for what we require, so I'm thinking we can use something significantly smaller, but wanted to discuss before I wrote a PR.

I'm thinking something like SecureRandom.urlsafe_base64(6) which should give us more than enough entropy in a tiny payload.

method chars bits example notes
SecureRandom.uuid 36 128 b6bb2535-808a-40bc-a3b1-cd71213c4d4d 36 = 32 chars + 4 -
SecureRandom.urlsafe_base64(16) 22 128 VKQxaJwgrmJkDTrjCj6NFA
SecureRandom.hex(6) 12 48 f788b7d2f448
SecureRandom.urlsafe_base64(8) 11 64 sBfQEpPiFVA I think this is what YouTube uses
SecureRandom.urlsafe_base64(6) 8 48 PHEghZQl *my choice
SecureRandom.hex(4) 8 32 c3b17c47

We don't actually need uuid's full 128 bits of entropy (2128 or 340 undecillion), since we're just trying to avoid collisions on very short-lived things. IMO 48 bits of entropy (248 or 281 trillion) is way more than enough.

@agrare @kbrock Just want to come to an agreement here, then I'll write up the PR.

Fryguy commented 1 year ago

Slightly more accurately, a straight urlsafe_base64 isn't entirely conforming to the naming rules for Kubernetes, but it's not hard to tweak that.

SecureRandom.urlsafe_base64(8).tr("_", "-")
kbrock commented 1 year ago

The only other thing I ran into was ensuring the names are all lowercase. Thought I think we are all set for this

Fryguy commented 1 year ago

Ohh I didn't notice that detail @kbrock - I thikn SecureRandom.urlsafe_base64 won't work then, because it can spit out upper and lower. So instead maybe .hex(4) or .hex(6) - what do you think?

agrare commented 1 year ago

.hex(4) is probably fine, and we can handle conflicts by just re-generating the name if we get an "Object Exists" error. We have this luxury since we don't need uniqueness across systems without communication.

Fryguy commented 1 year ago

Yeah good idea - I'll put in a PR with the hex(4), and we'll need a separate PR for and Object exists handling