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

Feature: Annotation for Default Security Group Rules #691

Closed
iAnomaly opened this issue Oct 24, 2018 · 13 comments

Comments

@iAnomaly
Copy link

commented Oct 24, 2018

Currently bring your own security group(s) is supported with the annotation alb.ingress.kubernetes.io/security-groups and absent that, you get a default SG on the ALB AND on the target instance(s) for the configured protocols and ports.

While this is a great start, it would also be convenient to have a middle ground where the default SGs could be configured with a custom whitelist not unlike the Kubernetes Service resource LoadBalancer type's loadBalancerSourceRanges annotation. Perhaps something like alb.ingress.kubernetes.io/default-security-group-source-ranges: that accepts a list of IP CIDR or SG IDS? This avoids having to maintain the security group(s) externally but still enables custom rules. This list should override the current default 0.0.0.0/0 rule; not merge with it such that 0.0.0.0/0 could be optionally omitted.

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator

commented Oct 26, 2018

Hi, what's your use case for custom rules for the default created securityGroup for supporting ingresses?
Since it's only purpose is for the alb to reach it.

@iAnomaly

This comment has been minimized.

Copy link
Author

commented Oct 29, 2018

Hi @M00nF1sh.

So obviously this is a proposal to expand the original purpose; I understand the current intent. For our use case we use the LoadBalancer Kubernetes Service type which supports whitelisting rules directly in the manifest without having to create and manage additional security groups somewhere else like CloudFormation, APIs, etc.

@iAnomaly

This comment has been minimized.

Copy link
Author

commented Nov 5, 2018

Ideally I'm just looking for the ALB Ingress Controller analog of this existing feature: https://kubernetes.io/docs/tasks/access-application-cluster/configure-cloud-provider-firewall/#restrict-access-for-loadbalancer-service

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator

commented Nov 5, 2018

/kind feature
Sorry for the late reply. This looks like a legit use case. We can add support for this(limit access to the internet facing LB)

BTW, not sure whether you are aware of this, if you only want your loadBalancer be accessible within your VPC, you can use alb.ingress.kubernetes.io/scheme:internal.

@iAnomaly

This comment has been minimized.

Copy link
Author

commented Nov 5, 2018

Thank you @M00nF1sh! Yes, I am aware of alb.ingress.kubernetes.io/scheme:internal and we're using that for internal ingresses.

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator

commented Nov 12, 2018

@iAnomaly Hi, I just noticed that this feature is already supported. You can add alb.ingress.kubernetes.io/security-group-inbound-cidrs: 10.0.0.0/24 annotation for it.

I'll update the docs for this in next release

@iAnomaly

This comment has been minimized.

Copy link
Author

commented Nov 13, 2018

Great finding this @M00nF1sh. I just tried it:

alb.ingress.kubernetes.io/security-group-inbound-cidrs:
  - 10.0.0.0/24
  - 10.0.1.0/24

But it fails:
error: error validating "deploy/ingress.sbxdev.yaml": error validating data: ValidationError(Ingress.metadata.annotations.alb.ingress.kubernetes.io/security-group-inbound-cidrs): invalid type for io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta.annotations: got "array", expected "string"; if you choose to ignore these errors, turn validation off with --validate=false

Given that it only supports type "string" the workaround for now is to use YAML's folded style block scalar with the strip block chomping to exclude the final newline:

alb.ingress.kubernetes.io/security-group-inbound-cidrs: >-
  10.0.0.0/24,
  10.0.1.0/24

We can make this work for now @M00nF1sh but I would love to see type "array" support added to allow additional YAML syntactic sugar such as inline comments and dropping trailing commas:

alb.ingress.kubernetes.io/security-group-inbound-cidrs:
  # Office CIDR
  - 10.0.0.0/24
  # Some other CIDR
  - 10.0.1.0/24

Thanks again for finding this.

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator

commented Nov 13, 2018

@iAnomaly hi, that annotation is actually comma-separated string, you should use 10.0.0.0/24,10.0.1.0/24 instead. 😄

@iAnomaly

This comment has been minimized.

Copy link
Author

commented Nov 13, 2018

Yes, thanks @M00nF1sh. As you can see I figured that out. 😄 Like I said, we can work with this for now but we would still love to see array support for the reasons I enumerated above. I appreciate your help!

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 26, 2019

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

This comment has been minimized.

Copy link

commented May 27, 2019

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

@fejta-bot

This comment has been minimized.

Copy link

commented Jun 26, 2019

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.