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

Add duplicate port validation, update devfile libraries, and add tests to check port name matches endpoint name #5407

Merged

Conversation

valaparthvi
Copy link
Contributor

@valaparthvi valaparthvi commented Feb 1, 2022

What type of PR is this:

/kind bug

What does this PR do / why we need it:
This PR adds duplicate port validation, updates devfile libraries, and adds tests to ensure port name matches endpoint names.
Which issue(s) this PR fixes:

Fixes #4737

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:
Port name matches endpoint name validation

odo create nodejs-vue myvue
odo url create myurl --port 3000 --host `minikube ip`
odo push
kubectl get svc myvue-app -oyaml -ojsonpath="{.spec.ports[*].name}"

Duplicate port validation

odo create nodejs-vue myvue
odo url create --port 8080

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 1, 2022
@netlify
Copy link

netlify bot commented Feb 1, 2022

✔️ Deploy Preview for odo-docusaurus-preview canceled.

🔨 Explore the source changes: 034f7ed

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62037b9e857193000826d26a

return fmt.Errorf("url %v already exists in devfile endpoint entry under container %q", url.Name, component.Name)
}
// Check for a duplicate port entry
if endpoint.TargetPort == url.Port {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to understand when updateURL is set to true. Do you think it could be needed to test its value here also?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is set to true when you do not pass a URL name, and an invalid endpoint exists in the devfile. I'm not sure why it is as such, but that is my understanding.

odo/pkg/envinfo/url.go

Lines 117 to 131 in b75fa55

// get the name for the URL if not provided
if len(url.Name) == 0 {
foundURL, err := findInvalidEndpoint(ei, url.Port)
if err != nil {
return err
}
if foundURL.Name != "" {
// found an URL that can be overridden or more info can be added to it
url.Name = foundURL.Name
ei.updateURL = true
} else {
url.Name = util.GetURLName(ei.GetName(), url.Port)
}
}

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2022

OpenShift Windows Tests on commit 5a9d99b finished successfully.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Feb 1, 2022

It seems you will have to adapt some tests that are using duplicated port numbers: https://s3.eu-de.cloud-object-storage.appdomain.cloud/odo-tests-openshift-logs/pr-5407-openshift-tests-441.html

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2022

Validate Tests on commit 5a9d99b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2022

Kubernetes Tests on commit 5a9d99b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2022

Unit Tests on commit 5a9d99b finished successfully.
View logs: TXT HTML

@dharmit dharmit mentioned this pull request Feb 2, 2022
3 tasks
@valaparthvi valaparthvi force-pushed the duplicate_port branch 2 times, most recently from 96422d9 to 4a8122b Compare February 2, 2022 09:49
@valaparthvi
Copy link
Contributor Author

@feloy Please hold your review on this one for now.

@valaparthvi valaparthvi force-pushed the duplicate_port branch 3 times, most recently from cbed450 to c767702 Compare February 8, 2022 10:39
@valaparthvi
Copy link
Contributor Author

/retest-required

@valaparthvi valaparthvi force-pushed the duplicate_port branch 2 times, most recently from f0f6806 to df5a550 Compare February 8, 2022 16:11
@feloy
Copy link
Contributor

feloy commented Feb 9, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Feb 9, 2022
@feloy
Copy link
Contributor

feloy commented Feb 9, 2022

This case is detected by tests on IBMCloud/Kubernetes but not on Prow tests:

• Failure [17.623 seconds]
odo devfile url command tests
/workspace/17f65e12-a2ec-43b6-bf47-4a8738e852ea/tests/integration/devfile/cmd_devfile_url_test.go:16
  when creating a Nodejs component
  /workspace/17f65e12-a2ec-43b6-bf47-4a8738e852ea/tests/integration/devfile/cmd_devfile_url_test.go:47
    should not allow to create URL with duplicate port [It]
    /workspace/17f65e12-a2ec-43b6-bf47-4a8738e852ea/tests/integration/devfile/cmd_devfile_url_test.go:70

    Expected
        <string>: I0209 06:58:40.232825   13236 util.go:816] Response will be cached in /tmp/odohttpcache for 1h0m0s
        I0209 06:58:40.236736   13236 util.go:829] Cached response used.
        I0209 06:58:40.405521   13236 implem.go:107] The path for preference file is /tmp/994218346/preference.yaml
        I0209 06:58:40.601851   13236 kclient.go:256] Checking if "routes" resource is supported
        I0209 06:58:40.607410   13236 context.go:141] absolute devfile path: '/tmp/730430071/devfile.yaml'
        I0209 06:58:40.607431   13236 context.go:92] absolute devfile path: '/tmp/730430071/devfile.yaml'
        I0209 06:58:40.608032   13236 content.go:33] converted devfile YAML to JSON
        I0209 06:58:40.608093   13236 apiVersion.go:45] devfile schemaVersion: '2.0.0'
        I0209 06:58:40.608110   13236 helper.go:41] devfile apiVersion '2.0.0' is supported
        I0209 06:58:40.686530   13236 schema.go:46] validated devfile schema
        I0209 06:58:40.703298   13236 validate.go:45] Successfully validated devfile sections
        I0209 06:58:40.705297   13236 validate.go:45] Successfully validated devfile sections
         ✗  URL creation failed:
        	- host must be provided in order to create URLS of Ingress Kind
        
    to contain substring
        <string>: port 3000 already exists in devfile endpoint entry

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Feb 9, 2022
@sonarcloud
Copy link

