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

Fixes #49445 by adding additional annotation to define ELB security group (AWS) #62774

Merged
merged 1 commit into from Oct 1, 2018

Conversation

@Raffo
Copy link
Contributor

Raffo commented Apr 18, 2018

What this PR does / why we need it:
This fixes #49445 by changing the way the SG are applied to AWS ELBs: previously, an ELB would always have a default SG, either a pre-created one or an ad hoc one that always allowed 0.0.0.0/0 even if an annotation with a different security would be specified. This PR changes that by having the "extra" SG be the only one applied when it is specified as this is the way most of the people use AWS anyways: they predefine Security Groups with rules that work with them and they want to be applied.
It would would be probably worth considering changing the name of the annotation as this change of behaviour doesn't really make it "extra".

Which issue(s) this PR fixes :
Fixes #49445

Special notes for your reviewer:
Please advise how to build and test this on AWS to do an integration test, I don't have prior experience with that.

Release note:

Adds `service.beta.kubernetes.io/aws-load-balancer-security-groups` annotation to set the security groups to the AWS ELB to be the only ones specified in the annotation in case this is present (does not add `0.0.0.0/0`). 

/cc @justinsb @chrislovecnm what do you think?

@dims

This comment has been minimized.

Copy link
Member

dims commented Apr 18, 2018

/ok-to-test

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Apr 30, 2018

