Aws-load-balancer-controller: Duplicate CIDR in inbound-cidrs causes security group inbound rules to be cleared

[tylerohlsen]

I accidentally added the same CIDR twice in the alb.ingress.kubernetes.io/inbound-cidrs annotation and the effect was all inbound rules on the security group were cleared effectively blocking all access to my services.

The following error was logged:

E0409 03:39:17.924063       1 controller.go:217] kubebuilder/controller "msg"="Reconciler error" "error"="failed to reconcile LB managed SecurityGroup: failed to reconcile managed LoadBalancer securityGroup due to failed to grant inbound permissions due to InvalidParameterValue: The same permission must not appear multiple times\n\tstatus code: 400, request id: *****"  "controller"="alb-ingress-controller" "request"={"Namespace":"*****","Name":"*****"}

Could you add some validation for this case and lighten the effect?
Maybe log a warning and remove the duplicate before calling the AWS update endpoints?

[SaranBalaji90]

https://github.com/kubernetes-sigs/aws-alb-ingress-controller/blob/4bba2f2b0ba2fb171c38c9382a85627c196f48e9/internal/alb/sg/association.go#L181-L187

Here we append the CIDRs to be associated to the list. We should remove duplicates here.

I0409 18:17:53.582056       1 security_group.go:75] 2048-game/2048-ingress: granting inbound permissions to securityGroup sg-0f4323016344a36e1: 
[{    FromPort: 80,    IpProtocol: "tcp",    IpRanges: [
{        CidrIp: "10.0.2.0/24",        Description: "Allow ingress on port 80 from 10.0.2.0/24"      },
{        CidrIp: "10.0.2.0/24",        Description: "Allow ingress on port 80 from 10.0.2.0/24"      }],    ToPort: 80  }]

Also one more thing I noticed is, whenever cidr list changes it invokes revokes on all existing CIDRs and then invokes grant on required CIDRs since it compares ipPermission instead of ipRanges. We should fix this too otherwise if revoke succeeds and grant fails for some reason then we will lose all inbound connection to alb.

I0409 18:44:34.604163       1 security_group.go:64] 2048-game/2048-ingress: revoking inbound permissions from securityGroup sg-0f4323016344a36e1: 
[{    FromPort: 80,    IpProtocol: "tcp",    IpRanges: 
[{        CidrIp: "10.0.2.0/24",        Description: "Allow ingress on port 80 from 10.0.2.0/24"      },
{        CidrIp: "10.0.1.0/24",        Description: "Allow ingress on port 80 from 10.0.1.0/24"      }], 
   ToPort: 80  }]
I0409 18:44:34.747686       1 security_group.go:75] 2048-game/2048-ingress: granting inbound permissions to securityGroup sg-0f4323016344a36e1:
 [{    FromPort: 80,    IpProtocol: "tcp",    IpRanges: 
[{        CidrIp: "10.0.2.0/24",        Description: "Allow ingress on port 80 from 10.0.2.0/24"      }],
    ToPort: 80  }]

[M00nF1sh]

@SaranBalaji90 Nice catch on the second issue.
For the first one, i think it's better to error out and issue an error event to Ingress instead of silently remove duplicates. Since the user is most likely to have typos where he actually meant to have two different CIDR.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[AFriemann]

/remove-lifecycle rotten

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[kishorj]

/kind feature
We will normalize and validate the CIDRs

[kishorj]

/kind enhancement

[k8s-ci-robot]

@kishorj: The label(s) kind/enhancement cannot be applied, because the repository doesn't have them

In response to this:

/kind enhancement

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.