Fix reusing instanceRoleARN for nodegroups authorized with access entry

.mockery.yaml

@@ -36,3 +36,15 @@ packages:
         config:
           dir: "{{.InterfaceDir}}/mocks"
           outpkg: mocks
+
+  github.com/weaveworks/eksctl/pkg/cfn/manager:
+    interfaces:
+      NodeGroupStackManager:
+        config:
+          dir: "{{.InterfaceDir}}/mocks"
+          outpkg: mocks
+
+      NodeGroupResourceSet:
+        config:
+          dir: "{{.InterfaceDir}}/mocks"
+          outpkg: mocks

integration/tests/accessentries/accessentries_test.go

@@ -6,27 +6,34 @@ package accessentries
 import (
 	"bytes"
 	"context"
+	_ "embed"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"strings"
 	"testing"
+	"time"
+
+	"github.com/kubicorn/kubicorn/pkg/namer"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
-	"github.com/weaveworks/eksctl/integration/runner"
-	. "github.com/weaveworks/eksctl/integration/runner"
-
 	"github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go-v2/service/cloudformation"
+	cfntypes "github.com/aws/aws-sdk-go-v2/service/cloudformation/types"
 	awseks "github.com/aws/aws-sdk-go-v2/service/eks"
 	ekstypes "github.com/aws/aws-sdk-go-v2/service/eks/types"
 	"github.com/aws/aws-sdk-go-v2/service/iam"
 	iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types"
 
+	"github.com/weaveworks/eksctl/integration/runner"
+	. "github.com/weaveworks/eksctl/integration/runner"
 	"github.com/weaveworks/eksctl/integration/tests"
+	clusterutils "github.com/weaveworks/eksctl/integration/utilities/cluster"
 	"github.com/weaveworks/eksctl/pkg/accessentry"
 	api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
+	"github.com/weaveworks/eksctl/pkg/cfn/outputs"
 	"github.com/weaveworks/eksctl/pkg/eks"
 	"github.com/weaveworks/eksctl/pkg/testutils"
 )
@@ -54,6 +61,9 @@ var (
 	apiDisabledCluster = "accessentries-api-disabled"
 )
 
+//go:embed testdata/node-role.yaml
+var nodeRoleJSON []byte
+
 func init() {
 	// Call testing.Init() prior to tests.NewParams(), as otherwise -test.* will not be recognised. See also: https://golang.org/doc/go1.13#testing
 	testing.Init()
@@ -69,7 +79,6 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 		err              error
 		alreadyExistsErr *iamtypes.EntityAlreadyExistsException
 	)
-
 	maybeCreateRoleAndGetARN := func(name string) (string, error) {
 		createOut, err := ctl.AWSProvider.IAM().CreateRole(context.Background(), &iam.CreateRoleInput{
 			RoleName:                 aws.String(name),
@@ -89,7 +98,6 @@ var _ = SynchronizedBeforeSuite(func() []byte {
 		}
 		return *getOut.Role.Arn, nil
 	}
-
 	ctl, err = eks.New(context.TODO(), &api.ProviderConfig{Region: params.Region}, nil)
 	Expect(err).NotTo(HaveOccurred())
 
@@ -403,6 +411,96 @@ var _ = Describe("(Integration) [AccessEntries Test]", func() {
 				}, "5m", "30s").ShouldNot(RunSuccessfully())
 			})
 		})