Thanks for the PR! So I may be missing something, but I think this is a breaking change - I might previously have been adding a security group to grant access via security-groups (which you can't otherwise do in the API), but I might still want access via a CIDR. This would block the CIDR.

One hacky workaround: you could set the allowed-CIDR to something like 127.0.0.1/32, so that the default rule was still present but you could ignore it. That is very hacky though (and I'm not 100% certain it would work!)

How about another annotation? e.g. service.beta.kubernetes.io/aws-load-balancer-security-groups (without extra)

I think the logic would be that aws-load-balancer-security-groups could replace the primary, as you have today. aws-load-balancer-extra-security-groups should probably still be added just for consistency, though I can't see why you would really use extra unless you wanted to keep the default rules.

Another option is to allow source ranges to specify security groups, or some syntax to disallow all traffic, though I suspect because it's both a field and an annotation (and the field is shared across cloudproviders) that would be harder..

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Apr 30, 2018

Hi, thanks for your review!
You are totally right, that's a change that would break the use case you mentioned, so it is probably not a good idea. In the end I saw already in the original comment that the name "extra" in the annotation was not fitting the change. I personally like your suggestion of introducing an additional annotation service.beta.kubernetes.io/aws-load-balancer-security-groups with the behaviour of this PR and keeping the service.beta.kubernetes.io/aws-load-balancer-extra-security-groups as it is right now (even though the name is somehow still a bit confusing).

@Raffo Raffo force-pushed the Raffo:bugfix/aws-custom-elb-sg branch from 4687813 to 92760af Apr 30, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Apr 30, 2018

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Apr 30, 2018

I update the PR with the discussed proposal: given that the code is indeed pretty minimal, I'd love to use that to reason if this is really what we are looking for and code makes it easier in this case.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented May 1, 2018

/test pull-kubernetes-verify

2 similar comments
@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented May 2, 2018

/test pull-kubernetes-verify

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented May 2, 2018

/test pull-kubernetes-verify

@wrstlrlp13

This comment has been minimized.

Copy link

wrstlrlp13 commented May 2, 2018

Would it not also make sense that if someone used the following under the spec:

loadBalancerSourceRanges:

  • xxx.xx.xxx.xx/32
  • xxx.xx.xxx.xx/27
  • etc.etc.etc/etc

The 0.0.0.0/0 inbound rule would no longer get created?

One would assume that if they're assigning specific load balancer source Ip ranges they don't want everyone to have access...

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented May 3, 2018

@wrstlrlp13 EDIT: that works, the 0.0.0.0/0 rule is only added for ICMP, so the 0.0.0.0/0 would not be allowed if we use the AnnotationLoadBalancerSourceRangesKey annotation.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented May 3, 2018

/retest

@christopherhein

This comment has been minimized.

Copy link
Member

christopherhein commented May 15, 2018

@Raffo this seems like a reasonable addition. like @justinsb said the alternative is pretty hacky but what I've used in the past.

Just for documentation sake it might make sense to make it more clear that it's replacing all SGs that would usually be defined. I would say change the last line in the comments to say "Differently from the annotation "service.beta.kubernetes.io/aws-load-balancer-extra-security-groups", this will replace all other security groups assigned to the ELB." at least if I'm reading the code correctly.

👍

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented May 15, 2018

You read it correctly, I'll make the change!

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jun 6, 2018

@justinsb @chrislovecnm can you please take another look?

@tabossert

This comment has been minimized.

Copy link

tabossert commented Jun 13, 2018

👍 Really want this

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Aug 7, 2018

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Aug 12, 2018

/lgtm

Can you approve?
/assign @justinsb @zmerlynn @gnufied @jsafrane

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Aug 27, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Aug 27, 2018

@Raffo: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@christopherhein

This comment has been minimized.

Copy link
Member

christopherhein commented Aug 28, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 28, 2018

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Aug 28, 2018

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Sep 12, 2018

/test all

Tests are more than 96 hours old. Re-running tests.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Sep 24, 2018

Gentle ping, this has been 5 months open right now /cc @justinsb @zmerlynn @gnufied @jsafrane

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Oct 1, 2018

(This should be targeted for cherry-pick once it's approved and merged)

@AishSundar

This comment has been minimized.

Copy link
Contributor

AishSundar commented Oct 1, 2018

Which release(s) does this need to be cherrypicked into. Also setting the milestone to 1.13 so its in our radar.

/milestone v1.13

@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Oct 1, 2018

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Oct 1, 2018

@AishSundar -- I'd say as many as possible

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Oct 1, 2018

Adding Nishi for follow-up.
/assign @d-nishi

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 1, 2018

@justaugustus: GitHub didn't allow me to assign the following users: d-nishi.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

Adding Nishi for follow-up.
/assign @d-nishi

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Oct 1, 2018

/approve

Thanks @Raffo!

I don't think this should be cherry-picked though - it is new functionality.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 1, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein, justaugustus, justinsb, Raffo

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

@justaugustus

This comment has been minimized.

Copy link
Member

justaugustus commented Oct 1, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 1ce5f67 into kubernetes:master Oct 1, 2018

18 of 19 checks passed

Submit Queue Milestone is for a future release and cannot be merged
Details
cla/linuxfoundation Raffo authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Oct 2, 2018

Thanks everybody for getting this merged!

@Raffo Raffo deleted the Raffo:bugfix/aws-custom-elb-sg branch Oct 2, 2018

@Raffo Raffo changed the title Fixes #49445 by not adding the default SG when using SG annotation (AWS) Fixes #49445 by adding additional annotation to define ELB security group (AWS) Oct 3, 2018

@matthias50

This comment has been minimized.

Copy link

matthias50 commented Nov 5, 2018

@Raffo Would it be possible to cherry-picked this to 1.12? It seems like a pretty small change and not having this capability causes security issues for us which we'd rather not leave open til 1.13.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Nov 5, 2018

@matthias50 It's technically easy I guess, but it was stated that it is a new feature (it kind of is even though it's fixing something that we can easily say is a bug) and will not be chosen for cherry pick.

@matthias50

This comment has been minimized.

Copy link

matthias50 commented Nov 5, 2018

@Raffo, I see the comment from @justinsb. I can see both sides of the story here in that we could look at this as a bug fix or a new feature. According to https://contributor.kubernetes.io/contributors/devel/cherry-picks/, it seems that the branch manager or the patch release team is the one that makes the call as to if this is something we can say is a bug which could be cherry picked or is a new feature.

If it isn't too much trouble, would you mind submitting a PR to cherry pick this and see what the branch managers for 1.12 think (see https://github.com/kubernetes/sig-release/blob/master/releases/release-1.12/release_team.md)? Maybe they agree with @justinb, maybe not. Worst that could happen is that they say no.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Nov 6, 2018

@matthias50

This comment has been minimized.

Copy link

matthias50 commented Nov 6, 2018

@dougm, @idealhack, @hoegaarden, @codenrhoden, is this PR one which could be cherry picked back to 1.12?

@cam-cen

This comment has been minimized.

Copy link

cam-cen commented Feb 8, 2019

I understand this is classified as a feature, but given this is kind of a bugfix (depending on how you look at it), is there any chance this could be cherry-picked to 1.11 as well @foxish ? EKS is still on 1.11 and this would solve a lot of problems with security group rule limits for restricted NLBs

@hoegaarden

This comment has been minimized.

Copy link
Member

hoegaarden commented Feb 8, 2019

@matthias50 @Raffo @cam-cen

For backporting you'd need the patch managers of the releases in question:

As the doc states, they are the authority to bring those changes into v1.1{1,2} or not.

Also you'd need the approval of the original approver (@justinsb in that case) on each of the cherry-pick PRs.
Justin however already stated:

I don't think this should be cherry-picked though - it is new functionality.

... so I'd start with convincing him that those changes need to go into v1.1{1,2} :)

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.