emissary icon indicating copy to clipboard operation
emissary copied to clipboard

Fix cluster naming for really long service names

Open kflynn opened this issue 2 years ago • 1 comments

Without this fix, the collision-handling code for a cluster with a really long service name will fail, because the really really long service name will push all the stuff that's supposed to disambiguate the cluster names off past the point of relevance. 🤦‍♂️

With this fix, we stick a cryptographic hash of the whole too-long cluster name into the truncated name, so that the stuff trimmed off the end is still correctly accounted for.

Note that I'm using CI to run tests here: I'm still working on getting things going on my M1 Pro Mac.

Fixes #4354.

Signed-off-by: Flynn [email protected]

  • [x] I made sure to update CHANGELOG.md.
  • [x] This is unlikely to impact how Emissary performs at scale. It's going to add an SHA1 to every too-long cluster name being generated, but that should be a small fraction of cluster names.
  • [ ] My change is adequately tested.
  • [x] I didn't need to update DEVELOPING.md.
  • [x] The changes in this PR have been reviewed for security concerns and adherence to security best practices.

kflynn avatar Jul 31 '22 03:07 kflynn

@kflynn - looks like you have a test failing.

Also, it make sense, can we just add a corresponding test to ensure it is working as reported in the issue.

LanceEa avatar Aug 01 '22 20:08 LanceEa

Hey @LanceEa! I ~modified~substantially rewrote the TestFakeCollision test to do a better job of testing this; more info in the lead comment above. 🙂

kflynn avatar Aug 25 '22 16:08 kflynn

@kflynn - it looks good to me. Let me know what you think of removing the suffix as part of this PR?

also, I would like to get a second review, mostly for knowledge transfer so the other team members are aware of this naming convention.

LanceEa avatar Aug 29 '22 13:08 LanceEa

@ddymko - Can you take a look at this, I think it would be good to understand this logic and how we consolidate clusters that have name collisions.

LanceEa avatar Aug 29 '22 13:08 LanceEa