+
+		Context("self-managed nodegroup with a pre-existing instanceRoleARN", func() {
+			waitAndGetRoleARN := func(ctx context.Context, stackName *string) (string, error) {
+				waiter := cloudformation.NewStackCreateCompleteWaiter(ctl.AWSProvider.CloudFormation())
+				output, err := waiter.WaitForOutput(ctx, &cloudformation.DescribeStacksInput{
+					StackName: stackName,
+				}, 5*time.Minute)
+				if err != nil {
+					return "", err
+				}
+				if len(output.Stacks) != 1 {
+					return "", fmt.Errorf("expected to find 1 stack; got %d", len(output.Stacks))
+				}
+				var roleARN string
+				if err := outputs.Collect(output.Stacks[0], map[string]outputs.Collector{
+					"NodeInstanceRoleARN": func(v string) error {
+						roleARN = v
+						return nil
+					},
+				}, nil); err != nil {
+					return "", err
+				}
+				return roleARN, nil
+			}
+
+			var roleARN string
+
+			BeforeAll(func() {
+				By("creating a CloudFormation stack for NodeInstanceRoleARN")
+				stackName := aws.String(fmt.Sprintf("%s-%s-role", apiEnabledCluster, namer.RandomName()))
+				ctx := context.Background()
+				_, err := ctl.AWSProvider.CloudFormation().CreateStack(ctx, &cloudformation.CreateStackInput{
+					StackName:    stackName,
+					TemplateBody: aws.String(string(nodeRoleJSON)),
+					Capabilities: []cfntypes.Capability{cfntypes.CapabilityCapabilityIam},
+				})
+				Expect(err).NotTo(HaveOccurred())
+				DeferCleanup(func() {
+					_, err := ctl.AWSProvider.CloudFormation().DeleteStack(ctx, &cloudformation.DeleteStackInput{
+						StackName: stackName,
+					})
+					Expect(err).NotTo(HaveOccurred())
+				})
+				By("waiting for the stack to be created successfully")
+				roleARN, err = waitAndGetRoleARN(ctx, stackName)
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("should create an access entry but not delete it", func() {
+				clusterConfig := makeClusterConfig(apiEnabledCluster)
+				ng := api.NewNodeGroup()
+				ng.Name = "with-instance-role-arn"
+				ng.IAM.InstanceRoleARN = roleARN
+				ng.ScalingConfig = &api.ScalingConfig{
+					DesiredCapacity: aws.Int(1),
+				}
+				clusterConfig.NodeGroups = []*api.NodeGroup{ng}
+
+				cmd := params.EksctlCreateNodegroupCmd.
+					WithArgs("--config-file", "-").
+					WithoutArg("--region", params.Region).
+					WithStdin(clusterutils.Reader(clusterConfig))
+				Expect(cmd).To(RunSuccessfullyWithOutputString(ContainSubstring(
+					fmt.Sprintf("nodegroup %s: created access entry for principal ARN %q", ng.Name, roleARN),
+				)))
+
+				assertAccessEntryExists := func() {
+					_, err = ctl.AWSProvider.EKS().DescribeAccessEntry(context.Background(), &awseks.DescribeAccessEntryInput{
+						ClusterName:  aws.String(apiEnabledCluster),
+						PrincipalArn: aws.String(roleARN),
+					})
+					ExpectWithOffset(1, err).NotTo(HaveOccurred())
+				}
+				By("asserting an access entry was created for the nodegroup")
+				assertAccessEntryExists()
+
+				By("deleting nodegroup with a pre-existing instance role ARN")
+				cmd = params.EksctlDeleteCmd.
+					WithArgs(
+						"nodegroup",
+						"--config-file", "-",
+						"--wait",
+					).
+					WithoutArg("--region", params.Region).
+					WithStdin(clusterutils.Reader(clusterConfig))
+				Expect(cmd).To(RunSuccessfully())
+				By("asserting the associated access entry is not deleted")
+				assertAccessEntryExists()
+			})
+		})
 	})
 })
 
