Cluster-api-provider-aws: CAPA fails silently when the instance profile of machine is incorrectly specified

[harishspqr]

/kind bug

What steps did you take and what happened:
Create a new cluster. Specify a different/wrong instance profile role for controlplane than whats available on the account.
CAPA fails silently with no errors or events being generated.

What did you expect to happen:
CAPA must log the failure and generate a warning/error event indicating the reason.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

• Cluster-api-provider-aws version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

[vincepri]

@harishspqr Which CAPA version are you using?

[ncdc]

I'm guessing 0.3.7 or 0.3.8

[vincepri]

I'm not sure if we should backport this issue to v0.3.x, but I set the milestone at least

[ncdc]

I'm going to move it to 0.4.x

[ncdc]

/assign

[ncdc]

Reproduces in v0.4.0. CAPA pod logs:

I0913 15:00:44.621730       1 awsmachine_controller.go:233] controllers/AWSMachine/awscluster-controller/namespace=default/awsMachine=capi-quickstart-controlplane-0/infrastructure.cluster.x-k8s.io/v1alpha2/machine=capi-quickstart-controlplane-0/cluster=capi-quickstart/awsCluster=capi-quickstart "level"=0 "msg"="Reconciling AWSMachine"
I0913 15:00:45.215074       1 awscluster_controller.go:140] controllers/AWSCluster/awscluster-controller/namespace=default/awsCluster=capi-quickstart/infrastructure.cluster.x-k8s.io/v1alpha2/cluster=capi-quickstart "level"=0 "msg"="Reconciling AWSCluster"
E0913 15:00:46.158773       1 controller.go:218] controller-runtime/controller "msg"="Reconciler error" "error"="failed to create AWSMachine instance: failed to run instance: InvalidParameterValue: Value (badcontrollers.cluster-api-provider-aws.sigs.k8s.io) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name\n\tstatus code: 400, request id: ce82bd19-6f70-45cc-bed3-3ad169f92a1b"  "controller"="awsmachine" "request"={"Namespace":"default","Name":"capi-quickstart-controlplane-0"}

[ncdc]

out, err := s.runInstance(scope.Role(), input)
    if err != nil {
        // Only record the failure event if the error is not related to failed dependencies.
        // This is to avoid spamming failure events since the machine will be requeued by the actuator.
        if !awserrors.IsFailedDependency(errors.Cause(err)) {
            record.Warnf(scope.Machine, "FailedCreate", "Failed to create instance: %v", err)
        }
        return nil, err
    }

[ncdc]

I guess InvalidParameterValue is considered a failed dependency? @detiber?

[detiber]

I guess InvalidParameterValue is considered a failed dependency? @detiber?

I'm not sure it should be considered a failed dependency in general, since giving something like an invalid instance type would also likely result in a similar failure.

[ncdc]

func IsFailedDependency(err error) bool {
    if ReasonForError(err) == http.StatusFailedDependency {
        return true
    }
    return false
}

Am in interpreting this correctly - we get InvalidParameterValue and awserrors.IsFailedDependency considers it a "failed dependency" so we don't record the event?

[ncdc]

Hmm the status code listed above in the log is 400, not 424.

[detiber]

@ncdc Ah, gotcha, it's the http status code that is causing it to be a failed dependency here.

This is tricky, do we mark a Machine as hard failed in these cases? There could be eventual consistency related concerns with things like Security Groups, but I wouldn't expect similar for IAM profile because it would require an external actor to intervene.

[vincepri]

I'd appreciate if we allow users to fix errors, if these occur before creating the actual machine

[detiber]

@vincepri I'm torn there, on one hand it would be nice, but on the other how long do we continue failing for before it should be considered a permanent error? Especially since events are ephemeral and the only way we have to really highlight errors to users otherwise is via ErrorMessage/ErrorReason.

[vincepri]

Yeah, what if we write warning events to users and leave the Machine in a pending state? Truthfully, a user will notice that the machines aren't coming up and start describing the objects.

In both cases (ErrorMessage/ErrorReason) or warning event, we'd require a user to describe to take any action.

I'd like to prioritize UX in this case mostly because it seems that these kind of errors can be frequent, especially on AWS. If we go the other route, we'll have to require users to delete and recreate machines, which is a little bit of a pain :(

[detiber]

The issue is that with events if a user creates a machine, walks away and comes back in an hour or so it is no longer there... Which is also a sub-par UX...

[vincepri]

Wouldn't we keep writing the warnings though? 🤔

[detiber]

Wouldn't we keep writing the warnings though? thinking

I think the key here is what warnings are actionable vs inactionable...

Another thing to keep in mind is higher level abstractions too, how would this surface in a MachineSet or a MachineDeployment?

[vincepri]

Either way seems we have bad UX, not sure what the right answer is

[ncdc]

I'm still confused why what appears to be http 400 is considered a failed dep. I want to investigate more.

[harishspqr]

This was in version v0.3.7.

[harishspqr]

https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/1134#issuecomment-531272816 - @ncdc I did not see this error log as well.

[ncdc]

/unassign
/assign @liztio

thanks for taking this over!

[liztio]

Added the following log line to https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/master/pkg/cloud/services/ec2/instances.go#L190
s.scope.V(2).Info("Error running instance", "cause", fmt.Sprintf("%v", errors.Cause(err)), "isFailed", awserrors.IsFailedDependency(errors.Cause(err)))

and it produces:

I1002 19:38:09.709996 1 instances.go:190] controllers/AWSMachine "msg"="Error running instance" "awsCluster"="cloudsdale" "awsMachine"="cloudsdale-controlplane-0" "cluster"="cloudsdale" "machine"="cloudsdale-controlplane-0" "namespace"="default" "cause"="InvalidParameterValue: Value (invalid-controllers.cluster-api-provider-aws.sigs.k8s.io) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name\n\tstatus code: 400, request id: d4fb95aa-f14c-4dc7-84b3-efb14c8fab2f" "isFailed"=false

so the error is _not_ categorized as a dependency error.

And actually, I _do_ see events being generated:

Events:
  Type     Reason        Age   From            Message
  ----     ------        ----  ----            -------
  Warning  FailedCreate  30m   aws-controller  Failed to create instance: failed to run instance: InvalidParameterValue: Value (invalid-controllers.cluster-api-provider-aws.sigs.k8s.io) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name
           status code: 400, request id: 616cc148-52fb-4b8a-a80b-704acd2568a1
  Warning  FailedCreate  30m  aws-controller  Failed to create instance: failed to run instance: InvalidParameterValue: Value (invalid-controllers.cluster-api-provider-aws.sigs.k8s.io) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name
           status code: 400, request id: 3b15a0dd-4bd0-4888-8f22-29b85888b41c
  Warning  FailedCreate  30m  aws-controller  Failed to create instance: failed to run instance: InvalidParameterValue: Value (invalid-controllers.cluster-api-provider-aws.sigs.k8s.io) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name

is it possible this issue got solved in the mean time?

[vincepri]

It might be that your PR to the broadcaster fixed this issue, where before we'd have lost the event after a while?

[liztio]

that sounds likely to me!

[liztio]

So v0.4.1 should work, but I don't know if it's going to be possible to port those broadcaster changes back to v0.3.x

[liztio]

@harishspqr are you able to upgrade to v1alpha2/0.4.x? if so we can close this out

[ncdc]

Thanks @liztio for double checking this!

/close

[k8s-ci-robot]

@ncdc: Closing this issue.

In response to this:

Thanks @liztio for double checking this!

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.