fix: use https protocol in the 'che-incubator/che-code/insiders' editor #1810

ibuziuk · 2023-10-10T12:58:01Z

What does this PR do?

use https protocol in the 'che-incubator/che-code/insiders' editor

Note that it only changes protocol for che-incubator/che-code/insiders, not sure if we should update it for the rest of the editors (some of which look obsolete now and should probably be removed).

Screenshot/screencast of this PR

Reviewers

Reviewers, please comment how you tested the PR when approving it.

github-actions · 2023-10-10T12:58:15Z

Click here to review and test in web IDE:

l0rd · 2023-10-10T13:23:48Z

I would set https to che-incubator/che-code/7.75.0 endpoints too (you only applied to che-incubator/che-code/insiders).

ibuziuk · 2023-10-10T13:28:44Z

@l0rd I can do that, but I thought that 7.75.0 has been released and we should not update it afterwards

l0rd · 2023-10-10T13:53:41Z

@l0rd I can do that, but I thought that 7.75.0 has been released and we should not update it afterwards

RIght. There are 2 versions of che-code, one for version next and one for stable. The thing is that if you don't update it then I am afraid that next stable release (7.76.0) will include a definition of vs code that uses http rather than https.

svor · 2023-10-10T14:11:08Z

we need to change che-incubator/che-code/7.75.0 as well it means like a latest version.
You can check the generated registry of this PR: https://pr-check-1810-che-plugin-registry.surge.sh/v3/plugins/che-incubator/che-code/
It provides 2 che-code editors: latest and isiders but latest doesn't have http --> https changes

probably we need the same changes in downstream as well: https://github.com/redhat-developer/devspaces/blob/devspaces-3-rhel-8/dependencies/che-plugin-registry/che-editors.yaml#L166

ibuziuk · 2023-10-10T14:14:13Z

thanks for the clarification, updated

ibuziuk · 2023-10-10T14:21:09Z

@svor looks like smth. weird is happening with the che-code endpoint resulting in httpss protocol. Looks like the script explicitly adds extra s enforcing the https, but not checking if https is already set. Not sure if we should update the script, or just do not update che-code endpoint, which would look weird imo

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

svor · 2023-10-10T17:44:10Z

tools/build/src/devfile/meta-yaml-to-devfile-yaml.ts

@@ -131,13 +131,14 @@ export class MetaYamlToDevfileYaml {
          devfileEndpoint.attributes['type'] = 'main';
        }

