Add support for enabling AWS Shield Advanced protection

[hhamalai]

When subscribed to AWS Shield Advanced service, the Shield protection must be turned on for each Application Load Balancer resource. While AWS offers automation to do this via services like Firewall Manager, their use cases are limited by their constraints (e.g., impossible to utilize rate-based WAF rules 1).

The proposed change adds an annotation for the ingress resource, which can is used to turn on the Shield Advanced protection for the load balancers created by aws-alb-ingress-controller.

[k8s-ci-robot]

Welcome @hhamalai!

It looks like this is your first PR to kubernetes-sigs/aws-alb-ingress-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-alb-ingress-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @hhamalai. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[alejandrox1]

/ok-to-test

[M00nF1sh]

Wow, thanks so much for contributing! looks really good :D
/ok-to-test

[hhamalai]

/assign @bigkraig

[hhamalai]

Hi, can @bigkraig give any feedback how to take this proposed change further? Should try to drag more review feedback or assign this into some person to review?

[M00nF1sh]

I'll test this out today

[M00nF1sh]

/lgtm
/approve
/hold

I'll approve this first sine it looks good at itself.
However, we need a few enhancements to this before cutting a new release(I can do it but feel free to ping me to take it if you got time)

1. we needs to add caches to this API. (since the current code will call reconcile on every pod relocate event, and this will be called when the annotation is absent). Similar to https://github.com/kubernetes-sigs/aws-alb-ingress-controller/blob/master/internal/alb/lb/waf.go

2. we need to check the protection name matches managed by aws-alb-ingress-controller before disassociate it to avoid unintentionally removed shield which might be attached by 3rd party rules. (e.g. if name mismatch, leave it as it is).

3. we need to add a feature Gate for this feature to allow Customer disable it. (since it need new IAM permissions and will fail the controller unless these permissions are added, some customers may don't want to add such permissions or even unable to do so).

4. Personally, I'd prefer change the semantic to like blow, but would like to have some discussing.

   • alb.ingress.kubernetes.io/shield-advanced-protection: 'true', enable shield
   • alb.ingress.kubernetes.io/shield-advanced-protection: 'false', disabled shield(also check for managed by aws-alb-ingress-controlle too, if mismatch, issue an warning but keep the shield association)
   • no alb.ingress.kubernetes.io/shield-advanced-protection annotation, don't do any shield API calls. (however, we may still need to make shield API to delete shield association when deleting LoadBalancer, i don't have good idea about this yet)

   This way, it won't impose any impact to existing customers when we add new features.
   I always want to do similar things for annotations like alb.ingress.kubernetes.io/waf-acl-id, since currently we have no way to express "don't touch my WAF settings"

[hhamalai]

Thanks for the great feedback, I'm happy to implement your suggestions and ping you once ready.

[hhamalai]

@M00nF1sh I've implemented & tested your proposed changes. Regarding the last one, it's now as you suggested, i.e. when the annotation is missing no Shield Advanced API calls are made. This means, as you noticed, that when annotation is absent, when it previously was present with shield protection enabled, the protection is left present.

Other than leaving it as is, one option would be to go through the current protections in the cache (no API calls made before and will be made if the cache is empty). If there are cache items, then these would be tried to remove. Few issues with this:

1. Cache expiration creates a requirement for another kind of cache
2. The cache usage to track protections creates persistence, if alb-ingress-controller pod is destroyed the cache is lost that the protections are left active as orphans.

[hhamalai]

@M00nF1sh Any comments on the suggested changes / how to push this forward?

[M00nF1sh]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hhamalai, M00nF1sh

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:

• OWNERS [M00nF1sh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[M00nF1sh]

@M00nF1sh Any comments on the suggested changes / how to push this forward?

Thanks for making these changes, i think it's good enough to merge for now.
For the annotation remove issue, i think we should train Cx to explicitly disable shield instead of remove annotations, maybe via docs.

[geofflancaster]

Does this work for NLBs as well?

[hhamalai]

Does this work for NLBs as well?

@geofflancaster , aws-alb-ingress-controller project is about ALBs, not NLBs. If you need NLBs with Shield Advanced enabled, you can use Elastic IPs, enable Shield Advanced protection on those IPs, and associate the IPs with your NLB.

[christopher-wong]

Now that the load balancer controller supports NLBs, is there an NLB annotation to enable Shield Advanced?

[hhamalai]

Now that the load balancer controller supports NLBs, is there an NLB annotation to enable Shield Advanced?

AFAIK Shield Advanced cannot be enabled directly on NLB, but for Elastic IPs attached to NLB instance with
service.beta.kubernetes.io/aws-load-balancer-eip-allocations annotation. These EIPs are not allocated by load balancer controller at the moment, so assigning shield protections to these EIPs should likely also be externally managed.

[abebars]

@hhamalai Since the Controller allows you to pass these EIPs as an optional parameter, Can we have service.beta.kubernetes.io/aws-load-balancer-eip-allocations as a prerequisite to service.beta.kubernetes.io/shield-advanced-protection: 'true' and in that case we can make the correlation between the resources?