K8SSAND-1476 ⁃ Revisit default values for images.Image struct

[adutra]

Image currently has a rather complex set of rules to determine default values for its fields. It turns out that the defaults for almost all fields can be guessed by Kubernetes, e.g. if no tag and no pull policy is provided, then the former defaults to latest and the latter to Always. Also, the registry and the repository are also automatically defaulted to docker.io and library respectively.

This logic is consequently not necessary at our level. We should be able to remove the logic altogether, but we might need to check how definitive image names are computed and that they remain valid.

On a side note, the current struct doesn't accommodate very nicely tags that are actually image hashes. See here for the format syntax. We may want to improve that too.

Update

After discussing it, we'd like to revisit how images are referenced and allow customizing registries, pull secrets, ... by using cass-operator's ImageConfig. This would require to:

• Replace all occurrences of images.Image in our CRDs and use a string instead (this means we'll have to think about upgrades and possibly introduce a conversion webhook)
• See if we can rely on the same ImageConfig entry that is used by cass-operator
• Evolve the format of the ImageConfig CRD to allow setting a dynamic list of images, each with their own pull secrets/registries/...

apiVersion: config.k8ssandra.io/v1beta1
kind: ImageConfig
metadata:
  name: image-config
images:
  system-logger:
     image: "k8ssandra/system-logger:latest"
     imageRegistry: ...
     imagePullSecret:
       name: ....
  config-builder:
     image: "datastax/cass-config-builder:1.0.4-ubi7"
  medusa:
     image: "....."
     imageRegistry: ...
     imagePullSecret:
       name: ....
  # Default settings unless overridden at the image level
  imageRegistry: "localhost:5000"
  imagePullPolicy: Always
  imagePullSecret:
    name: my-secret-pull-registry
defaults:
  # Note, postfix is ignored if repository is not set
  cassandra:
    repository: "k8ssandra/cass-management-api"
  dse:
    repository: "datastax/dse-mgmtapi-6_8"

┆Issue is synchronized with this Jira Story by Unito ┆Issue Number: K8OP-272

[jsanda]

Also, the registry and the repository are also automatically defaulted to docker.io

@bradfordcp is working on migrating images to quay.io.

[Gellardo]

:+1: to removing the default logic.

I wanted to use an alternative registry for (among others) the jmxInit image and could not figure out how to remove the library repository from the final image spec. So instead of my.registry/k8s-deployment-helper the podspec always contained my.registry/library/k8s-deployment-helper, no matter what I tried in the K8ssandraCluster CRD (using the helm chart & starting from only setting the registry and name keys and leaving repository empty).

[adutra]

Thanks for the feedback. The more it goes the more I think we shouldn't split an image URL into many pieces as we do, for at least 2 reasons:

1. The number of URL parts is variable and depends on the registry being used;
2. When using commit SHAs, the image name must end with @sha256.

None of the above fits nicely with the current Image struct.

Maybe this is enough:

type Image struct {
    Name string `json:"name,omitempty"`
    PullPolicy corev1.PullPolicy `json:"pullPolicy,omitempty"`
    PullSecretRef *corev1.LocalObjectReference `json:"pullSecretRef,omitempty"`
}

\cc @jsanda @Miles-Garnsey

[Miles-Garnsey]

@adutra, we've discussed this before and you've added two additional good reasons above to review our current Image struct.

I agree that we should move back to using the Kubernetes standard here by using simple strings to define image coordinates. I would change the field Name to Location or Coordinates though.

Additionally, I am not sure that we should encapsulate PullSecretRef in a struct with the coordinates. There is an impedance mismatch here because ImagePullSecrets (which is actually a list, not a scalar!) is defined at the pod/ServiceAccount level, not the container level (where the image coordinates are defined). As a result, I suspect it may be cleaner to define ImagePullSecrets per pod, instead of per Image.

Obviously, it is possible to write some logic to juggle the multiple PullSecretRefs into an array for the pods/ServiceAccounts, but I think it would be better to adhere as closely as possible to the k8s APIs, wdyt?

[jsanda]

I agree that we should move back to using the Kubernetes standard here by using simple strings to define image coordinates.

There are a number of images used with a K8ssandraCluster. I am inclined to think that supporting different registries is the most important part of configuring images. Using a different registry shouldn't require me to specify the registry multiple times. I should be able to specify it once. If we use simple strings, doesn't that make it more difficult to use an alternate registry without having to specify it for each image?

@burmanm did a lot of work in this area for cass-operator in support of OpenShift. We should have a consistent approach across operators and ideally k8ssandra-operator will also have the same level of OpenShift support so I would like @burmanm to provide some background on what is done in cass-operator and why.

[Miles-Garnsey]

I think we'd probably save ourselves a lot of heartache by adhering to the way that Pods/Deployments think about images, and treating them as strings. It does mean that users would have to wholly specify the image location for every image.

Using a different registry shouldn't require me to specify the registry multiple times. I should be able to specify it once. If we use simple strings, doesn't that make it more difficult to use an alternate registry without having to specify it for each image?

Yes, using simple strings means you'd need fully specified coordinates everywhere. Specifying the registry once is a nice idea but it isn't how it works in the rest of the Kubernetes ecosystem AFAIK. Why is it important?

[jsanda]

I think the DRY principle is still applicable. If we were only dealing with one or two images, it would be less of a concern, but that's not the case.

[adutra]

Additionally, I am not sure that we should encapsulate PullSecretRef in a struct with the coordinates [...] it may be cleaner to define ImagePullSecrets per pod, instead of per Image.

I'm not sold on that. If you are pulling from different registries, it's more user-friendly to co-locate the image to pull, the pull policy, and the secret to use in the same place in the CRD, rather than in scattered places. The slice of secrets is computed dynamically by images.CollectPullSecrets.

[adutra]

FYI I met a K8ssandra user at KubeCon that is waiting for a fix for this.

[jsanda]

Here is a quick summary of my thoughts after spending some time looking into this:

Private registries The most important thing is to be able to specify a private registry and ideally only once. Users can specify a different registry but it has to be done for every image which is pretty painful IMO.

Pull secrets Pull secrets feel like they can be treated like the registry in that it is something you probably only need to specify once.

Image struct The Image struct feels cumbersome and complex. I like flat strings provided we can override the default registry in a single place.

Handle images consistently in cass-operator and in k8ssandra-operator It makes sense to have a consistent approach to handling images across the operators. That should also open the door for some code reuse.

[burmanm]

After trying to implement this, I've left with more questions. I'm not entirely sure of the end result, what was wanted here. I did extend the ImageConfig to include a different pullSecret+repository override for each imageType as well as ability to add new imageTypes, so all medusa/reaper/stargate/etc are supported.

However, when adding this to k8ssandra-operator, was the idea to remove current image.Image struct entirely also and replace with just a string or was it just to add that ImageConfig as the default (instead of the defaults set in the codebase and CRD generation - latter which feels very odd) ?

@bradfordcp