sonarcloud bot commented Feb 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

@valaparthvi
Copy link
Contributor Author

@feloy Can you review it again?

@feloy
Copy link
Contributor

feloy commented Feb 9, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Feb 9, 2022
@cdrage
Copy link
Member

cdrage commented Feb 9, 2022

Doesn't seem to work, or am I doing it incorrect?

git clone https://github.com/odo-devfiles/nodejs-ex
odo create nodejs foobar
odo push=
odo url create --port 3000 foobar --host k8s.land
 ✗  URL creation failed:
        - port 3000 already exists in devfile endpoint entry under container "runtime"

@feloy
Copy link
Contributor

feloy commented Feb 9, 2022

Doesn't seem to work, or am I doing it incorrect?

git clone https://github.com/odo-devfiles/nodejs-ex
odo create nodejs foobar
odo push=
odo url create --port 3000 foobar --host k8s.land
 ✗  URL creation failed:
        - port 3000 already exists in devfile endpoint entry under container "runtime"

The error is expected, as the nodejs devfile already contains an endpoint for this specific port and you cannot create several endpoints with same port anymore:

$ odo create nodejs foobar
$ cat devfile.ymal
[...]
components:
- container:
    endpoints:
    - name: http-3000
      targetPort: 3000
    image: registry.access.redhat.com/ubi8/nodejs-14:latest
    memoryLimit: 1024Mi
    mountSources: true
    sourceMapping: /project
  name: runtime
[...]

@cdrage
Copy link
Member

cdrage commented Feb 9, 2022

after discussion with @valaparthvi

this

/approve

@openshift-ci
Copy link

openshift-ci bot commented Feb 9, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdrage

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Feb 9, 2022
@openshift-merge-robot openshift-merge-robot merged commit 752c928 into redhat-developer:main Feb 9, 2022
kadel pushed a commit to kadel/odo that referenced this pull request Feb 21, 2022
…s to check port name matches endpoint name (redhat-developer#5407)

* Add duplicate port validation, update devfile libraries, and add tests to check port name matches endpoint name

* Fix k8s test
@valaparthvi
Copy link
Contributor Author

/cherrypick v2

@openshift-cherrypick-robot

@valaparthvi: new pull request created: #5475

In response to this:

/cherrypick v2

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.

cdrage pushed a commit to cdrage/odo that referenced this pull request Aug 31, 2022
…s to check port name matches endpoint name (redhat-developer#5407)

* Add duplicate port validation, update devfile libraries, and add tests to check port name matches endpoint name

* Fix k8s test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

port names should match endpoint names
5 participants