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

AWS: Remove obsolete permissions on ELB security group #21987

Merged
merged 1 commit into from
Mar 5, 2016
Merged

AWS: Remove obsolete permissions on ELB security group #21987

merged 1 commit into from
Mar 5, 2016

Conversation

justinsb
Copy link
Member

The ingress CIDRs are going to be dynamic, and in general we don't want
to leave old ingress rules around.

Fix #21895

Depends on #21907

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 25, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 25, 2016

GCE e2e test build/test passed for commit aaa8c36beb556e505b74f1038e49ffa3b8ffd731.

@justinsb justinsb modified the milestone: v1.2 Feb 25, 2016
@justinsb justinsb mentioned this pull request Feb 26, 2016
Closed
@k8s-github-robot
Copy link

PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2016
@justinsb justinsb mentioned this pull request Mar 2, 2016
Closed
@mikedanese
Copy link
Member

Ok, 21907 is lgtm'd, i plan on reviewing this tomorrow.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/XL

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2016
@justinsb
Copy link
Member Author

justinsb commented Mar 3, 2016

Thanks @mikedanese ... #21907 got caught up in the merge problems last night (#22435). But I'm trying to push it through...

I did rebase this on the LGTMed #21907, so hopefully 21907 will go through shortly (it just made it back onto the submit queue) and then this one should be clean.

This PR is going to just be for the final commit.

@k8s-bot
Copy link

k8s-bot commented Mar 3, 2016

GCE e2e build/test passed for commit d9386e3545b5e8c90fcca3006431fe00d6873160.

mikedanese
mikedanese reviewed Mar 3, 2016
View reviewed changes
pkg/cloudprovider/providers/aws/sets_ippermissions.go

type IPPermissionSet map[string]*ec2.IpPermission

func NewIPPermissionSet(items ...*ec2.IpPermission) IPPermissionSet {
Copy link
Member

Choose a reason for hiding this comment

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

Was this autogenerated? cc @lavalamp

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't - we can't currently autogenerate things that can't be map-keys. We had the same thing in #21907 for sets of net.IPNet. There we ended up putting it into pkg/util/net/sets, expected alias for the import being netsets, with a readme that we will likely move it to pkg/util/sets when/if we get autogen to work. And the API for netsets.IPNet should match the API for sets, at least as much as is implemented.

This one is AWS only, so I don't think we should let it escape pkg/cloudproviders/providers/aws.

@mikedanese
Copy link
Member

IIUC this makes it so people can't add their own rules to a security group. I think theirs a use case for being able to add your own rules to the k8s security group, and also seems like it might break someone if they upgrade and had been using manually created rules.

@mikedanese
Copy link
Member

Ahh different security groups for elbs than nodes so last point probably doesn't make sense right?

@justinsb
Copy link
Member Author

justinsb commented Mar 3, 2016

IIUC this makes it so people can't add their own rules to a security group

Good observation. Yes, you can't (or you can't expect them to survive).

I think theirs a use case for being able to add your own rules to the k8s security group,

The way around this that I've been pushing is to have two security groups on your ELBs/instances. The one that is tagged with the KubernetesCluster tag is owned by kubernetes, and abandon hope all you who enter there. But k8s will not touch the second security group, and if you want to add your own rules, that is where you would do that. In general though, I think going behind k8s's back and doing stuff directly in the API is not something we can support generally. You should expect that a future version of k8s will reconcile your changes to the k8s state of the world, as k8s adds more and more functionality.

We can carve out exceptions which we will allow, like "if you want to change security group rules manually on an ELB, add a second security group".

and also seems like it might break someone if they upgrade and had been using manually created rules.

Yes. But upgrades are non-existent right now anyway on AWS... still hoping to get to it for 1.2.

@justinsb
Copy link
Member Author

justinsb commented Mar 3, 2016

Ahh diffent security groups for elbs than nodes so last point probably doesn't make sense right?

No, I think it makes just as much sense to add a security group rule to a node as to an ELB :-) Arguably #21907 reduces the need for the ELB case, but there's always an exception...

@mikedanese
Copy link
Member

The way around this that I've been pushing is to have two security groups on your ELBs/instances.

That sounds fine

@justinsb
AWS: Remove extra load balancer security group ingress rules
62e34da 
The ingress CIDRs are going to be dynamic, and in general we don't want
to leave old ingress rules around.

Fix kubernetes#21895
@justinsb
Copy link
Member Author

justinsb commented Mar 3, 2016

Added a comment that we should probably regroup rules in future, and rebased so there is only 1 commit now in this PR. Also #21907 has now merged so this is now mergeable if you approve @mikedanese

@justinsb justinsb changed the title [Depends on #21907] Remove obsolete permissions on ELB security group AWS: Remove obsolete permissions on ELB security group Mar 3, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 4, 2016
@justinsb justinsb mentioned this pull request Mar 4, 2016
Closed
@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 4, 2016

GCE e2e build/test passed for commit 62e34da.

@justinsb justinsb added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 4, 2016
@justinsb
Copy link
Member Author

justinsb commented Mar 4, 2016

Fixes a P1 so marking as P1

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Mar 4, 2016

GCE e2e build/test passed for commit 62e34da.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Mar 4, 2016

GCE e2e build/test passed for commit 62e34da.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Mar 4, 2016

GCE e2e build/test passed for commit 62e34da.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Mar 4, 2016

GCE e2e build/test failed for commit 62e34da.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@justinsb
Copy link
Member Author

justinsb commented Mar 4, 2016

@k8s-bot test this github flake #22270

@k8s-bot
Copy link

k8s-bot commented Mar 4, 2016

GCE e2e build/test passed for commit 62e34da.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Mar 5, 2016

GCE e2e build/test passed for commit 62e34da.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 5, 2016
Merge pull request #21987 from justinsb/fixx_21895
264f578 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 264f578 into kubernetes:master Mar 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mikedanese mikedanese

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@justinsb @k8s-github-robot @k8s-bot @mikedanese @googlebot @eparis