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.

Sign up for GitHub

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

Jump to bottom

Respect Allocation IDs #69263

Merged
merged 1 commit into from
Jun 23, 2019
Merged

Respect Allocation IDs #69263

merged 1 commit into from
Jun 23, 2019

Conversation

brooksgarrett
Copy link
Contributor

@brooksgarrett brooksgarrett commented Sep 29, 2018 •
edited

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
Contributor Author

/assign @justinsb

@nikhita
Copy link
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
Contributor Author

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
@gyuho
Copy link
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
Contributor Author

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

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 mentioned this pull request Oct 12, 2018
Closed
6 tasks
@brooksgarrett
Copy link
Contributor Author

Is anything else needed for this PR?

1 similar comment
@brooksgarrett
Copy link
Contributor Author

Is anything else needed for this PR?

@geekofalltrades
Copy link

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

@d-nishi
Copy link

d-nishi commented Nov 26, 2018

/assign @M00nF1sh

@brooksgarrett
Copy link
Contributor Author

I'll take a look later today.

@catalinvr
Copy link

Thank you

@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
Contributor Author

Rebased

@catalinvr
Copy link

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
staging/src/k8s.io/legacy-cloud-providers/aws/aws_loadbalancer.go
response = append(response, &elbv2.SubnetMapping{SubnetId: aws.String(id)})
for index, id := range subnetIDs {
sm := &elbv2.SubnetMapping{SubnetId: aws.String(id)}
if len(allocationIDs) > 0 {
Copy link
Contributor

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
Contributor

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

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

@M00nF1sh
Copy link
Contributor

M00nF1sh commented Jun 6, 2019

/assign @micahhausler

@mcrute
Copy link
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
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

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
Contributor Author

@mcrute Squashed all commits down per request.

@mcrute
Copy link
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
Contributor

[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

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

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

How to get Allocation IDs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

@chrislovecnm chrislovecnm Awaiting requested review from chrislovecnm

@justinsb justinsb Awaiting requested review from justinsb

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