Cluster-api-provider-aws: NPE if MachineVersion is not set.

[chuckha]

/kind bug

What steps did you take and what happened:
I tried to create a machine and got an NPE because I forgot to set the Machine version.

What did you expect to happen:
I expected a default value

Anything else you would like to add:
This is the line throwing the NPE https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/89e8ab62a6ee0de8f6644ae44cb7f224fc202f38/pkg/cloud/services/ec2/instances.go#L126

[chuckha]

/priority longterm-imporatant
/milestone Next

[ncdc]

@chuckha please update the link to the source code line to a specific commit (you can press y in the browser and it will change from master to a commit) - thanks!

[chuckha]

yes, very good call. thx

[tahsinrahman]

@chuckha @ncdc if you’re not working on this, can take this? Thanks!

[chuckha]

@tahsinrahman yes, absolutely

/assign @tahsinrahman

[chuckha]

/lifecycle active

[chuckha]

The lifecycle active is for @tahsinrahman so that other people do not swoop in and take this issue away from them.

[tahsinrahman]

In kubeadm bootstrapper does the following

• kubernetes version defined in machine.Spec.Version, not in kubeadm cluster config, uses the machine.Spec.Version
• Not defined in machine.Spec.Version, but defined in kubeadm cluster config, then it uses kubeadm cluster config
• Defined in both machine.Spec.Version and kubeadm cluster config, then it uses the kubeadm config
• Not defined anywhere, then kubeadm defaults to a value itself

For aws provider, the machine.Spec.Version must be defined, and it must match the bootstrapper version, as we're using it for AMI lookup. So

• the the machine.Spec.Version should be updated/reconciled from bootstrap provider to set it to correct version, or
• machine.Spec.Versoin should be a required field, and bootstrap provider must use the version defined here

[ncdc]

Worth pointing out: we only do an AMI lookup if you haven't specified an AMI to use in your AWSMachine spec.

[tahsinrahman]

IMO, both the versions should be consistent. If one take precedence over the other, the other field should be updated

[ncdc]

cc @chuckha @fabriziopandini - PTAL at the logic described above and let me know if you have any thoughts.

[fabriziopandini]

the machine.Spec.Version should be updated/reconciled from bootstrap provider to set it to correct version

If for the AWS providers only there are specific consistency requirements around versions, at least at a first sight, I find a little bit weird to delegate the enforcement of such requirements to the bootstrap provider. So I don't think the bootstrap provider should not update in any case machine.Spec.Version.

[ncdc]

For CAPA, if machine.spec.version is nil or empty, possible options could include:

1. Set the AWSMachine's status.error* fields to indicate a permanent failure
2. Record an event on the AWSMachine indicating the Machine needs a version
3. Record an event on the Machine indicating it needs a version

Does any of these make sense? Anything I'm missing?

[detiber]

@ncdc I'd like to avoid 1, since manually fixing it would require a user to remove ErrorMessage/ErrorReason in addition to adding the version.

I like the idea of 2 or 3, but it is not persistent, so maybe we need a Conditions field or similar to record it in a more persistent way?

[detiber]

Alternatively, we could register a validation webhook as part of deploying CAPA that could enforce the requirement.

[ncdc]

I originally left out the webhook as an option because I didn't think it would cover all the cases, but now I'm thinking it should work.

Are we able to state that webhooks are required for correct operation? In other words, can we assume that things like machine.spec.version will always be populated because the validating webhook guards against that?

[vincepri]

Is the webhook you're talking about a CAPI one or an additional one that CAPA runs only on Machines that are attached to AWSMachines?

[ncdc]

CAPA on Machines with infra ref = AWSMachine

[detiber]

I think once we have webhooks it is more than reasonable to say that they are required for correct operation

[ncdc]

🎶 to my ears

[ncdc]

/milestone v0.5.0

[joonas]

@tahsinrahman is this something you're still interested in implementing?

[tahsinrahman]

/unassign
/remove lifecycle-active

[aaroniscode]

/assign

[ncdc]

Recording some Slack discussions: because we can't control or guarantee the order that the Machine & AWSMachine are created, and we also don't have an OwnerReference until after the AWSMachine is initially persisted, I don't think this is a very easy problem to solve. We could instead:

1. Mark the AWSMachine as terminally failed and require the user to delete it, update the machine with a version, and create a new AWSMachine
2. Don't NPE, but instead emit an event when we hit this scenario

I think option 1 has a better UX because events are not always the first thing people look at, and they have a TTL.

[ncdc]

Trying to minimize the scope of 0.5, so I'm going to move this to Next. If we get a fix in before the release, that's great too. We can document this is a known issue until a fix is in place.

/milestone Next