From ab40882129815b598019d7931cd6c92b2f670c12 Mon Sep 17 00:00:00 2001 From: Eytan Avisror Date: Tue, 12 May 2020 22:09:55 -0700 Subject: [PATCH 1/6] bootstrap improvements add locking around bootstrap, and call bootstrap on updates before we wait for nodes --- controllers/instancegroup_controller.go | 9 ++++---- controllers/provisioners/eks/create.go | 2 +- controllers/provisioners/eks/delete.go | 5 ++-- controllers/provisioners/eks/eks.go | 7 ++++++ controllers/provisioners/eks/eks_test.go | 3 +++ controllers/provisioners/eks/helpers.go | 29 ++++++++++++++++++++++++ controllers/provisioners/eks/update.go | 7 ++++++ controllers/provisioners/eks/upgrade.go | 19 ++++++++++++---- controllers/provisioners/provisioners.go | 2 ++ 9 files changed, 72 insertions(+), 11 deletions(-) diff --git a/controllers/instancegroup_controller.go b/controllers/instancegroup_controller.go index bbe0c90c..aa7bf84e 100644 --- a/controllers/instancegroup_controller.go +++ b/controllers/instancegroup_controller.go @@ -46,7 +46,7 @@ import ( // InstanceGroupReconciler reconciles an InstanceGroup object type InstanceGroupReconciler struct { - client.Client + Client client.Client Log logr.Logger ControllerConfPath string ControllerTemplatePath string @@ -106,6 +106,7 @@ func (r *InstanceGroupReconciler) NewProvisionerInput(instanceGroup *v1alpha1.In InstanceGroup: instanceGroup, Configuration: config, Log: r.Log, + Client: r.Client, } return input, nil } @@ -121,7 +122,7 @@ func (r *InstanceGroupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro _ = r.Log.WithValues("instancegroup", req.NamespacedName) instanceGroup := &v1alpha1.InstanceGroup{} - err := r.Get(context.Background(), req.NamespacedName, instanceGroup) + err := r.Client.Get(context.Background(), req.NamespacedName, instanceGroup) if err != nil { if kerrors.IsNotFound(err) { r.Log.Info("instancegroup not found", "instancegroup", req.Name, "namespace", req.Namespace) @@ -152,7 +153,7 @@ func (r *InstanceGroupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro switch provisionerKind { case eks.ProvisionerName: ctx := eks.New(input) - defer r.Update(context.Background(), ctx.GetInstanceGroup()) + defer r.Client.Update(context.Background(), ctx.GetInstanceGroup()) err = HandleReconcileRequest(ctx) if err != nil { r.Log.Error(err, @@ -174,7 +175,7 @@ func (r *InstanceGroupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro } case eksmanaged.ProvisionerName: ctx := eksmanaged.New(input) - defer r.Update(context.Background(), ctx.GetInstanceGroup()) + defer r.Client.Update(context.Background(), ctx.GetInstanceGroup()) err = HandleReconcileRequest(ctx) if err != nil { r.Log.Error(err, diff --git a/controllers/provisioners/eks/create.go b/controllers/provisioners/eks/create.go index dc0a8f2a..64437459 100644 --- a/controllers/provisioners/eks/create.go +++ b/controllers/provisioners/eks/create.go @@ -58,7 +58,7 @@ func (ctx *EksInstanceGroupContext) Create() error { return nil } -func (ctx EksInstanceGroupContext) CreateScalingGroup() error { +func (ctx *EksInstanceGroupContext) CreateScalingGroup() error { var ( instanceGroup = ctx.GetInstanceGroup() spec = instanceGroup.GetEKSSpec() diff --git a/controllers/provisioners/eks/delete.go b/controllers/provisioners/eks/delete.go index dc33be73..020ced2e 100644 --- a/controllers/provisioners/eks/delete.go +++ b/controllers/provisioners/eks/delete.go @@ -17,7 +17,6 @@ package eks import ( "github.com/keikoproj/instance-manager/api/v1alpha1" - "github.com/keikoproj/instance-manager/controllers/common" "github.com/pkg/errors" "github.com/aws/aws-sdk-go/aws" @@ -39,7 +38,9 @@ func (ctx *EksInstanceGroupContext) Delete() error { } // if scaling group is deleted, defer removal from aws-auth - defer common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN}) + if err := ctx.RemoveAuthRole(roleARN); err != nil { + return errors.Wrap(err, "failed to remove auth role") + } // delete launchconfig err = ctx.DeleteLaunchConfiguration() diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go index 4727a240..26bf9b20 100644 --- a/controllers/provisioners/eks/eks.go +++ b/controllers/provisioners/eks/eks.go @@ -16,6 +16,10 @@ limitations under the License. package eks import ( + "sync" + + "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/go-logr/logr" "github.com/keikoproj/instance-manager/api/v1alpha1" awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws" @@ -40,6 +44,7 @@ func New(p provisioners.ProvisionerInput) *EksInstanceGroupContext { KubernetesClient: p.Kubernetes, AwsWorker: p.AwsWorker, Log: p.Log.WithName("eks"), + RuntimeClient: p.Client, } instanceGroup := ctx.GetInstanceGroup() configuration := instanceGroup.GetEKSConfiguration() @@ -71,7 +76,9 @@ type EksDefaultConfiguration struct { } type EksInstanceGroupContext struct { + sync.Mutex InstanceGroup *v1alpha1.InstanceGroup + RuntimeClient client.Client KubernetesClient kubeprovider.KubernetesClientSet AwsWorker awsprovider.AwsWorker DiscoveredState *DiscoveredState diff --git a/controllers/provisioners/eks/eks_test.go b/controllers/provisioners/eks/eks_test.go index c8d5c657..1c7db6a9 100644 --- a/controllers/provisioners/eks/eks_test.go +++ b/controllers/provisioners/eks/eks_test.go @@ -36,7 +36,9 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" dynamic "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" + fclient "sigs.k8s.io/controller-runtime/pkg/client/fake" ) var ( @@ -74,6 +76,7 @@ func MockKubernetesClientSet() kubeprovider.KubernetesClientSet { func MockContext(instanceGroup *v1alpha1.InstanceGroup, kube kubeprovider.KubernetesClientSet, w awsprovider.AwsWorker) *EksInstanceGroupContext { input := provisioners.ProvisionerInput{ + Client: fclient.NewFakeClientWithScheme(scheme.Scheme), AwsWorker: w, Kubernetes: kube, InstanceGroup: instanceGroup, diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go index 7402601a..a5c2b903 100644 --- a/controllers/provisioners/eks/helpers.go +++ b/controllers/provisioners/eks/helpers.go @@ -16,14 +16,18 @@ limitations under the License. package eks import ( + "context" "fmt" "reflect" "sort" "strings" + "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/autoscaling" "github.com/keikoproj/instance-manager/api/v1alpha1" + "github.com/keikoproj/instance-manager/controllers/common" awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws" kubeprovider "github.com/keikoproj/instance-manager/controllers/providers/kubernetes" "github.com/keikoproj/instance-manager/controllers/provisioners" @@ -329,3 +333,28 @@ func (ctx *EksInstanceGroupContext) GetManagedPoliciesList(additionalPolicies [] } return managedPolicies } + +func (ctx *EksInstanceGroupContext) RemoveAuthRole(arn string) error { + ctx.Lock() + defer ctx.Unlock() + + var instanceGroup = ctx.GetInstanceGroup() + instanceGroups := &v1alpha1.InstanceGroupList{} + ctx.RuntimeClient.List(context.Background(), instanceGroups, client.MatchingField("status.nodesInstanceRoleArn", arn)) + // If there are other instance groups using the same role we should not remove it from aws-auth + if len(instanceGroups.Items) > 1 { + ctx.Log.Info( + "skipping removal of auth role, is used by another instancegroup", + "instancegroup", instanceGroup.GetName(), + "arn", arn, + ) + return nil + } + + err := common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{arn}) + if err != nil { + return err + } + + return nil +} diff --git a/controllers/provisioners/eks/update.go b/controllers/provisioners/eks/update.go index 65712ac4..d0a98018 100644 --- a/controllers/provisioners/eks/update.go +++ b/controllers/provisioners/eks/update.go @@ -63,6 +63,13 @@ func (ctx *EksInstanceGroupContext) Update() error { return errors.Wrap(err, "failed to update scaling group") } + // we should try to bootstrap the role before we wait for nodes to be ready + // to avoid getting locked if someone made a manual change to aws-auth + err = ctx.BootstrapNodes() + if err != nil { + ctx.Log.Error(err, "failed to bootstrap role", "instancegroup", instanceGroup.GetName()) + } + // update readiness conditions nodesReady := ctx.UpdateNodeReadyCondition() if nodesReady { diff --git a/controllers/provisioners/eks/upgrade.go b/controllers/provisioners/eks/upgrade.go index 125c1754..2577549e 100644 --- a/controllers/provisioners/eks/upgrade.go +++ b/controllers/provisioners/eks/upgrade.go @@ -74,12 +74,23 @@ func (ctx *EksInstanceGroupContext) UpgradeNodes() error { func (ctx *EksInstanceGroupContext) BootstrapNodes() error { var ( - state = ctx.GetDiscoveredState() - role = state.GetRole() - roleARN = aws.StringValue(role.Arn) + state = ctx.GetDiscoveredState() + instanceGroup = ctx.GetInstanceGroup() + role = state.GetRole() + roleARN = aws.StringValue(role.Arn) ) + ctx.Log.Info("bootstrapping arn to aws-auth", "instancegroup", instanceGroup.GetName(), "arn", roleARN) + + // lock to guarantee Upsert and Remove cannot conflict when roles are shared between instancegroups + ctx.Lock() + defer ctx.Unlock() - return common.UpsertAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN}) + err := common.UpsertAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN}) + if err != nil { + return err + } + + return nil } func (ctx *EksInstanceGroupContext) NewRollingUpdateRequest() *kubeprovider.RollingUpdateRequest { diff --git a/controllers/provisioners/provisioners.go b/controllers/provisioners/provisioners.go index 507c17bb..8e1ae93a 100644 --- a/controllers/provisioners/provisioners.go +++ b/controllers/provisioners/provisioners.go @@ -5,6 +5,7 @@ import ( "github.com/keikoproj/instance-manager/api/v1alpha1" awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws" kubeprovider "github.com/keikoproj/instance-manager/controllers/providers/kubernetes" + "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -21,6 +22,7 @@ type ProvisionerInput struct { InstanceGroup *v1alpha1.InstanceGroup Configuration ProvisionerConfiguration Log logr.Logger + Client client.Client } type ProvisionerConfiguration struct { From c7139900f4a37cdd234acca79749bff4166f2a26 Mon Sep 17 00:00:00 2001 From: Eytan Avisror Date: Wed, 13 May 2020 08:40:29 -0700 Subject: [PATCH 2/6] add test --- controllers/instancegroup_controller.go | 9 ++- controllers/providers/kubernetes/utils.go | 12 ++++ controllers/provisioners/eks/delete_test.go | 70 +++++++++++++++++++++ controllers/provisioners/eks/eks.go | 1 - controllers/provisioners/eks/eks_test.go | 3 - controllers/provisioners/eks/helpers.go | 13 ++-- controllers/provisioners/provisioners.go | 2 - 7 files changed, 93 insertions(+), 17 deletions(-) diff --git a/controllers/instancegroup_controller.go b/controllers/instancegroup_controller.go index aa7bf84e..bbe0c90c 100644 --- a/controllers/instancegroup_controller.go +++ b/controllers/instancegroup_controller.go @@ -46,7 +46,7 @@ import ( // InstanceGroupReconciler reconciles an InstanceGroup object type InstanceGroupReconciler struct { - Client client.Client + client.Client Log logr.Logger ControllerConfPath string ControllerTemplatePath string @@ -106,7 +106,6 @@ func (r *InstanceGroupReconciler) NewProvisionerInput(instanceGroup *v1alpha1.In InstanceGroup: instanceGroup, Configuration: config, Log: r.Log, - Client: r.Client, } return input, nil } @@ -122,7 +121,7 @@ func (r *InstanceGroupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro _ = r.Log.WithValues("instancegroup", req.NamespacedName) instanceGroup := &v1alpha1.InstanceGroup{} - err := r.Client.Get(context.Background(), req.NamespacedName, instanceGroup) + err := r.Get(context.Background(), req.NamespacedName, instanceGroup) if err != nil { if kerrors.IsNotFound(err) { r.Log.Info("instancegroup not found", "instancegroup", req.Name, "namespace", req.Namespace) @@ -153,7 +152,7 @@ func (r *InstanceGroupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro switch provisionerKind { case eks.ProvisionerName: ctx := eks.New(input) - defer r.Client.Update(context.Background(), ctx.GetInstanceGroup()) + defer r.Update(context.Background(), ctx.GetInstanceGroup()) err = HandleReconcileRequest(ctx) if err != nil { r.Log.Error(err, @@ -175,7 +174,7 @@ func (r *InstanceGroupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro } case eksmanaged.ProvisionerName: ctx := eksmanaged.New(input) - defer r.Client.Update(context.Background(), ctx.GetInstanceGroup()) + defer r.Update(context.Background(), ctx.GetInstanceGroup()) err = HandleReconcileRequest(ctx) if err != nil { r.Log.Error(err, diff --git a/controllers/providers/kubernetes/utils.go b/controllers/providers/kubernetes/utils.go index 9f7bb84a..5865707c 100644 --- a/controllers/providers/kubernetes/utils.go +++ b/controllers/providers/kubernetes/utils.go @@ -24,10 +24,12 @@ import ( "strings" "github.com/ghodss/yaml" + "github.com/keikoproj/instance-manager/api/v1alpha1" "github.com/keikoproj/instance-manager/controllers/common" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" @@ -40,6 +42,16 @@ type KubernetesClientSet struct { KubeDynamic dynamic.Interface } +func GetUnstructuredInstanceGroup(instanceGroup *v1alpha1.InstanceGroup) (*unstructured.Unstructured, error) { + var obj = &unstructured.Unstructured{} + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(instanceGroup) + if err != nil { + return obj, err + } + obj.Object = content + return obj, nil +} + func IsDesiredNodesReady(kube kubernetes.Interface, instanceIds []string, desiredCount int) (bool, error) { nodes, err := kube.CoreV1().Nodes().List(metav1.ListOptions{}) if err != nil { diff --git a/controllers/provisioners/eks/delete_test.go b/controllers/provisioners/eks/delete_test.go index 59271056..bb3c3eb9 100644 --- a/controllers/provisioners/eks/delete_test.go +++ b/controllers/provisioners/eks/delete_test.go @@ -18,12 +18,16 @@ package eks import ( "testing" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/autoscaling" "github.com/aws/aws-sdk-go/service/iam" + awsauth "github.com/keikoproj/aws-auth/pkg/mapper" "github.com/keikoproj/instance-manager/api/v1alpha1" + kubeprovider "github.com/keikoproj/instance-manager/controllers/providers/kubernetes" "github.com/onsi/gomega" "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestDeletePositive(t *testing.T) { @@ -122,5 +126,71 @@ func TestDeleteAutoScalingGroupNegative(t *testing.T) { g.Expect(err).To(gomega.HaveOccurred()) g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileDeleting)) asgMock.DeleteAutoScalingGroupErr = nil +} + +func TestRemoveAuthRoleNegative(t *testing.T) { + var ( + g = gomega.NewGomegaWithT(t) + k = MockKubernetesClientSet() + ig = MockInstanceGroup() + ig2 = MockInstanceGroup() + asgMock = NewAutoScalingMocker() + iamMock = NewIamMocker() + ) + + w := MockAwsWorker(asgMock, iamMock) + ctx := MockContext(ig, k, w) + + // two instancegroups with same role arn + ig.Status.NodesArn = "same-role" + ig2.Name = "instance-group-2" + ig2.Namespace = "different-namespace" + ig2.Status.NodesArn = "same-role" + + igObj, err := kubeprovider.GetUnstructuredInstanceGroup(ig) + g.Expect(err).NotTo(gomega.HaveOccurred()) + ig2Obj, err := kubeprovider.GetUnstructuredInstanceGroup(ig2) + g.Expect(err).NotTo(gomega.HaveOccurred()) + + _, err = ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).Namespace("instance-manager").Create(igObj, metav1.CreateOptions{}) + g.Expect(err).NotTo(gomega.HaveOccurred()) + + _, err = ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).Namespace(ig2.Namespace).Create(ig2Obj, metav1.CreateOptions{}) + g.Expect(err).NotTo(gomega.HaveOccurred()) + ctx.SetDiscoveredState(&DiscoveredState{ + IAMRole: &iam.Role{ + Arn: aws.String("same-role"), + }, + ScalingGroup: &autoscaling.Group{}, + }) + + ctx.BootstrapNodes() + + // Only one role is added to aws-auth + auth, _, err := awsauth.ReadAuthMap(k.Kubernetes) + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(len(auth.MapRoles)).To(gomega.Equal(1)) + + // after delete, the role should not be deleted + err = ctx.Delete() + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileDeleting)) + + auth, _, err = awsauth.ReadAuthMap(k.Kubernetes) + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(len(auth.MapRoles)).To(gomega.Equal(1)) + + // this time the role should be successfully removed + err = ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).Namespace(ig2.Namespace).Delete(ig2.Name, &metav1.DeleteOptions{}) + g.Expect(err).NotTo(gomega.HaveOccurred()) + + err = ctx.Delete() + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileDeleting)) + + auth, _, err = awsauth.ReadAuthMap(k.Kubernetes) + t.Log(auth) + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(len(auth.MapRoles)).To(gomega.Equal(0)) } diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go index 26bf9b20..1e86d4f2 100644 --- a/controllers/provisioners/eks/eks.go +++ b/controllers/provisioners/eks/eks.go @@ -44,7 +44,6 @@ func New(p provisioners.ProvisionerInput) *EksInstanceGroupContext { KubernetesClient: p.Kubernetes, AwsWorker: p.AwsWorker, Log: p.Log.WithName("eks"), - RuntimeClient: p.Client, } instanceGroup := ctx.GetInstanceGroup() configuration := instanceGroup.GetEKSConfiguration() diff --git a/controllers/provisioners/eks/eks_test.go b/controllers/provisioners/eks/eks_test.go index 1c7db6a9..c8d5c657 100644 --- a/controllers/provisioners/eks/eks_test.go +++ b/controllers/provisioners/eks/eks_test.go @@ -36,9 +36,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" dynamic "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" - fclient "sigs.k8s.io/controller-runtime/pkg/client/fake" ) var ( @@ -76,7 +74,6 @@ func MockKubernetesClientSet() kubeprovider.KubernetesClientSet { func MockContext(instanceGroup *v1alpha1.InstanceGroup, kube kubeprovider.KubernetesClientSet, w awsprovider.AwsWorker) *EksInstanceGroupContext { input := provisioners.ProvisionerInput{ - Client: fclient.NewFakeClientWithScheme(scheme.Scheme), AwsWorker: w, Kubernetes: kube, InstanceGroup: instanceGroup, diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go index a5c2b903..df3974f3 100644 --- a/controllers/provisioners/eks/helpers.go +++ b/controllers/provisioners/eks/helpers.go @@ -16,13 +16,12 @@ limitations under the License. package eks import ( - "context" "fmt" "reflect" "sort" "strings" - "sigs.k8s.io/controller-runtime/pkg/client" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/autoscaling" @@ -339,10 +338,12 @@ func (ctx *EksInstanceGroupContext) RemoveAuthRole(arn string) error { defer ctx.Unlock() var instanceGroup = ctx.GetInstanceGroup() - instanceGroups := &v1alpha1.InstanceGroupList{} - ctx.RuntimeClient.List(context.Background(), instanceGroups, client.MatchingField("status.nodesInstanceRoleArn", arn)) + list, err := ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).List(metav1.ListOptions{ + FieldSelector: fmt.Sprintf("%v=%v", ".status.nodesInstanceRoleArn", arn), + }) + // If there are other instance groups using the same role we should not remove it from aws-auth - if len(instanceGroups.Items) > 1 { + if len(list.Items) > 1 { ctx.Log.Info( "skipping removal of auth role, is used by another instancegroup", "instancegroup", instanceGroup.GetName(), @@ -351,7 +352,7 @@ func (ctx *EksInstanceGroupContext) RemoveAuthRole(arn string) error { return nil } - err := common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{arn}) + err = common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{arn}) if err != nil { return err } diff --git a/controllers/provisioners/provisioners.go b/controllers/provisioners/provisioners.go index 8e1ae93a..507c17bb 100644 --- a/controllers/provisioners/provisioners.go +++ b/controllers/provisioners/provisioners.go @@ -5,7 +5,6 @@ import ( "github.com/keikoproj/instance-manager/api/v1alpha1" awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws" kubeprovider "github.com/keikoproj/instance-manager/controllers/providers/kubernetes" - "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -22,7 +21,6 @@ type ProvisionerInput struct { InstanceGroup *v1alpha1.InstanceGroup Configuration ProvisionerConfiguration Log logr.Logger - Client client.Client } type ProvisionerConfiguration struct { From 50eaf7cc54f6fbae7a164d97145dcc4f63dc5269 Mon Sep 17 00:00:00 2001 From: Eytan Avisror Date: Wed, 13 May 2020 09:05:05 -0700 Subject: [PATCH 3/6] cleanup --- controllers/provisioners/eks/eks.go | 3 --- controllers/provisioners/eks/helpers.go | 7 +------ 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go index 1e86d4f2..4d1470d0 100644 --- a/controllers/provisioners/eks/eks.go +++ b/controllers/provisioners/eks/eks.go @@ -18,8 +18,6 @@ package eks import ( "sync" - "sigs.k8s.io/controller-runtime/pkg/client" - "github.com/go-logr/logr" "github.com/keikoproj/instance-manager/api/v1alpha1" awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws" @@ -77,7 +75,6 @@ type EksDefaultConfiguration struct { type EksInstanceGroupContext struct { sync.Mutex InstanceGroup *v1alpha1.InstanceGroup - RuntimeClient client.Client KubernetesClient kubeprovider.KubernetesClientSet AwsWorker awsprovider.AwsWorker DiscoveredState *DiscoveredState diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go index df3974f3..94982e4f 100644 --- a/controllers/provisioners/eks/helpers.go +++ b/controllers/provisioners/eks/helpers.go @@ -352,10 +352,5 @@ func (ctx *EksInstanceGroupContext) RemoveAuthRole(arn string) error { return nil } - err = common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{arn}) - if err != nil { - return err - } - - return nil + return common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{arn}) } From 2cf8fe06bde95070f58e4cb4e8d3c7fb097379e9 Mon Sep 17 00:00:00 2001 From: Eytan Avisror Date: Wed, 13 May 2020 09:09:19 -0700 Subject: [PATCH 4/6] Update upgrade.go --- controllers/provisioners/eks/upgrade.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/controllers/provisioners/eks/upgrade.go b/controllers/provisioners/eks/upgrade.go index 2577549e..8e83ebfd 100644 --- a/controllers/provisioners/eks/upgrade.go +++ b/controllers/provisioners/eks/upgrade.go @@ -85,12 +85,7 @@ func (ctx *EksInstanceGroupContext) BootstrapNodes() error { ctx.Lock() defer ctx.Unlock() - err := common.UpsertAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN}) - if err != nil { - return err - } - - return nil + return common.UpsertAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN}) } func (ctx *EksInstanceGroupContext) NewRollingUpdateRequest() *kubeprovider.RollingUpdateRequest { From 5428859bf0cb9733fe578f286e43edec5054e893 Mon Sep 17 00:00:00 2001 From: Eytan Avisror Date: Wed, 13 May 2020 09:28:01 -0700 Subject: [PATCH 5/6] avoid field selectors --- controllers/provisioners/eks/helpers.go | 26 +++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go index 94982e4f..e0c0dfbc 100644 --- a/controllers/provisioners/eks/helpers.go +++ b/controllers/provisioners/eks/helpers.go @@ -21,8 +21,6 @@ import ( "sort" "strings" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/autoscaling" "github.com/keikoproj/instance-manager/api/v1alpha1" @@ -31,6 +29,8 @@ import ( kubeprovider "github.com/keikoproj/instance-manager/controllers/providers/kubernetes" "github.com/keikoproj/instance-manager/controllers/provisioners" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func (ctx *EksInstanceGroupContext) GetLaunchConfigurationInput(name string) *autoscaling.CreateLaunchConfigurationInput { @@ -338,16 +338,30 @@ func (ctx *EksInstanceGroupContext) RemoveAuthRole(arn string) error { defer ctx.Unlock() var instanceGroup = ctx.GetInstanceGroup() - list, err := ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).List(metav1.ListOptions{ - FieldSelector: fmt.Sprintf("%v=%v", ".status.nodesInstanceRoleArn", arn), - }) + var list = &unstructured.UnstructuredList{} + var sharedGroups = make([]string, 0) + + list, err := ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).List(metav1.ListOptions{}) + if err != nil { + return err + } + + // find objects which share the same nodesInstanceRoleArn + for _, obj := range list.Items { + if val, ok, _ := unstructured.NestedString(obj.Object, "status", "nodesInstanceRoleArn"); ok { + if strings.EqualFold(arn, val) { + sharedGroups = append(sharedGroups, obj.GetName()) + } + } + } // If there are other instance groups using the same role we should not remove it from aws-auth - if len(list.Items) > 1 { + if len(sharedGroups) > 1 { ctx.Log.Info( "skipping removal of auth role, is used by another instancegroup", "instancegroup", instanceGroup.GetName(), "arn", arn, + "conflict", strings.Join(sharedGroups, ","), ) return nil } From 43ad28878909e42d5ca92be7dc868618b40433ea Mon Sep 17 00:00:00 2001 From: Eytan Avisror Date: Wed, 13 May 2020 09:36:55 -0700 Subject: [PATCH 6/6] Update delete.go --- controllers/provisioners/eks/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/provisioners/eks/delete.go b/controllers/provisioners/eks/delete.go index 020ced2e..72c1f03c 100644 --- a/controllers/provisioners/eks/delete.go +++ b/controllers/provisioners/eks/delete.go @@ -37,7 +37,7 @@ func (ctx *EksInstanceGroupContext) Delete() error { return errors.Wrap(err, "failed to delete scaling group") } - // if scaling group is deleted, defer removal from aws-auth + // if scaling group is deleted, remove the role from aws-auth if it's not in use by other groups if err := ctx.RemoveAuthRole(roleARN); err != nil { return errors.Wrap(err, "failed to remove auth role") }