From e57c7efff96847b4bc8d027e6ce0bec35fe550b0 Mon Sep 17 00:00:00 2001 From: Berk Dehrioglu Date: Tue, 3 Oct 2023 10:23:59 +0300 Subject: [PATCH] refactoring (#191) * delete awscluster reconciler * add barebones for iam tests * use aws arn package to extract account, drop requeue * remove kiam role creation/deletion * remove kiam leftovers --- CHANGELOG.md | 6 + controllers/awscluster_controller.go | 275 --------------- controllers/awscluster_controller_test.go | 244 -------------- controllers/awsmachinepool_controller.go | 131 +++----- controllers/awsmachinetemplate_controller.go | 313 +++++++++--------- .../awsmachinetemplate_controller_test.go | 34 -- .../awsmanagedcontrolplane_controller.go | 2 +- controllers/common_test.go | 23 -- main.go | 22 -- pkg/iam/iam.go | 27 -- pkg/iam/iam_suite_test.go | 13 + pkg/iam/iam_test.go | 105 ++++++ pkg/iam/route53_template.go | 7 - pkg/key/key.go | 11 + pkg/test/mocks/generate.go | 1 + 15 files changed, 334 insertions(+), 880 deletions(-) delete mode 100644 controllers/awscluster_controller.go delete mode 100644 controllers/awscluster_controller_test.go create mode 100644 pkg/iam/iam_suite_test.go create mode 100644 pkg/iam/iam_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ce7a6c7f..0fb39395 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Remove SecretReconciler. +- Refactor Reconcilers. + ### Added - Add new role for AWS Load Balancer Controller. +- Add tests for iam package. ## [0.10.0] - 2023-08-11 diff --git a/controllers/awscluster_controller.go b/controllers/awscluster_controller.go deleted file mode 100644 index e055de19..00000000 --- a/controllers/awscluster_controller.go +++ /dev/null @@ -1,275 +0,0 @@ -/* -Copyright 2021. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controllers - -import ( - "context" - "fmt" - "regexp" - "strings" - - awsclientgo "github.com/aws/aws-sdk-go/aws/client" - "github.com/giantswarm/microerror" - "github.com/pkg/errors" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" - capa "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1" - "sigs.k8s.io/cluster-api/util/patch" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - - "github.com/aws/aws-sdk-go/service/iam/iamiface" - "github.com/go-logr/logr" - - "github.com/giantswarm/capa-iam-operator/pkg/awsclient" - "github.com/giantswarm/capa-iam-operator/pkg/iam" - "github.com/giantswarm/capa-iam-operator/pkg/key" -) - -const ( - IRSASecretSuffix = "irsa-cloudfront" -) - -// AWSClusterReconciler reconciles a AWSCluster object -type AWSClusterReconciler struct { - client.Client - Log logr.Logger - IAMClientFactory func(awsclientgo.ConfigProvider) iamiface.IAMAPI - AWSClient awsclient.AwsClientInterface -} - -// +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch;delete -// +kubebuilder:rbac:groups=core,resources=secrets/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=core,resources=secrets/finalizers,verbs=update - -func (r *AWSClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := r.Log.WithValues("namespace", req.Namespace, "AWSCluster", req.Name) - logger.Info("Reconciling IRSA roles") - - awsCluster := &capa.AWSCluster{} - err := r.Get(ctx, req.NamespacedName, awsCluster) - if err != nil { - return ctrl.Result{}, errors.WithStack(client.IgnoreNotFound(err)) - } - - if awsCluster.DeletionTimestamp != nil { - return r.reconcileDelete(ctx, logger, awsCluster) - } - - return r.reconcileNormal(ctx, logger, awsCluster) -} - -func (r *AWSClusterReconciler) reconcileNormal(ctx context.Context, logger logr.Logger, awsCluster *capa.AWSCluster) (ctrl.Result, error) { - logger.Info("reconcile normal") - // add finalizer to AWSCluster - if !controllerutil.ContainsFinalizer(awsCluster, key.FinalizerName(iam.IRSARole)) { - patchHelper, err := patch.NewHelper(awsCluster, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.AddFinalizer(awsCluster, key.FinalizerName(iam.IRSARole)) - err = patchHelper.Patch(ctx, awsCluster) - if err != nil { - logger.Error(err, "failed to add finalizer on AWSCluster") - return ctrl.Result{}, errors.WithStack(err) - } - logger.Info("successfully added finalizer to AWSCluster", "finalizer_name", key.FinalizerName(iam.IRSARole)) - } - - cm := &corev1.ConfigMap{} - err := r.Get( - ctx, - types.NamespacedName{ - Namespace: awsCluster.Namespace, - Name: fmt.Sprintf("%s-%s", awsCluster.Name, "cluster-values"), - }, - cm) - if err != nil { - logger.Error(err, "Failed to get the cluster-values configmap for cluster") - return ctrl.Result{}, errors.WithStack(err) - } - - if !controllerutil.ContainsFinalizer(cm, key.FinalizerName(iam.IRSARole)) { - patchHelper, err := patch.NewHelper(cm, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.AddFinalizer(cm, key.FinalizerName(iam.IRSARole)) - err = patchHelper.Patch(ctx, cm) - if err != nil { - logger.Error(err, "failed to add finalizer to configmap", "configmap", fmt.Sprintf("%s-%s", awsCluster.Name, "cluster-values")) - return ctrl.Result{}, errors.WithStack(err) - } - logger.Info("successfully added finalizer to configmap", "finalizer_name", iam.IRSARole, "configmap", fmt.Sprintf("%s-%s", awsCluster.Name, "cluster-values")) - } - - awsClusterRoleIdentity, err := key.GetAWSClusterRoleIdentity(ctx, r.Client, awsCluster.Spec.IdentityRef.Name) - if err != nil { - logger.Error(err, "could not get AWSClusterRoleIdentity") - return ctrl.Result{}, errors.WithStack(err) - } - - accountID, err := getAWSAccountID(awsClusterRoleIdentity) - if err != nil { - logger.Error(err, "Could not get account ID") - return ctrl.Result{}, errors.WithStack(err) - } - - baseDomain, err := key.GetBaseDomain(ctx, r.Client, awsCluster.Name, awsCluster.Namespace) - if err != nil { - logger.Error(err, "Could not get base domain") - return ctrl.Result{}, errors.WithStack(err) - } - - cloudFrontDomain := key.CloudFrontAlias(baseDomain) - - awsClientSession, err := r.AWSClient.GetAWSClientSession(awsClusterRoleIdentity.Spec.RoleArn, awsCluster.Spec.Region) - if err != nil { - logger.Error(err, "Failed to get aws client session", "cluster_name", awsCluster) - return ctrl.Result{}, errors.WithStack(err) - } - - var iamService *iam.IAMService - { - c := iam.IAMServiceConfig{ - AWSSession: awsClientSession, - ClusterName: awsCluster.Name, - MainRoleName: "-", - RoleType: iam.IRSARole, - Region: awsCluster.Spec.Region, - Log: logger, - IAMClientFactory: r.IAMClientFactory, - } - iamService, err = iam.New(c) - if err != nil { - logger.Error(err, "Failed to generate IAM service") - return ctrl.Result{}, errors.WithStack(err) - } - } - - err = iamService.ReconcileRolesForIRSA(accountID, cloudFrontDomain) - if err != nil { - logger.Error(err, "Unable to reconcile role") - return ctrl.Result{}, errors.WithStack(err) - } - - return ctrl.Result{}, nil -} - -func (r *AWSClusterReconciler) reconcileDelete(ctx context.Context, logger logr.Logger, awsCluster *capa.AWSCluster) (ctrl.Result, error) { - logger.Info("reconcile delete") - awsClusterRoleIdentity, err := key.GetAWSClusterRoleIdentity(ctx, r.Client, awsCluster.Spec.IdentityRef.Name) - if err != nil { - logger.Error(err, "could not get AWSClusterRoleIdentity") - return ctrl.Result{}, microerror.Mask(err) - } - awsClientSession, err := r.AWSClient.GetAWSClientSession(awsClusterRoleIdentity.Spec.RoleArn, awsCluster.Spec.Region) - if err != nil { - logger.Error(err, "Failed to get aws client session") - return ctrl.Result{}, errors.WithStack(err) - } - - var iamService *iam.IAMService - { - c := iam.IAMServiceConfig{ - AWSSession: awsClientSession, - ClusterName: awsCluster.Name, - MainRoleName: "-", - RoleType: iam.IRSARole, - Log: logger, - IAMClientFactory: r.IAMClientFactory, - } - iamService, err = iam.New(c) - if err != nil { - logger.Error(err, "Failed to generate IAM service") - } - } - - err = iamService.DeleteRolesForIRSA() - if err != nil { - logger.Error(err, "Unable to reconcile role") - return ctrl.Result{}, err - } - - if controllerutil.ContainsFinalizer(awsCluster, key.FinalizerName(iam.IRSARole)) { - patchHelper, err := patch.NewHelper(awsCluster, r.Client) - if err != nil { - return ctrl.Result{}, err - } - controllerutil.RemoveFinalizer(awsCluster, key.FinalizerName(iam.IRSARole)) - err = patchHelper.Patch(ctx, awsCluster) - if err != nil { - logger.Error(err, "failed to remove finalizer from awsCluster", "finalizer_name", key.FinalizerName(iam.IRSARole), "cluster_name", awsCluster) - return ctrl.Result{}, err - } - logger.Info("successfully removed finalizer from awsCluster", "finalizer_name", key.FinalizerName(iam.IRSARole), "cluster_name", awsCluster) - } - - cm := &corev1.ConfigMap{} - err = r.Get( - ctx, - types.NamespacedName{ - Namespace: awsCluster.Namespace, - Name: fmt.Sprintf("%s-%s", awsCluster.Name, "cluster-values"), - }, - cm) - if err != nil { - logger.Error(err, "Failed to get the cluster-values configmap for cluster") - return ctrl.Result{}, err - } - - if controllerutil.ContainsFinalizer(cm, key.FinalizerName(iam.IRSARole)) { - patchHelper, err := patch.NewHelper(cm, r.Client) - if err != nil { - return ctrl.Result{}, err - } - controllerutil.RemoveFinalizer(cm, key.FinalizerName(iam.IRSARole)) - err = patchHelper.Patch(ctx, cm) - if err != nil { - logger.Error(err, "failed to remove finalizer from configmap") - return ctrl.Result{}, err - } - logger.Info("successfully removed finalizer from configmap", "finalizer_name", iam.IRSARole) - } - return ctrl.Result{}, nil -} - -func getAWSAccountID(awsClusterRoleIdentity *capa.AWSClusterRoleIdentity) (string, error) { - arn := awsClusterRoleIdentity.Spec.RoleArn - if arn == "" || len(strings.TrimSpace(arn)) < 1 { - err := fmt.Errorf("unable to extract ARN from AWSClusterRoleIdentity %s", awsClusterRoleIdentity.Name) - return "", err - } - - re := regexp.MustCompile(`[-]?\d[\d,]*[\.]?[\d{2}]*`) - accountID := re.FindAllString(arn, 1)[0] - - if accountID == "" || len(strings.TrimSpace(accountID)) < 1 { - err := fmt.Errorf("unable to extract AWS account ID from ARN %s", arn) - return "", err - } - - return accountID, nil -} - -// SetupWithManager sets up the controller with the Manager. -func (r *AWSClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { - return ctrl.NewControllerManagedBy(mgr). - For(&capa.AWSCluster{}). - Complete(r) -} diff --git a/controllers/awscluster_controller_test.go b/controllers/awscluster_controller_test.go deleted file mode 100644 index 0392900a..00000000 --- a/controllers/awscluster_controller_test.go +++ /dev/null @@ -1,244 +0,0 @@ -package controllers_test - -import ( - "context" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" - awsclientupstream "github.com/aws/aws-sdk-go/aws/client" - "github.com/aws/aws-sdk-go/aws/session" - "github.com/aws/aws-sdk-go/service/iam" - "github.com/aws/aws-sdk-go/service/iam/iamiface" - "github.com/golang/mock/gomock" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - capa "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1" - capi "sigs.k8s.io/cluster-api/api/v1beta1" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/log/zap" - - "github.com/giantswarm/capa-iam-operator/controllers" - "github.com/giantswarm/capa-iam-operator/pkg/test/mocks" -) - -var _ = Describe("AWSClusterReconciler", func() { - var ( - ctx context.Context - mockCtrl *gomock.Controller - mockAwsClient *mocks.MockAwsClientInterface - mockIAMClient *mocks.MockIAMAPI - reconcileErr error - reconciler *controllers.AWSClusterReconciler - req ctrl.Request - namespace string - sess *session.Session - ) - - SetupNamespaceBeforeAfterEach(&namespace) - - BeforeEach(func() { - logger := zap.New(zap.WriteTo(GinkgoWriter)) - ctx = log.IntoContext(context.Background(), logger) - - mockCtrl = gomock.NewController(GinkgoT()) - - ctx := context.TODO() - - mockAwsClient = mocks.NewMockAwsClientInterface(mockCtrl) - mockIAMClient = mocks.NewMockIAMAPI(mockCtrl) - - reconciler = &controllers.AWSClusterReconciler{ - Client: k8sClient, - Log: ctrl.Log, - AWSClient: mockAwsClient, - IAMClientFactory: func(session awsclientupstream.ConfigProvider) iamiface.IAMAPI { - return mockIAMClient - }, - } - - err := k8sClient.Create(ctx, &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-cluster-cluster-values", - Namespace: namespace, - }, - Data: map[string]string{ - "values": "baseDomain: test.gaws.gigantic.io\n", - }, - }) - Expect(err).NotTo(HaveOccurred()) - - _ = k8sClient.Create(ctx, &capa.AWSClusterRoleIdentity{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-2", - }, - Spec: capa.AWSClusterRoleIdentitySpec{ - AWSRoleSpec: capa.AWSRoleSpec{ - RoleArn: "arn:aws:iam::012345678901:role/giantswarm-test-capa-controller", - }, - AWSClusterIdentitySpec: capa.AWSClusterIdentitySpec{ - AllowedNamespaces: &capa.AllowedNamespaces{}, - }, - }, - }) - - err = k8sClient.Create(ctx, &capa.AWSCluster{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "cluster.x-k8s.io/cluster-name": "test-cluster", - }, - Name: "test-cluster", - Namespace: namespace, - }, - Spec: capa.AWSClusterSpec{ - IdentityRef: &capa.AWSIdentityReference{ - Name: "test-2", - Kind: "AWSClusterRoleIdentity", - }, - Region: "eu-west-1", - }, - }) - Expect(err).NotTo(HaveOccurred()) - - err = k8sClient.Create(ctx, &capi.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-cluster", - Namespace: namespace, - }, - Spec: capi.ClusterSpec{ - ControlPlaneEndpoint: capi.APIEndpoint{ - Host: "api.testcluster-apiserver-123456789.eu-west-2.elb.amazonaws.com", - }, - }, - }) - Expect(err).NotTo(HaveOccurred()) - - Expect(namespace).NotTo(BeEmpty()) - req = ctrl.Request{ - NamespacedName: client.ObjectKey{ - Name: "test-cluster", - Namespace: namespace, - }, - } - - sess, err = session.NewSession(&aws.Config{ - Region: aws.String("eu-west-1")}, - ) - Expect(err).NotTo(HaveOccurred()) - - }) - - AfterEach(func() { - mockCtrl.Finish() - }) - - // TODO We create multiple equal policies (`control-plane-test-cluster-policy` - // vs. `irsa-role-test-cluster-policy`. Until we fix that, the test checks - // the current, wrong behavior :( - externalDnsRoleInfoCopy := externalDnsRoleInfo - Expect(externalDnsRoleInfoCopy.ExpectedPolicyName).To(Equal("control-plane-test-cluster-policy")) - externalDnsRoleInfoCopy.ExpectedPolicyName = irsaRoleName - certManagerRoleInfoCopy := certManagerRoleInfo - Expect(certManagerRoleInfoCopy.ExpectedPolicyName).To(Equal("control-plane-test-cluster-policy")) - certManagerRoleInfoCopy.ExpectedPolicyName = irsaRoleName - ALBControllerRoleInfoCopy := ALBControllerRoleInfo - Expect(ALBControllerRoleInfoCopy.ExpectedPolicyName).To(Equal("control-plane-test-cluster-policy")) - ALBControllerRoleInfoCopy.ExpectedPolicyName = irsaRoleName - - expectedRoleStatusesOnSuccess := []RoleInfo{ - certManagerRoleInfoCopy, - externalDnsRoleInfoCopy, - ALBControllerRoleInfoCopy, - } - - expectedIAMTags := []*iam.Tag{ - { - Key: aws.String("capi-iam-controller/owned"), - Value: aws.String(""), - }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), - Value: aws.String("owned"), - }, - } - - When("KIAM role was already created by other controller", func() { - BeforeEach(func() { - mockAwsClient.EXPECT().GetAWSClientSession("arn:aws:iam::012345678901:role/giantswarm-test-capa-controller", "eu-west-1").Return(sess, nil) - // Implementation detail: KIAM role gets looked up for each role, therefore `MinTimes(1)` - mockIAMClient.EXPECT().GetRole(&iam.GetRoleInput{ - RoleName: aws.String("test-cluster-IAMManager-Role"), - }).MinTimes(1).Return(&iam.GetRoleOutput{ - Role: &iam.Role{ - Arn: aws.String("arn:aws:iam::999666333:role/test-cluster-IAMManager-Role"), - Tags: expectedIAMTags, - }, - }, nil) - }) - - When("a role does not exist", func() { - BeforeEach(func() { - for _, info := range expectedRoleStatusesOnSuccess { - mockIAMClient.EXPECT().GetRole(&iam.GetRoleInput{ - RoleName: aws.String(info.ExpectedName), - }).Return(nil, awserr.New(iam.ErrCodeNoSuchEntityException, "unit test", nil)) - } - }) - - It("creates the role", func() { - for _, info := range expectedRoleStatusesOnSuccess { - mockIAMClient.EXPECT().CreateRole(&iam.CreateRoleInput{ - AssumeRolePolicyDocument: aws.String(info.ExpectedAssumeRolePolicyDocument), - RoleName: aws.String(info.ExpectedName), - Tags: expectedIAMTags, - }).Return(&iam.CreateRoleOutput{}, nil) - - mockIAMClient.EXPECT().CreateInstanceProfile(&iam.CreateInstanceProfileInput{ - InstanceProfileName: aws.String(info.ExpectedName), - Tags: expectedIAMTags, - }).Return(&iam.CreateInstanceProfileOutput{}, nil) - - mockIAMClient.EXPECT().AddRoleToInstanceProfile(&iam.AddRoleToInstanceProfileInput{ - InstanceProfileName: aws.String(info.ExpectedName), - RoleName: aws.String(info.ExpectedName), - }).Return(&iam.AddRoleToInstanceProfileOutput{}, nil) - - // TODO This is different from `AWSMachineTemplateReconciler`. We should update the policy - // document if and only if it differs from the desired one. - mockIAMClient.EXPECT().UpdateAssumeRolePolicy(&iam.UpdateAssumeRolePolicyInput{ - PolicyDocument: aws.String(info.ExpectedAssumeRolePolicyDocument), - RoleName: aws.String(info.ExpectedName), - }).Return(&iam.UpdateAssumeRolePolicyOutput{}, nil) - - // Implementation detail: instead of storing the ARN, the controller calls `GetRole` multiple times - // from different places. Remove once we don't do this anymore (hence the `MinTimes` call so we - // would notice). - mockIAMClient.EXPECT().GetRole(&iam.GetRoleInput{ - RoleName: aws.String(info.ExpectedName), - }).MinTimes(1).Return(&iam.GetRoleOutput{ - Role: &iam.Role{ - Arn: aws.String(info.ReturnRoleArn), - Tags: expectedIAMTags, - }, - }, nil) - - mockIAMClient.EXPECT().ListRolePolicies(&iam.ListRolePoliciesInput{ - RoleName: aws.String(info.ExpectedName), - }).Return(&iam.ListRolePoliciesOutput{}, nil) - - mockIAMClient.EXPECT().PutRolePolicy(&iam.PutRolePolicyInput{ - PolicyName: aws.String(info.ExpectedPolicyName), - PolicyDocument: aws.String(info.ExpectedPolicyDocument), - RoleName: aws.String(info.ExpectedName), - }).Return(&iam.PutRolePolicyOutput{}, nil) - } - - _, reconcileErr = reconciler.Reconcile(ctx, req) - Expect(reconcileErr).To(BeNil()) - }) - }) - }) -}) diff --git a/controllers/awsmachinepool_controller.go b/controllers/awsmachinepool_controller.go index 03a22587..8bb1426e 100644 --- a/controllers/awsmachinepool_controller.go +++ b/controllers/awsmachinepool_controller.go @@ -19,7 +19,6 @@ package controllers import ( "context" "fmt" - "time" awsclientgo "github.com/aws/aws-sdk-go/aws/client" "github.com/aws/aws-sdk-go/service/iam/iamiface" @@ -96,14 +95,12 @@ func (r *AWSMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, errors.WithStack(err) } - mainRoleName := awsMachinePool.Spec.AWSLaunchTemplate.IamInstanceProfile - var iamService *iam.IAMService { c := iam.IAMServiceConfig{ AWSSession: awsClientSession, ClusterName: clusterName, - MainRoleName: mainRoleName, + MainRoleName: awsMachinePool.Spec.AWSLaunchTemplate.IamInstanceProfile, Log: logger, RoleType: iam.NodesRole, Region: awsCluster.Spec.Region, @@ -117,102 +114,64 @@ func (r *AWSMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Reque } if awsMachinePool.DeletionTimestamp != nil { - roleUsed, err := isRoleUsedElsewhere(ctx, r.Client, mainRoleName) + return r.reconcileDelete(ctx, awsMachinePool, iamService, logger) + } + return r.reconcileNormal(ctx, awsMachinePool, iamService, logger) +} + +func (r *AWSMachinePoolReconciler) reconcileDelete(ctx context.Context, awsMachinePool *expcapa.AWSMachinePool, iamService *iam.IAMService, logger logr.Logger) (ctrl.Result, error) { + roleUsed, err := isRoleUsedElsewhere(ctx, r.Client, awsMachinePool.Spec.AWSLaunchTemplate.IamInstanceProfile) + if err != nil { + return ctrl.Result{}, errors.WithStack(err) + } + + if !roleUsed { + err = iamService.DeleteRole() if err != nil { return ctrl.Result{}, errors.WithStack(err) } + } - if !roleUsed { - err = iamService.DeleteRole() - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } + // remove finalizer from AWSMachinePool + if controllerutil.ContainsFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) { + patchHelper, err := patch.NewHelper(awsMachinePool, r.Client) + if err != nil { + return ctrl.Result{}, errors.WithStack(err) } - - // remove finalizer from AWSCluster - { - awsCluster, err := key.GetAWSClusterByName(ctx, r.Client, clusterName, awsMachinePool.GetNamespace()) - if err != nil { - logger.Error(err, "failed to get awsCluster") - return ctrl.Result{}, errors.WithStack(err) - } - if controllerutil.ContainsFinalizer(awsCluster, key.FinalizerName(iam.NodesRole)) { - patchHelper, err := patch.NewHelper(awsCluster, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.RemoveFinalizer(awsCluster, key.FinalizerName(iam.NodesRole)) - err = patchHelper.Patch(ctx, awsCluster) - if err != nil { - logger.Error(err, "failed to remove finalizer on AWSCluster") - return ctrl.Result{}, errors.WithStack(err) - } - logger.Info("successfully removed finalizer from AWSCluster", "finalizer_name", iam.NodesRole) - } + controllerutil.RemoveFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) + err = patchHelper.Patch(ctx, awsMachinePool) + if err != nil { + logger.Error(err, "failed to remove finalizer from AWSMachinePool") + return ctrl.Result{}, errors.WithStack(err) } + logger.Info("successfully removed finalizer from AWSMachinePool", "finalizer_name", iam.NodesRole) + } - // remove finalizer from AWSMachinePool - if controllerutil.ContainsFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) { - patchHelper, err := patch.NewHelper(awsMachinePool, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.RemoveFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) - err = patchHelper.Patch(ctx, awsMachinePool) - if err != nil { - logger.Error(err, "failed to remove finalizer from AWSMachinePool") - return ctrl.Result{}, errors.WithStack(err) - } - logger.Info("successfully removed finalizer from AWSMachinePool", "finalizer_name", iam.NodesRole) - } - } else { - // add finalizer to AWSMachinePool - if !controllerutil.ContainsFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) { - patchHelper, err := patch.NewHelper(awsMachinePool, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.AddFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) - err = patchHelper.Patch(ctx, awsMachinePool) - if err != nil { - logger.Error(err, "failed to add finalizer on AWSMachinePool") - return ctrl.Result{}, errors.WithStack(err) - } - logger.Info("successfully added finalizer to AWSMachinePool", "finalizer_name", iam.NodesRole) - } + return ctrl.Result{}, nil +} - // add finalizer to AWSCluster - { - awsCluster, err := key.GetAWSClusterByName(ctx, r.Client, clusterName, awsMachinePool.GetNamespace()) - if err != nil { - logger.Error(err, "failed to get awsCluster") - return ctrl.Result{}, errors.WithStack(err) - } - if !controllerutil.ContainsFinalizer(awsCluster, key.FinalizerName(iam.NodesRole)) { - patchHelper, err := patch.NewHelper(awsCluster, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.AddFinalizer(awsCluster, key.FinalizerName(iam.NodesRole)) - err = patchHelper.Patch(ctx, awsCluster) - if err != nil { - logger.Error(err, "failed to add finalizer on AWSCluster") - return ctrl.Result{}, errors.WithStack(err) - } - logger.Info("successfully added finalizer to AWSCluster", "finalizer_name", iam.NodesRole) - } +func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, awsMachinePool *expcapa.AWSMachinePool, iamService *iam.IAMService, logger logr.Logger) (ctrl.Result, error) { + // add finalizer to AWSMachinePool + if !controllerutil.ContainsFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) { + patchHelper, err := patch.NewHelper(awsMachinePool, r.Client) + if err != nil { + return ctrl.Result{}, errors.WithStack(err) } - - err = iamService.ReconcileRole() + controllerutil.AddFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) + err = patchHelper.Patch(ctx, awsMachinePool) if err != nil { + logger.Error(err, "failed to add finalizer on AWSMachinePool") return ctrl.Result{}, errors.WithStack(err) } + logger.Info("successfully added finalizer to AWSMachinePool", "finalizer_name", iam.NodesRole) + } + + err := iamService.ReconcileRole() + if err != nil { + return ctrl.Result{}, errors.WithStack(err) } - return ctrl.Result{ - Requeue: true, - RequeueAfter: time.Minute * 5, - }, nil + return ctrl.Result{}, nil } // SetupWithManager sets up the controller with the Manager. diff --git a/controllers/awsmachinetemplate_controller.go b/controllers/awsmachinetemplate_controller.go index 97d7bb39..df861d4c 100644 --- a/controllers/awsmachinetemplate_controller.go +++ b/controllers/awsmachinetemplate_controller.go @@ -19,7 +19,6 @@ package controllers import ( "context" "fmt" - "time" awsclientgo "github.com/aws/aws-sdk-go/aws/client" "github.com/aws/aws-sdk-go/service/iam/iamiface" @@ -115,14 +114,12 @@ func (r *AWSMachineTemplateReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - mainRoleName := awsMachineTemplate.Spec.Template.Spec.IAMInstanceProfile - var iamService *iam.IAMService { c := iam.IAMServiceConfig{ AWSSession: awsClientSession, ClusterName: clusterName, - MainRoleName: mainRoleName, + MainRoleName: awsMachineTemplate.Spec.Template.Spec.IAMInstanceProfile, Log: logger, RoleType: role, Region: awsCluster.Spec.Region, @@ -136,209 +133,203 @@ func (r *AWSMachineTemplateReconciler) Reconcile(ctx context.Context, req ctrl.R } if awsMachineTemplate.DeletionTimestamp != nil { - roleUsed, err := isRoleUsedElsewhere(ctx, r.Client, mainRoleName) - if err != nil { - return ctrl.Result{}, err - } + return r.reconcileDelete(ctx, iamService, awsMachineTemplate, logger, clusterName, req.Namespace, role) + } + return r.reconcileNormal(ctx, iamService, awsMachineTemplate, logger, clusterName, req.Namespace, role) - if !roleUsed { - err = iamService.DeleteRole() - if err != nil { - return ctrl.Result{}, err - } - if role == iam.ControlPlaneRole { - if r.EnableKiamRole { - err = iamService.DeleteKiamRole() - if err != nil { - return ctrl.Result{}, err - } - } +} - if r.EnableRoute53Role { - err = iamService.DeleteRoute53Role() - if err != nil { - return ctrl.Result{}, err - } - } - } - } - // remove finalizer from AWSCluster - { - awsCluster, err := key.GetAWSClusterByName(ctx, r.Client, clusterName, awsMachineTemplate.GetNamespace()) - if err != nil { - logger.Error(err, "failed to get awsCluster") - return ctrl.Result{}, err - } +func (r *AWSMachineTemplateReconciler) reconcileDelete(ctx context.Context, iamService *iam.IAMService, awsMachineTemplate *capa.AWSMachineTemplate, logger logr.Logger, clusterName, namespace, role string) (ctrl.Result, error) { + roleUsed, err := isRoleUsedElsewhere(ctx, r.Client, awsMachineTemplate.Spec.Template.Spec.IAMInstanceProfile) + if err != nil { + return ctrl.Result{}, err + } - if controllerutil.ContainsFinalizer(awsCluster, key.FinalizerName(iam.ControlPlaneRole)) { - patchHelper, err := patch.NewHelper(awsCluster, r.Client) - if err != nil { - return ctrl.Result{}, err - } - controllerutil.RemoveFinalizer(awsCluster, key.FinalizerName(iam.ControlPlaneRole)) - err = patchHelper.Patch(ctx, awsCluster) + if !roleUsed { + err = iamService.DeleteRole() + if err != nil { + return ctrl.Result{}, err + } + if role == iam.ControlPlaneRole { + if r.EnableRoute53Role { + err = iamService.DeleteRoute53Role() if err != nil { - logger.Error(err, "failed to remove finalizer on AWSCluster") return ctrl.Result{}, err } - logger.Info("successfully removed finalizer from AWSCluster", "finalizer_name", iam.ControlPlaneRole) } } + } + // remove finalizer from AWSCluster + { + awsCluster, err := key.GetAWSClusterByName(ctx, r.Client, clusterName, awsMachineTemplate.GetNamespace()) + if err != nil { + logger.Error(err, "failed to get awsCluster") + return ctrl.Result{}, err + } - // remove finalizer from AWSMachineTemplate - if controllerutil.ContainsFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) { - patchHelper, err := patch.NewHelper(awsMachineTemplate, r.Client) + if controllerutil.ContainsFinalizer(awsCluster, key.FinalizerName(iam.ControlPlaneRole)) { + patchHelper, err := patch.NewHelper(awsCluster, r.Client) if err != nil { - return ctrl.Result{}, errors.WithStack(err) + return ctrl.Result{}, err } - controllerutil.RemoveFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) - err = patchHelper.Patch(ctx, awsMachineTemplate) + controllerutil.RemoveFinalizer(awsCluster, key.FinalizerName(iam.ControlPlaneRole)) + err = patchHelper.Patch(ctx, awsCluster) if err != nil { - logger.Error(err, "failed to remove finalizer from AWSMachineTemplate") - return ctrl.Result{}, errors.WithStack(err) + logger.Error(err, "failed to remove finalizer on AWSCluster") + return ctrl.Result{}, err } - logger.Info("successfully removed finalizer from AWSMachineTemplate", "finalizer_name", iam.ControlPlaneRole) + logger.Info("successfully removed finalizer from AWSCluster", "finalizer_name", iam.ControlPlaneRole) } + } - cm := &corev1.ConfigMap{} - err = r.Get( - ctx, - types.NamespacedName{ - Namespace: req.NamespacedName.Namespace, - Name: fmt.Sprintf("%s-%s", clusterName, "cluster-values"), - }, - cm) + // remove finalizer from AWSMachineTemplate + if controllerutil.ContainsFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) { + patchHelper, err := patch.NewHelper(awsMachineTemplate, r.Client) if err != nil { - logger.Error(err, "Failed to get the cluster-values configmap for cluster") return ctrl.Result{}, errors.WithStack(err) } + controllerutil.RemoveFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) + err = patchHelper.Patch(ctx, awsMachineTemplate) + if err != nil { + logger.Error(err, "failed to remove finalizer from AWSMachineTemplate") + return ctrl.Result{}, errors.WithStack(err) + } + logger.Info("successfully removed finalizer from AWSMachineTemplate", "finalizer_name", iam.ControlPlaneRole) + } - if controllerutil.ContainsFinalizer(cm, key.FinalizerName(iam.ControlPlaneRole)) { - patchHelper, err := patch.NewHelper(cm, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.RemoveFinalizer(cm, key.FinalizerName(iam.ControlPlaneRole)) - err = patchHelper.Patch(ctx, cm) - if err != nil { - logger.Error(err, "failed to remove finalizer from configmap") - return ctrl.Result{}, errors.WithStack(err) - } - logger.Info("successfully removed finalizer from configmap", "finalizer_name", iam.ControlPlaneRole) + cm := &corev1.ConfigMap{} + err = r.Get( + ctx, + types.NamespacedName{ + Namespace: namespace, + Name: fmt.Sprintf("%s-%s", clusterName, "cluster-values"), + }, + cm) + if err != nil { + logger.Error(err, "Failed to get the cluster-values configmap for cluster") + return ctrl.Result{}, errors.WithStack(err) + } + + if controllerutil.ContainsFinalizer(cm, key.FinalizerName(iam.ControlPlaneRole)) { + patchHelper, err := patch.NewHelper(cm, r.Client) + if err != nil { + return ctrl.Result{}, errors.WithStack(err) } - } else { - // add finalizer to AWSMachineTemplate - if !controllerutil.ContainsFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) { - patchHelper, err := patch.NewHelper(awsMachineTemplate, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.AddFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) - err = patchHelper.Patch(ctx, awsMachineTemplate) + controllerutil.RemoveFinalizer(cm, key.FinalizerName(iam.ControlPlaneRole)) + err = patchHelper.Patch(ctx, cm) + if err != nil { + logger.Error(err, "failed to remove finalizer from configmap") + return ctrl.Result{}, errors.WithStack(err) + } + logger.Info("successfully removed finalizer from configmap", "finalizer_name", iam.ControlPlaneRole) + } + + return ctrl.Result{}, nil +} + +func (r *AWSMachineTemplateReconciler) reconcileNormal(ctx context.Context, iamService *iam.IAMService, awsMachineTemplate *capa.AWSMachineTemplate, logger logr.Logger, clusterName, namespace, role string) (ctrl.Result, error) { + // add finalizer to AWSMachineTemplate + if !controllerutil.ContainsFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) { + patchHelper, err := patch.NewHelper(awsMachineTemplate, r.Client) + if err != nil { + return ctrl.Result{}, errors.WithStack(err) + } + controllerutil.AddFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) + err = patchHelper.Patch(ctx, awsMachineTemplate) + if err != nil { + logger.Error(err, "failed to add finalizer on AWSMachineTemplate") + return ctrl.Result{}, errors.WithStack(err) + } + logger.Info("successfully added finalizer to AWSMachineTemplate", "finalizer_name", iam.ControlPlaneRole) + } + var awsCluster *capa.AWSCluster + var err error + // add finalizer to AWSCluster + { + awsCluster, err = key.GetAWSClusterByName(ctx, r.Client, clusterName, awsMachineTemplate.GetNamespace()) + if err != nil { + logger.Error(err, "failed to get awsCluster") + return ctrl.Result{}, errors.WithStack(err) + } + if !controllerutil.ContainsFinalizer(awsCluster, key.FinalizerName(iam.ControlPlaneRole)) { + patchHelper, err := patch.NewHelper(awsCluster, r.Client) if err != nil { - logger.Error(err, "failed to add finalizer on AWSMachineTemplate") return ctrl.Result{}, errors.WithStack(err) } - logger.Info("successfully added finalizer to AWSMachineTemplate", "finalizer_name", iam.ControlPlaneRole) - } - var awsCluster *capa.AWSCluster - // add finalizer to AWSCluster - { - awsCluster, err = key.GetAWSClusterByName(ctx, r.Client, clusterName, awsMachineTemplate.GetNamespace()) + controllerutil.AddFinalizer(awsCluster, key.FinalizerName(iam.ControlPlaneRole)) + err = patchHelper.Patch(ctx, awsCluster) if err != nil { - logger.Error(err, "failed to get awsCluster") + logger.Error(err, "failed to add finalizer on AWSCluster") return ctrl.Result{}, errors.WithStack(err) } - if !controllerutil.ContainsFinalizer(awsCluster, key.FinalizerName(iam.ControlPlaneRole)) { - patchHelper, err := patch.NewHelper(awsCluster, r.Client) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } - controllerutil.AddFinalizer(awsCluster, key.FinalizerName(iam.ControlPlaneRole)) - err = patchHelper.Patch(ctx, awsCluster) - if err != nil { - logger.Error(err, "failed to add finalizer on AWSCluster") - return ctrl.Result{}, errors.WithStack(err) - } - logger.Info("successfully added finalizer to AWSCluster", "finalizer_name", iam.ControlPlaneRole) - } + logger.Info("successfully added finalizer to AWSCluster", "finalizer_name", iam.ControlPlaneRole) } + } - cm := &corev1.ConfigMap{} - err = r.Get( - ctx, - types.NamespacedName{ - Namespace: req.NamespacedName.Namespace, - Name: fmt.Sprintf("%s-%s", clusterName, "cluster-values"), - }, - cm) + cm := &corev1.ConfigMap{} + err = r.Get( + ctx, + types.NamespacedName{ + Namespace: namespace, + Name: fmt.Sprintf("%s-%s", clusterName, "cluster-values"), + }, + cm) + if err != nil { + logger.Error(err, "Failed to get the cluster-values configmap for cluster") + return ctrl.Result{}, errors.WithStack(err) + } + + if controllerutil.ContainsFinalizer(cm, key.FinalizerName(iam.ControlPlaneRole)) { + patchHelper, err := patch.NewHelper(cm, r.Client) + if err != nil { + return ctrl.Result{}, errors.WithStack(err) + } + controllerutil.RemoveFinalizer(cm, key.FinalizerName(iam.ControlPlaneRole)) + err = patchHelper.Patch(ctx, cm) if err != nil { - logger.Error(err, "Failed to get the cluster-values configmap for cluster") + logger.Error(err, "failed to remove finalizer from configmap") return ctrl.Result{}, errors.WithStack(err) } + logger.Info("successfully removed finalizer from configmap", "finalizer_name", iam.ControlPlaneRole) + } - if controllerutil.ContainsFinalizer(cm, key.FinalizerName(iam.ControlPlaneRole)) { - patchHelper, err := patch.NewHelper(cm, r.Client) + err = iamService.ReconcileRole() + if err != nil { + return ctrl.Result{}, err + } + if role == iam.ControlPlaneRole { + // route53 role depends on KIAM role + if r.EnableRoute53Role { + logger.Info("reconciling IRSA roles") + identityRefName := awsCluster.Spec.IdentityRef.Name + awsClusterRoleIdentity, err := key.GetAWSClusterRoleIdentity(ctx, r.Client, identityRefName) if err != nil { + logger.Error(err, "could not get AWSClusterRoleIdentity") return ctrl.Result{}, errors.WithStack(err) } - controllerutil.RemoveFinalizer(cm, key.FinalizerName(iam.ControlPlaneRole)) - err = patchHelper.Patch(ctx, cm) + + accountID, err := key.GetAWSAccountID(awsClusterRoleIdentity) if err != nil { - logger.Error(err, "failed to remove finalizer from configmap") + logger.Error(err, "Could not get account ID") return ctrl.Result{}, errors.WithStack(err) } - logger.Info("successfully removed finalizer from configmap", "finalizer_name", iam.ControlPlaneRole) - } - err = iamService.ReconcileRole() - if err != nil { - return ctrl.Result{}, err - } - if role == iam.ControlPlaneRole { - if r.EnableKiamRole { - err = iamService.ReconcileKiamRole() - if err != nil { - // IAM role for control plane may have been created already, but not known to IAM yet - // (returns `MalformedPolicyDocument: Invalid principal in policy: "AWS":"arn:aws:iam::[...]:role/control-plane-[...]"`). - // That will succeed after requeueing. - return ctrl.Result{}, errors.WithStack(err) - } + baseDomain, err := key.GetBaseDomain(ctx, r.Client, clusterName, namespace) + if err != nil { + logger.Error(err, "Could not get base domain") + return ctrl.Result{}, errors.WithStack(err) } - // route53 role depends on KIAM role - if r.EnableKiamRole && r.EnableRoute53Role { - awsClusterRoleIdentity, err := key.GetAWSClusterRoleIdentity(ctx, r.Client, awsCluster.Spec.IdentityRef.Name) - if err != nil { - logger.Error(err, "could not get AWSClusterRoleIdentity") - return ctrl.Result{}, errors.WithStack(err) - } - - accountID, err := getAWSAccountID(awsClusterRoleIdentity) - if err != nil { - logger.Error(err, "Could not get account ID") - return ctrl.Result{}, errors.WithStack(err) - } - baseDomain, err := key.GetBaseDomain(ctx, r.Client, clusterName, req.Namespace) - if err != nil { - logger.Error(err, "Could not get base domain") - return ctrl.Result{}, errors.WithStack(err) - } + cloudFrontDomain := key.CloudFrontAlias(baseDomain) - cloudFrontDomain := key.CloudFrontAlias(baseDomain) - - err = iamService.ReconcileRolesForIRSA(accountID, cloudFrontDomain) - if err != nil { - return ctrl.Result{}, errors.WithStack(err) - } + err = iamService.ReconcileRolesForIRSA(accountID, cloudFrontDomain) + if err != nil { + return ctrl.Result{}, errors.WithStack(err) } } } - return ctrl.Result{ - Requeue: true, - RequeueAfter: time.Minute * 5, - }, nil + return ctrl.Result{}, nil } // SetupWithManager sets up the controller with the Manager. diff --git a/controllers/awsmachinetemplate_controller_test.go b/controllers/awsmachinetemplate_controller_test.go index d5d5351a..c75b8565 100644 --- a/controllers/awsmachinetemplate_controller_test.go +++ b/controllers/awsmachinetemplate_controller_test.go @@ -328,41 +328,7 @@ var _ = Describe("AWSMachineTemplateReconciler", func() { ReturnRoleArn: "arn:aws:iam::12345678:role/the-profile", }, - - // KIAM - { - ExpectedName: "test-cluster-IAMManager-Role", - - ExpectedAssumeRolePolicyDocument: `{ - "Version": "2012-10-17", - "Statement": [ - { - "Effect": "Allow", - "Principal": { - "AWS": "arn:aws:iam::12345678:role/the-profile" - }, - "Action": "sts:AssumeRole" - } - ] -} -`, - - ExpectedPolicyName: "control-plane-test-cluster-policy", - ExpectedPolicyDocument: `{ - "Version": "2012-10-17", - "Statement": { - "Action": "sts:AssumeRole", - "Resource": "*", - "Effect": "Allow" - } -} -`, - - ReturnRoleArn: "arn:aws:iam::999666333:role/test-cluster-IAMManager-Role", - }, - externalDnsRoleInfo, - certManagerRoleInfo, ALBControllerRoleInfo, } diff --git a/controllers/awsmanagedcontrolplane_controller.go b/controllers/awsmanagedcontrolplane_controller.go index a49cf80e..1977e8a7 100644 --- a/controllers/awsmanagedcontrolplane_controller.go +++ b/controllers/awsmanagedcontrolplane_controller.go @@ -136,7 +136,7 @@ func (r *AWSManagedControlPlaneReconciler) Reconcile(ctx context.Context, req ct logger.Info("successfully added finalizer to AWSManagedControlPlane", "finalizer_name", iam.IRSARole) } - accountID, err := getAWSAccountID(awsClusterRoleIdentity) + accountID, err := key.GetAWSAccountID(awsClusterRoleIdentity) if err != nil { logger.Error(err, "Could not get account ID") return ctrl.Result{}, microerror.Mask(err) diff --git a/controllers/common_test.go b/controllers/common_test.go index f660b349..43a76770 100644 --- a/controllers/common_test.go +++ b/controllers/common_test.go @@ -8,21 +8,12 @@ type RoleInfo struct { ReturnRoleArn string } -const irsaRoleName = "irsa-role-test-cluster-policy" - var certManagerRoleInfo = RoleInfo{ ExpectedName: "test-cluster-CertManager-Role", ExpectedAssumeRolePolicyDocument: `{ "Version": "2012-10-17", "Statement": [ - { - "Effect": "Allow", - "Principal": { - "AWS": "arn:aws:iam::999666333:role/test-cluster-IAMManager-Role" - }, - "Action": "sts:AssumeRole" - }, { "Effect": "Allow", "Principal": { @@ -74,13 +65,6 @@ var externalDnsRoleInfo = RoleInfo{ ExpectedAssumeRolePolicyDocument: `{ "Version": "2012-10-17", "Statement": [ - { - "Effect": "Allow", - "Principal": { - "AWS": "arn:aws:iam::999666333:role/test-cluster-IAMManager-Role" - }, - "Action": "sts:AssumeRole" - }, { "Effect": "Allow", "Principal": { @@ -129,13 +113,6 @@ var ALBControllerRoleInfo = RoleInfo{ ExpectedAssumeRolePolicyDocument: `{ "Version": "2012-10-17", "Statement": [ - { - "Effect": "Allow", - "Principal": { - "AWS": "arn:aws:iam::999666333:role/test-cluster-IAMManager-Role" - }, - "Action": "sts:AssumeRole" - }, { "Effect": "Allow", "Principal": { diff --git a/main.go b/main.go index d71c6ede..edfa5515 100644 --- a/main.go +++ b/main.go @@ -142,28 +142,6 @@ func main() { os.Exit(1) } - if enableIRSARole { - setupLog.Info("IRSA is enabled") - - awsClientAWSCluster, err := awsclient.New(awsclient.AWSClientConfig{ - CtrlClient: mgr.GetClient(), - Log: ctrl.Log.WithName("controllers").WithName("Secrets"), - }) - if err != nil { - setupLog.Error(err, "unable to create aws client for controller", "controller", "Secrets") - os.Exit(1) - } - if err = (&controllers.AWSClusterReconciler{ - Client: mgr.GetClient(), - Log: ctrl.Log.WithName("controllers").WithName("Secrets"), - AWSClient: awsClientAWSCluster, - IAMClientFactory: iamClientFactory, - }).SetupWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "Secret") - os.Exit(1) - } - } - if err = (&controllers.AWSManagedControlPlaneReconciler{ Client: mgr.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("AWSManagedControlPlane"), diff --git a/pkg/iam/iam.go b/pkg/iam/iam.go index e147fa53..d27d8e4f 100644 --- a/pkg/iam/iam.go +++ b/pkg/iam/iam.go @@ -180,39 +180,12 @@ func (s *IAMService) generateRoute53RoleParams(roleTypeToReconcile string, awsAc return Route53RoleParams{}, err } - var principalRoleARN string - if s.principalRoleARN != "" { - principalRoleARN = s.principalRoleARN - } else { - i := &awsiam.GetRoleInput{ - RoleName: aws.String(roleName(KIAMRole, s.clusterName)), - } - - o, err := s.iamClient.GetRole(i) - if err != nil { - s.log.Error(err, "failed to fetch KIAM role") - return Route53RoleParams{}, err - } - - principalRoleARN = *o.Role.Arn - } - - if s.roleType == KIAMRole { - params := Route53RoleParams{ - EC2ServiceDomain: ec2ServiceDomain(s.region), - PrincipalRoleARN: principalRoleARN, - } - - return params, nil - } - params := Route53RoleParams{ EC2ServiceDomain: ec2ServiceDomain(s.region), AccountID: awsAccountID, CloudFrontDomain: cloudFrontDomain, Namespace: namespace, ServiceAccount: serviceAccount, - PrincipalRoleARN: principalRoleARN, } return params, nil diff --git a/pkg/iam/iam_suite_test.go b/pkg/iam/iam_suite_test.go new file mode 100644 index 00000000..43dcce12 --- /dev/null +++ b/pkg/iam/iam_suite_test.go @@ -0,0 +1,13 @@ +package iam_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestIam(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Iam Suite") +} diff --git a/pkg/iam/iam_test.go b/pkg/iam/iam_test.go new file mode 100644 index 00000000..60945480 --- /dev/null +++ b/pkg/iam/iam_test.go @@ -0,0 +1,105 @@ +package iam_test + +import ( + "errors" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/awserr" + awsclientgo "github.com/aws/aws-sdk-go/aws/client" + "github.com/aws/aws-sdk-go/aws/session" + awsIAM "github.com/aws/aws-sdk-go/service/iam" + "github.com/aws/aws-sdk-go/service/iam/iamiface" + "github.com/golang/mock/gomock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + ctrl "sigs.k8s.io/controller-runtime" + + "github.com/giantswarm/capa-iam-operator/pkg/iam" + "github.com/giantswarm/capa-iam-operator/pkg/test/mocks" +) + +var _ = Describe("ReconcileRole", func() { + + var ( + mockCtrl *gomock.Controller + mockIAMClient *mocks.MockIAMAPI + iamService *iam.IAMService + err error + sess awsclientgo.ConfigProvider + ) + + BeforeEach(func() { + //create me new iam service config with mocks + sess, err = session.NewSession(&aws.Config{ + Region: aws.String("eu-west-1")}, + ) + Expect(err).NotTo(HaveOccurred()) + + mockCtrl = gomock.NewController(GinkgoT()) + mockIAMClient = mocks.NewMockIAMAPI(mockCtrl) + + iamConfig := iam.IAMServiceConfig{ + ClusterName: "test-cluster", + MainRoleName: "test-role", + Region: "test-region", + RoleType: "control-plane", + PrincipalRoleARN: "test-principal-role-arn", + Log: ctrl.Log, + AWSSession: sess, + IAMClientFactory: func(session awsclientgo.ConfigProvider) iamiface.IAMAPI { + return mockIAMClient + }, + } + iamService, err = iam.New(iamConfig) + Expect(err).To(BeNil()) + }) + + When("role is present", func() { + BeforeEach(func() { + mockIAMClient.EXPECT().GetRole(gomock.Any()).Return(&awsIAM.GetRoleOutput{Role: &awsIAM.Role{ + Tags: []*awsIAM.Tag{{Key: aws.String("capi-iam-controller/owned"), Value: aws.String("test-cluster")}}, + }}, nil).AnyTimes() + }) + When("inline policy is already attached", func() { + BeforeEach(func() { + mockIAMClient.EXPECT().ListRolePolicies(gomock.Any()).Return(&awsIAM.ListRolePoliciesOutput{PolicyNames: aws.StringSlice([]string{"control-plane-test-cluster-policy"})}, nil) + }) + It("should return nil", func() { + err := iamService.ReconcileRole() + Expect(err).To(BeNil()) + }) + }) + When("could not attach InlinePolicy", func() { + JustBeforeEach(func() { + mockIAMClient.EXPECT().ListRolePolicies(gomock.Any()).Return(&awsIAM.ListRolePoliciesOutput{}, nil).AnyTimes() + mockIAMClient.EXPECT().PutRolePolicy(gomock.Any()).Return(&awsIAM.PutRolePolicyOutput{}, errors.New("test error")).AnyTimes() + }) + It("should return error", func() { + err := iamService.ReconcileRole() + Expect(err).NotTo(BeNil()) + }) + }) + }) + + When("role is not present", func() { + BeforeEach(func() { + mockIAMClient.EXPECT().GetRole(gomock.Any()).Return(&awsIAM.GetRoleOutput{}, awserr.New(awsIAM.ErrCodeNoSuchEntityException, "test", nil)).Times(1) + mockIAMClient.EXPECT().CreateRole(gomock.Any()).Return(&awsIAM.CreateRoleOutput{}, nil) + mockIAMClient.EXPECT().CreateInstanceProfile(gomock.Any()).Return(&awsIAM.CreateInstanceProfileOutput{}, nil) + mockIAMClient.EXPECT().AddRoleToInstanceProfile(gomock.Any()).Return(&awsIAM.AddRoleToInstanceProfileOutput{}, nil) + mockIAMClient.EXPECT().GetRole(gomock.Any()).Return(&awsIAM.GetRoleOutput{Role: &awsIAM.Role{ + Tags: []*awsIAM.Tag{{Key: aws.String("capi-iam-controller/owned"), Value: aws.String("test-cluster")}}, + }}, nil).AnyTimes() + mockIAMClient.EXPECT().ListRolePolicies(gomock.Any()).Return(&awsIAM.ListRolePoliciesOutput{}, nil).AnyTimes() + mockIAMClient.EXPECT().PutRolePolicy(gomock.Any()).Return(&awsIAM.PutRolePolicyOutput{}, nil).AnyTimes() + }) + It("should create the role", func() { + err := iamService.ReconcileRole() + Expect(err).To(BeNil()) + }) + }) + + AfterEach(func() { + mockCtrl.Finish() + }) +}) diff --git a/pkg/iam/route53_template.go b/pkg/iam/route53_template.go index a801330a..2ad2bf76 100644 --- a/pkg/iam/route53_template.go +++ b/pkg/iam/route53_template.go @@ -3,13 +3,6 @@ package iam const trustIdentityPolicyKIAMAndIRSA = `{ "Version": "2012-10-17", "Statement": [ - { - "Effect": "Allow", - "Principal": { - "AWS": "{{.PrincipalRoleARN}}" - }, - "Action": "sts:AssumeRole" - }, { "Effect": "Allow", "Principal": { diff --git a/pkg/key/key.go b/pkg/key/key.go index f916e533..2b94d939 100644 --- a/pkg/key/key.go +++ b/pkg/key/key.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + awsarn "github.com/aws/aws-sdk-go/aws/arn" corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -147,3 +148,13 @@ func GetBaseDomain(ctx context.Context, ctrlClient client.Client, clusterName, n return baseDomain, nil } + +func GetAWSAccountID(awsClusterRoleIdentity *capa.AWSClusterRoleIdentity) (string, error) { + arn := awsClusterRoleIdentity.Spec.RoleArn + a, err := awsarn.Parse(arn) + if err != nil { + return "", microerror.Mask(err) + } + + return a.AccountID, nil +} diff --git a/pkg/test/mocks/generate.go b/pkg/test/mocks/generate.go index 2bb43e52..24b5afe9 100644 --- a/pkg/test/mocks/generate.go +++ b/pkg/test/mocks/generate.go @@ -1,4 +1,5 @@ //go:generate ../../../tools/mockgen -destination aws_iam_mock.go -package mocks github.com/aws/aws-sdk-go/service/iam/iamiface IAMAPI //go:generate ../../../tools/mockgen -destination awsclient_mock.go -package mocks -source ../../awsclient/awsclient.go AWSClient +//go:generate ../../../tools/mockgen -destination eks_mock.go -package mocks github.com/aws/aws-sdk-go/service/eks/eksiface EKSAPI package mocks