Add tests, fix permanent errors and inconsistent behavior in controllers (#164)Co-authored-by: calvix <vaclav@giantswarm.io> #164
Conversation
…not get service account for specified role - control-plane`
… make behavior equal to other controller
…he same name in different namespaces, i.e. look up cluster within same namespace
|
|
||
| // irsa-operator may not have created the secret yet | ||
| if apierrors.IsNotFound(err) { | ||
| return ctrl.Result{Requeue: true, RequeueAfter: 1 * time.Minute}, err |
There was a problem hiding this comment.
If we return an error, it will be requeued by default. Do we want to requeue after specifically 1 minute? I'd either
return ctrl.Result{}, err
or log the error but don't return it
return ctrl.Result{Requeue: true, RequeueAfter: 1 * time.Minute}, nil
I think I'd choose the first approach.
There was a problem hiding this comment.
I looked into controller-runtime and you're right. So the re-queueing on error fixed it for my cases, then. I'll change this.
| @@ -126,7 +134,7 @@ func (r *AWSMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Reque | |||
|
|
|||
| // remove finalizer from AWSCluster | |||
| { | |||
| awsCluster, err := key.GetAWSClusterByName(ctx, r.Client, clusterName) | |||
| awsCluster, err := key.GetAWSClusterByName(ctx, r.Client, clusterName, awsMachinePool.GetNamespace()) | |||
There was a problem hiding this comment.
Isn't this controller reconciling awsmachinepools? How come we are adding and removing finalizers to / from AWSClusters?
There was a problem hiding this comment.
Can't answer that right now, but added it to giantswarm/roadmap#2113
| return ctrl.Result{}, err | ||
| // 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-[...]"`) | ||
| return ctrl.Result{Requeue: true, RequeueAfter: 1 * time.Minute}, err |
There was a problem hiding this comment.
Same as above about requeuing.
This reverts commit b361d03.
…-runtime would always requeue
| Labels: map[string]string{ | ||
| "cluster.x-k8s.io/cluster-name": "test-cluster", | ||
| }, | ||
| Name: "my-awsc", |
There was a problem hiding this comment.
Generally, the AWSCluster and Cluster share the same name.
| Name: "my-awsc", | |
| Name: "test-cluster", |
There was a problem hiding this comment.
They don't have to, and that's why we have to look up by label. This is strongly on purpose, to avoid wrong assumptions in the code.
calvix
changed the title
Towards giantswarm/roadmap#2073, giantswarm/roadmap#1981. This is part of the attempt to get to a working release again, and allow some refactoring.
Tested together with giantswarm/cert-manager-app#295 and giantswarm/default-apps-aws#170 to ensure that we can actually release this. Since the controllers applied different policies, I've decided that we anyway keep IRSA enabled and therefore switched to only using the "allow both KIAM and IRSA" policy for consistency.
This also does not fix existing, malformed IAM resources. The future controller code should reconcile to the desired state, not "create once and then skip any further reconciliation runs". We will have to either implement that now, or go through MCs where the operator already ran and filled in the wrong info (e.g.
grizzly-CertManager-Role).This only hacks the existing code instead of making it nice. But at least we now have behavioral tests that can be reused and match the tooling we discussed as nice-to-have for unit tests.
P.S. This supersedes #161 after we had lots of productive discussion about test tooling, of which most is integrated here now.