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

port names should match endpoint names #4737

Closed
kadel opened this issue May 20, 2021 · 18 comments · Fixed by #5407
Closed

port names should match endpoint names #4737

kadel opened this issue May 20, 2021 · 18 comments · Fixed by #5407
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). v2 Issue or PR that applies to the v2 of odo

Comments

@kadel
Copy link
Member

kadel commented May 20, 2021

Assuming that devfile containers have following endpoints

      endpoints:
        - name: first
          targetPort: 3000
        - name: second
          targetPort: 4000

The k8s Service that gets generated for odo should have matching port names

Current situation:

  ports:
  - name: port-3000
    port: 3000
    protocol: TCP
    targetPort: 3000
  - name: port-4000
    port: 4000
    protocol: TCP
    targetPort: 4000

/priority high
/kind bug

@openshift-ci openshift-ci bot added priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). kind/bug Categorizes issue or PR as related to a bug. labels May 20, 2021
@mik-dass
Copy link
Contributor

@kadel
Copy link
Member Author

kadel commented May 25, 2021

The service port naming is done here https://github.com/devfile/library/blob/5d88bd3c1220097161ef07d78c9fe7605be24ab7/pkg/devfile/generator/utils.go#L229.

We should coordinate with the Devfiles team to change this.

@mik-dass
Copy link
Contributor

Currently we allow multiple URLs on the same port, but k8s doesn't allow services to have two ports with the same port number and protocol combination.

@kadel kadel added priority/Medium Nice to have issue. Getting it done before priority changes would be great. area/url and removed priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). labels Jun 3, 2021
@dharmit dharmit added the triage/needs-information Indicates an issue needs more information in order to work on it. label Jun 14, 2021
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 12, 2021
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 12, 2021
@kadel
Copy link
Member Author

kadel commented Oct 18, 2021 via email

@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci
Copy link

openshift-ci bot commented Nov 17, 2021

@openshift-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kadel
Copy link
Member Author

kadel commented Nov 22, 2021

/reopen.

@kadel kadel reopened this Nov 22, 2021
@kadel kadel added priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). and removed priority/Medium Nice to have issue. Getting it done before priority changes would be great. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Nov 24, 2021
@kadel
Copy link
Member Author

kadel commented Nov 24, 2021

This problem was again raised by user on slack

I'm using odo 2.3.1 with OpenShift 4.9 and trying to set the Service port name in the devfile, but it wont stick. I always get a default value of name: "port-8080" in my Service when what I want is a port name of "http" (for Service Mesh and Kiali usage). Here is my devfile snippet, any ideas what I need to set:
components:

  • container:
    endpoints:
    • exposure: public
      name: "http"
      path: /
      targetPort: 8080

@valaparthvi
Copy link
Contributor

The service port naming is done here https://github.com/devfile/library/blob/5d88bd3c1220097161ef07d78c9fe7605be24ab7/pkg/devfile/generator/utils.go#L229.

These changes were made to accommodate another issue that odo was facing, see #4060, devfile/api#287, and devfile/library#51.

@yangcao77
Copy link
Contributor

yangcao77 commented Dec 1, 2021

Currently we allow multiple URLs on the same port, but k8s doesn't allow services to have two ports with the same port number and protocol combination.

This is also another issue that tools will face, and why the change was initially made in library. As devfile allows multiple endpoints within same component to use same port. but you can only expose a port number once in a K8s service. In other word, if it behavior as described in the main issue description, the K8s service spec generated will be invalid :

      endpoints:
        - name: first
          targetPort: 3000
        - name: second
          targetPort: 3000

The k8s Service that gets generated is valid:

  ports:
  - name: first
    port: 3000
    protocol: TCP
    targetPort: 3000
  - name: second
    port: 3000
    protocol: TCP
    targetPort: 3000

@yangcao77
Copy link
Contributor

The service port naming is done here https://github.com/devfile/library/blob/5d88bd3c1220097161ef07d78c9fe7605be24ab7/pkg/devfile/generator/utils.go#L229.

These changes were made to accommodate another issue that odo was facing, see #4060, devfile/api#287, and devfile/library#51.

This is not the same issue with devfile/api#287, and devfile/library#51. those were pod container port name
The service port naming conversion was done in this PR: devfile/library@ccd9d18#diff-9126bba4cbc803f4a34bfee5d830ed5af0ed5ebb1450bd7f2ebd4f554ea15a97
And IIRC, we were trying to resolve the issue mention in my last comment: #4737 (comment)

@yangcao77
Copy link
Contributor

To summarize the current issue:
The devfile Spec & Odo:

  • Endpoint has limitation of 63 chars length
  • allows multiple endpoints under the same component shares the same port number

K8s:

  • pod container port name has to be within 15 chars length
  • service cannot have duplicate port exposure
    This was the reasons we decided to use our own naming conversion for pod container port name and service port name.

Now, a Odo user reports the service port name does not match the actual endpoint name. And we are looking for a more appropriate solution for the container port name & service port name.

@kadel
Copy link
Member Author

kadel commented Dec 7, 2021

  • allows multiple endpoints under the same component shares the same port number

Can we make this invalid? I'm not sure if there is a case where it would be required to have endpoints exposing the same port on one container component

@elsony
Copy link

elsony commented Dec 7, 2021

Need to check to make sure it doesn't causes problems on update/migration

@valaparthvi
Copy link
Contributor

  • Remove the duplicate endpoint support from odo
  • Check if limiting the endpoint char length from 63 to 15 has any side effect on update/migration.

@valaparthvi
Copy link
Contributor

valaparthvi commented Jan 12, 2022

Once devfile/api#702 is merged, we should add validation check to odo url create to ensure that the name length does not exceed 15 before writing the information to devfile otherwise it will cause an error every time the devfile is parsed.

Example
➜  odo url create --port 3030 --ingress --host `minikube ip`                                                                      
 ✓  URL nodejs-abc-cusv-3050 created for component: nodejs-abc-cusv                                                                                        

To apply the URL configuration changes, please use `odo push`      

➜  odo url create --port 3030 3030-myport --ingress --host `minikube ip`                                                                      
 ✓  URL 3030-myport created for component: nodejs-abc-cusv                                                                                        

To apply the URL configuration changes, please use `odo push`    

➜  odo push                                                                 
 ✗  1 error occurred:                                                           
        * devfile contains multiple endpoint entries with same TargetPort: 3030 

➜  odo url delete 3030-myport                                               
 ✗  failed to parse the devfile devfile.yaml, with error: 1 error occurred:     
        * devfile contains multiple endpoint entries with same TargetPort: 3030                                                                      

Scope of the issue:

    • Add validation check for duplicate endpoint to odo url create, and tests.
  1. Once container & service port name should use endpoint name  devfile/api#700 adds changes to devfile to use the endpoint names as container and service port name,
      • add tests to ensure this feature.
      • Add 15 char length validation for url name to odo url create .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). v2 Issue or PR that applies to the v2 of odo
Projects
Archived in project
Status: Done
9 participants