Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openshift-maven-plugin: service.yml fragment with ports creates service with unnamed port mapping #513

Closed
robinroos opened this issue Dec 7, 2020 · 9 comments · Fixed by #519
Labels
bug Something isn't working
Milestone

Comments

@robinroos
Copy link
Contributor

Description

Using openshift-maven-plugin in a Spring Boot app, with the "spring boot" generator.

In the absence of any fragments the project builds and deploys successfully to OpenShift. Once service is defined, "http" TCP 80 -> 8080

The intention of the fragment is to define an ADDITIONAL service, "hazelcast" TCP 5701 -> 5701.

src/main/jkube/service.yml:

spec:
  ports:
    - name: hazelcast
      protocol: TCP
      port: 5701
      targetPort: 5701

With the above fragment present, openshift-maven-plugin generates:

target/jkube/applyJson/namespace/service-xxx.json

including the following extract:

  "spec" : {
    "clusterIP" : "172.30.157.230",
    "ports" : [ {
      "port" : 5701,
      "protocol" : "TCP",
      "targetPort" : 5701
    } ],

Note that the port entry has no name.

At this point deployment is successful; TCP 5701 -> 5701 is exposed by an unnamed port mapping in service-xxx, but there is no HTTP port exposed.

If the fragment is extended to include "http" as follows:

src/main/jkube/service.yml:

spec:
  ports:
    - name: http
      protocol: TCP
      port: 80
      targetPort: 8080
    - name: hazelcast
      protocol: TCP
      port: 5701
      targetPort: 5701

then openshift-maven-plugin fails (silently) to generate anything in target/jkube/applyJson, possibly due to name collision for two unnamed service port entries, and deployment subsequenly fails with:

[ERROR] oc: Failed to update Service from openshift.yml.

since openshift.yml is absent.

Info

OpenShift: 4.6
Platform: Windows 10
Eclipse JKube version : 1.0.2

> mvn -v
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: C:\dev\apache-maven-3.6.3\bin\..
Java version: 15.0.1, vendor: AdoptOpenJDK, runtime: C:\Program Files\AdoptOpenJDK\jdk-15.0.1.9-openj9
Default locale: en_GB, platform encoding: Cp1252
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

Potentially relevant maven properties include:

<version.jkube>1.0.2</version.jkube>
<maven.compiler.source>11</maven.compiler.source>
<maven.compiler.target>11</maven.compiler.target>
<kubernetes.trust.certificates>true</kubernetes.trust.certificates>
<jkube.enricher.jkube-service.type>ClusterIP</jkube.enricher.jkube-service.type>
<jkube.enricher.jkube-service.normalizePort>true</jkube.enricher.jkube-service.normalizePort>
<jkube.generator.alias>${project.artifactId}</jkube.generator.alias>

I will endeavour to add a simple reproducer project as soon as possible.

@robinroos
Copy link
Contributor Author

With the service.yml fragment removed from src/main/jkube, which is then empty, the generated service-xxx.json then contains the following correct definition of the "http" port mapping, appropriately named:

    "ports" : [ {
      "name" : "http",
      "port" : 80,
      "protocol" : "TCP",
      "targetPort" : 8080
    } ],

@robinroos robinroos changed the title openshift-maven-plugin: service.yml fragment with ports creates service with no name openshift-maven-plugin: service.yml fragment with ports creates service with unnamed poirt mapping Dec 7, 2020
@robinroos robinroos changed the title openshift-maven-plugin: service.yml fragment with ports creates service with unnamed poirt mapping openshift-maven-plugin: service.yml fragment with ports creates service with unnamed port mapping Dec 7, 2020
@rohanKanojia rohanKanojia added the bug Something isn't working label Dec 8, 2020
@robinroos
Copy link
Contributor Author

Here is the best I can do for a reproducer project:

https://github.com/robinroos/jkube-513

Please see the REDME file in that project.

@rohanKanojia
Copy link
Member

@robinroos : Thanks a lot for taking time to provide a reproducer 👍 . It will be really helpful for us in investigating the issue.

@rohanKanojia
Copy link
Member

Seems like Service name is being set null here, on line 607:
https://github.com/eclipse/jkube/blob/bf88230a5c110d686ea02ffb45dba8a294f5e2e9/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/DefaultServiceEnricher.java#L604-L608

getDefaultPortName only seems to be considering a specific set of ports for TCP protocol and is returning null in case port number doesn't match the desired list of ports or is not in IANA databse.
https://github.com/eclipse/jkube/blob/bf88230a5c110d686ea02ffb45dba8a294f5e2e9/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/DefaultServiceEnricher.java#L488-L500

I think we can simply avoid renaming ServicePort name if getDefaultPortName() returns null. When I change ensurePortName like this:

     private void ensurePortName(ServicePort port, String protocol) {
         if (port.getName() != null && !port.getName().isEmpty()) {
-            port.setName(getDefaultPortName(port.getPort(), getProtocol(protocol)));
+            String defaultPortName = getDefaultPortName(port.getPort(), getProtocol(protocol));
+            if (defaultPortName != null) {
+                port.setName(defaultPortName);
+            }
         }
     }

I'm able to see ports being generated correctly:

  "spec" : {
    "clusterIP" : "172.30.241.108",
    "ports" : [ {
      "name" : "http",
      "port" : 80,
      "protocol" : "TCP",
      "targetPort" : 8080
    }, {
      "name" : "hazelcast",
      "port" : 5701,
      "protocol" : "TCP",
      "targetPort" : 5701
    } ]

@rohanKanojia
Copy link
Member

The intention of the fragment is to define an ADDITIONAL service, "hazelcast" TCP 5701 -> 5701.

Right now from you seem to adding another ServicePort to your main application's Service. If you want to create an additional Service you should have something like this:

jkube-513 : $ tree src/main/jkube/
src/main/jkube/
├── hazelcast-service.yml   <- Hazelcast's Service
└── service.yml             <- Main Application's Service

0 directories, 2 files

@robinroos
Copy link
Contributor Author

In ensurePortName() you only det a default name if the port originally had a non-null non-empty name. I would have expected that in such a case you should not set a default name, but that you should set a default name if the port has a name which is null or empty.

@robinroos
Copy link
Contributor Author

I have submitted a PR #516 to resolve this, having built 1.1.0-SNAPSHOT from my branch and shown the fix to be working.

I will try to get the contributor's agreement signed tonight.

@robinroos
Copy link
Contributor Author

I have signed the ECA.

@rohanKanojia rohanKanojia added this to Backlog in Sprint #194 + #195 Dec 9, 2020
@manusa manusa moved this from Backlog to Planned in Sprint #194 + #195 Dec 9, 2020
@rohanKanojia rohanKanojia added this to the 1.1.0 milestone Dec 10, 2020
@manusa manusa added this to Planned in Sprint #196 Jan 20, 2021
@manusa manusa moved this from Planned to Review in Sprint #196 Jan 21, 2021
@manusa
Copy link
Member

manusa commented Jan 21, 2021

So apparently, this issue was introduced in this commit (probably due to a typo):

The bug not only affects the port name, but service name and others

Sprint #196 automation moved this from Review to Done Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Sprint #196
  
Done
3 participants