Skip to content

Conversation

@amisevsk
Copy link
Collaborator

@amisevsk amisevsk commented Feb 1, 2023

What does this PR do?

Improves how the basic routing solver handles endpoints that define paths/queries/fragments.

What issues does this PR fix or reference?

Closes #1031

Is it tested? How?

Tests are updated. To test directly, apply the following workspace and test that resolved endpoints in the DevWorkspaceRouting match what is expected on OpenShift and Kubernetes:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-devworkspace
spec:
  started: true
  routingClass: 'basic'
  template:
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryLimit: 512Mi
          mountSources: true
          command:
            - "tail"
            - "-f"
            - "/dev/null"
          endpoints:
            - name: test-query
              attributes:
                type: main
              targetPort: 8888
              exposure: public
              path: /?test-param
              protocol: http
            - name: test-path-query
              targetPort: 8889
              exposure: public
              path: /test/path/?test-param
              protocol: http
            - name: test-path
              targetPort: 8890
              exposure: public
              path: /test/path/
              protocol: http
            - name: test-no-path
              targetPort: 8891
              exposure: public
              protocol: http

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

@amisevsk amisevsk requested a review from ibuziuk as a code owner February 1, 2023 21:52
@amisevsk amisevsk requested a review from AObuchow February 1, 2023 21:52
@openshift-ci openshift-ci bot added the approved label Feb 1, 2023
@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Base: 50.17% // Head: 50.20% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (26f78bb) compared to base (27b6c98).
Patch coverage: 82.35% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1032      +/-   ##
==========================================
+ Coverage   50.17%   50.20%   +0.03%     
==========================================
  Files          70       70              
  Lines        6021     6031      +10     
==========================================
+ Hits         3021     3028       +7     
- Misses       2770     2773       +3     
  Partials      230      230              
Impacted Files Coverage Δ
...r/devworkspacerouting/solvers/resolve_endpoints.go 33.33% <82.35%> (+5.91%) ⬆️
controllers/workspace/devworkspace_controller.go 61.58% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@AObuchow AObuchow mentioned this pull request Feb 2, 2023
3 tasks
Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

Tested on OpenShift 4.12 and checked the status of the DWR object on the cluster and things seemed correct:

apiVersion: controller.devfile.io/v1alpha1
kind: DevWorkspaceRouting
(...)
spec:
  devworkspaceId: workspacee5084d468fca49bb
  endpoints:
    web-terminal:
      - attributes:
          type: main
        exposure: public
        name: test-query
        path: /?test-param
        protocol: http
        targetPort: 8888
      - exposure: public
        name: test-path-query
        path: /test/path/?test-param
        protocol: http
        targetPort: 8889
      - exposure: public
        name: test-path
        path: /test/path/
        protocol: http
        targetPort: 8890
      - exposure: public
        name: test-no-path
        protocol: http
        targetPort: 8891
  podSelector:
    controller.devfile.io/devworkspace_id: workspacee5084d468fca49bb
  routingClass: basic
status:
  exposedEndpoints:
    web-terminal:
      - attributes:
          type: main
        name: test-query
        url: >-
          http://workspacee5084d468fca49bb.apps.ci-ln-vj4kpzb-72292.origin-ci-int-gce.dev.rhcloud.com/test-query/?test-param
      - name: test-path-query
        url: >-
          http://workspacee5084d468fca49bb.apps.ci-ln-vj4kpzb-72292.origin-ci-int-gce.dev.rhcloud.com/test-path-query/test/path/?test-param
      - name: test-path
        url: >-
          http://workspacee5084d468fca49bb.apps.ci-ln-vj4kpzb-72292.origin-ci-int-gce.dev.rhcloud.com/test-path/test/path/
      - name: test-no-path
        url: >-
          http://workspacee5084d468fca49bb.apps.ci-ln-vj4kpzb-72292.origin-ci-int-gce.dev.rhcloud.com/test-no-path/
  message: DevWorkspaceRouting prepared
  phase: Ready

baseUrl := fmt.Sprintf("%s://%s", protocol, basehost)

url, err := url.Parse(baseUrl)
// hostWithProtocol := fmt.Sprintf("%s://%s", protocol, host)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this commented out code was intentional or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, left-over from refactoring

@openshift-ci
Copy link

openshift-ci bot commented Feb 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow

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

The pull request process is described here

Details 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

Add test cases to represent container components with endpoints like

    - name: test-endpoint
      targetPort: 8888
      exposure: public
      path: /?query-param
      protocol: http

These tests currently fail.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Fix issues where endpoints were incorrectly being processed if they
defined a path. With basic routing, routes/ingresses are created with
path referring to the endpoint name. Previously, this path was being
dropped in the resolved endpoint, resulting in invalid paths (from the
cluster perspective)

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
* Ignore trailing slash on base urls, as it is not required by any spec
* Test that resolving endpoints distinguishes between
  /test/path?query=param and /test/path/?query=param

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk amisevsk force-pushed the fix-endpoints-handling branch from 8b22d49 to 26f78bb Compare February 2, 2023 19:18
@openshift-ci openshift-ci bot removed the lgtm label Feb 2, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 2, 2023

New changes are detected. LGTM label has been removed.

@amisevsk amisevsk merged commit 41650ff into devfile:main Feb 2, 2023
@amisevsk amisevsk deleted the fix-endpoints-handling branch February 2, 2023 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DevWorkspaceRouting reconciler breaks endpoints when they specify a path

2 participants