@@ -423,6 +521,7 @@ var _ = SynchronizedAfterSuite(func() {}, func() {
 		WithArgs(
 			"cluster",
 			"--name", apiEnabledCluster,
+			"--disable-nodegroup-eviction",
 			"--wait",
 		)).To(RunSuccessfully())
 

integration/tests/accessentries/testdata/node-role.yaml

@@ -0,0 +1,55 @@
+{
+  "AWSTemplateFormatVersion": "2010-09-09",
+  "Description": "Node role for access entry test",
+  "Resources": {
+    "NodeInstanceRole": {
+      "Type": "AWS::IAM::Role",
+      "Properties": {
+        "AssumeRolePolicyDocument": {
+          "Statement": [
+            {
+              "Action": [
+                "sts:AssumeRole"
+              ],
+              "Effect": "Allow",
+              "Principal": {
+                "Service": "ec2.amazonaws.com"
+              }
+            }
+          ],
+          "Version": "2012-10-17"
+        },
+        "ManagedPolicyArns": [
+          {
+            "Fn::Sub": "arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly"
+          },
+          {
+            "Fn::Sub": "arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy"
+          },
+          {
+            "Fn::Sub": "arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy"
+          },
+          {
+            "Fn::Sub": "arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore"
+          }
+        ],
+        "Path": "/",
+        "Tags": [
+          {
+            "Key": "Name",
+            "Value": {
+              "Fn::Sub": "${AWS::StackName}/NodeInstanceRole"
+            }
+          }
+        ]
+      }
+    }
+  },
+  "Outputs": {
+    "NodeInstanceRoleARN": {
+      "Value": {
+        "Fn::GetAtt": "NodeInstanceRole.Arn"
+      }
+    }
+  }
+}

pkg/actions/nodegroup/create.go

@@ -254,8 +254,8 @@ func (m *Manager) nodeCreationTasks(ctx context.Context, isOwnedCluster, skipEgr
 		Parallel: true,
 	}
 	disableAccessEntryCreation := !m.accessEntry.IsEnabled() || updateAuthConfigMap != nil
-	nodeGroupTasks := m.stackManager.NewUnmanagedNodeGroupTask(ctx, cfg.NodeGroups, !awsNodeUsesIRSA, skipEgressRules, disableAccessEntryCreation, vpcImporter)
-	if nodeGroupTasks.Len() > 0 {
+	if nodeGroupTasks := m.stackManager.NewUnmanagedNodeGroupTask(ctx, cfg.NodeGroups, !awsNodeUsesIRSA, skipEgressRules,
+		disableAccessEntryCreation, vpcImporter); nodeGroupTasks.Len() > 0 {
 		allNodeGroupTasks.Append(nodeGroupTasks)
 	}
 	managedTasks := m.stackManager.NewManagedNodeGroupTask(ctx, cfg.ManagedNodeGroups, !awsNodeUsesIRSA, vpcImporter)

pkg/actions/nodegroup/nodegroup.go

@@ -3,11 +3,10 @@ package nodegroup
 import (
 	"time"
 
-	"github.com/weaveworks/eksctl/pkg/accessentry"
-
 	"github.com/aws/aws-sdk-go/aws/request"
 	"k8s.io/client-go/kubernetes"
 
+	"github.com/weaveworks/eksctl/pkg/accessentry"
 	api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
 	"github.com/weaveworks/eksctl/pkg/cfn/builder"
 	"github.com/weaveworks/eksctl/pkg/cfn/manager"

pkg/apis/eksctl.io/v1alpha5/access_entry.go

@@ -42,6 +42,28 @@ type AccessScope struct {
 	Namespaces []string `json:"namespaces,omitempty"`
 }
 
+// AccessEntryType represents the type of access entry.
+type AccessEntryType string
+
+const (
+	// AccessEntryTypeLinux specifies the EC2 Linux access entry type.
+	AccessEntryTypeLinux AccessEntryType = "EC2_LINUX"
+	// AccessEntryTypeWindows specifies the Windows access entry type.
+	AccessEntryTypeWindows AccessEntryType = "EC2_WINDOWS"
+	// AccessEntryTypeFargateLinux specifies the Fargate Linux access entry type.
+	AccessEntryTypeFargateLinux AccessEntryType = "FARGATE_LINUX"
+	// AccessEntryTypeStandard specifies a standard access entry type.
+	AccessEntryTypeStandard AccessEntryType = "STANDARD"
+)
+
+// GetAccessEntryType returns the access entry type for the specified AMI family.
+func GetAccessEntryType(ng *NodeGroup) AccessEntryType {
+	if IsWindowsImage(ng.GetAMIFamily()) {
+		return AccessEntryTypeWindows
+	}
+	return AccessEntryTypeLinux
+}
+
 type ARN arn.ARN
 
 // ARN provides custom unmarshalling for an AWS ARN.
@@ -103,9 +125,9 @@ func validateAccessEntries(accessEntries []AccessEntry) error {
 			return fmt.Errorf("%s.principalARN must be set to a valid AWS ARN", path)
 		}
 
-		switch ae.Type {
-		case "", "STANDARD":
-		case "EC2_LINUX", "EC2_WINDOWS", "FARGATE_LINUX":
+		switch AccessEntryType(ae.Type) {
+		case "", AccessEntryTypeStandard:
+		case AccessEntryTypeLinux, AccessEntryTypeWindows, AccessEntryTypeFargateLinux:
 			if len(ae.KubernetesGroups) > 0 || ae.KubernetesUsername != "" {
 				return fmt.Errorf("cannot specify %s.kubernetesGroups nor %s.kubernetesUsername when type is set to %s", path, path, ae.Type)
 			}

pkg/cfn/builder/iam.go

@@ -124,30 +124,31 @@ func (n *NodeGroupResourceSet) WithNamedIAM() bool {
 }
 
 func (n *NodeGroupResourceSet) addResourcesForIAM(ctx context.Context) error {
-	if n.spec.IAM.InstanceProfileARN != "" {
+	nodeGroupIAM := n.options.NodeGroup.IAM
+	if nodeGroupIAM.InstanceProfileARN != "" {
 		n.rs.withIAM = false
 		n.rs.withNamedIAM = false
 
 		// if instance profile is given, as well as the role, we simply use both and export the role
 		// (which is needed in order to authorise the nodegroup)
-		n.instanceProfileARN = gfnt.NewString(n.spec.IAM.InstanceProfileARN)
-		if n.spec.IAM.InstanceRoleARN != "" {
-			n.rs.defineOutputWithoutCollector(outputs.NodeGroupInstanceProfileARN, n.spec.IAM.InstanceProfileARN, true)
-			n.rs.defineOutputWithoutCollector(outputs.NodeGroupInstanceRoleARN, n.spec.IAM.InstanceRoleARN, true)
+		n.instanceProfileARN = gfnt.NewString(nodeGroupIAM.InstanceProfileARN)
+		if nodeGroupIAM.InstanceRoleARN != "" {
+			n.rs.defineOutputWithoutCollector(outputs.NodeGroupInstanceProfileARN, nodeGroupIAM.InstanceProfileARN, true)
+			n.rs.defineOutputWithoutCollector(outputs.NodeGroupInstanceRoleARN, nodeGroupIAM.InstanceRoleARN, true)
 			return nil
 		}
 		// if instance role is not given, export profile and use the getter to call importer function
-		n.rs.defineOutput(outputs.NodeGroupInstanceProfileARN, n.spec.IAM.InstanceProfileARN, true, func(v string) error {
-			return iam.ImportInstanceRoleFromProfileARN(ctx, n.iamAPI, n.spec, v)
+		n.rs.defineOutput(outputs.NodeGroupInstanceProfileARN, nodeGroupIAM.InstanceProfileARN, true, func(v string) error {
+			return iam.ImportInstanceRoleFromProfileARN(ctx, n.iamAPI, n.options.NodeGroup, v)
 		})
 
 		return nil
 	}
 
 	n.rs.withIAM = true
 
-	if n.spec.IAM.InstanceRoleARN != "" {
-		roleARN := NormalizeARN(n.spec.IAM.InstanceRoleARN)
+	if nodeGroupIAM.InstanceRoleARN != "" {
+		roleARN := NormalizeARN(nodeGroupIAM.InstanceRoleARN)
 
 		// if role is set, but profile isn't - create profile
 		n.newResource(cfnIAMInstanceProfileName, &gfniam.InstanceProfile{
@@ -156,7 +157,7 @@ func (n *NodeGroupResourceSet) addResourcesForIAM(ctx context.Context) error {
 		})
 		n.instanceProfileARN = gfnt.MakeFnGetAttString(cfnIAMInstanceProfileName, "Arn")
 		n.rs.defineOutputFromAtt(outputs.NodeGroupInstanceProfileARN, cfnIAMInstanceProfileName, "Arn", true, func(v string) error {
-			n.spec.IAM.InstanceProfileARN = v
+			nodeGroupIAM.InstanceProfileARN = v
 			return nil
 		})
 		n.rs.defineOutputWithoutCollector(outputs.NodeGroupInstanceRoleARN, roleARN, true)
@@ -165,12 +166,12 @@ func (n *NodeGroupResourceSet) addResourcesForIAM(ctx context.Context) error {
 
 	// if neither role nor profile is given - create both
 
-	if n.spec.IAM.InstanceRoleName != "" {
+	if nodeGroupIAM.InstanceRoleName != "" {
 		// setting role name requires additional capabilities
 		n.rs.withNamedIAM = true
 	}
 
-	if err := createRole(n.rs, n.clusterSpec.IAM, n.spec.IAM, false, n.forceAddCNIPolicy); err != nil {
+	if err := createRole(n.rs, n.options.ClusterConfig.IAM, nodeGroupIAM, false, n.options.ForceAddCNIPolicy); err != nil {
 		return err
 	}
 
@@ -181,11 +182,11 @@ func (n *NodeGroupResourceSet) addResourcesForIAM(ctx context.Context) error {
 	n.instanceProfileARN = gfnt.MakeFnGetAttString(cfnIAMInstanceProfileName, "Arn")
 
 	n.rs.defineOutputFromAtt(outputs.NodeGroupInstanceProfileARN, cfnIAMInstanceProfileName, "Arn", true, func(v string) error {
-		n.spec.IAM.InstanceProfileARN = v
+		nodeGroupIAM.InstanceProfileARN = v
 		return nil
 	})
 	n.rs.defineOutputFromAtt(outputs.NodeGroupInstanceRoleARN, cfnIAMInstanceRoleName, "Arn", true, func(v string) error {
-		n.spec.IAM.InstanceRoleARN = v
+		nodeGroupIAM.InstanceRoleARN = v
 		return nil
 	})
 	return nil