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

Make ALB event target group names unique #6322

Merged

Conversation

@cbm-egoubely
Copy link
Contributor

commented Jun 28, 2019

What did you implement:

Closes #6320

This is a fix proposal, might have to wait until a better solution is found.

Instead of using <normalizedFunctionName>-<stage> as a Target Group name, use a md5 hash of the combination of service name, function name and stage. It would ensure uniqueness across services.

Also, to make Target Groups searchable in the EC2 console, add a Name tag to the Target Group with value <service name>-<function name>-<stage>.

Important: Target Group names are limited to 32 chars, that's why I had to use a hash instead of using a readable name. This is not satisfying but I could not come up with a better solution, so if someone has a better one, that would be great ! I hope AWS will increase this limitation in the futur.

How did you implement it:

Added some functions in lib/plugins/aws/lib/naming.test.js using the crypto module.

Use these functions in targetGroups.js.

How can we verify it:

See the instructions to reproduce the bug in #6320

Todos:

  • Write tests and confirm existing functionality is not broken.
  • Write documentation
  • Ensure there are no lint errors.
  • Ensure introduced changes match Prettier formatting.
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens self-requested a review Jun 30, 2019

@pmuens pmuens self-assigned this Jun 30, 2019

@pmuens
Copy link
Member

left a comment

This looks good! I like that change. Thanks for working on this @cbm-egoubely 👍

Just to be 100% sure. This doesn't seem to be a breaking change since it only touches the internals, correct? Also ccing @medikoo here.

If that's not the case we can test and merge it soon 🤘

return crypto
.createHash('md5')
.update(this.getAlbTargetGroupNameTagValue(functionName))
.digest('hex');

This comment has been minimized.

Copy link
@medikoo

medikoo Jul 1, 2019

Member

Why hashing is needed? Wouldn't adding service name be good enough?

This comment has been minimized.

Copy link
@pmuens

pmuens Jul 2, 2019

Member

Good point. I think it's preferable because of the max length for the name. We could truncate the name, but some service names are pretty long.

This comment has been minimized.

Copy link
@medikoo

medikoo Jul 2, 2019

Member

And do we know what's allowed max length? Is it e.g. different from function name on AWS side?
In unhashed form iirc this name equals final function name (which is also combination of service, base function name and stage tokens)

This comment has been minimized.

Copy link
@cbm-egoubely

cbm-egoubely Jul 2, 2019

Author Contributor

Max length is 32 (see PR description) and name has to be unique per service/function/stage, I think it would be hard to have a non-hash name that fits, even with truncating.

This comment has been minimized.

Copy link
@medikoo

medikoo Jul 2, 2019

Member

Indeed, not sure why I didn't read through Important note in description.

LGTM

@medikoo

medikoo approved these changes Jul 2, 2019

@pmuens pmuens added this to In progress in Serverless via automation Jul 2, 2019

@pmuens pmuens added this to the 1.47.0 milestone Jul 2, 2019

Serverless automation moved this from In progress to Reviewer approved Jul 2, 2019

@pmuens

pmuens approved these changes Jul 2, 2019

Copy link
Member

left a comment

Just tested this and it works fine. Thanks for fixing this @cbm-egoubely 👍

LGTM :shipit:

@pmuens pmuens merged commit 88e0da5 into serverless:master Jul 2, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (medikoo) No manifest changes detected

Serverless automation moved this from Reviewer approved to Done Jul 2, 2019

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