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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Oct 31, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
66fe442
fix: use https protocol in the 'che-incubator/che-code/insiders' editor
ibuziuk Oct 10, 2023
1bbcce6
fixup! set https protocol in the 7.75.0 version of the che-code editor
ibuziuk Oct 10, 2023
ae05276
fix: do not add extra 's' if the protocol is not 'http'
ibuziuk Oct 10, 2023
de4d721
test: adding test for https
ibuziuk Oct 10, 2023
0786b84
test: Adding protocol test
ibuziuk Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions che-editors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,25 +208,25 @@ editors:
targetPort: 3100
exposure: public
secure: true
protocol: http
protocol: https
- name: code-redirect-1
targetPort: 13131
exposure: public
protocol: http
protocol: https
attributes:
discoverable: false
urlRewriteSupported: false
Comment on lines 212 to 218
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ibuziuk ibuziuk Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@dmytro-ndp dmytro-ndp Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 :
Screenshot from 2023-10-17 13-30-08

Copy link
Contributor

Choose a reason for hiding this comment

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

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:
Screenshot from 2023-10-17 16-15-29

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just tested it on minikube (the workspace has been started, all plugins have been installed):
screenshot-192 168 49 2 nip io-2023 10 30-17_03_01
screenshot-192 168 49 2 nip io-2023 10 30-17_02_25

- name: code-redirect-2
targetPort: 13132
exposure: public
protocol: http
protocol: https
attributes:
discoverable: false
urlRewriteSupported: false
- name: code-redirect-3
targetPort: 13133
exposure: public
protocol: http
protocol: https
attributes:
discoverable: false
urlRewriteSupported: false
Expand Down Expand Up @@ -295,25 +295,25 @@ editors:
targetPort: 3100
exposure: public
secure: true
protocol: http
protocol: https
- name: code-redirect-1
targetPort: 13131
exposure: public
protocol: http
protocol: https
attributes:
discoverable: false
urlRewriteSupported: false
- name: code-redirect-2
targetPort: 13132
exposure: public
protocol: http
protocol: https
attributes:
discoverable: false
urlRewriteSupported: false
- name: code-redirect-3
targetPort: 13133
exposure: public
protocol: http
protocol: https
attributes:
discoverable: false
urlRewriteSupported: false
Expand Down
9 changes: 5 additions & 4 deletions tools/build/src/devfile/meta-yaml-to-devfile-yaml.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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

Copy link
Member Author

Choose a reason for hiding this comment

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

cc: @amisevsk ^

Copy link
Contributor

@l0rd l0rd Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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

delete devfileEndpoint.attributes['secure'];
// add extra s
if (devfileEndpoint.attributes.protocol) {
devfileEndpoint.attributes.protocol = `${devfileEndpoint.attributes.protocol}s`;
// switch to secure protocol (https or wss)
const protocol = devfileEndpoint.attributes.protocol;
if (protocol && (protocol === 'http' || protocol === 'ws')) {
devfileEndpoint.attributes.protocol = `${protocol}s`;
}
}

Expand Down
8 changes: 4 additions & 4 deletions tools/build/tests/_data/meta/che-code-meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ spec:
cookiesAuthEnabled: true
discoverable: false
urlRewriteSupported: true
protocol: http
protocol: https
secure: true
targetPort: 3100
public: true
Expand All @@ -28,21 +28,21 @@ spec:
attributes:
discoverable: false
urlRewriteSupported: false
protocol: http
protocol: https
public: true
- name: code-redirect-2
targetPort: 13132
attributes:
discoverable: false
urlRewriteSupported: false
protocol: http
protocol: https
public: true
- name: code-redirect-3
targetPort: 13133
attributes:
discoverable: false
urlRewriteSupported: false
protocol: http
protocol: https
public: true
containers:
- image: quay.io/devfile/universal-developer-image:latest
Expand Down
20 changes: 20 additions & 0 deletions tools/build/tests/common/endpoints-helper.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,26 @@ describe('Test EndpointsHelper', () => {
expect(endpoint.secure).toBeUndefined();
});

test('https', async () => {
const endpointYaml: CommonEndpointYaml = {
name: 'endpoint-name',
path: 'endpoint-path',
exposure: 'public',
protocol: 'https',
secure: true,
};
const volumes = new Map();
volumes.set('example', { ephemeral: true });
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const endpoint: any = await endpointsHelper.resolve(endpointYaml);
expect(endpoint).toBeDefined();
expect(endpoint.public).toBeTruthy();
expect(endpoint.attributes.protocol).toBe('https');
expect(endpoint.attributes.secure).toBeTruthy();
expect(endpoint.exposure).toBeUndefined();
expect(endpoint.secure).toBeUndefined();
});

test('change endpoint type', async () => {
const endpointYaml: CommonEndpointYaml = {
name: 'endpoint-name',
Expand Down
1 change: 1 addition & 0 deletions tools/build/tests/devfile/meta-yaml-to-devfile-yaml.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ describe('Test MetaYamlToDevfileYaml', () => {
const codeIdeFirstEndpoint = codeIdeComponentContainer.endpoints[0];
expect(codeIdeFirstEndpoint.name).toBe('che-code');
expect(codeIdeFirstEndpoint.exposure).toBe('public');
expect(codeIdeFirstEndpoint.protocol).toBe('https');
const codeIdeFirstEndpointAttributes = codeIdeFirstEndpoint.attributes;
expect(codeIdeFirstEndpointAttributes.type).toBe('main');

Expand Down