Operator rewrites declarative image paths in an inconsistent manner

[clausstrommer]

When instantiating vmagent custom resources, if the spec contains image.repository then the operator adds a library prefix to the container's repository string https://github.com/VictoriaMetrics/operator/blob/master/internal/controller/operator/factory/build/container.go#L245 . This causes several inconsistencies:

• Organisations using cloud-hosted private registries would want to use [registry url]/[organisation] as the global registry with the operator chart's global.image.registry value, and [image name] as the image.repository in the vmagent custom resource's spec. In this scenario, instead of [registry url]/[organisation]/[image name] the vmagent's image is rewritten as [registry url]/[organisation]/library/[image name]
• By comparison, the operator does not add the library infix for the config-reloader images.
• When setting global.image.registry, the image.registry parameter in the vmagent's spec is ignored. This means that one cannot override it as a workaround for the agent's image by setting the registry to [registry url] and the image to [organisation]/[image name].

I believe the assumption from #1816 about requiring the library infix when image.repository does not contain / is incorrect, and that the default mode should be to not add the infix unless the registry matches a certain pattern.

[clausstrommer]

PS: Additional bug - neither the operator's global.image.registry nor the VMAgent custom resource's image.registry are respected when containerImage contains /, because of https://github.com/VictoriaMetrics/operator/blob/master/internal/controller/operator/factory/build/container.go#L242 . It should be return registry + containerImage, which makes the first part of the if condition redundant since it's the same as the final return value. This again supports my assumption that the injection if the library/ infix should be only a special case, not a default operation.

[AndrewChubatiuk]

Organisations using cloud-hosted private registries would want to use [registry url]/[organisation] as the global registry with the operator chart's global.image.registry value, and [image name] as the image.repository in the vmagent custom resource's spec. In this scenario, instead of [registry url]/[organisation]/[image name] the vmagent's image is rewritten as [registry url]/[organisation]/library/[image name]

most likely besides setting custom registry you also override repository for each CR. if you're already modifying repository why don't you set it to <organization>/<image_name>?

By comparison, the operator does not add the library infix for the config-reloader images

in this case probably you don't override image for config reloader, like you most likely do for spec.image.repository

When setting global.image.registry, the image.registry parameter in the vmagent's spec is ignored. This means that one cannot override it as a workaround for the agent's image by setting the registry to [registry url] and the image to [organisation]/[image name]

is it k8s stack chart? it's hard to answer without exact cases

Additional bug - neither the operator's global.image.registry nor the VMAgent custom resource's image.registry are respected when containerImage contains /

this is not true, container image should contain more than one slash to ignore registry
here's a case, that covers it

[clausstrommer]

most likely besides setting custom registry you also override repository for each CR. if you're already modifying repository why don't you set it to /<image_name>?

As per my earlier comment, this is not sufficient. I have worked around this by leaving registry as "" and setting repository to [registry]/[organisation]/[image], but that means that I cannot set global.image.registry on the operator to enforce the registry to be our mirror.

By comparison, the operator does not add the library infix for the config-reloader images
in this case probably you don't override image for config reloader, like you most likely do for spec.image.repository

We do in fact override the image for config reloader. The difference is that the config reloader image is specified in the operator's Helm release, whereas VMAgent's image is specified in the VMAgent custom resource.

is it k8s stack chart? it's hard to answer without exact cases

victoria-metrics-operator chart, version 0.66.2

values set as:

global:
  imagePullSecrets:
    - name: gcr-pull-secret
  image: 
    registry: gcr.io/[redacted]
image:
  repository: republished_docker.io-victoriametrics-operator
configReloader:
  image:
    repository: republished_docker.io-victoriametrics-operator                     
    tag: config-reloader-v0.73.1

Both the operator and config-reloader images correctly render as gcr.io/[redacted]/republished_docker.io-victoriametrics-operator:[tag]

With VMAgent specced as

apiVersion: operator.victoriametrics.com/v1beta1
kind: VMAgent
metadata:
  name: vmagent
  namespace: vmagent
spec:
  externalLabels:
    cluster_name: [redacted]
    project: [redacted]
    workspace: personal
  image:
    registry: gcr.io/[redacted]
    repository: republished_docker.io-victoriametrics-vmagent
  remoteWrite:
  - [redacted]
  selectAllByDefault: true

the vmagent container gets generated as gcr.io/[redacted]/library/republished_docker.io-victoriametrics-vmagent:v1.147.0

Changing the spec to

spec:
  image:
    registry: gcr.io
    repository: [redacted]/republished_docker.io-victoriametrics-vmagent

generates gcr.io/[redacted]/[redacted]/republished_docker.io-victoriametrics-vmagent:v1.147.0 (the global registry overrides the custom resource registry).

If I change the global operator registry to "", the vmagent container changes to [redacted]/republished_docker.io-victoriametrics-vmagent:v1.147.0 (note that it does not add spec.image.registry from the VMAgent custom resource).

[AndrewChubatiuk]

there's no spec.image.registry. check api spec here
if you want to override global registry you need to set spec.image.repository to gcr.io/[redacted]/republished_docker.io-victoriametrics-vmagent

[AndrewChubatiuk]

in operator helm chart configReloader section is used to generate value of VM_CONFIG_RELOADER_IMAGE variable

[AndrewChubatiuk]

added a PR, which removes library/ from result image, as a workaround configure properly spec.image.repository as I've mentioned above

[clausstrommer]

there's no spec.image.registry.

Well that would explain it. I was assuming that it did exist, because the operator did not reject the spec. Should the operator's validating webhook be catching this?

added a PR, which removes library/ from result image

Thanks!

[AndrewChubatiuk]

starting 0.71.0 strict unmarshalling was enabled, but only for CR creation, for update it will be enabled later