Skip to content

Commit 379a3bd

Browse files
cPu1cPu1
committed
Skip adding egress rules if default egress rule is present
1 parent 3635fe8 commit 379a3bd

File tree

10 files changed

+145
-61
lines changed

10 files changed

+145
-61
lines changed

‎pkg/actions/nodegroup/create.go‎

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,11 @@ func (m *Manager) Create(ctx context.Context, options CreateOpts, nodegroupFilte
5959
return errors.New(msg)
6060
}
6161

62-
isOwnedCluster := true
62+
var (
63+
isOwnedCluster = true
64+
skipEgressRules = false
65+
)
66+
6367
clusterStack, err := m.stackManager.DescribeClusterStack(ctx)
6468
if err != nil {
6569
switch err.(type) {
@@ -72,6 +76,10 @@ func (m *Manager) Create(ctx context.Context, options CreateOpts, nodegroupFilte
7276
return errors.Wrapf(err, "loading VPC spec for cluster %q", meta.Name)
7377
}
7478
isOwnedCluster = false
79+
skipEgressRules, err = validateSecurityGroup(ctx, ctl.AWSProvider.EC2(), cfg.VPC.SecurityGroup)
80+
if err != nil {
81+
return err
82+
}
7583

7684
default:
7785
return fmt.Errorf("getting existing configuration for cluster %q: %w", meta.Name, err)
@@ -156,7 +164,7 @@ func (m *Manager) Create(ctx context.Context, options CreateOpts, nodegroupFilte
156164
return cmdutils.PrintNodeGroupDryRunConfig(clusterConfigCopy, options.DryRunSettings.OutStream)
157165
}
158166

159-
if err := m.nodeCreationTasks(ctx, isOwnedCluster); err != nil {
167+
if err := m.nodeCreationTasks(ctx, isOwnedCluster, skipEgressRules); err != nil {
160168
return err
161169
}
162170

@@ -188,7 +196,7 @@ func makeOutpostsService(clusterConfig *api.ClusterConfig, provider api.ClusterP
188196
}
189197
}
190198

191-
func (m *Manager) nodeCreationTasks(ctx context.Context, isOwnedCluster bool) error {
199+
func (m *Manager) nodeCreationTasks(ctx context.Context, isOwnedCluster, skipEgressRules bool) error {
192200
cfg := m.cfg
193201
meta := cfg.Metadata
194202

@@ -242,7 +250,7 @@ func (m *Manager) nodeCreationTasks(ctx context.Context, isOwnedCluster bool) er
242250
allNodeGroupTasks := &tasks.TaskTree{
243251
Parallel: true,
244252
}
245-
nodeGroupTasks := m.stackManager.NewUnmanagedNodeGroupTask(ctx, cfg.NodeGroups, !awsNodeUsesIRSA, vpcImporter)
253+
nodeGroupTasks := m.stackManager.NewUnmanagedNodeGroupTask(ctx, cfg.NodeGroups, !awsNodeUsesIRSA, skipEgressRules, vpcImporter)
246254
if nodeGroupTasks.Len() > 0 {
247255
allNodeGroupTasks.Append(nodeGroupTasks)
248256
}
@@ -322,13 +330,10 @@ func loadVPCFromConfig(ctx context.Context, provider api.ClusterProvider, cfg *a
322330
logger.Critical("unable to use given %s", cfg.SubnetInfo())
323331
return err
324332
}
325-
if err := cfg.CanUseForPrivateNodeGroups(); err != nil {
326-
return err
327-
}
328-
return validateSecurityGroup(ctx, provider.EC2(), cfg.VPC.SecurityGroup)
333+
return cfg.CanUseForPrivateNodeGroups()
329334
}
330335

331-
func validateSecurityGroup(ctx context.Context, ec2API awsapi.EC2, securityGroupID string) error {
336+
func validateSecurityGroup(ctx context.Context, ec2API awsapi.EC2, securityGroupID string) (hasDefaultEgressRule bool, err error) {
332337
paginator := ec2.NewDescribeSecurityGroupRulesPaginator(ec2API, &ec2.DescribeSecurityGroupRulesInput{
333338
Filters: []ec2types.Filter{
334339
{
@@ -341,22 +346,30 @@ func validateSecurityGroup(ctx context.Context, ec2API awsapi.EC2, securityGroup
341346
for paginator.HasMorePages() {
342347
output, err := paginator.NextPage(ctx)
343348
if err != nil {
344-
return err
349+
return false, err
345350
}
346351
sgRules = append(sgRules, output.SecurityGroupRules...)
347352
}
348353

349354
makeError := func(sgRuleID string) error {
350355
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)
356+
"vpc.securityGroup should not contain any non-default external egress rules on a cluster not created by eksctl (rule ID: %s)", securityGroupID, sgRuleID)
357+
}
358+
359+
isDefaultEgressRule := func(sgRule ec2types.SecurityGroupRule) bool {
360+
return aws.ToString(sgRule.IpProtocol) == "-1" && aws.ToInt32(sgRule.FromPort) == -1 && aws.ToInt32(sgRule.ToPort) == -1 && aws.ToString(sgRule.CidrIpv4) == "0.0.0.0/0"
352361
}
353362

354363
for _, sgRule := range sgRules {
355364
if !aws.ToBool(sgRule.IsEgress) {
356365
continue
357366
}
367+
if !hasDefaultEgressRule && isDefaultEgressRule(sgRule) {
368+
hasDefaultEgressRule = true
369+
continue
370+
}
358371
if !strings.HasPrefix(aws.ToString(sgRule.Description), builder.ControlPlaneEgressRuleDescriptionPrefix) {
359-
return makeError(aws.ToString(sgRule.SecurityGroupRuleId))
372+
return false, makeError(aws.ToString(sgRule.SecurityGroupRuleId))
360373
}
361374
matched := false
362375
for _, egressRule := range builder.ControlPlaneNodeGroupEgressRules {
@@ -368,8 +381,8 @@ func validateSecurityGroup(ctx context.Context, ec2API awsapi.EC2, securityGroup
368381
}
369382
}
370383
if !matched {
371-
return makeError(aws.ToString(sgRule.SecurityGroupRuleId))
384+
return false, makeError(aws.ToString(sgRule.SecurityGroupRuleId))
372385
}
373386
}
374-
return nil
387+
return hasDefaultEgressRule, nil
375388
}

‎pkg/actions/nodegroup/create_test.go‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ type stackManagerDelegate struct {
4949
manager.StackManager
5050
}
5151

52-
func (s *stackManagerDelegate) NewUnmanagedNodeGroupTask(context.Context, []*api.NodeGroup, bool, vpc.Importer) *tasks.TaskTree {
52+
func (s *stackManagerDelegate) NewUnmanagedNodeGroupTask(context.Context, []*api.NodeGroup, bool, bool, vpc.Importer) *tasks.TaskTree {
5353
return &tasks.TaskTree{
5454
Tasks: []tasks.Task{noopTask},
5555
}
@@ -158,7 +158,26 @@ var _ = DescribeTable("Create", func(t ngEntry) {
158158
})
159159

160160
},
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"),
161+
expectedErr: errors.New("vpc.securityGroup (sg-custom) has egress rules that were not attached by eksctl; vpc.securityGroup should not contain any non-default external egress rules on a cluster not created by eksctl (rule ID: sgr-5)"),
162+
}),
163+
164+
Entry("when cluster is unowned and vpc.securityGroup contains a default egress rule, 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, ec2types.SecurityGroupRule{
168+
Description: aws.String(""),
169+
CidrIpv4: aws.String("0.0.0.0/0"),
170+
FromPort: aws.Int32(-1),
171+
ToPort: aws.Int32(-1),
172+
GroupId: aws.String("sg-custom"),
173+
IpProtocol: aws.String("-1"),
174+
IsEgress: aws.Bool(true),
175+
SecurityGroupRuleId: aws.String("sgr-5"),
176+
})
177+
p.MockEC2().On("DescribeImages", mock.Anything, mock.Anything).Return(nil, errors.New("DescribeImages error"))
178+
179+
},
180+
expectedErr: errors.New("DescribeImages error"),
162181
}),
163182

164183
Entry("when cluster is unowned and vpc.securityGroup contains no external egress rules, it passes validation but fails if DescribeImages fails", ngEntry{

‎pkg/cfn/builder/nodegroup.go‎

Lines changed: 36 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,16 @@ const (
3535
taintsPrefix = nodeTemplatePrefix + "taint/"
3636
)
3737

38+
// NodeGroupOptions represents options passed to a NodeGroupResourceSet.
39+
type NodeGroupOptions struct {
40+
ClusterConfig *api.ClusterConfig
41+
NodeGroup *api.NodeGroup
42+
Bootstrapper nodebootstrap.Bootstrapper
43+
ForceAddCNIPolicy bool
44+
VPCImporter vpc.Importer
45+
SkipEgressRules bool
46+
}
47+
3848
// NodeGroupResourceSet stores the resource information of the nodegroup
3949
type NodeGroupResourceSet struct {
4050
rs *resourceSet
@@ -49,19 +59,21 @@ type NodeGroupResourceSet struct {
4959
vpc *gfnt.Value
5060
vpcImporter vpc.Importer
5161
bootstrapper nodebootstrap.Bootstrapper
62+
skipEgressRules bool
5263
}
5364

5465
// NewNodeGroupResourceSet returns a resource set for a nodegroup embedded in a cluster config
55-
func NewNodeGroupResourceSet(ec2API awsapi.EC2, iamAPI awsapi.IAM, spec *api.ClusterConfig, ng *api.NodeGroup, bootstrapper nodebootstrap.Bootstrapper, forceAddCNIPolicy bool, vpcImporter vpc.Importer) *NodeGroupResourceSet {
66+
func NewNodeGroupResourceSet(ec2API awsapi.EC2, iamAPI awsapi.IAM, options NodeGroupOptions) *NodeGroupResourceSet {
5667
return &NodeGroupResourceSet{
5768
rs: newResourceSet(),
58-
forceAddCNIPolicy: forceAddCNIPolicy,
59-
clusterSpec: spec,
60-
spec: ng,
69+
forceAddCNIPolicy: options.ForceAddCNIPolicy,
70+
clusterSpec: options.ClusterConfig,
71+
spec: options.NodeGroup,
6172
ec2API: ec2API,
6273
iamAPI: iamAPI,
63-
vpcImporter: vpcImporter,
64-
bootstrapper: bootstrapper,
74+
vpcImporter: options.VPCImporter,
75+
bootstrapper: options.Bootstrapper,
76+
skipEgressRules: options.SkipEgressRules,
6577
}
6678
}
6779

@@ -194,22 +206,24 @@ func (n *NodeGroupResourceSet) addResourcesForSecurityGroups() {
194206
n.securityGroups = append(n.securityGroups, efaSG)
195207
}
196208

197-
n.newResource("EgressInterCluster", &gfnec2.SecurityGroupEgress{
198-
GroupId: refControlPlaneSG,
199-
DestinationSecurityGroupId: refNodeGroupLocalSG,
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),
204-
})
205-
n.newResource("EgressInterClusterAPI", &gfnec2.SecurityGroupEgress{
206-
GroupId: refControlPlaneSG,
207-
DestinationSecurityGroupId: refNodeGroupLocalSG,
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),
212-
})
209+
if !n.skipEgressRules {
210+
n.newResource("EgressInterCluster", &gfnec2.SecurityGroupEgress{
211+
GroupId: refControlPlaneSG,
212+
DestinationSecurityGroupId: refNodeGroupLocalSG,
213+
Description: gfnt.NewString(ControlPlaneEgressRuleDescriptionPrefix + desc + " (kubelet and workload TCP ports)"),
214+
IpProtocol: gfnt.NewString(controlPlaneEgressInterCluster.IPProtocol),
215+
FromPort: gfnt.NewInteger(controlPlaneEgressInterCluster.FromPort),
216+
ToPort: gfnt.NewInteger(controlPlaneEgressInterCluster.ToPort),
217+
})
218+
n.newResource("EgressInterClusterAPI", &gfnec2.SecurityGroupEgress{
219+
GroupId: refControlPlaneSG,
220+
DestinationSecurityGroupId: refNodeGroupLocalSG,
221+
Description: gfnt.NewString(ControlPlaneEgressRuleDescriptionPrefix + desc + " (workloads using HTTPS port, commonly used with extension API servers)"),
222+
IpProtocol: gfnt.NewString(controlPlaneEgressInterClusterAPI.IPProtocol),
223+
FromPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.FromPort),
224+
ToPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.ToPort),
225+
})
226+
}
213227
n.newResource("IngressInterClusterCP", &gfnec2.SecurityGroupIngress{
214228
GroupId: refControlPlaneSG,
215229
SourceSecurityGroupId: refNodeGroupLocalSG,

‎pkg/cfn/builder/nodegroup_test.go‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/aws/aws-sdk-go-v2/aws"
1212
"github.com/aws/aws-sdk-go-v2/service/ec2"
1313
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
14+
1415
"github.com/weaveworks/eksctl/pkg/testutils/mockprovider"
1516

1617
. "github.com/onsi/ginkgo/v2"
@@ -35,10 +36,12 @@ var _ = Describe("Unmanaged NodeGroup Template Builder", func() {
3536
fakeVPCImporter *vpcfakes.FakeImporter
3637
fakeBootstrapper *bootstrapfakes.FakeBootstrapper
3738
p = mockprovider.NewMockProvider()
39+
skipEgressRules bool
3840
)
3941

4042
BeforeEach(func() {
4143
forceAddCNIPolicy = false
44+
skipEgressRules = false
4245
fakeVPCImporter = new(vpcfakes.FakeImporter)
4346
fakeBootstrapper = new(bootstrapfakes.FakeBootstrapper)
4447
cfg, ng = newClusterAndNodeGroup()
@@ -51,7 +54,14 @@ var _ = Describe("Unmanaged NodeGroup Template Builder", func() {
5154
})
5255

5356
JustBeforeEach(func() {
54-
ngrs = builder.NewNodeGroupResourceSet(p.MockEC2(), p.MockIAM(), cfg, ng, fakeBootstrapper, forceAddCNIPolicy, fakeVPCImporter)
57+
ngrs = builder.NewNodeGroupResourceSet(p.MockEC2(), p.MockIAM(), builder.NodeGroupOptions{
58+
ClusterConfig: cfg,
59+
NodeGroup: ng,
60+
Bootstrapper: fakeBootstrapper,
61+
ForceAddCNIPolicy: forceAddCNIPolicy,
62+
VPCImporter: fakeVPCImporter,
63+
SkipEgressRules: skipEgressRules,
64+
})
5565
})
5666

5767
Describe("AddAllResources", func() {
@@ -1312,6 +1322,24 @@ var _ = Describe("Unmanaged NodeGroup Template Builder", func() {
13121322
Expect(properties.LaunchTemplateData.Monitoring.Enabled).To(Equal(true))
13131323
})
13141324
})
1325+
1326+
Context("skipEgressRules is false", func() {
1327+
It("should add egress rules", func() {
1328+
Expect(ngTemplate.Resources).To(HaveKey("EgressInterCluster"))
1329+
Expect(ngTemplate.Resources).To(HaveKey("EgressInterClusterAPI"))
1330+
})
1331+
})
1332+
1333+
Context("skipEgressRules is true", func() {
1334+
BeforeEach(func() {
1335+
skipEgressRules = true
1336+
})
1337+
1338+
It("should not add egress rules", func() {
1339+
Expect(ngTemplate.Resources).NotTo(HaveKey("EgressInterCluster"))
1340+
Expect(ngTemplate.Resources).NotTo(HaveKey("EgressInterClusterAPI"))
1341+
})
1342+
})
13151343
})
13161344
})
13171345

‎pkg/cfn/manager/create_tasks.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ func (c *StackCollection) NewTasksToCreateClusterWithNodeGroups(ctx context.Cont
4242
Parallel: true,
4343
IsSubTask: true,
4444
}
45-
if unmanagedNodeGroupTasks := c.NewUnmanagedNodeGroupTask(ctx, nodeGroups, false, vpcImporter); unmanagedNodeGroupTasks.Len() > 0 {
45+
if unmanagedNodeGroupTasks := c.NewUnmanagedNodeGroupTask(ctx, nodeGroups, false, false, vpcImporter); unmanagedNodeGroupTasks.Len() > 0 {
4646
unmanagedNodeGroupTasks.IsSubTask = true
4747
nodeGroupTasks.Append(unmanagedNodeGroupTasks)
4848
}
@@ -72,7 +72,7 @@ func (c *StackCollection) NewTasksToCreateClusterWithNodeGroups(ctx context.Cont
7272
}
7373

7474
// NewUnmanagedNodeGroupTask defines tasks required to create all of the nodegroups
75-
func (c *StackCollection) NewUnmanagedNodeGroupTask(ctx context.Context, nodeGroups []*api.NodeGroup, forceAddCNIPolicy bool, vpcImporter vpc.Importer) *tasks.TaskTree {
75+
func (c *StackCollection) NewUnmanagedNodeGroupTask(ctx context.Context, nodeGroups []*api.NodeGroup, forceAddCNIPolicy, skipEgressRules bool, vpcImporter vpc.Importer) *tasks.TaskTree {
7676
taskTree := &tasks.TaskTree{Parallel: true}
7777

7878
for _, ng := range nodeGroups {
@@ -83,6 +83,7 @@ func (c *StackCollection) NewUnmanagedNodeGroupTask(ctx context.Context, nodeGro
8383
stackCollection: c,
8484
forceAddCNIPolicy: forceAddCNIPolicy,
8585
vpcImporter: vpcImporter,
86+
skipEgressRules: skipEgressRules,
8687
})
8788
// TODO: move authconfigmap tasks here using kubernetesTask and kubernetes.CallbackClientSet
8889
}

‎pkg/cfn/manager/fakes/fake_stack_manager.go‎

Lines changed: 12 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/cfn/manager/interface.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ type StackManager interface {
9191
NewTasksToDeleteIAMServiceAccounts(ctx context.Context, serviceAccounts []string, clientSetGetter kubernetes.ClientSetGetter, wait bool) (*tasks.TaskTree, error)
9292
NewTasksToDeleteNodeGroups(stacks []NodeGroupStack, shouldDelete func(_ string) bool, wait bool, cleanup func(chan error, string) error) (*tasks.TaskTree, error)
9393
NewTasksToDeleteOIDCProviderWithIAMServiceAccounts(ctx context.Context, newOIDCManager NewOIDCManager, cluster *ekstypes.Cluster, clientSetGetter kubernetes.ClientSetGetter, force bool) (*tasks.TaskTree, error)
94-
NewUnmanagedNodeGroupTask(ctx context.Context, nodeGroups []*v1alpha5.NodeGroup, forceAddCNIPolicy bool, importer vpc.Importer) *tasks.TaskTree
94+
NewUnmanagedNodeGroupTask(ctx context.Context, nodeGroups []*v1alpha5.NodeGroup, forceAddCNIPolicy, skipEgressRules bool, importer vpc.Importer) *tasks.TaskTree
9595
PropagateManagedNodeGroupTagsToASG(ngName string, ngTags map[string]string, asgNames []string, errCh chan error) error
9696
RefreshFargatePodExecutionRoleARN(ctx context.Context) error
9797
StackStatusIsNotTransitional(s *Stack) bool

0 commit comments

Comments
 (0)