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

Support managing multiple tagged security groups for a node #666

Open
damdo opened this issue Sep 18, 2023 · 12 comments
Open

Support managing multiple tagged security groups for a node #666

damdo opened this issue Sep 18, 2023 · 12 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@damdo
Copy link
Member

damdo commented Sep 18, 2023

What would you like to be added:
Add support for managing multiple tagged security groups for a node.

Why is this needed:
At the moment we don't support finding and managing multiple tagged SecurityGroups per node (only one), and when we find more than one we error (ref code here).

As such if a node has 2 or more tagged SecurityGroups we would fail to modify them in order to allow inbound traffic from the LB SecurityGroup.

Is there anything preventing us to extend this functionality to more than one tagged SecurityGroup?
If not, would it be possible to consider implementing it?
Thanks

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 18, 2023
@damdo
Copy link
Member Author

damdo commented Sep 25, 2023

@cartermckinnon do you have any thoughts on this one?
Thanks!

@damdo
Copy link
Member Author

damdo commented Oct 2, 2023

cc. @dims

@dims
Copy link
Member

dims commented Oct 2, 2023

cc @kmala @oliviassss @M00nF1sh

@kmala
Copy link
Member

kmala commented Oct 2, 2023

I don't think there is any technical issue for having more than one tagged security groups as far as i can tell. This code was added long back when SG's were enabled which used to be a warning but latest got changed to error with the reason mentioned as comment for the function . So, i think its mostly security related than any limitation. Can you please describe your use case a bit more in detail on why/how the instance would have multiple SG with tags?

@damdo
Copy link
Member Author

damdo commented Oct 5, 2023

Sure @kmala.
By default when we install Kubernetes we create a worker security group (SG1), which we then tag for CCM ownership.
Then we let users create extra security groups (SG2,..., SGx) that they can attach to the worker instances without touching the default worker SG (SG1).

When one of those workers get attached to the Load Balancer, the LB security group rules are propagated by the CCM to the tagged Security Group attached to the worker instance.

Here though, in our case, we have multiple security groups, but since the CCM only considers one SG with the tag, the rule changes get propagated only to that (SG1), and not also to the other SGs attached (SG2,..., SGx).

This causes traffic issues.

@kmala
Copy link
Member

kmala commented Oct 13, 2023

/triage accepted

@k8s-ci-robot
Copy link
Contributor

@kmala: The label triage/accepted cannot be applied. Only GitHub organization members can add the label.

In response to this:

/triage accepted

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.

@kmala
Copy link
Member

kmala commented Oct 13, 2023

Sounds good. I think we can add support for this.

@cartermckinnon
Copy link
Contributor

cartermckinnon commented Oct 13, 2023

/triage accepted

sounds legit to me!

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 13, 2023
@damdo
Copy link
Member Author

damdo commented Oct 27, 2023

Great! Thanks @kmala & @cartermckinnon
How do you think it is best to proceed here to implement this?

@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Oct 26, 2024
@damdo
Copy link
Member Author

damdo commented Oct 27, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

6 participants