-        // if it's secured, remove secure option for now but add extra s on the protocol
+        // if it's secured, remove 'secure' option for now but change protocol to secure (https or wss)
        if (devfileEndpoint.attributes && devfileEndpoint.attributes.secure === true) {
          devfileEndpoint.secure = false;


I don't understand why we change secure to false when it was set to true.
I think we shouldn't change the value.

@l0rd do you happen to remember why it was done that way? I recall that secure attribute might be in general not supported by DWO

cc: @amisevsk ^

No I don't remember. My guess would be that secured is currently ignored (it was used by che-server when it was creating routes) and to have a secured endpoint (that requires authentication) you need to set the supportURLRewrite to true.

The Devfile API docs give

Describes whether the endpoint should be secured and protected by some authentication process. This requires a protocol of https or wss.

but I think (haven't verified) that this field is either ignored in Che or is interpreted as "use https instead of http" (which isn't really appropriate, API-wise). I think the "most correct" solution would be something like

If secured and supportURLRewrite, use gateway URL

If either is false, use plain route

If secured but not supportURLRewrite, fail workspace with error as we can't put auth on a plain route

@l0rd @amisevsk I would probably leave it as-is for now (just fixing httpss / wssscases), and I think we need a separate issue for supporting secure attribute properly and not do any weird patching on plugin-registry end. Please, approve if this makes sense

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

codecov · 2023-10-10T19:04:10Z

Codecov Report

Merging #1810 (0786b84) into main (fa10086) will not change coverage.
Report is 7 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1810   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          563       564    +1     
  Branches        88        88           
=========================================
+ Hits           563       564    +1

Files	Coverage Δ
...ols/build/src/devfile/meta-yaml-to-devfile-yaml.ts	`100.00% <100.00%> (ø)`

svor

Workspace was started successfully with current editor's configuration

openshift-ci · 2023-10-10T20:01:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ibuziuk, svor

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

Needs approval from an approver in each of these files:

OWNERS

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

amisevsk · 2023-10-11T15:30:09Z

che-editors.yaml

            - name: code-redirect-1
              targetPort: 13131
              exposure: public
-              protocol: http
+              protocol: https
              attributes:
                discoverable: false
                urlRewriteSupported: false


I haven't had time to verify this directly and so I'm not sure it's an actual problem, but does this cause issues when running on Kubernetes? Since these endpoints have urlRewriteSupported: false, I believe Che will support them via plain ingresses, and I'm not sure if Che is able to configure TLS there.

yeah, good point, I need to check this on minikube

tried to verify on minikube but it looks like Che is not supported on this arch in general:

❯ Eclipse Che Operator pod bootstrap ✔ Scheduling...[OK] ✖ Downloading images → Failed to download image, reason: ErrImagePull, message: rpc error: co … Starting Create ValidatingWebhookConfiguration org.eclipse.che Create MutatingWebhookConfiguration org.eclipse.che Create CheCluster Custom Resource Error: Command server:deploy failed with the error: Failed to download image, reason: ErrImagePull, message: rpc error: code = Unknown desc = no matching manifest for linux/arm64/v8 in the manifest list entries. See details: /Users/ibuziuk/Library/Caches/chectl/error.log. Eclipse Che logs: /var/folders/87/t16qbhjd6sn7cz51_g716sh40000gn/T/chectl-logs/1697121281363. at newError (/Users/ibuziuk/chectl/lib/utils/utls.js:39:19) at wrapCommandError (/Users/ibuziuk/chectl/lib/utils/command-utils.js:53:32) at Deploy.<anonymous> (/Users/ibuziuk/chectl/lib/commands/server/deploy.js:82:65) at Generator.throw (<anonymous>) at rejected (/Users/ibuziuk/chectl/node_modules/tslib/tslib.js:165:69) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) Cause: Error: Failed to download image, reason: ErrImagePull, message: rpc error: code = Unknown desc = no matching manifest for linux/arm64/v8 in the manifest list entries. at /Users/ibuziuk/chectl/lib/tasks/pod-tasks.js:149:39 at Generator.next (<anonymous>) at fulfilled (/Users/ibuziuk/chectl/node_modules/tslib/tslib.js:164:62) at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

However, Smoke Test PR check should cover the minikube verification. Adding @dmytro-ndp for PR approval

I got Network Error when tried to start workspace of Eclipse Che Next on minikube which is using external plugin registry with URL https://pr-check-1810-che-plugin-registry.surge.sh/v3 given by @ibuziuk :

BTW: the error Failed to fetch a devfile from URL: https://pr-check-1810-che-plugin-registry.surge.sh/v3/plugins/che-incubator/che-code/latest/devfile.yaml, reason: Error: Network Error has reproduced on OpenShift 4.10 as well:

@olexii4 @akurinnoy folks, it looks like in general it is not possible to reference external plugin registry from the CR since we would hit CORS on dashboard end, right?

It looks like CORS requests are disabled on PR checks like https://pr-check-1810-che-plugin-registry.surge.sh/v3
@svor do you happen to know if it is expected?

@ibuziuk yes, it is expected. Published content on surge.sh doesn't support CORS requests. I've checked external plugin registry with https://eclipse-che.github.io/che-plugin-registry/main/ and it works good.

@dmytro-ndp you can verify current changes by patching Che CR, here is the image that was built with current changes: quay.io/vsvydenk/che-plugin-registry:https-pr

You can deploy Che on minikube by chectl server:deploy --batch --platform minikube --che-operator-cr-patch-yaml patch.yaml
where patch.yaml is:

