Fallback specialized kubernetes config to vanilla kubernetes

[radcortez]

• Fixes Reduce verbosity of kubernetes extension config with inheritance or more opinionated defaulting #34025

[geoand]

The test failures seems related

[radcortez]

The test failures seems related

Yes, but I'm not sure if this is a bug caused by the change or the change discovered an existent bug :)

Because of the config fallback rule, the Openshift processor reads quarkus.openshift.name to test-it (from quarkus.kubernetes.name=test-it). The processor produces a ContainerImageCustomNameBuildItem in case a name is available in the Openshift config:

quarkus/integration-tests/kubernetes/quarkus-standard-way/src/test/resources/kubernetes-with-application.properties

Lines 1 to 15 in 8068cb2

	quarkus.http.port=9090
	quarkus.kubernetes.name=test-it
	quarkus.kubernetes.namespace=applications
	quarkus.kubernetes.labels.foo=bar
	quarkus.kubernetes.annotations.bar=baz
	quarkus.kubernetes.env-vars.my-env-var.value=SOMEVALUE
	quarkus.kubernetes.env-vars.my-name.field=metadata.name
	quarkus.kubernetes.add-version-to-label-selectors=false
	quarkus.container-image.group=grp
	quarkus.container-image.registry=quay.io
	quarkus.kubernetes.ingress.expose=true
	quarkus.kubernetes.ingress.host=example.com
	quarkus.kubernetes.service-type=NodePort
	quarkus.kubernetes.image-pull-policy=IfNotPresent
	quarkus.kubernetes.replicas=3

quarkus/extensions/kubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftProcessor.java

Lines 103 to 107 in d01808f

	@BuildStep
	public void populateCustomImageName(OpenshiftConfig openshiftConfig,
	BuildProducer<ContainerImageCustomNameBuildItem> containerImageCustomName) {
	openshiftConfig.name.ifPresent(name -> containerImageCustomName.produce(new ContainerImageCustomNameBuildItem(name)));
	}

Which ends up modifying the container image name that fails the test:

Expected :"quay.io/grp/kubernetes-with-application-properties:0.1-SNAPSHOT"
Actual   :"quay.io/grp/test-it:0.1-SNAPSHOT"

Shouldn't quarkus.kubernetes.name on its own also modify the container image name?

[radcortez]

//cc @Sgitario @edeandrea @holly-cummins

[Sgitario]

The test failures seems related

Yes, but I'm not sure if this is a bug caused by the change or the change discovered an existent bug :)

Because of the config fallback rule, the Openshift processor reads quarkus.openshift.name to test-it (from quarkus.kubernetes.name=test-it). The processor produces a ContainerImageCustomNameBuildItem in case a name is available in the Openshift config:

quarkus/integration-tests/kubernetes/quarkus-standard-way/src/test/resources/kubernetes-with-application.properties

Lines 1 to 15 in 8068cb2

	quarkus.http.port=9090
	quarkus.kubernetes.name=test-it
	quarkus.kubernetes.namespace=applications
	quarkus.kubernetes.labels.foo=bar
	quarkus.kubernetes.annotations.bar=baz
	quarkus.kubernetes.env-vars.my-env-var.value=SOMEVALUE
	quarkus.kubernetes.env-vars.my-name.field=metadata.name
	quarkus.kubernetes.add-version-to-label-selectors=false
	quarkus.container-image.group=grp
	quarkus.container-image.registry=quay.io
	quarkus.kubernetes.ingress.expose=true
	quarkus.kubernetes.ingress.host=example.com
	quarkus.kubernetes.service-type=NodePort
	quarkus.kubernetes.image-pull-policy=IfNotPresent
	quarkus.kubernetes.replicas=3

quarkus/extensions/kubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftProcessor.java

Lines 103 to 107 in d01808f

	@BuildStep
	public void populateCustomImageName(OpenshiftConfig openshiftConfig,
	BuildProducer<ContainerImageCustomNameBuildItem> containerImageCustomName) {
	openshiftConfig.name.ifPresent(name -> containerImageCustomName.produce(new ContainerImageCustomNameBuildItem(name)));
	}

Which ends up modifying the container image name that fails the test:

Expected :"quay.io/grp/kubernetes-with-application-properties:0.1-SNAPSHOT"
Actual   :"quay.io/grp/test-it:0.1-SNAPSHOT"

Shouldn't quarkus.kubernetes.name on its own also modify the container image name?

This is the new behaviour after #33724. So Kubernetes name or openshift name should never modify the container image name, so if this is still happening in the OpenShift extension, it's a bug. wdty @iocanel ?

[edeandrea]

What ever happened with this?

[radcortez]

What ever happened with this?

The change may have discovered a possible bug described in #34487 (comment). We need clarification on the proper behaviour to proceed.

[edeandrea]

Thank you @radcortez

This is the new behaviour after #33724. So Kubernetes name or openshift name should never modify the container image name, so if this is still happening in the OpenShift extension, it's a bug. wdty @iocanel ?

Reading the documentation it seems that quarkus.kubernetes.name should only apply to labels. But then reading further about the property itself it says The name of the application. This value will be used for naming Kubernetes resources like: - Deployment - Service and so on …​

So what exactly does "naming" mean in this context? The name of the Deployment/Service/etc? Would that be the metadata.name? Or the app.kubernetes.io/name? Or the image name?

To me, the documentation itself isn't clear, so I'm unsure of what the proper behavior should be today before we introduce any of these changes.

Shouldn't the container image name itself be driven by the quarkus.container-image.name property?

[iocanel]

