eclipse-jkube / jkube

Build and Deploy java applications on Kubernetes
https://www.eclipse.dev/jkube/
Eclipse Public License 2.0
775 stars 521 forks source link

Using property `jkube.enricher.jkube-name.name` overrides the names set in the resources fragments #2823

Open amihalfa opened 8 months ago

amihalfa commented 8 months ago

Describe the bug

I saw in documentation that jkube.enricher.jkube-name.name is supposed to override "Add a default name to every object which misses a name."

When I have 2 pods fragments such as : src/main/jkube/test-health.yml

apiVersion: v1
kind: Pod
metadata:
  namespace: mynamespace
  name: myapp-test-health
spec:
...

and src/main/jkube/test-metrics.yml

apiVersion: v1
kind: Pod
metadata:
  namespace: mynamespace
  name: myapp-test-metrics
spec:
...

And I set this config in my pom.xml

     <plugin>
                <groupId>org.eclipse.jkube</groupId>
                <artifactId>kubernetes-maven-plugin</artifactId>
                <version>${jkube.version}</version>
                <configuration>
                    <enricher>
                        <config>
                            <jkube-name>
                                <name>myapp</name>
                            </jkube-name>
                            <jkube-well-known-labels>
                                <name>myapp</name>
                            </jkube-well-known-labels>
                        </config>

                    </enricher>

                </configuration>
            </plugin>

The result after build is that I get 2 files named : myapp-pod.yaml and myapp-1-pod.yaml having exactly the same name (set in the pom.xml) :

apiVersion: v1
kind: Pod
metadata:
  namespace: mynamespace
  name: myapp
spec:
...

Eclipse JKube version

1.16.1

Component

Kubernetes Maven Plugin

Apache Maven version

3.8.5

Gradle version

None

Steps to reproduce

  1. Create a pod fragment files with a name :
    apiVersion: v1
    kind: Pod
    metadata:
    namespace: mynamespace
    name: myapp-test
    spec:
    ...
  2. Override the property jkube.enricher.jkube-name.name in the pom.xml :
                    <enricher>
                        <config>
                            <jkube-name>
                                <name>myapp</name>
                            </jkube-name>
                        </config>
                    </enricher>
  3. Build the project using : ./mvnw k8s:resource k8s:helm
  4. Check in your target the generated yaml file myapp-pod.yaml, you will see that the name was changed :
    apiVersion: v1
    kind: Pod
    metadata:
    namespace: mynamespace
    name: myapp
    spec:
    ...

Expected behavior

If a name is set in the fragment, keep the name, if not, override it.

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

1.25.3

Environment

macOS

Eclipse JKube Logs

No response

Sample Reproducer Project

No response

Additional context

No response

rohanKanojia commented 8 months ago

@amihalfa : Is it possible for you to debug this to check what's wrong with Enricher behavior?

  1. Set a breakpoint in Enricher
  2. Run mvnDebug k8s:resource from terminal
  3. Connect Via Remote JVM Debug from your IDE

I see that in code we're only setting name when it's blank: https://github.com/eclipse/jkube/blob/c4ee9d4e9432297c50d9385a8c06d1fea0f75e3a/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/NameEnricher.java#L57-L60

amihalfa commented 8 months ago

Hello @rohanKanojia

When I debug :

The resource name is always overwritten if a configuredName is set. From my point of view, if the resource already has a name, we keep the name, even if there is a configuredName.

My expected behaviour is to have in NameEnricher something like this :

        builder.accept(new TypedVisitor<ObjectMetaBuilder>() {
            @Override
            public void visit(ObjectMetaBuilder resource) {
                if (StringUtils.isBlank(resource.getName())) {
                    if (StringUtils.isNotBlank(configuredName)) {
                        resource.withName(configuredName);
                    } else {
                        resource.withName(defaultName);
                    }
                }
            }
        });

Thanks !

rohanKanojia commented 8 months ago

@amihalfa : Your approach looks correct to me. Is it possible to contribute a pull request for this?

manusa commented 7 months ago

We need to further discuss this to set the basic scenarios and user expectations.

The suggested approach breaks the existing functionality to override a name via system property or CLI flag.

The current approach is that regardless of what you have in the static resources (such as fragments), you can always perform an override in the command-line by providing a -Djkube.enricher.jkube-name.name=overridden.

If we want to change this, we'll need to properly document it and also provide specific examples and scenarios for users: