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

Respect Allocation IDs #69263

Merged
merged 1 commit into from Jun 23, 2019

Conversation

@brooksgarrett
Copy link
Contributor

commented Sep 29, 2018

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.
@brooksgarrett

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2018

/assign @justinsb

@nikhita

This comment has been minimized.

Copy link
Member

commented Sep 29, 2018

@brooksgarrett Thanks for your first contribution to Kubernetes! 🎉

/ok-to-test

This change it looks like a release note?

@brooksgarrett

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

I'm AFK today but will document it tomorrow.

@gyuho

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

Have we tested this? Or already covered somewhere?

@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Oct 1, 2018
@brooksgarrett

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link

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 referenced this pull request Oct 12, 2018
6 of 6 tasks complete
@brooksgarrett

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Is anything else needed for this PR?

1 similar comment
@brooksgarrett

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Is anything else needed for this PR?

@geekofalltrades

This comment has been minimized.

Copy link

commented Nov 26, 2018

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

@d-nishi

This comment has been minimized.

Copy link

commented Nov 26, 2018

/assign @M00nF1sh

@brooksgarrett

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

I'll take a look later today.

@catalinvr

This comment has been minimized.

Copy link

commented May 23, 2019

Thank you

@brooksgarrett

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Rebased

@catalinvr

This comment has been minimized.

Copy link

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.

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 {

This comment has been minimized.

Copy link
@M00nF1sh

M00nF1sh Jun 5, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

/lgtm
/ok-to-test

@catalinvr

This comment has been minimized.

Copy link

commented Jun 6, 2019

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

@M00nF1sh

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

/assign @micahhausler

@mcrute

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

/approve

@mcrute

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Jun 17, 2019

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

@brooksgarrett

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

@mcrute Squashed all commits down per request.

@mcrute

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 23, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

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

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
23 checks passed
23 checks passed
cla/linuxfoundation brooksgarrett authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@catalinvr

This comment has been minimized.

Copy link

commented Jun 23, 2019

Thank you so much @brooksgarrett 🥇

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.