diff --git a/pkg/actions/nodegroup/create.go b/pkg/actions/nodegroup/create.go index 64d9f2575a..6366dbcd2d 100644 --- a/pkg/actions/nodegroup/create.go +++ b/pkg/actions/nodegroup/create.go @@ -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" @@ -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) { @@ -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) @@ -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 } @@ -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 @@ -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) } @@ -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 +} diff --git a/pkg/actions/nodegroup/create_test.go b/pkg/actions/nodegroup/create_test.go index 71653017f1..f205a6490a 100644 --- a/pkg/actions/nodegroup/create_test.go +++ b/pkg/actions/nodegroup/create_test.go @@ -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}, } @@ -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. @@ -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", + }, + }, + }, + } +} diff --git a/pkg/cfn/builder/nodegroup.go b/pkg/cfn/builder/nodegroup.go index b086964a8f..a62d7126c4 100644 --- a/pkg/cfn/builder/nodegroup.go +++ b/pkg/cfn/builder/nodegroup.go @@ -35,6 +35,16 @@ const ( taintsPrefix = nodeTemplatePrefix + "taint/" ) +// NodeGroupOptions represents options passed to a NodeGroupResourceSet. +type NodeGroupOptions struct { + ClusterConfig *api.ClusterConfig + NodeGroup *api.NodeGroup + Bootstrapper nodebootstrap.Bootstrapper + ForceAddCNIPolicy bool + VPCImporter vpc.Importer + SkipEgressRules bool +} + // NodeGroupResourceSet stores the resource information of the nodegroup type NodeGroupResourceSet struct { rs *resourceSet @@ -49,19 +59,21 @@ type NodeGroupResourceSet struct { vpc *gfnt.Value vpcImporter vpc.Importer bootstrapper nodebootstrap.Bootstrapper + skipEgressRules bool } // NewNodeGroupResourceSet returns a resource set for a nodegroup embedded in a cluster config -func NewNodeGroupResourceSet(ec2API awsapi.EC2, iamAPI awsapi.IAM, spec *api.ClusterConfig, ng *api.NodeGroup, bootstrapper nodebootstrap.Bootstrapper, forceAddCNIPolicy bool, vpcImporter vpc.Importer) *NodeGroupResourceSet { +func NewNodeGroupResourceSet(ec2API awsapi.EC2, iamAPI awsapi.IAM, options NodeGroupOptions) *NodeGroupResourceSet { return &NodeGroupResourceSet{ rs: newResourceSet(), - forceAddCNIPolicy: forceAddCNIPolicy, - clusterSpec: spec, - spec: ng, + forceAddCNIPolicy: options.ForceAddCNIPolicy, + clusterSpec: options.ClusterConfig, + spec: options.NodeGroup, ec2API: ec2API, iamAPI: iamAPI, - vpcImporter: vpcImporter, - bootstrapper: bootstrapper, + vpcImporter: options.VPCImporter, + bootstrapper: options.Bootstrapper, + skipEgressRules: options.SkipEgressRules, } } @@ -132,6 +144,34 @@ func (n *NodeGroupResourceSet) AddAllResources(ctx context.Context) error { return n.addResourcesForNodeGroup(ctx) } +// A PartialEgressRule represents a partial security group egress rule. +type PartialEgressRule struct { + FromPort int + ToPort int + IPProtocol string +} + +var controlPlaneEgressInterCluster = PartialEgressRule{ + FromPort: 1025, + ToPort: 65535, + IPProtocol: "tcp", +} + +var controlPlaneEgressInterClusterAPI = PartialEgressRule{ + FromPort: 443, + ToPort: 443, + IPProtocol: "tcp", +} + +// ControlPlaneNodeGroupEgressRules is a slice of egress rules attached to the control plane security group. +var ControlPlaneNodeGroupEgressRules = []PartialEgressRule{ + controlPlaneEgressInterCluster, + controlPlaneEgressInterClusterAPI, +} + +// ControlPlaneEgressRuleDescriptionPrefix is the prefix applied to the description for control plane security group egress rules. +var ControlPlaneEgressRuleDescriptionPrefix = "Allow control plane to communicate with " + func (n *NodeGroupResourceSet) addResourcesForSecurityGroups() { for _, id := range n.spec.SecurityGroups.AttachIDs { n.securityGroups = append(n.securityGroups, gfnt.NewString(id)) @@ -166,22 +206,24 @@ func (n *NodeGroupResourceSet) addResourcesForSecurityGroups() { n.securityGroups = append(n.securityGroups, efaSG) } - n.newResource("EgressInterCluster", &gfnec2.SecurityGroupEgress{ - GroupId: refControlPlaneSG, - DestinationSecurityGroupId: refNodeGroupLocalSG, - Description: gfnt.NewString("Allow control plane to communicate with " + desc + " (kubelet and workload TCP ports)"), - IpProtocol: sgProtoTCP, - FromPort: sgMinNodePort, - ToPort: sgMaxNodePort, - }) - n.newResource("EgressInterClusterAPI", &gfnec2.SecurityGroupEgress{ - GroupId: refControlPlaneSG, - DestinationSecurityGroupId: refNodeGroupLocalSG, - Description: gfnt.NewString("Allow control plane to communicate with " + desc + " (workloads using HTTPS port, commonly used with extension API servers)"), - IpProtocol: sgProtoTCP, - FromPort: sgPortHTTPS, - ToPort: sgPortHTTPS, - }) + if !n.skipEgressRules { + n.newResource("EgressInterCluster", &gfnec2.SecurityGroupEgress{ + GroupId: refControlPlaneSG, + DestinationSecurityGroupId: refNodeGroupLocalSG, + Description: gfnt.NewString(ControlPlaneEgressRuleDescriptionPrefix + desc + " (kubelet and workload TCP ports)"), + IpProtocol: gfnt.NewString(controlPlaneEgressInterCluster.IPProtocol), + FromPort: gfnt.NewInteger(controlPlaneEgressInterCluster.FromPort), + ToPort: gfnt.NewInteger(controlPlaneEgressInterCluster.ToPort), + }) + n.newResource("EgressInterClusterAPI", &gfnec2.SecurityGroupEgress{ + GroupId: refControlPlaneSG, + DestinationSecurityGroupId: refNodeGroupLocalSG, + Description: gfnt.NewString(ControlPlaneEgressRuleDescriptionPrefix + desc + " (workloads using HTTPS port, commonly used with extension API servers)"), + IpProtocol: gfnt.NewString(controlPlaneEgressInterClusterAPI.IPProtocol), + FromPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.FromPort), + ToPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.ToPort), + }) + } n.newResource("IngressInterClusterCP", &gfnec2.SecurityGroupIngress{ GroupId: refControlPlaneSG, SourceSecurityGroupId: refNodeGroupLocalSG, @@ -197,16 +239,16 @@ func makeNodeIngressRules(ng *api.NodeGroupBase, controlPlaneSG *gfnt.Value, vpc { SourceSecurityGroupId: controlPlaneSG, Description: gfnt.NewString(fmt.Sprintf("[IngressInterCluster] Allow %s to communicate with control plane (kubelet and workload TCP ports)", description)), - IpProtocol: sgProtoTCP, - FromPort: sgMinNodePort, - ToPort: sgMaxNodePort, + IpProtocol: gfnt.NewString(controlPlaneEgressInterCluster.IPProtocol), + FromPort: gfnt.NewInteger(controlPlaneEgressInterCluster.FromPort), + ToPort: gfnt.NewInteger(controlPlaneEgressInterCluster.ToPort), }, { SourceSecurityGroupId: controlPlaneSG, Description: gfnt.NewString(fmt.Sprintf("[IngressInterClusterAPI] Allow %s to communicate with control plane (workloads using HTTPS port, commonly used with extension API servers)", description)), - IpProtocol: sgProtoTCP, - FromPort: sgPortHTTPS, - ToPort: sgPortHTTPS, + IpProtocol: gfnt.NewString(controlPlaneEgressInterClusterAPI.IPProtocol), + FromPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.FromPort), + ToPort: gfnt.NewInteger(controlPlaneEgressInterClusterAPI.ToPort), }, } diff --git a/pkg/cfn/builder/nodegroup_test.go b/pkg/cfn/builder/nodegroup_test.go index c09965eb1b..d984b96925 100644 --- a/pkg/cfn/builder/nodegroup_test.go +++ b/pkg/cfn/builder/nodegroup_test.go @@ -11,6 +11,7 @@ import ( "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/weaveworks/eksctl/pkg/testutils/mockprovider" . "github.com/onsi/ginkgo/v2" @@ -35,10 +36,12 @@ var _ = Describe("Unmanaged NodeGroup Template Builder", func() { fakeVPCImporter *vpcfakes.FakeImporter fakeBootstrapper *bootstrapfakes.FakeBootstrapper p = mockprovider.NewMockProvider() + skipEgressRules bool ) BeforeEach(func() { forceAddCNIPolicy = false + skipEgressRules = false fakeVPCImporter = new(vpcfakes.FakeImporter) fakeBootstrapper = new(bootstrapfakes.FakeBootstrapper) cfg, ng = newClusterAndNodeGroup() @@ -51,7 +54,14 @@ var _ = Describe("Unmanaged NodeGroup Template Builder", func() { }) JustBeforeEach(func() { - ngrs = builder.NewNodeGroupResourceSet(p.MockEC2(), p.MockIAM(), cfg, ng, fakeBootstrapper, forceAddCNIPolicy, fakeVPCImporter) + ngrs = builder.NewNodeGroupResourceSet(p.MockEC2(), p.MockIAM(), builder.NodeGroupOptions{ + ClusterConfig: cfg, + NodeGroup: ng, + Bootstrapper: fakeBootstrapper, + ForceAddCNIPolicy: forceAddCNIPolicy, + VPCImporter: fakeVPCImporter, + SkipEgressRules: skipEgressRules, + }) }) Describe("AddAllResources", func() { @@ -1312,6 +1322,24 @@ var _ = Describe("Unmanaged NodeGroup Template Builder", func() { Expect(properties.LaunchTemplateData.Monitoring.Enabled).To(Equal(true)) }) }) + + Context("skipEgressRules is false", func() { + It("should add egress rules", func() { + Expect(ngTemplate.Resources).To(HaveKey("EgressInterCluster")) + Expect(ngTemplate.Resources).To(HaveKey("EgressInterClusterAPI")) + }) + }) + + Context("skipEgressRules is true", func() { + BeforeEach(func() { + skipEgressRules = true + }) + + It("should not add egress rules", func() { + Expect(ngTemplate.Resources).NotTo(HaveKey("EgressInterCluster")) + Expect(ngTemplate.Resources).NotTo(HaveKey("EgressInterClusterAPI")) + }) + }) }) }) diff --git a/pkg/cfn/builder/vpc_ipv4.go b/pkg/cfn/builder/vpc_ipv4.go index c3d32a6d21..c0d0f381fb 100644 --- a/pkg/cfn/builder/vpc_ipv4.go +++ b/pkg/cfn/builder/vpc_ipv4.go @@ -335,7 +335,6 @@ var ( sgSourceAnywhereIPv6 = gfnt.NewString("::/0") sgPortZero = gfnt.NewInteger(0) - sgMinNodePort = gfnt.NewInteger(1025) sgMaxNodePort = gfnt.NewInteger(65535) sgPortHTTPS = gfnt.NewInteger(443) diff --git a/pkg/cfn/manager/create_tasks.go b/pkg/cfn/manager/create_tasks.go index 09d0a7416f..817c9feb30 100644 --- a/pkg/cfn/manager/create_tasks.go +++ b/pkg/cfn/manager/create_tasks.go @@ -42,7 +42,7 @@ func (c *StackCollection) NewTasksToCreateClusterWithNodeGroups(ctx context.Cont Parallel: true, IsSubTask: true, } - if unmanagedNodeGroupTasks := c.NewUnmanagedNodeGroupTask(ctx, nodeGroups, false, vpcImporter); unmanagedNodeGroupTasks.Len() > 0 { + if unmanagedNodeGroupTasks := c.NewUnmanagedNodeGroupTask(ctx, nodeGroups, false, false, vpcImporter); unmanagedNodeGroupTasks.Len() > 0 { unmanagedNodeGroupTasks.IsSubTask = true nodeGroupTasks.Append(unmanagedNodeGroupTasks) } @@ -72,7 +72,7 @@ func (c *StackCollection) NewTasksToCreateClusterWithNodeGroups(ctx context.Cont } // NewUnmanagedNodeGroupTask defines tasks required to create all of the nodegroups -func (c *StackCollection) NewUnmanagedNodeGroupTask(ctx context.Context, nodeGroups []*api.NodeGroup, forceAddCNIPolicy bool, vpcImporter vpc.Importer) *tasks.TaskTree { +func (c *StackCollection) NewUnmanagedNodeGroupTask(ctx context.Context, nodeGroups []*api.NodeGroup, forceAddCNIPolicy, skipEgressRules bool, vpcImporter vpc.Importer) *tasks.TaskTree { taskTree := &tasks.TaskTree{Parallel: true} for _, ng := range nodeGroups { @@ -83,6 +83,7 @@ func (c *StackCollection) NewUnmanagedNodeGroupTask(ctx context.Context, nodeGro stackCollection: c, forceAddCNIPolicy: forceAddCNIPolicy, vpcImporter: vpcImporter, + skipEgressRules: skipEgressRules, }) // TODO: move authconfigmap tasks here using kubernetesTask and kubernetes.CallbackClientSet } diff --git a/pkg/cfn/manager/fakes/fake_stack_manager.go b/pkg/cfn/manager/fakes/fake_stack_manager.go index 2aa38a5bce..98204c742c 100644 --- a/pkg/cfn/manager/fakes/fake_stack_manager.go +++ b/pkg/cfn/manager/fakes/fake_stack_manager.go @@ -733,13 +733,14 @@ type FakeStackManager struct { result1 *tasks.TaskTree result2 error } - NewUnmanagedNodeGroupTaskStub func(context.Context, []*v1alpha5.NodeGroup, bool, vpc.Importer) *tasks.TaskTree + NewUnmanagedNodeGroupTaskStub func(context.Context, []*v1alpha5.NodeGroup, bool, bool, vpc.Importer) *tasks.TaskTree newUnmanagedNodeGroupTaskMutex sync.RWMutex newUnmanagedNodeGroupTaskArgsForCall []struct { arg1 context.Context arg2 []*v1alpha5.NodeGroup arg3 bool - arg4 vpc.Importer + arg4 bool + arg5 vpc.Importer } newUnmanagedNodeGroupTaskReturns struct { result1 *tasks.TaskTree @@ -4195,7 +4196,7 @@ func (fake *FakeStackManager) NewTasksToDeleteOIDCProviderWithIAMServiceAccounts }{result1, result2} } -func (fake *FakeStackManager) NewUnmanagedNodeGroupTask(arg1 context.Context, arg2 []*v1alpha5.NodeGroup, arg3 bool, arg4 vpc.Importer) *tasks.TaskTree { +func (fake *FakeStackManager) NewUnmanagedNodeGroupTask(arg1 context.Context, arg2 []*v1alpha5.NodeGroup, arg3 bool, arg4 bool, arg5 vpc.Importer) *tasks.TaskTree { var arg2Copy []*v1alpha5.NodeGroup if arg2 != nil { arg2Copy = make([]*v1alpha5.NodeGroup, len(arg2)) @@ -4207,14 +4208,15 @@ func (fake *FakeStackManager) NewUnmanagedNodeGroupTask(arg1 context.Context, ar arg1 context.Context arg2 []*v1alpha5.NodeGroup arg3 bool - arg4 vpc.Importer - }{arg1, arg2Copy, arg3, arg4}) + arg4 bool + arg5 vpc.Importer + }{arg1, arg2Copy, arg3, arg4, arg5}) stub := fake.NewUnmanagedNodeGroupTaskStub fakeReturns := fake.newUnmanagedNodeGroupTaskReturns - fake.recordInvocation("NewUnmanagedNodeGroupTask", []interface{}{arg1, arg2Copy, arg3, arg4}) + fake.recordInvocation("NewUnmanagedNodeGroupTask", []interface{}{arg1, arg2Copy, arg3, arg4, arg5}) fake.newUnmanagedNodeGroupTaskMutex.Unlock() if stub != nil { - return stub(arg1, arg2, arg3, arg4) + return stub(arg1, arg2, arg3, arg4, arg5) } if specificReturn { return ret.result1 @@ -4228,17 +4230,17 @@ func (fake *FakeStackManager) NewUnmanagedNodeGroupTaskCallCount() int { return len(fake.newUnmanagedNodeGroupTaskArgsForCall) } -func (fake *FakeStackManager) NewUnmanagedNodeGroupTaskCalls(stub func(context.Context, []*v1alpha5.NodeGroup, bool, vpc.Importer) *tasks.TaskTree) { +func (fake *FakeStackManager) NewUnmanagedNodeGroupTaskCalls(stub func(context.Context, []*v1alpha5.NodeGroup, bool, bool, vpc.Importer) *tasks.TaskTree) { fake.newUnmanagedNodeGroupTaskMutex.Lock() defer fake.newUnmanagedNodeGroupTaskMutex.Unlock() fake.NewUnmanagedNodeGroupTaskStub = stub } -func (fake *FakeStackManager) NewUnmanagedNodeGroupTaskArgsForCall(i int) (context.Context, []*v1alpha5.NodeGroup, bool, vpc.Importer) { +func (fake *FakeStackManager) NewUnmanagedNodeGroupTaskArgsForCall(i int) (context.Context, []*v1alpha5.NodeGroup, bool, bool, vpc.Importer) { fake.newUnmanagedNodeGroupTaskMutex.RLock() defer fake.newUnmanagedNodeGroupTaskMutex.RUnlock() argsForCall := fake.newUnmanagedNodeGroupTaskArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4, argsForCall.arg5 } func (fake *FakeStackManager) NewUnmanagedNodeGroupTaskReturns(result1 *tasks.TaskTree) { diff --git a/pkg/cfn/manager/interface.go b/pkg/cfn/manager/interface.go index 82d596423d..0056327839 100644 --- a/pkg/cfn/manager/interface.go +++ b/pkg/cfn/manager/interface.go @@ -91,7 +91,7 @@ type StackManager interface { NewTasksToDeleteIAMServiceAccounts(ctx context.Context, serviceAccounts []string, clientSetGetter kubernetes.ClientSetGetter, wait bool) (*tasks.TaskTree, error) NewTasksToDeleteNodeGroups(stacks []NodeGroupStack, shouldDelete func(_ string) bool, wait bool, cleanup func(chan error, string) error) (*tasks.TaskTree, error) NewTasksToDeleteOIDCProviderWithIAMServiceAccounts(ctx context.Context, newOIDCManager NewOIDCManager, cluster *ekstypes.Cluster, clientSetGetter kubernetes.ClientSetGetter, force bool) (*tasks.TaskTree, error) - NewUnmanagedNodeGroupTask(ctx context.Context, nodeGroups []*v1alpha5.NodeGroup, forceAddCNIPolicy bool, importer vpc.Importer) *tasks.TaskTree + NewUnmanagedNodeGroupTask(ctx context.Context, nodeGroups []*v1alpha5.NodeGroup, forceAddCNIPolicy, skipEgressRules bool, importer vpc.Importer) *tasks.TaskTree PropagateManagedNodeGroupTagsToASG(ngName string, ngTags map[string]string, asgNames []string, errCh chan error) error RefreshFargatePodExecutionRoleARN(ctx context.Context) error StackStatusIsNotTransitional(s *Stack) bool diff --git a/pkg/cfn/manager/nodegroup.go b/pkg/cfn/manager/nodegroup.go index f20101a9ff..bfe2eac969 100644 --- a/pkg/cfn/manager/nodegroup.go +++ b/pkg/cfn/manager/nodegroup.go @@ -36,7 +36,7 @@ func (c *StackCollection) makeNodeGroupStackName(name string) string { } // createNodeGroupTask creates the nodegroup -func (c *StackCollection) createNodeGroupTask(ctx context.Context, errs chan error, ng *api.NodeGroup, forceAddCNIPolicy bool, vpcImporter vpc.Importer) error { +func (c *StackCollection) createNodeGroupTask(ctx context.Context, errs chan error, ng *api.NodeGroup, forceAddCNIPolicy, skipEgressRules bool, vpcImporter vpc.Importer) error { name := c.makeNodeGroupStackName(ng.Name) logger.Info("building nodegroup stack %q", name) @@ -44,7 +44,14 @@ func (c *StackCollection) createNodeGroupTask(ctx context.Context, errs chan err if err != nil { return errors.Wrap(err, "error creating bootstrapper") } - stack := builder.NewNodeGroupResourceSet(c.ec2API, c.iamAPI, c.spec, ng, bootstrapper, forceAddCNIPolicy, vpcImporter) + stack := builder.NewNodeGroupResourceSet(c.ec2API, c.iamAPI, builder.NodeGroupOptions{ + ClusterConfig: c.spec, + NodeGroup: ng, + Bootstrapper: bootstrapper, + ForceAddCNIPolicy: forceAddCNIPolicy, + VPCImporter: vpcImporter, + SkipEgressRules: skipEgressRules, + }) if err := stack.AddAllResources(ctx); err != nil { return err } @@ -195,7 +202,6 @@ func (c *StackCollection) DescribeNodeGroupStacksAndResources(ctx context.Contex } func (c *StackCollection) GetAutoScalingGroupName(ctx context.Context, s *Stack) (string, error) { - nodeGroupType, err := GetNodeGroupType(s.Tags) if err != nil { return "", err @@ -220,7 +226,7 @@ func (c *StackCollection) GetAutoScalingGroupName(ctx context.Context, s *Stack) } } -// GetNodeGroupAutoScalingGroupName returns the unmanaged nodegroup's AutoScalingGroupName +// GetUnmanagedNodeGroupAutoScalingGroupName returns the unmanaged nodegroup's AutoScalingGroupName. func (c *StackCollection) GetUnmanagedNodeGroupAutoScalingGroupName(ctx context.Context, s *Stack) (string, error) { input := &cfn.DescribeStackResourceInput{ StackName: s.StackName, diff --git a/pkg/cfn/manager/tasks.go b/pkg/cfn/manager/tasks.go index 1dcd016599..2569c31140 100644 --- a/pkg/cfn/manager/tasks.go +++ b/pkg/cfn/manager/tasks.go @@ -33,11 +33,12 @@ type nodeGroupTask struct { forceAddCNIPolicy bool vpcImporter vpc.Importer stackCollection *StackCollection + skipEgressRules bool } func (t *nodeGroupTask) Describe() string { return t.info } func (t *nodeGroupTask) Do(errs chan error) error { - return t.stackCollection.createNodeGroupTask(t.ctx, errs, t.nodeGroup, t.forceAddCNIPolicy, t.vpcImporter) + return t.stackCollection.createNodeGroupTask(t.ctx, errs, t.nodeGroup, t.forceAddCNIPolicy, t.skipEgressRules, t.vpcImporter) } type managedNodeGroupTask struct { diff --git a/pkg/cfn/manager/tasks_test.go b/pkg/cfn/manager/tasks_test.go index ccc336f89b..a4f2d051ac 100644 --- a/pkg/cfn/manager/tasks_test.go +++ b/pkg/cfn/manager/tasks_test.go @@ -80,22 +80,22 @@ var _ = Describe("StackCollection Tasks", func() { // The supportsManagedNodes argument has no effect on the Describe call, so the values are alternated // in these tests { - tasks := stackManager.NewUnmanagedNodeGroupTask(context.Background(), makeNodeGroups("bar", "foo"), false, fakeVPCImporter) + tasks := stackManager.NewUnmanagedNodeGroupTask(context.Background(), makeNodeGroups("bar", "foo"), false, false, fakeVPCImporter) Expect(tasks.Describe()).To(Equal(` 2 parallel tasks: { create nodegroup "bar", create nodegroup "foo" } `)) } { - tasks := stackManager.NewUnmanagedNodeGroupTask(context.Background(), makeNodeGroups("bar"), false, fakeVPCImporter) + tasks := stackManager.NewUnmanagedNodeGroupTask(context.Background(), makeNodeGroups("bar"), false, false, fakeVPCImporter) Expect(tasks.Describe()).To(Equal(`1 task: { create nodegroup "bar" }`)) } { - tasks := stackManager.NewUnmanagedNodeGroupTask(context.Background(), makeNodeGroups("foo"), false, fakeVPCImporter) + tasks := stackManager.NewUnmanagedNodeGroupTask(context.Background(), makeNodeGroups("foo"), false, false, fakeVPCImporter) Expect(tasks.Describe()).To(Equal(`1 task: { create nodegroup "foo" }`)) } { - tasks := stackManager.NewUnmanagedNodeGroupTask(context.Background(), nil, false, fakeVPCImporter) + tasks := stackManager.NewUnmanagedNodeGroupTask(context.Background(), nil, false, false, fakeVPCImporter) Expect(tasks.Describe()).To(Equal(`no tasks`)) } {