fix: default endpoint [IDE-377] #466

teodora-sandu · 2024-06-04T12:51:35Z

Description

Changes the default auth host used when a custom endpoint is not provided so it's https://app.snyk.io instead of https://snyk.io and improves the computation logic for the URLs needed by the backend.

Re-enables the integration tests too so they run in the pipeline. They would have caught this bug before if they were enabled.

It's best to review this commit by commit.

Checklist

Tests added and all succeed
Linted
CHANGELOG.md updated
README.md updated, if user-facing

Screenshots / GIFs

Because of https://snyksec.atlassian.net/browse/IDE-354 it's important to restart VSCode when changing the organisation because the feature flag doesn't get updated in-between scans.

without feature flag (stable LS)
- without custom endpoint
- with prod custom endpoint
- with dev custom endpoint
without feature flag (newest LS)
- without custom endpoint
- with prod custom endpoint
- with dev custom endpoint
with feature flag
- without custom endpoint
- with prod custom endpoint
- with dev custom endpoint

teodora-sandu · 2024-06-04T16:59:49Z

src/snyk/common/configuration/configuration.ts

@@ -498,10 +486,6 @@ export class Configuration implements IConfiguration {

  private getConfigName = (setting: string) => setting.replace(`${CONFIGURATION_IDENTIFIER}.`, '');

-  private static isSingleTenant(url: URL): boolean {


This check is only for when the custom endpoint is provided, which is now only going to be api.*.snyk.io because of the validation we added in the Settings page. So it's irrelevant

Just realized: this may be why fedramp is having problems right now - we need snykgov.io as valid domain :). But yeah, I think I need to look deeper into the code - settings and how it flows together.

bastiandoetsch · 2024-06-05T12:25:07Z

.github/workflows/ci.yaml

+
+      - name: Run integration tests
+        run: npm run test:integration
+


I think the integration tests in vscode never have worked. When I said that we should re-enable them, I thought we were talking about LS? Anyhoo, if you get them running, no harm done :D

I did say snyk-ls in my message on Slack but meant vscode-extension 🤦 But I added some new tests to VSCode that I would like to run in the pipeline so this is still useful if there's no harm done

bastiandoetsch · 2024-06-05T12:26:47Z

src/snyk/common/configuration/configuration.ts

-        url.host = `deeproxy.${url.host}`;
-      }
-      url.pathname = url.pathname.replace('api', '');
+      url.host = url.host.replace('api', 'deeproxy');


I think we should be a bit more supportive here - e.g. automatically transform "old" app.snyk.io endpoints into the new correct format to ease migration to the new rules. At least in IntelliJ, the new format & validation has been causing problems for some ppl. WDYT?

I could be missing something but in this case the customEndpoint will be exactly what's provided in the UI, which is validated anyway and can only be the new format (api.snyk.io). And so I don't think there's any situation in which the old format will be provided?

This function actually was not used anymore so I removed it from the code but the same answer could apply to #466 (comment) I think. Let me know what you think!

Hm, what about people already having configured a custom endpoint? They would see a validation error without having changed anything. This may already happen, now, but I'd love to improve the UX.

I agree, in hindsight, that the validation is very inconvenient for customers who have the old endpoint configured. In https://snyksec.atlassian.net/browse/IDE-126 we said we want to enforce the new endpoint to be used, is there some other way we can enforce it while not breaking existing customer's setup?

src/snyk/common/configuration/configuration.ts

src/test/unit/common/configuration.test.ts

Added in 6aefeb4 but since was refactored and not used.

It was added in 94648e7 but removed since.

teodora-sandu changed the title ~~chore: enable integration tests in pipeline~~ fix: default endpoint [IDE-377] Jun 4, 2024

teodora-sandu force-pushed the chore/run-integration-tests branch 2 times, most recently from 8683425 to d0abf1e Compare June 4, 2024 16:58

teodora-sandu commented Jun 4, 2024

View reviewed changes

bastiandoetsch reviewed Jun 5, 2024

View reviewed changes

src/snyk/common/configuration/configuration.ts Outdated Show resolved Hide resolved

teodora-sandu force-pushed the chore/run-integration-tests branch 15 times, most recently from 0cdb15a to cf57970 Compare June 5, 2024 15:59

teodora-sandu marked this pull request as ready for review June 6, 2024 12:19

teodora-sandu requested a review from a team as a code owner June 6, 2024 12:19

bastiandoetsch reviewed Jun 7, 2024

View reviewed changes

src/test/unit/common/configuration.test.ts Show resolved Hide resolved

bastiandoetsch approved these changes Jun 7, 2024

View reviewed changes

teodora-sandu closed this Jun 7, 2024

teodora-sandu reopened this Jun 7, 2024

teodora-sandu added 2 commits June 10, 2024 10:06

chore: enable integration tests in pipeline

8f2f82a

test: fix integration tests

872b752

teodora-sandu added 7 commits June 10, 2024 10:06

refactor: remove redundant snyk code base url

938f72b

Added in 6aefeb4 but since was refactored and not used.

refactor: remove redundant snyk code token

c0e5b0a

It was added in 94648e7 but removed since.

refactor: remove unused code

78c6238

fix: default endpoint

ca55c40

chore: CHANGELOG

a36e828

fix: actually update the oss and iac enablement

77febd0

test: add snykgov test

7bf0ab3

teodora-sandu force-pushed the chore/run-integration-tests branch 7 times, most recently from 41cd6b4 to dfa5b98 Compare June 10, 2024 12:02

chore: fix pipeline for integration tests

0e1916e

teodora-sandu force-pushed the chore/run-integration-tests branch from dfa5b98 to 0e1916e Compare June 10, 2024 12:47

chore: linting

cd8a244

teodora-sandu merged commit 157a8b2 into main Jun 10, 2024
7 checks passed

teodora-sandu deleted the chore/run-integration-tests branch June 10, 2024 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: default endpoint [IDE-377] #466

fix: default endpoint [IDE-377] #466

teodora-sandu commented Jun 4, 2024 •

edited

Loading

teodora-sandu Jun 4, 2024 •

edited

Loading

bastiandoetsch Jun 7, 2024

bastiandoetsch Jun 5, 2024

teodora-sandu Jun 5, 2024

bastiandoetsch Jun 5, 2024

teodora-sandu Jun 5, 2024

teodora-sandu Jun 5, 2024

bastiandoetsch Jun 7, 2024

teodora-sandu Jun 7, 2024

		@@ -498,10 +486,6 @@ export class Configuration implements IConfiguration {

		private getConfigName = (setting: string) => setting.replace(`${CONFIGURATION_IDENTIFIER}.`, '');

		private static isSingleTenant(url: URL): boolean {

fix: default endpoint [IDE-377] #466

fix: default endpoint [IDE-377] #466

Conversation

teodora-sandu commented Jun 4, 2024 • edited Loading

Description

Checklist

Screenshots / GIFs

teodora-sandu Jun 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teodora-sandu commented Jun 4, 2024 •

edited

Loading

teodora-sandu Jun 4, 2024 •

edited

Loading