Skip to content

Commit 6f82046

Browse files
cPu1cPu1
committed
Disallow passing a vpc.securityGroup with external SG rules for unowned clusters
1 parent 0c2ced9 commit 6f82046

File tree

4 files changed

+244
-17
lines changed

4 files changed

+244
-17
lines changed

‎pkg/actions/nodegroup/create.go‎

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,19 @@ import (
44
"context"
55
"fmt"
66
"io"
7+
"strings"
8+
9+
"github.com/aws/aws-sdk-go-v2/aws"
10+
"github.com/aws/aws-sdk-go-v2/service/ec2"
11+
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
712

813
"github.com/kris-nova/logger"
914
"github.com/pkg/errors"
1015

1116
defaultaddons "github.com/weaveworks/eksctl/pkg/addons/default"
1217
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
18+
"github.com/weaveworks/eksctl/pkg/awsapi"
19+
"github.com/weaveworks/eksctl/pkg/cfn/builder"
1320
"github.com/weaveworks/eksctl/pkg/cfn/manager"
1421
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
1522
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils/filter"
@@ -311,11 +318,58 @@ func loadVPCFromConfig(ctx context.Context, provider api.ClusterProvider, cfg *a
311318
if err := vpc.ImportSubnetsFromSpec(ctx, provider, cfg); err != nil {
312319
return err
313320
}
314-
315321
if err := cfg.HasSufficientSubnets(); err != nil {
316322
logger.Critical("unable to use given %s", cfg.SubnetInfo())
317323
return err
318324
}
325+
if err := cfg.CanUseForPrivateNodeGroups(); err != nil {
326+
return err
327+
}
328+
return validateSecurityGroup(ctx, provider.EC2(), cfg.VPC.SecurityGroup)
329+
}
330+
331+
func validateSecurityGroup(ctx context.Context, ec2API awsapi.EC2, securityGroupID string) error {
332+
paginator := ec2.NewDescribeSecurityGroupRulesPaginator(ec2API, &ec2.DescribeSecurityGroupRulesInput{
333+
Filters: []ec2types.Filter{
334+
{
335+
Name: aws.String("group-id"),
336+
Values: []string{securityGroupID},
337+
},
338+
},
339+
})
340+
var sgRules []ec2types.SecurityGroupRule
341+
for paginator.HasMorePages() {
342+
output, err := paginator.NextPage(ctx)
343+
if err != nil {
344+
return err
345+
}
346+
sgRules = append(sgRules, output.SecurityGroupRules...)
347+
}
319348

320-
return cfg.CanUseForPrivateNodeGroups()
349+
makeError := func(sgRuleID string) error {
350+
return fmt.Errorf("vpc.securityGroup (%s) has egress rules that were not attached by eksctl; "+
351+
"vpc.securityGroup should not contain any external egress rules on a cluster not created by eksctl (rule ID: %s)", securityGroupID, sgRuleID)
352+
}
353+
354+
for _, sgRule := range sgRules {
355+
if !aws.ToBool(sgRule.IsEgress) {
356+
continue
357+
}
358+
if !strings.HasPrefix(aws.ToString(sgRule.Description), builder.ControlPlaneEgressRuleDescriptionPrefix) {
359+
return makeError(aws.ToString(sgRule.SecurityGroupRuleId))
360+
}
361+
matched := false
362+
for _, egressRule := range builder.ControlPlaneNodeGroupEgressRules {
363+
if aws.ToString(sgRule.IpProtocol) == egressRule.IpProtocol &&
364+
aws.ToInt32(sgRule.FromPort) == int32(egressRule.FromPort) &&
365+
aws.ToInt32(sgRule.ToPort) == int32(egressRule.ToPort) {
366+
matched = true
367+
break
368+
}
369+
}
370+
if !matched {
371+
return makeError(aws.ToString(sgRule.SecurityGroupRuleId))
372+
}
373+
}
374+
return nil
321375
}

‎pkg/actions/nodegroup/create_test.go‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,33 @@ var _ = DescribeTable("Create", func(t ngEntry) {
144144
expectedErr: errors.Wrapf(errors.New("VPC configuration required for creating nodegroups on clusters not owned by eksctl: vpc.subnets, vpc.id, vpc.securityGroup"), "loading VPC spec for cluster %q", "my-cluster"),
145145
}),
146146

147+
Entry("when cluster is unowned and vpc.securityGroup contains external egress rules, it fails validation", ngEntry{
148+
updateClusterConfig: makeUnownedClusterConfig,
149+
mockCalls: func(k *fakes.FakeKubeProvider, f *utilFakes.FakeNodegroupFilter, p *mockprovider.MockProvider, _ *fake.Clientset) {
150+
mockProviderForUnownedCluster(p, k, ec2types.SecurityGroupRule{
151+
Description: aws.String("Allow control plane to communicate with a custom nodegroup on a custom port"),
152+
FromPort: aws.Int32(8443),
153+
ToPort: aws.Int32(8443),
154+
GroupId: aws.String("sg-custom"),
155+
IpProtocol: aws.String("https"),
156+
IsEgress: aws.Bool(true),
157+
SecurityGroupRuleId: aws.String("sgr-5"),
158+
})
159+
160+
},
161+
expectedErr: fmt.Errorf("loading VPC spec for cluster %q: vpc.securityGroup (sg-custom) has egress rules that were not attached by eksctl; vpc.securityGroup should not contain any external egress rules on a cluster not created by eksctl (rule ID: sgr-5)", "my-cluster"),
162+
}),
163+
164+
Entry("when cluster is unowned and vpc.securityGroup contains no external egress rules, it passes validation but fails if DescribeImages fails", ngEntry{
165+
updateClusterConfig: makeUnownedClusterConfig,
166+
mockCalls: func(k *fakes.FakeKubeProvider, f *utilFakes.FakeNodegroupFilter, p *mockprovider.MockProvider, _ *fake.Clientset) {
167+
mockProviderForUnownedCluster(p, k)
168+
p.MockEC2().On("DescribeImages", mock.Anything, mock.Anything).Return(nil, errors.New("DescribeImages error"))
169+
170+
},
171+
expectedErr: errors.New("DescribeImages error"),
172+
}),
173+
147174
Entry("fails when cluster is not compatible with ng config", ngEntry{
148175
mockCalls: func(k *fakes.FakeKubeProvider, f *utilFakes.FakeNodegroupFilter, p *mockprovider.MockProvider, _ *fake.Clientset) {
149176
// no shared security group will trigger a compatibility check failure later in the call chain.
@@ -587,3 +614,122 @@ func mockProviderWithConfig(p *mockprovider.MockProvider, describeStacksOutput [
587614
},
588615
}, nil)
589616
}
617+
618+
func mockProviderForUnownedCluster(p *mockprovider.MockProvider, k *fakes.FakeKubeProvider, extraSGRules ...ec2types.SecurityGroupRule) {
619+
k.NewRawClientReturns(&kubernetes.RawClient{}, nil)
620+
k.ServerVersionReturns("1.27", nil)
621+
p.MockCloudFormation().On("ListStacks", mock.Anything, mock.Anything).Return(&cloudformation.ListStacksOutput{
622+
StackSummaries: []cftypes.StackSummary{
623+
{
624+
StackName: aws.String("eksctl-my-cluster-cluster"),
625+
StackStatus: "CREATE_COMPLETE",
626+
},
627+
},
628+
}, nil)
629+
p.MockCloudFormation().On("DescribeStacks", mock.Anything, mock.Anything).Return(&cloudformation.DescribeStacksOutput{
630+
Stacks: []cftypes.Stack{
631+
{
632+
StackName: aws.String("eksctl-my-cluster-cluster"),
633+
StackStatus: "CREATE_COMPLETE",
634+
},
635+
},
636+
}, nil)
637+
638+
vpcID := aws.String("vpc-custom")
639+
p.MockEC2().On("DescribeVpcs", mock.Anything, mock.Anything).Return(&ec2.DescribeVpcsOutput{
640+
Vpcs: []ec2types.Vpc{
641+
{
642+
CidrBlock: aws.String("192.168.0.0/19"),
643+
VpcId: vpcID,
644+
CidrBlockAssociationSet: []ec2types.VpcCidrBlockAssociation{
645+
{
646+
CidrBlock: aws.String("192.168.0.0/19"),
647+
},
648+
},
649+
},
650+
},
651+
}, nil)
652+
p.MockEC2().On("DescribeSubnets", mock.Anything, mock.Anything).Return(&ec2.DescribeSubnetsOutput{
653+
Subnets: []ec2types.Subnet{
654+
{
655+
SubnetId: aws.String("subnet-custom1"),
656+
CidrBlock: aws.String("192.168.160.0/19"),
657+
AvailabilityZone: aws.String("us-west-2a"),
658+
VpcId: vpcID,
659+
},
660+
{
661+
SubnetId: aws.String("subnet-custom2"),
662+
CidrBlock: aws.String("192.168.96.0/19"),
663+
AvailabilityZone: aws.String("us-west-2b"),
664+
VpcId: vpcID,
665+
},
666+
},
667+
}, nil)
668+
669+
sgID := aws.String("sg-custom")
670+
p.MockEC2().On("DescribeSecurityGroupRules", mock.Anything, mock.MatchedBy(func(input *ec2.DescribeSecurityGroupRulesInput) bool {
671+
if len(input.Filters) != 1 {
672+
return false
673+
}
674+
filter := input.Filters[0]
675+
return *filter.Name == "group-id" && len(filter.Values) == 1 && filter.Values[0] == *sgID
676+
})).Return(&ec2.DescribeSecurityGroupRulesOutput{
677+
SecurityGroupRules: append([]ec2types.SecurityGroupRule{
678+
{
679+
Description: aws.String("Allow control plane to communicate with worker nodes in group ng-1 (kubelet and workload TCP ports"),
680+
FromPort: aws.Int32(1025),
681+
ToPort: aws.Int32(65535),
682+
GroupId: sgID,
683+
IpProtocol: aws.String("tcp"),
684+
IsEgress: aws.Bool(true),
685+
SecurityGroupRuleId: aws.String("sgr-1"),
686+
},
687+
{
688+
Description: aws.String("Allow control plane to communicate with worker nodes in group ng-1 (workload using HTTPS port, commonly used with extension API servers"),
689+
FromPort: aws.Int32(443),
690+
ToPort: aws.Int32(443),
691+
GroupId: sgID,
692+
IpProtocol: aws.String("tcp"),
693+
IsEgress: aws.Bool(true),
694+
SecurityGroupRuleId: aws.String("sgr-2"),
695+
},
696+
{
697+
Description: aws.String("Allow control plane to receive API requests from worker nodes in group ng-1"),
698+
FromPort: aws.Int32(443),
699+
ToPort: aws.Int32(443),
700+
GroupId: sgID,
701+
IpProtocol: aws.String("tcp"),
702+
IsEgress: aws.Bool(false),
703+
SecurityGroupRuleId: aws.String("sgr-3"),
704+
},
705+
{
706+
Description: aws.String("Allow control plane to communicate with worker nodes in group ng-2 (workload using HTTPS port, commonly used with extension API servers"),
707+
FromPort: aws.Int32(443),
708+
ToPort: aws.Int32(443),
709+
GroupId: sgID,
710+
IpProtocol: aws.String("tcp"),
711+
IsEgress: aws.Bool(true),
712+
SecurityGroupRuleId: aws.String("sgr-4"),
713+
},
714+
}, extraSGRules...),
715+
}, nil)
716+
}
717+
718+
func makeUnownedClusterConfig(clusterConfig *api.ClusterConfig) {
719+
clusterConfig.VPC = &api.ClusterVPC{
720+
SecurityGroup: "sg-custom",
721+
Network: api.Network{
722+
ID: "vpc-custom",
723+
},
724+
Subnets: &api.ClusterSubnets{
725+
Private: api.AZSubnetMapping{
726+
"us-west-2a": api.AZSubnetSpec{
727+
ID: "subnet-custom1",
728+
},
729+
"us-west-2b": api.AZSubnetSpec{
730+
ID: "subnet-custom2",
731+
},
732+
},
733+
},
734+
}
735+
}

‎pkg/cfn/builder/nodegroup.go‎

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,34 @@ func (n *NodeGroupResourceSet) AddAllResources(ctx context.Context) error {
132132
return n.addResourcesForNodeGroup(ctx)
133133
}
134134

135+
// A PartialEgressRule represents a partial security group egress rule.
136+
type PartialEgressRule struct {
137+
FromPort int
138+
ToPort int
139+
IpProtocol string
140+
}
141+
142+
var controlPlaneEgressInterCluster = PartialEgressRule{
143+
FromPort: 1025,
144+
ToPort: 65535,
145+
IpProtocol: "tcp",
146+
}
147+
148+
var controlPlaneEgressInterClusterAPI = PartialEgressRule{
149+
FromPort: 443,
150+
ToPort: 443,
151+
IpProtocol: "tcp",
152+
}
153+
154+
// ControlPlaneNodeGroupEgressRules is a slice of egress rules attached to the control plane security group.
155+
var ControlPlaneNodeGroupEgressRules = []PartialEgressRule{
156+
controlPlaneEgressInterCluster,
157+
controlPlaneEgressInterClusterAPI,
158+
}
159+
160+
// ControlPlaneEgressRuleDescriptionPrefix is the prefix applied to the description for control plane security group egress rules.
161+
var ControlPlaneEgressRuleDescriptionPrefix = "Allow control plane to communicate with "
162+
135163
func (n *NodeGroupResourceSet) addResourcesForSecurityGroups() {
136164
for _, id := range n.spec.SecurityGroups.AttachIDs {
137165
n.securityGroups = append(n.securityGroups, gfnt.NewString(id))
@@ -169,18 +197,18 @@ func (n *NodeGroupResourceSet) addResourcesForSecurityGroups() {
169197
n.newResource("EgressInterCluster", &gfnec2.SecurityGroupEgress{
170198
GroupId: refControlPlaneSG,
171199
DestinationSecurityGroupId: refNodeGroupLocalSG,
172-
Description: gfnt.NewString("Allow control plane to communicate with " + desc + " (kubelet and workload TCP ports)"),
173-
IpProtocol: sgProtoTCP,
174-
FromPort: sgMinNodePort,
175-
ToPort: sgMaxNodePort,
200+
Description: gfnt.NewString(ControlPlaneEgressRuleDescriptionPrefix + desc + " (kubelet and workload TCP ports)"),
201+
IpProtocol: gfnt.NewString(controlPlaneEgressInterCluster.IpProtocol),
202+
FromPort: gfnt.NewInteger(controlPlaneEgressInterCluster.FromPort),
203+
ToPort: gfnt.NewInteger(controlPlaneEgressInterCluster.ToPort),
176204
})
177205
n.newResource("EgressInterClusterAPI", &gfnec2.SecurityGroupEgress{
178206
GroupId: refControlPlaneSG,
179207
DestinationSecurityGroupId: refNodeGroupLocalSG,
180-
Description: gfnt.NewString("Allow control plane to communicate with " + desc + " (workloads using HTTPS port, commonly used with extension API servers)"),
181-
IpProtocol: sgProtoTCP,
182-
FromPort: sgPortHTTPS,
183-
ToPort: sgPortHTTPS,
208+
Description: gfnt.NewString(ControlPlaneEgressRuleDescriptionPrefix + desc + " (workloads using HTTPS port, commonly used with extension API servers)"),
209+
IpProtocol: gfnt.NewString(controlPlaneEgressInterClusterAPI.IpProtocol),
210+
FromPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.FromPort),
211+
ToPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.ToPort),
184212
})
185213
n.newResource("IngressInterClusterCP", &gfnec2.SecurityGroupIngress{
186214
GroupId: refControlPlaneSG,
@@ -197,16 +225,16 @@ func makeNodeIngressRules(ng *api.NodeGroupBase, controlPlaneSG *gfnt.Value, vpc
197225
{
198226
SourceSecurityGroupId: controlPlaneSG,
199227
Description: gfnt.NewString(fmt.Sprintf("[IngressInterCluster] Allow %s to communicate with control plane (kubelet and workload TCP ports)", description)),
200-
IpProtocol: sgProtoTCP,
201-
FromPort: sgMinNodePort,
202-
ToPort: sgMaxNodePort,
228+
IpProtocol: gfnt.NewString(controlPlaneEgressInterCluster.IpProtocol),
229+
FromPort: gfnt.NewInteger(controlPlaneEgressInterCluster.FromPort),
230+
ToPort: gfnt.NewInteger(controlPlaneEgressInterCluster.ToPort),
203231
},
204232
{
205233
SourceSecurityGroupId: controlPlaneSG,
206234
Description: gfnt.NewString(fmt.Sprintf("[IngressInterClusterAPI] Allow %s to communicate with control plane (workloads using HTTPS port, commonly used with extension API servers)", description)),
207-
IpProtocol: sgProtoTCP,
208-
FromPort: sgPortHTTPS,
209-
ToPort: sgPortHTTPS,
235+
IpProtocol: gfnt.NewString(controlPlaneEgressInterClusterAPI.IpProtocol),
236+
FromPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.FromPort),
237+
ToPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.ToPort),
210238
},
211239
}
212240

‎pkg/cfn/builder/vpc_ipv4.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,6 @@ var (
335335
sgSourceAnywhereIPv6 = gfnt.NewString("::/0")
336336

337337
sgPortZero = gfnt.NewInteger(0)
338-
sgMinNodePort = gfnt.NewInteger(1025)
339338
sgMaxNodePort = gfnt.NewInteger(65535)
340339

341340
sgPortHTTPS = gfnt.NewInteger(443)

0 commit comments

Comments
 (0)