aws-auth: controller may remove role when it is not intended

[eytan-avisror]

A common pitfall people are facing when trying out instance-manager is that they create an IG using a role that is already bootstrapped in the cluster. When the IG is deleted the role is removed causing all nodes using that role to go to NotReady state.
We should evaluate ways to avoid this such as:

• Not removing from aws-auth when a role is not managed
• Add a flag for controlling the behavior, e.g. --no-remove-auth etc
• Find a clever way to detect that there are nodes in the cluster using that role, without making too many calls - possibly cache all ec2 instances, which can map to nodes and IAM roles

Current behavior is to avoid removing roles when other IGs use them, but the case which is not covered is if a regular nodegroup is sharing a role with an IG

[David-Tamrazov]

Sort of related to this - I ran into a situation where the AWS CDK overwrote the aws-auth ConfigMap and removed the instance group role arn from the mapping that instance-manager had originally added. I'd figured the controller would reconcile that on its own but we actually had to restart the deployment to get our instance roles re-propagated to aws-auth.

I checked controller logs and I only see what seem to be startup logs which would make make sense why a reboot worked; it retriggered the aws auth sync:

$ kc logs instance-manager-5967747fff-l7qkk -n kube-system
2022-08-25T10:48:38.252Z	INFO	scaling	drift not detected	{"instancegroup": "my-runners/davos"}
2022-08-25T10:48:38.252Z	INFO	controllers.instancegroup.eks	bootstrapping arn to aws-auth	{"instancegroup": "my-runners/davos", "arn": "arn:aws:iam::122222222222:role/my-cluster-my-runners-davos"}
2022-08-25T10:48:38.268Z	INFO	controllers.instancegroup.eks	waiting for node readiness conditions	{"instancegroup": "my-runners/davos"}
2022-08-25T10:48:38.268Z	INFO	controllers.instancegroup.eks	desired nodes are ready	{"instancegroup": "my-runners/davos", "instances": ""}
2022-08-25T10:48:38.268Z	INFO	v1alpha1	state transition occured	{"instancegroup": "my-runners/davos", "state": "ReconcileModified", "previousState": "ReconcileModifying"}
2022-08-25T10:48:38.268Z	INFO	controllers.instancegroup.eks	bootstrapping arn to aws-auth	{"instancegroup": "my-runners/davos", "arn": "arn:aws:iam::122222222222:role/my-cluster-my-runners-davos"}

would it be possible to reconcile the aws-auth configmap on a loop at a certain interval? or is that not ideal because of potential conflicts?

[eytan-avisror]

Yeah, the key here is triggering a reconcile.
What we could do here since we already have a watch on configmaps, is also watch for aws-auth configmap changes, and trigger a reconcile for all instance groups.
This would immediately revert any role removals

[David-Tamrazov]

That'd be great. If we wanted to give people opt-in we could do it conditionally on an annotation on aws-auth but I'd be fine with it as a baseline behaviour.

[eytan-avisror]

Yeah, I was thinking more of a controller flag e.g. --enforce-auth that people can add with default being OFF.
Primary reason being that it's a change in behavior and also that it has implications on cluster with many IGs if there are components that are touching aws-auth a lot it might get a bit noisy with reconciles.

If you wanted to work on it I can help guide you to the areas in the code where this can be added, otherwise not sure when we'll have the capacity to implement this

[David-Tamrazov]

Sure I'd be happy to take a look; where would I get started?

[eytan-avisror]

Main place would be here:

instance-manager/controllers/reconcilers.go

Line 69 in ae893c4

func (r *InstanceGroupReconciler) configMapReconciler(obj client.Object) []ctrl.Request {

If you look at that configMapReconciler, it gets called by configmap watch here:

instance-manager/controllers/reconcilers.go

Line 62 in ae893c4

Watches(&source.Kind{Type: &corev1.ConfigMap{}}, handler.EnqueueRequestsFromMapFunc(r.configMapReconciler)).

Currently that reconciler is used to reconcile changes to the instance-manager configmap - there is an early if to only proceed if the configmap being reconciled is the instance-manager one:

instance-manager/controllers/reconcilers.go

Line 75 in ae893c4

if strings.EqualFold(name, ConfigMapName) && strings.EqualFold(namespace, r.ConfigNamespace) {

We can add an else if to that statement and check if this is the aws-auth (can be a const) in the kube-system namespace (and the flag is ON), and if that is the case, similarly to how the same reconciler triggers a reconcile to all IGs we can do the same:

instance-manager/controllers/reconcilers.go

Lines 106 to 120 in ae893c4

	requests := make([]ctrl.Request, 0)
	for _, instanceGroup := range instanceGroupList.Items {
	if instanceGroup.Status.ConfigHash != configHash {
	namespacedName := types.NamespacedName{}
	namespacedName.Name = instanceGroup.GetName()
	namespacedName.Namespace = instanceGroup.GetNamespace()
	ctrl.Log.Info("found config diff for instancegroup", "instancegroup", namespacedName, "old", instanceGroup.Status.ConfigHash, "new", configHash)
	r := ctrl.Request{
	NamespacedName: namespacedName,
	}
	requests = append(requests, r)
	}
	}
	return requests

In the above it compares the MD5 hash of the configmap data, to the last known hash (persisted in status.ConfigHash), we can probably skip all of that and simply reconcile all IGs for any aws-auth change.

There is some common code between those two if's that can probably be refactored a bit.

For adding the flag you need to pass it through from main to InstanceGroupReconciler:

instance-manager/main.go

Lines 80 to 92 in ae893c4

	flag.IntVar(&maxParallel, "max-workers", 5, "The number of maximum parallel reconciles")
	flag.IntVar(&maxAPIRetries, "max-api-retries", 12, "The number of maximum retries for failed AWS API calls")
	flag.IntVar(&configRetention, "config-retention", 2, "The number of launch configuration/template versions to retain")
	flag.Float64Var(&spotRecommendationTime, "spot-recommendation-time", 10.0, "The maximum age of spot recommendation events to consider in minutes")
	flag.StringVar(&configNamespace, "config-namespace", "instance-manager", "the namespace to watch for instance-manager configmap")
	flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.")
	flag.BoolVar(&enableLeaderElection, "enable-leader-election", false,
	"Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.")
	flag.BoolVar(&nodeRelabel, "node-relabel", true, "relabel nodes as they join with kubernetes.io/role label via controller")
	flag.BoolVar(&disableWinClusterInjection, "disable-windows-cluster-ca-injection", false, "Setting this to true will cause the ClusterCA and Endpoint to not be injected for Windows nodes")
	flag.Parse()
	ctrl.SetLogger(zap.New(zap.UseDevMode(true)))

instance-manager/main.go

Lines 152 to 168 in ae893c4

	err = (&controllers.InstanceGroupReconciler{
	Metrics: controllerCollector,
	ConfigMap: cm,
	ConfigRetention: configRetention,
	SpotRecommendationTime: spotRecommendationTime,
	ConfigNamespace: configNamespace,
	Namespaces: make(map[string]corev1.Namespace),
	NamespacesLock: &sync.RWMutex{},
	NodeRelabel: nodeRelabel,
	DisableWinClusterInjection: disableWinClusterInjection,
	Client: mgr.GetClient(),
	Log: ctrl.Log.WithName("controllers").WithName("instancegroup"),
	MaxParallel: maxParallel,
	Auth: &controllers.InstanceGroupAuthenticator{
	Aws: awsWorker,
	Kubernetes: kube,
	},

This will make it available to the configMapReconciler

Feel free to ping me if you need any help.

[David-Tamrazov]

Great, I'll poke in and hopefully get this sorted, thanks!