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 Request] Ability to specify additional node security group rules #97

Closed
lbogdan opened this issue Apr 1, 2019 · 4 comments
Closed
Assignees
Labels
kind/enhancement Improvements or new features

Comments

@lbogdan
Copy link

lbogdan commented Apr 1, 2019

Currently the node security group rules are hardcoded in securitygroup.ts:35, and the only way to alter them is to create the security group ourselves, copy-paste those rules, add additional ones, and send it in nodeSecurityGroup when creating a cluster or node group.

My specific use case for this is allowing SSH to a cluster / node group's nodes.

In the name of convenience, I can think of 3 ways to improve this:

  1. Be able to specify additional node security group rules when creating clusters / node groups.
  2. Be able to specify a callback that gets the default rules and can alter them (same mechanism as transformations callback in ConfigGroup, for example).
  3. Have an allowSSH property in ClusterOptions / ClusterNodeGroupOptions.
@lbogdan
Copy link
Author

lbogdan commented Apr 3, 2019

The proposed API for 1. would be to add

additionalNodeSecurityGroupRules: pulumi.Input<{
  egress?: pulumi.Input<pulumi.Input<{
      cidrBlocks?: pulumi.Input<pulumi.Input<string>[]>;
      description?: pulumi.Input<string>;
      fromPort: pulumi.Input<number>;
      ipv6CidrBlocks?: pulumi.Input<pulumi.Input<string>[]>;
      prefixListIds?: pulumi.Input<pulumi.Input<string>[]>;
      protocol: pulumi.Input<string>;
      securityGroups?: pulumi.Input<pulumi.Input<string>[]>;
      self?: pulumi.Input<boolean>;
      toPort: pulumi.Input<number>;
  }>[]>,
  ingress?: pulumi.Input<pulumi.Input<{
    cidrBlocks?: pulumi.Input<pulumi.Input<string>[]>;
    description?: pulumi.Input<string>;
    fromPort: pulumi.Input<number>;
    ipv6CidrBlocks?: pulumi.Input<pulumi.Input<string>[]>;
    prefixListIds?: pulumi.Input<pulumi.Input<string>[]>;
    protocol: pulumi.Input<string>;
    securityGroups?: pulumi.Input<pulumi.Input<string>[]>;
    self?: pulumi.Input<boolean>;
    toPort: pulumi.Input<number>;
  }>[]>
}>;

rules that will be simply merged with the default rules in createNodeGroupSecurityGroup(https://github.com/pulumi/pulumi-eks/blob/master/nodejs/eks/securitygroup.ts#L35).

Although I'd prefer 2., which is more flexible (for example, you can't replace the default egress default "allow all" with something more specific by using 1.), I would propose something like

nodeSecurityGroupTransform?: (pulumi.Input<aws.ec2.SecurityGroup>) => pulumi.Input<aws.ec2.SecurityGroup>;

which can either mutate the provided security group, or return a new one.

@lukehoban @metral What do you think?

@metral
Copy link
Contributor

metral commented Apr 4, 2019

I'm thinking option 1 is the best path forward here as it makes expectations clear about the user brings and what the cluster requires.

@lukehoban thoughts?

@d-nishi
Copy link
Contributor

d-nishi commented Apr 9, 2019

@lbogdan @lukehoban @metral

agree that option 1 is better because a) the alb ingress group implementation relies on the EKS node-security group; and b) nodes allow for multiple security groups;

When implementing this the logic 1 thing to consider is that the ALB/ELB/NLB in k8s will use the default security group on the node with the clustertag. If a new SG with the same clustertag is found then an error will be logged, so we need to factor that in when implementing option 1.

@metral
Copy link
Contributor

metral commented Mar 20, 2020

Closing in favor of #275

@metral metral closed this as completed Mar 20, 2020
@infin8x infin8x added the kind/enhancement Improvements or new features label Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests

5 participants