Cluster-api-provider-azure: Make Image Parameters More User Friendly

[juan-lee]

/kind feature

Describe the solution you'd like
The various ways a user can specify an image is confusing due to the rules for knowing what parameters are required.

Consider using a claim like approach or using RuntimeExtension for the image field. That way, each of the image options can be expressed as a concrete type where it is obvious which fields are required.

https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/291/files#r332150770

[CecileRobertMichon]

/help

[k8s-ci-robot]

@CecileRobertMichon:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

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.

[richardcase]

I look at this

[richardcase]

/assign @richardcase

[richardcase]

@juan-lee - looking at your comments here, are you thinking that there will be 3 new resource kinds?

For example, AzureImageMarketplace, AzureImageSharedGallery, AzureImageWithID. Something like this:

type AzureImageMarketplace struct {
    metav1.TypeMeta   `json:",inline"`
    metav1.ObjectMeta `json:"metadata,omitempty"`

    Spec AzureImageMarketplaceSpec `json:"spec,omitempty"`
}

type AzureImageMarketplaceSpec struct {
    Publisher string `json:"publisher"`
    Offer     string `json:"offer"`
    SKU       string `json:"sku"`
    Version   string `json:"version"`
}

and then Image in VM becomes something like:

Image         corev1.ObjectReference `json:"image,omitempty"`

(or TypedLocalObjectReference if the same namespace is assumed)
There would then be validation of allowed types for Image. A bit like DataSource in PersistentVolumeClaimSpec

Another option would be to have an 'image source' interface and 3 structs with just the specific fields required, for example:

type ImageMarketplace struct {
    Publisher string `json:"publisher"`
    Offer     string `json:"offer"`
    SKU       string `json:"sku"`
    Version   string `json:"version"`
}

type ImageSharedGallery struct {
    SubscriptionID string `json:"subscriptionID"`
    ResourceGroup  string `json:"resourceGroup"`
    Gallery        string `json:"gallery"`
    Name           string `json:"name"`
    Version        string `json:"version"`
}

[CecileRobertMichon]

related to #330

[CecileRobertMichon]

@richardcase thanks for taking a look at this! Your proposal sounds good to me, I like the idea of using types for each way of specifying an image. Hopefully this will allow us to build stronger tests, documentation and input validation for user provided images.

[devigned]

+1 on the thx to @richardcase.

I prefer the consistency and specificity of second approach where the Image will be treated like a PVC DataSource. The different types of image structs will provide strong typing with validation / documentation to help guide a user down a path of success.

[richardcase]

Thanks @CecileRobertMichon & @devigned

I started on the different types / ObjectReference approach like PVC DataSource, so i'm glad you both thought that was the better way. Will get it complete asap.

[richardcase]

@devigned @CecileRobertMichon - could i get your opinion on something? If we go down the different image structs/kind and use PVC DataSource pattern we'll end up with something like this:

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2
kind: AzureImageMarketplace
metadata:
  name: debian_buster
spec:
  publisher: "Debian"
  offer: "debian-10"
  sku: "10"
  version: "latest"
---
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2
kind: AzureMachineTemplate
metadata:
  name: ${CLUSTER_NAME}-md-0
spec:
  template:
    spec:
      location: ${AZURE_LOCATION}
      vmSize: ${NODE_MACHINE_TYPE}
      osDisk:
        osType: "Linux"
        diskSizeGB: 30
        managedDisk:
          storageAccountType: "Premium_LRS"
      sshPublicKey: ${SSH_PUBLIC_KEY}
      image:
        apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2
        kind: AzureImageMarketplace
        name: debian_buster

Do you think this is ok? Or is this over complicating it?

If it is ok, then i have some thoughts on when we check that we have a valid image and the status contents.

[devigned]

I don't think it's too complicated. In fact, I think it is easier to grok than the existing solution.

What are you thinking about status. Just check if the market place image exists? If so, this PR is doing something similar in aks-engine: https://github.com/Azure/aks-engine/pull/2342.

[serbrech]

much better than a generic struct for every type of image.
I like this approach, anytime we have a common concept with multiple representations, I think it's clearer to have a reference to a kind, rather than cramming it into a half-fitting structure.
It also allows for cleaner extensibility both in the API and in code. 👍

[richardcase]

I don't think it's too complicated. In fact, I think it is easier to grok than the existing solution.

Great, thanks. I have started down this line but i thought i'd better get another opinion before i got too far.

What are you thinking about status. Just check if the market place image exists? If so, this PR is doing something similar in aks-engine: Azure/aks-engine#2342.

Yeah basically just check the image exists and update the phase in the status. Thanks for link, that code will be helpful!

Not sure if this is a crazy idea but should the images have a reference back to the machines as well?

[devigned]

Not sure if this is a crazy idea but should the images have a reference back to the machines as well?

Hmm... might be early optimization. What do you think about leaving it out for now? If needed in the future, it would be an additive change.

[richardcase]

Sounds like a plan to me.

[richardcase]

/lifecycle active

[vincepri]

