Skip to content

Disallow passing a vpc.securityGroup with external security group rules #7030

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.

Sign up for GitHub

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

Merged
cPu1 merged 2 commits into from
Sep 5, 2023
Merged

Disallow passing a vpc.securityGroup with external security group rules #7030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3635fe8
Disallow passing a vpc.securityGroup with external SG rules for unown…
cPu1 Aug 31, 2023
379a3bd
Skip adding egress rules if default egress rule is present
cPu1 Sep 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 73 additions & 6 deletions pkg/actions/nodegroup/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,19 @@ import (
"context"
"fmt"
"io"
"strings"

"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/service/ec2"
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"

"github.com/kris-nova/logger"
"github.com/pkg/errors"

defaultaddons "github.com/weaveworks/eksctl/pkg/addons/default"
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/awsapi"
"github.com/weaveworks/eksctl/pkg/cfn/builder"
"github.com/weaveworks/eksctl/pkg/cfn/manager"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils/filter"
Expand Down Expand Up @@ -52,7 +59,11 @@ func (m *Manager) Create(ctx context.Context, options CreateOpts, nodegroupFilte
return errors.New(msg)
}

isOwnedCluster := true
var (
isOwnedCluster = true
skipEgressRules = false
)

clusterStack, err := m.stackManager.DescribeClusterStack(ctx)
if err != nil {
switch err.(type) {
Expand All @@ -65,6 +76,10 @@ func (m *Manager) Create(ctx context.Context, options CreateOpts, nodegroupFilte
return errors.Wrapf(err, "loading VPC spec for cluster %q", meta.Name)
}
isOwnedCluster = false
skipEgressRules, err = validateSecurityGroup(ctx, ctl.AWSProvider.EC2(), cfg.VPC.SecurityGroup)
if err != nil {
return err
}

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

if err := m.nodeCreationTasks(ctx, isOwnedCluster); err != nil {
if err := m.nodeCreationTasks(ctx, isOwnedCluster, skipEgressRules); err != nil {
return err
}

Expand Down Expand Up @@ -181,7 +196,7 @@ func makeOutpostsService(clusterConfig *api.ClusterConfig, provider api.ClusterP
}
}

func (m *Manager) nodeCreationTasks(ctx context.Context, isOwnedCluster bool) error {
func (m *Manager) nodeCreationTasks(ctx context.Context, isOwnedCluster, skipEgressRules bool) error {
cfg := m.cfg
meta := cfg.Metadata

Expand Down Expand Up @@ -235,7 +250,7 @@ func (m *Manager) nodeCreationTasks(ctx context.Context, isOwnedCluster bool) er
allNodeGroupTasks := &tasks.TaskTree{
Parallel: true,
}
nodeGroupTasks := m.stackManager.NewUnmanagedNodeGroupTask(ctx, cfg.NodeGroups, !awsNodeUsesIRSA, vpcImporter)
nodeGroupTasks := m.stackManager.NewUnmanagedNodeGroupTask(ctx, cfg.NodeGroups, !awsNodeUsesIRSA, skipEgressRules, vpcImporter)
if nodeGroupTasks.Len() > 0 {
allNodeGroupTasks.Append(nodeGroupTasks)
}
Expand Down Expand Up @@ -311,11 +326,63 @@ func loadVPCFromConfig(ctx context.Context, provider api.ClusterProvider, cfg *a
if err := vpc.ImportSubnetsFromSpec(ctx, provider, cfg); err != nil {
return err
}

if err := cfg.HasSufficientSubnets(); err != nil {
logger.Critical("unable to use given %s", cfg.SubnetInfo())
return err
}

return cfg.CanUseForPrivateNodeGroups()
}

func validateSecurityGroup(ctx context.Context, ec2API awsapi.EC2, securityGroupID string) (hasDefaultEgressRule bool, err error) {
paginator := ec2.NewDescribeSecurityGroupRulesPaginator(ec2API, &ec2.DescribeSecurityGroupRulesInput{
Filters: []ec2types.Filter{
{
Name: aws.String("group-id"),
Values: []string{securityGroupID},
},
},
})
var sgRules []ec2types.SecurityGroupRule
for paginator.HasMorePages() {
output, err := paginator.NextPage(ctx)
if err != nil {
return false, err
}
sgRules = append(sgRules, output.SecurityGroupRules...)
}

makeError := func(sgRuleID string) error {
return fmt.Errorf("vpc.securityGroup (%s) 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: %s)", securityGroupID, sgRuleID)
}

isDefaultEgressRule := func(sgRule ec2types.SecurityGroupRule) bool {
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"
}

for _, sgRule := range sgRules {
if !aws.ToBool(sgRule.IsEgress) {
continue
}
if !hasDefaultEgressRule && isDefaultEgressRule(sgRule) {
hasDefaultEgressRule = true
continue
}
if !strings.HasPrefix(aws.ToString(sgRule.Description), builder.ControlPlaneEgressRuleDescriptionPrefix) {
return false, makeError(aws.ToString(sgRule.SecurityGroupRuleId))
}
matched := false
for _, egressRule := range builder.ControlPlaneNodeGroupEgressRules {
if aws.ToString(sgRule.IpProtocol) == egressRule.IPProtocol &&
aws.ToInt32(sgRule.FromPort) == int32(egressRule.FromPort) &&
aws.ToInt32(sgRule.ToPort) == int32(egressRule.ToPort) {
matched = true
break
}
}
if !matched {
return false, makeError(aws.ToString(sgRule.SecurityGroupRuleId))
}
}
return hasDefaultEgressRule, nil
}
167 changes: 166 additions & 1 deletion pkg/actions/nodegroup/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ type stackManagerDelegate struct {
manager.StackManager
}

func (s *stackManagerDelegate) NewUnmanagedNodeGroupTask(context.Context, []*api.NodeGroup, bool, vpc.Importer) *tasks.TaskTree {
func (s *stackManagerDelegate) NewUnmanagedNodeGroupTask(context.Context, []*api.NodeGroup, bool, bool, vpc.Importer) *tasks.TaskTree {
return &tasks.TaskTree{
Tasks: []tasks.Task{noopTask},
}
Expand Down Expand Up @@ -144,6 +144,52 @@ var _ = DescribeTable("Create", func(t ngEntry) {
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"),
}),

Entry("when cluster is unowned and vpc.securityGroup contains external egress rules, it fails validation", ngEntry{
updateClusterConfig: makeUnownedClusterConfig,
mockCalls: func(k *fakes.FakeKubeProvider, f *utilFakes.FakeNodegroupFilter, p *mockprovider.MockProvider, _ *fake.Clientset) {
mockProviderForUnownedCluster(p, k, ec2types.SecurityGroupRule{
Description: aws.String("Allow control plane to communicate with a custom nodegroup on a custom port"),
FromPort: aws.Int32(8443),
ToPort: aws.Int32(8443),
GroupId: aws.String("sg-custom"),
IpProtocol: aws.String("https"),
IsEgress: aws.Bool(true),
SecurityGroupRuleId: aws.String("sgr-5"),
})

},
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)"),
}),

Entry("when cluster is unowned and vpc.securityGroup contains a default egress rule, it passes validation but fails if DescribeImages fails", ngEntry{
updateClusterConfig: makeUnownedClusterConfig,
mockCalls: func(k *fakes.FakeKubeProvider, f *utilFakes.FakeNodegroupFilter, p *mockprovider.MockProvider, _ *fake.Clientset) {
mockProviderForUnownedCluster(p, k, ec2types.SecurityGroupRule{
Description: aws.String(""),
CidrIpv4: aws.String("0.0.0.0/0"),
FromPort: aws.Int32(-1),
ToPort: aws.Int32(-1),
GroupId: aws.String("sg-custom"),
IpProtocol: aws.String("-1"),
IsEgress: aws.Bool(true),
SecurityGroupRuleId: aws.String("sgr-5"),
})
p.MockEC2().On("DescribeImages", mock.Anything, mock.Anything).Return(nil, errors.New("DescribeImages error"))

},
expectedErr: errors.New("DescribeImages error"),
}),

Entry("when cluster is unowned and vpc.securityGroup contains no external egress rules, it passes validation but fails if DescribeImages fails", ngEntry{
updateClusterConfig: makeUnownedClusterConfig,
mockCalls: func(k *fakes.FakeKubeProvider, f *utilFakes.FakeNodegroupFilter, p *mockprovider.MockProvider, _ *fake.Clientset) {
mockProviderForUnownedCluster(p, k)
p.MockEC2().On("DescribeImages", mock.Anything, mock.Anything).Return(nil, errors.New("DescribeImages error"))

},
expectedErr: errors.New("DescribeImages error"),
}),

Entry("fails when cluster is not compatible with ng config", ngEntry{
mockCalls: func(k *fakes.FakeKubeProvider, f *utilFakes.FakeNodegroupFilter, p *mockprovider.MockProvider, _ *fake.Clientset) {
// no shared security group will trigger a compatibility check failure later in the call chain.
Expand Down Expand Up @@ -587,3 +633,122 @@ func mockProviderWithConfig(p *mockprovider.MockProvider, describeStacksOutput [
},
}, nil)
}

func mockProviderForUnownedCluster(p *mockprovider.MockProvider, k *fakes.FakeKubeProvider, extraSGRules ...ec2types.SecurityGroupRule) {
k.NewRawClientReturns(&kubernetes.RawClient{}, nil)
k.ServerVersionReturns("1.27", nil)
p.MockCloudFormation().On("ListStacks", mock.Anything, mock.Anything).Return(&cloudformation.ListStacksOutput{
StackSummaries: []cftypes.StackSummary{
{
StackName: aws.String("eksctl-my-cluster-cluster"),
StackStatus: "CREATE_COMPLETE",
},
},
}, nil)
p.MockCloudFormation().On("DescribeStacks", mock.Anything, mock.Anything).Return(&cloudformation.DescribeStacksOutput{
Stacks: []cftypes.Stack{
{
StackName: aws.String("eksctl-my-cluster-cluster"),
StackStatus: "CREATE_COMPLETE",
},
},
}, nil)

vpcID := aws.String("vpc-custom")
p.MockEC2().On("DescribeVpcs", mock.Anything, mock.Anything).Return(&ec2.DescribeVpcsOutput{
Vpcs: []ec2types.Vpc{
{
CidrBlock: aws.String("192.168.0.0/19"),
VpcId: vpcID,
CidrBlockAssociationSet: []ec2types.VpcCidrBlockAssociation{
{
CidrBlock: aws.String("192.168.0.0/19"),
},
},
},
},
}, nil)
p.MockEC2().On("DescribeSubnets", mock.Anything, mock.Anything).Return(&ec2.DescribeSubnetsOutput{
Subnets: []ec2types.Subnet{
{
SubnetId: aws.String("subnet-custom1"),
CidrBlock: aws.String("192.168.160.0/19"),
AvailabilityZone: aws.String("us-west-2a"),
VpcId: vpcID,
},
{
SubnetId: aws.String("subnet-custom2"),
CidrBlock: aws.String("192.168.96.0/19"),
AvailabilityZone: aws.String("us-west-2b"),
VpcId: vpcID,
},
},
}, nil)

sgID := aws.String("sg-custom")
p.MockEC2().On("DescribeSecurityGroupRules", mock.Anything, mock.MatchedBy(func(input *ec2.DescribeSecurityGroupRulesInput) bool {
if len(input.Filters) != 1 {
return false
}
filter := input.Filters[0]
return *filter.Name == "group-id" && len(filter.Values) == 1 && filter.Values[0] == *sgID
})).Return(&ec2.DescribeSecurityGroupRulesOutput{
SecurityGroupRules: append([]ec2types.SecurityGroupRule{
{
Description: aws.String("Allow control plane to communicate with worker nodes in group ng-1 (kubelet and workload TCP ports"),
FromPort: aws.Int32(1025),
ToPort: aws.Int32(65535),
GroupId: sgID,
IpProtocol: aws.String("tcp"),
IsEgress: aws.Bool(true),
SecurityGroupRuleId: aws.String("sgr-1"),
},
{
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"),
FromPort: aws.Int32(443),
ToPort: aws.Int32(443),
GroupId: sgID,
IpProtocol: aws.String("tcp"),
IsEgress: aws.Bool(true),
SecurityGroupRuleId: aws.String("sgr-2"),
},
{
Description: aws.String("Allow control plane to receive API requests from worker nodes in group ng-1"),
FromPort: aws.Int32(443),
ToPort: aws.Int32(443),
GroupId: sgID,
IpProtocol: aws.String("tcp"),
IsEgress: aws.Bool(false),
SecurityGroupRuleId: aws.String("sgr-3"),
},
{
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"),
FromPort: aws.Int32(443),
ToPort: aws.Int32(443),
GroupId: sgID,
IpProtocol: aws.String("tcp"),
IsEgress: aws.Bool(true),
SecurityGroupRuleId: aws.String("sgr-4"),
},
}, extraSGRules...),
}, nil)
}

func makeUnownedClusterConfig(clusterConfig *api.ClusterConfig) {
clusterConfig.VPC = &api.ClusterVPC{
SecurityGroup: "sg-custom",
Network: api.Network{
ID: "vpc-custom",
},
Subnets: &api.ClusterSubnets{
Private: api.AZSubnetMapping{
"us-west-2a": api.AZSubnetSpec{
ID: "subnet-custom1",
},
"us-west-2b": api.AZSubnetSpec{
ID: "subnet-custom2",
},
},
},
}
}
Loading