Skip to content

Respect Allocation IDs#69263

Merged
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
brooksgarrett:issue63959
Jun 23, 2019
Merged

Respect Allocation IDs#69263
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
brooksgarrett:issue63959

Conversation

@brooksgarrett
Copy link
Copy Markdown
Contributor

@brooksgarrett brooksgarrett commented Sep 29, 2018 •
edited
Loading

What this PR does / why we need it:
AWS NLB supports the use of static IP addresses (EIP) with Network Load Balancers. This PR supports a new annotation on services service.beta.kubernetes.io/aws-load-balancer-eip-allocations which is a comma separated list of AWS Allocation IDs. The number of Allocation IDs must match the number of subnets used for the load balancer.

/kind feature
/sig aws

Fixes #63959

Special notes for your reviewer:
None

Release note:

Creates an annotation 'service.beta.kubernetes.io/aws-load-balancer-eip-allocations' to assign AWS EIP to the newly created Network Load Balancer. Number of allocations and subnets must match.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. sig/aws cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 29, 2018
@k8s-ci-robot k8s-ci-robot added the sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. label Sep 29, 2018
@brooksgarrett
Copy link
Copy Markdown
Contributor Author

brooksgarrett commented Sep 29, 2018

/assign @justinsb

@nikhita
Copy link
Copy Markdown
Member

nikhita commented Sep 29, 2018

@brooksgarrett Thanks for your first contribution to Kubernetes! 🎉

/ok-to-test

This change it looks like a release note?

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 29, 2018
@brooksgarrett
Copy link
Copy Markdown
Contributor Author

brooksgarrett commented Sep 30, 2018

I'm AFK today but will document it tomorrow.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 30, 2018
pho-enix
pho-enix reviewed Oct 1, 2018
View reviewed changes
Comment thread pkg/cloudprovider/providers/aws/aws.go Outdated
gyuho
gyuho reviewed Oct 1, 2018
View reviewed changes
Comment thread pkg/cloudprovider/providers/aws/aws_loadbalancer.go Outdated
Comment thread pkg/cloudprovider/providers/aws/aws_loadbalancer.go Outdated
Comment thread pkg/cloudprovider/providers/aws/aws_loadbalancer.go Outdated
@gyuho
Copy link
Copy Markdown
Member

gyuho commented Oct 1, 2018

Have we tested this? Or already covered somewhere?

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 1, 2018
@brooksgarrett
Copy link
Copy Markdown
Contributor Author

brooksgarrett commented Oct 1, 2018

I'm currently rolling it into our soak environment today. It was approved for test over the weekend so I'm just now able to get the binaries rolled out. I tested it in local go harnesses before implementing but as seen with the quoted variable issue I may have bugs. I'll comment here with clear results of testing by tomorrow or Wednesday (3 Oct).

@pho-enix
Copy link
Copy Markdown

pho-enix commented Oct 2, 2018

Tested this on a kops deployed cluster on AWS. Cluster spans over 3 AZs.

Created a service with 3 EIP allocations.

NLB was created correctly and EIPs are correctly associated with the ENIs.

gyuho
gyuho reviewed Oct 2, 2018
View reviewed changes
Comment thread pkg/cloudprovider/providers/aws/aws_loadbalancer.go Outdated
@gyuho gyuho mentioned this pull request Oct 12, 2018
Closed
6 tasks
@brooksgarrett
Copy link
Copy Markdown
Contributor Author

brooksgarrett commented Oct 15, 2018

Is anything else needed for this PR?

1 similar comment
@brooksgarrett
Copy link
Copy Markdown
Contributor Author

brooksgarrett commented Nov 6, 2018

Is anything else needed for this PR?

@geekofalltrades
Copy link
Copy Markdown

geekofalltrades commented Nov 26, 2018

Pinging assignee @justinsb for an update. Our company would desperately like this in.

@d-nishi
Copy link
Copy Markdown

d-nishi commented Nov 26, 2018

/assign @M00nF1sh

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 24, 2019
@brooksgarrett
Copy link
Copy Markdown
Contributor Author

brooksgarrett commented May 24, 2019

Rebased

@catalinvr
Copy link
Copy Markdown

catalinvr commented May 27, 2019

Hi @justinsb, Hi @chrislovecnm

When you will have time, please can you review the pull request that it's described below?

Thank you.

M00nF1sh
M00nF1sh reviewed Jun 5, 2019
View reviewed changes
Comment thread staging/src/k8s.io/legacy-cloud-providers/aws/aws_loadbalancer.go Outdated
Copy link
Copy Markdown
Member

@M00nF1sh M00nF1sh Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len(allocationIDs)>index+1 might be better to understand.
But keep it as it is good enough too(relying on the fact allocationIDs will be either 0 or equal to length of subnetIDs at caller-side).

@M00nF1sh
Copy link
Copy Markdown
Member

M00nF1sh commented Jun 5, 2019

/lgtm
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 5, 2019
@catalinvr
Copy link
Copy Markdown

catalinvr commented Jun 6, 2019

Hi team, What will be the next step for this pull request? Thank you

@M00nF1sh
Copy link
Copy Markdown
Member

M00nF1sh commented Jun 6, 2019

/assign @micahhausler

@mcrute
Copy link
Copy Markdown
Contributor

mcrute commented Jun 14, 2019

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2019
@mcrute
Copy link
Copy Markdown
Contributor

mcrute commented Jun 14, 2019

@brooksgarrett commit messages that at-mention people are no longer permitted for merge in Kubernetes. Would you please either fix the commit message mentioning @gyuho or squash all of these commits into one and update this PR. I'll re-lgtm and approve after that push. Thanks!

@catalinvr
Copy link
Copy Markdown

catalinvr commented Jun 17, 2019

Hi @brooksgarrett,
Please, can you help me to fix the comments or squash all commits.
Thank you :)

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Jun 23, 2019
@brooksgarrett
Copy link
Copy Markdown
Contributor Author

brooksgarrett commented Jun 23, 2019

@mcrute Squashed all commits down per request.

@mcrute
Copy link
Copy Markdown
Contributor

mcrute commented Jun 23, 2019

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jun 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brooksgarrett, mcrute

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit bf55a99 into kubernetes:master Jun 23, 2019
@catalinvr
Copy link
Copy Markdown

catalinvr commented Jun 23, 2019

Thank you so much @brooksgarrett 🥇

@plumdog plumdog mentioned this pull request Sep 5, 2019
Closed
@jicowan jicowan mentioned this pull request Sep 25, 2020
Closed
@melissajenner22
Copy link
Copy Markdown

melissajenner22 commented Oct 9, 2020

How to get Allocation IDs?

This was referenced Feb 27, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrislovecnm chrislovecnm Awaiting requested review from chrislovecnm

@justinsb justinsb Awaiting requested review from justinsb

4 more reviewers

@gyuho gyuho gyuho left review comments

@pho-enix pho-enix pho-enix left review comments

@M00nF1sh M00nF1sh M00nF1sh left review comments

@raulcd raulcd raulcd requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mcrute mcrute

@micahhausler micahhausler

@M00nF1sh M00nF1sh

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support EIP Allocations with AWS NLB