apiVersion: org.eclipse.che/v2 spec: components: pluginRegistry: deployment: containers: - image: quay.io/vsvydenk/che-plugin-registry:https-pr

I've just tested it on minikube (the workspace has been started, all plugins have been installed):

fix: use https protocol in the 'che-incubator/che-code/insiders' editor

66fe442

ibuziuk requested review from vinokurig and svor as code owners October 10, 2023 12:58

ibuziuk requested a review from l0rd October 10, 2023 12:58

fixup! set https protocol in the 7.75.0 version of the che-code editor

1bbcce6

ibuziuk force-pushed the ibuziuk-patch-3 branch 3 times, most recently from 9319594 to 5a582ad Compare October 10, 2023 16:00

fix: do not add extra 's' if the protocol is not 'http'

ae05276

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk force-pushed the ibuziuk-patch-3 branch from 5a582ad to ae05276 Compare October 10, 2023 16:04

test: adding test for https

de4d721

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

svor reviewed Oct 10, 2023

View reviewed changes

test: Adding protocol test

0786b84

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

svor approved these changes Oct 10, 2023

View reviewed changes

openshift-ci bot assigned svor Oct 10, 2023

openshift-ci bot added the lgtm label Oct 10, 2023

amisevsk reviewed Oct 11, 2023

View reviewed changes

ibuziuk requested a review from dmytro-ndp October 12, 2023 13:37

ibuziuk merged commit a1073a0 into main Oct 31, 2023
20 checks passed

ibuziuk deleted the ibuziuk-patch-3 branch October 31, 2023 14:42

ibuziuk mentioned this pull request Nov 7, 2023

fix: use https endpoints for the che-code editors redhat-developer/devspaces#1022

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use https protocol in the 'che-incubator/che-code/insiders' editor #1810

fix: use https protocol in the 'che-incubator/che-code/insiders' editor #1810

ibuziuk commented Oct 10, 2023 •

edited

github-actions bot commented Oct 10, 2023

l0rd commented Oct 10, 2023

ibuziuk commented Oct 10, 2023

l0rd commented Oct 10, 2023

svor commented Oct 10, 2023

ibuziuk commented Oct 10, 2023

ibuziuk commented Oct 10, 2023

svor Oct 10, 2023

ibuziuk Oct 10, 2023

ibuziuk Oct 10, 2023

l0rd Oct 11, 2023 •

edited

amisevsk Oct 11, 2023

ibuziuk Oct 11, 2023

codecov bot commented Oct 10, 2023 •

edited

svor left a comment •

edited

openshift-ci bot commented Oct 10, 2023

amisevsk Oct 11, 2023

ibuziuk Oct 11, 2023 •

edited

ibuziuk Oct 12, 2023

dmytro-ndp Oct 17, 2023 •

edited

dmytro-ndp Oct 17, 2023

ibuziuk Oct 25, 2023

ibuziuk Oct 25, 2023

svor Oct 30, 2023

svor Oct 30, 2023

fix: use https protocol in the 'che-incubator/che-code/insiders' editor #1810

fix: use https protocol in the 'che-incubator/che-code/insiders' editor #1810

Conversation

ibuziuk commented Oct 10, 2023 • edited

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

github-actions bot commented Oct 10, 2023

l0rd commented Oct 10, 2023

ibuziuk commented Oct 10, 2023

l0rd commented Oct 10, 2023

svor commented Oct 10, 2023

ibuziuk commented Oct 10, 2023

ibuziuk commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rd Oct 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2023 • edited

Codecov Report

svor left a comment • edited

Choose a reason for hiding this comment

openshift-ci bot commented Oct 10, 2023

Choose a reason for hiding this comment

ibuziuk Oct 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-ndp Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuziuk commented Oct 10, 2023 •

edited

l0rd Oct 11, 2023 •

edited

codecov bot commented Oct 10, 2023 •

edited

svor left a comment •

edited

ibuziuk Oct 11, 2023 •

edited

dmytro-ndp Oct 17, 2023 •

edited