quarkus.kubernetes.name should affect kubernetes resource names, labels etc (not images).
By default we are naming everything after the application. If we need to to use a different name this is that way to go.

At some point we allowed the name to be affected by the image name, too.
This didn't make much sense so we changed that in #33724.

To answer @Sgitario question, AFAIR we never named images after quarkus.kuberntes.name, and if that happens it should be indeed considered a bug.

[Sgitario]

At some point we allowed the name to be affected by the image name, too. This didn't make much sense so we changed that in #33724.

To answer @Sgitario question, AFAIR we never named images after quarkus.kuberntes.name, and if that happens it should be indeed considered a bug.

These two sentences seem contradictory to me unless we're not speaking about the same objects.
This is my original question where I mentioned that quarkus.kuberntes.name affected the container image before #33724. So, never is not correct, we did it before #33724.

Therefore, if I understood what the issue was, if there are some container images (in the OpenShift) where quarkus.kuberntes.name is still affecting the container image, this is a leftover of #33724 and it should be addressed.

Are we on the same page?

[radcortez]

Therefore, if I understood what the issue was, if there are some container images (in the OpenShift) where quarkus.kuberntes.name is still affecting the container image, this is a leftover of #33724 and it should be addressed.

It is. Look at the processor code:

quarkus/extensions/kubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftProcessor.java

Lines 103 to 107 in d01808f

	@BuildStep
	public void populateCustomImageName(OpenshiftConfig openshiftConfig,
	BuildProducer<ContainerImageCustomNameBuildItem> containerImageCustomName) {
	openshiftConfig.name.ifPresent(name -> containerImageCustomName.produce(new ContainerImageCustomNameBuildItem(name)));
	}

In this case, it is not quarkus.kubernetes.name, but quarkus.openshift.name. If the name is present it produces a ContainerImageCustomNameBuildItem which overrides the container name. I guess this was never noticed because we don't regularly use quarkus.openshift.name. Once we added the fallback logic and made quarkus.openshift.* to fallback to quarkus.kubernetes.*, then quarkus.openshift.name gets a name if available in quarkus.kubernetes.name (the test case failing), which overrides the image name.

I believe we only need to remove the processor producer. Regardless, I just wanted to confirm which one should be the correct behaviour.

[radcortez]

Ok, I've updated the PR and removed the build producer with the customized image name.

[Sgitario]

LGTM! Except for a couple of minor things.
One concern I had is that when using for example quarkus.kubernetes.ingress.expose, this property does not exist in OpenShift, so I didn't want to see a warning message saying that quarkus.openshift.ingress.expose. But I tried it and this is not happening, so perfect!

[iocanel]

At some point we allowed the name to be affected by the image name, too. This didn't make much sense so we changed that in #33724.
To answer @Sgitario question, AFAIR we never named images after quarkus.kuberntes.name, and if that happens it should be indeed considered a bug.

These two sentences seem contradictory to me unless we're not speaking about the same objects. This is my original question where I mentioned that quarkus.kuberntes.name affected the container image before #33724. So, never is not correct, we did it before #33724.

From my point of view there is no contradiction.

The property quarkus.kubernetes.name never influenced the name of the container image that we build (emphasis on build. I am not refering to the image we used in the manifests). It was the other way around up until #33724. More specifically quarkus.kubernetes.name used the quarkus.container-image.name as a default value up until #33724.

I have no recollection of quarkus.kubernetes.name influencing in any way the name of the container image that we build.
It may influence the image used in the manifests, but if this is the case, its simply wrong, as the image should draw its value from the ContainerImageInfoBuildItem and this should be only created using the container image configuration and nothing more.

Therefore, if I understood what the issue was, if there are some container images (in the OpenShift) where quarkus.kuberntes.name is still affecting the container image, this is a leftover of #33724 and it should be addressed.

Are we on the same page?

Partially.
I agree that if quarkus.kubernetes.name should not affect in any way the container image (neither the image we actually build, nor the one we end up using in our manifests).

The part that I disagree, is that this has nothing to do with #33724.

[Sgitario]

LGTM!

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[Sgitario]

@iocanel @radcortez I've added the release/noteworthy-feature label because I think this feature is quite a huge improvement, though let me know if you prefer not mentioning it in the release notes.

[radcortez]

Sure.

[edeandrea]

Hi @radcortez / @Sgitario / @iocanel should this feature have made it into the 3.3.0release? If so, it does not seem to work fully.

If I specify the following (the kubernetes, openshift, knative, and minikube extensions are all present):

quarkus.kubernetes.part-of=villains-service
quarkus.kubernetes.annotations."app.openshift.io/connects-to"=villains-db,otel-collector
quarkus.kubernetes.env.configmaps=${quarkus.application.name}-config
quarkus.kubernetes.env.secrets=${quarkus.application.name}-config-creds
quarkus.kubernetes.labels.app=${quarkus.application.name}
quarkus.kubernetes.labels.application=${quarkus.kubernetes.part-of}
quarkus.kubernetes.labels.system=quarkus-super-heroes
quarkus.openshift.route.expose=true

when the OpenShift & KNative resources are generated, the resources (the OpenShift DeploymentConfig and the KNative Service) do not have the expected annotations or labels. They do, however, have the expected ConfigMaps and Secrets.

The minikube Deployment, though, does look to be ok.

I'm not sure if this is because for OpenShift/Knative the resource types are different (kubernetes Deployment vs OpenShift DeploymentConfig/KNative Service)?

In any event, it does not work as expected. Should I file a new issue?

[edeandrea]

I've created a new issue for this - #35608