I did a quick review of the linked PR, the thing that stood out the most is the use of runtime.RawExtension, in the past we've used this in Cluster API and quickly turned around because of lack of type safety and code complexity.

One alternative:

type Image struct {
    // +optional
    ID *string `json:"id,omitempty"`

    // +optional
    SharedGallery  *AzureSharedGalleryImage `json:"sharedGallery,omitempty"`

    // +optional
    Marketplace *AzureMarketplaceImage `json:"marketplace,omitempty"`
}

By having all of these marked as +optional and omitempty we're giving free reign to users to choose which one to pick. The defaulting webhook sets the default case when the user doesn't specify anything, while the validation webhook rejects any Machine with more than one option set.

wdyt?

[richardcase]

I can change it to this, which is very similar to the original implementation with just a bit more structure to Image. I was advised to use runtime.RawExtension but happy to change based on experience gained elsewhere

[richardcase]

Shall I make this change?

[detiber]

I'm +1 for the change, I believe we should strive for type safety wherever we can.

[CecileRobertMichon]

+1
I would personally prefer we follow Vince's recommendation if you don't mind the extra work @richardcase. Sorry for the back and forth. I'll prioritize getting your PR merged so it doesn't have to rebased again after the changes.

[richardcase]

Sounds like good consensus to me and thinking about it I also prefer the suggestion. RawExtension did feel a little ugly.

@CecileRobertMichon - will make the change tomorrow

[devigned]

I'm pretty new to this stuff, so I might be asking a silly question, but if we used an ObjectRef, could we add validation, not webhook validation but rather JSON schema validation, to ensure the Kind specified is one of an enumeration? That way, the consumer of the API would have clear guidance in the JSON schema that the ObjectRef must be one of a set of Kinds. From that, we could then have those kinds implement an interface to apply themselves to the spec, so that we can treat them uniformly.

Outside of being able to constrain the type, +1 to Vince's recommendation.

Originally, we had all optionals with no clear guidance on what combination needed to be set. At least with Vince's proposal, there are clear options even though all the things are optional. Seems like a pragmatic solution for this instance of the union type problem.

[detiber]

From the documentation for ObjectReference:

ObjectReference contains enough information to let you inspect or modify the referred object. --- New uses of this type are discouraged because of difficulty describing its usage when embedded in APIs.

In practice it also leads to quite a bit of user confusion around what fields are used/ignored in practice. We are looking at replacing our use of it in sigs.k8s.io/cluster-api for this reason as well.

You should be able to have json schema validation of enum types by specifying a structural schema: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#specifying-a-structural-schema, however I'm not sure if controller-tools has been updated for it yet.

[devigned]

@detiber, that's exactly what I was thinking about with the validation of the enum, but was also unsure about controller-tool support.

In practice it also leads to quite a bit of user confusion around what fields are used/ignored in practice. We are looking at replacing our use of it in sigs.k8s.io/cluster-api for this reason as well.

How does cluster-api intend on dealing with union types like this going forward? Is there an on going discussion / issue?

[detiber]

Not directly related to union types, but we already have a rough proposal for removing existing use of ObjectReference: https://github.com/kubernetes-sigs/cluster-api/issues/2318

We also plan on adopting structural schema, but I don't think we have a tracking issue for that, more of a high level idea that we should do it.

[devigned]

That link is super helpful. I see what you mean when you say, not just about union types. The goal is to trim down the fields to the minimal amount needed to reference an object in the given context. The pattern of representing union types through a reference to an object is not really covered.

Perhaps, the way to handle union types in the spirit of having the least exposed fields to a consumer would be to create a custom reference type for each set of union types with enum validations on the Group/Kind field, something similar to TypedLocalReference, but constrained to the types allowed.

[detiber]

Perhaps, the way to handle union types in the spirit of having the least exposed fields to a consumer would be to create a custom reference type for each set of union types with enum validations on the Group/Kind field, something similar to TypedLocalReference, but constrained to the types allowed

That's definitely a neat idea, I can see us leveraging it in a few different places at least when the group/kinds are known. It wouldn't necessarily work for some of the core types where we allow different provider types to be referenced.

On a side note, I am glad that you excluded Version when you mentioned the validations... As we've recently found in Cluster API, it is sometimes best to ignore the Version, otherwise it could introduce potential headaches come upgrade time (especially with the alpha types we are currently using).

[devigned]

On a side note, I am glad that you excluded Version when you mentioned the validations... As we've recently found in Cluster API, it is sometimes best to ignore the Version, otherwise it could introduce potential headaches come upgrade time (especially with the alpha types we are currently using).

So, to be clear, the version the controller would work with would be the controller's preferred version?

[detiber]

So, to be clear, the version the controller would work with would be the controller's preferred version?

In the case of known types, yes.

In the case of provider-specified types, we are doing some hacky introspection currently around thee deployed CRD types to determine which version to use: https://master.cluster-api.sigs.k8s.io/developer/providers/v1alpha2-to-v1alpha3.html#apply-the-contract-version-label-clusterx-k8sioversion-version1version2version3-to-your-crds

We are looking to find a better alternative in the future compared to reading labels off the deployed CRDs.