Fix AWS IPPermission check for case with preexisting groups and ranges #20790
Conversation
|
Labelling this PR as size/M |
k8s-github-robot
added
the
size/M
|
GCE e2e test build/test passed for commit a2fbccbf63fb21ee6aa46cbc6ed2bb565acf0a6c. |
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
|
@justinsb can you take a look at this? |
|
Ping? I'm hoping I can get this in 1.2. |
| } | ||
| exists = ipPermissionExists(&existingIpPermission2, &oldIpPermission, false) | ||
| if !exists { | ||
| t.Errorf("Should have been considered existing since 172.* is in oldIpPermission2's array of ranges") |
There was a problem hiding this comment.
this should say 192.* I think?
There was a problem hiding this comment.
Good catch. I fixed that and a misspelling of 'against'.
therc
force-pushed
the
ip-permission-iprange
branch
from
a2fbccb
to
da4ba61
Compare
|
GCE e2e test build/test passed for commit da4ba61. |
|
LGTM |
davidopp
added
ok-to-merge
lgtm
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit da4ba61. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
|
Sorry @therc - can you explain a bit why this was needed? ipPermissionExists is called from ensureSecurityGroupIngress and removeSecurityGroupIngress. In removeSecurityGroupIngress it seems incorrect. We will remove a security group ingress rule that is potentially wider than what we intended. ensureSecurityGroupIngress is called as part of the LoadBalancer creation/update code. It is called for the ELB security group and the instance security group. Both groups are owned by k8s. In neither case should any peered IPs be present. I am planning on fixing this as part of the fix for #21895, which appears to be a long-standing issue but I only noticed as part of #21907. The fix will set the ELB security groups to be exactly what the user has requested them to be - no extra rules allowed unless specified through k8s using the new security group annotation. |
|
This happens when you run in the default VPC and the cloud provider picks the first security group for the instance, I think. There's no guarantee that we own the latter. |
|
Ah OK. We should make sure that security groups are tagged, and that the cloudprovider checks the tags. For backcompat I guess we'll have to only check the tags if we find more than one security group. I've opened #21986, so we can be sure that we can rely on tagging. The k8s-owned security group will be owned by k8s. If you want to add another security group, that's OK as long as it isn't tagged :-) |
If you have "alien" ranges (e.g. from peering), ipPermissionExists will never return true, because the length of the new range array will always be smaller than the existing one. So we'll end up trying to add a range that is already there, which causes EC2 to return a "duplicate rule" error and eventually throttle us (probably what might be happening in #20314). Turn the strict equality check into a subset check.
cc @justinsb