feat: add per-connection proxy settings COMPASS-8142 #6133

addaleax · 2024-08-16T21:23:10Z

Remove the ssh-tunnel package from Compass and its usage from data-service
Add relevant fields to the connection form and integrate them into data-service

Description

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

- Remove the `ssh-tunnel` package from Compass and its usage from data-service - Add relevant fields to the connection form and integrate them into data-service

… specify whatwg-url

…-test-envs

gribnoysup

Overall looks pretty good! I would suggest to move away from using compass preferences (even through their provider, yeah...) and globalAppRegistry directly in connection-form if my reasoning makes sense to you

gribnoysup · 2024-08-27T12:04:16Z

...nection-form/src/components/advanced-options-tabs/authentication-tab/authentication-oidc.tsx

@@ -16,6 +17,8 @@ import { errorMessageByFieldName } from '../../../utils/validation';
 import { getConnectionStringUsername } from '../../../utils/connection-string-helpers';
 import type { OIDCOptions } from '../../../utils/oidc-handler';
 import { useConnectionFormPreference } from '../../../hooks/use-connect-form-preferences';
+import { usePreference } from 'compass-preferences-model/provider';


So, we did a poor job documenting this well, but connection-form, being used by vscode too, uses this use-connect-form-preferences as a way to have a access to preferences inside the form so that we don't need to pull anything compass specific into runtime and depend on any compass logic for default preferences at all. There are a few places where preferences got used directly, and we already have ticket to clean those up, so let's add this as a new value to the connection form preferences to avoid adding more work for this planned cleanup

So – I've stuck to this pattern because I've been using it for a feature flag only, similar to what we do for enableMultipleConnectionSystem (where we also call usePreference() in connection-form). The feature flag gets removed in the next PR anyway, so I'd be tempted to just leave it like this? (Will address the other comments below tho)

Hmmm, I get this reasoning, at the same time the multiple connection feature flag directly used from the preferences was one of the things that broke connection form for vscode based on Alenas update attempt investigation, so this case feels similar and it might be a bit inconvenient if periodically vscode couldn't update while feature flags are used in the form. The place where we map compass preferences to connection form ones is in this file, so I wonder that maybe the logic you have here where it derives the value for enableProxySettings from the form preference value and a feature flag can be combined there? Would that make sense to you?

Any chance you linked the wrong file here? Anyway, I moved this out of connection-form in 03fe506, I hope that looks like what you had in mind (and, again, this is very temporary 🙂)

I don't think so, unless github is glitching out somehow, I'm linking to the same file where you moved the logic to though so it definitely looks good to me 😄 Sorry for being a nuisance with this, I'm planning to extend the eslint rules in this package shortly to cover more packages that we should avoid using directly here, so just trying to find some relatively easy ways for this PR to avoid these imports in advance

gribnoysup · 2024-08-27T12:06:00Z

...nection-form/src/components/advanced-options-tabs/authentication-tab/authentication-oidc.tsx

@@ -16,6 +17,8 @@ import { errorMessageByFieldName } from '../../../utils/validation';
 import { getConnectionStringUsername } from '../../../utils/connection-string-helpers';
 import type { OIDCOptions } from '../../../utils/oidc-handler';
 import { useConnectionFormPreference } from '../../../hooks/use-connect-form-preferences';
+import { usePreference } from 'compass-preferences-model/provider';
+import { useGlobalAppRegistry } from 'hadron-app-registry';


This is less of a problem than using preferences directly, but just from the perspective of the connection-form being a component that is not exactly aware of compass, we might want to have just a normal callback prop like onOpenProxySettingsClick passed down from the connection form root instead of emitting on global app registry directly in the form code. Would changing this up make sense to you?

Yup, done in 42c22a6 👍

gribnoysup · 2024-08-27T12:07:03Z

packages/connection-form/src/components/advanced-options-tabs/ssh-tunnel-tab/app-proxy.tsx

+import React, { useCallback } from 'react';
+
+export function AppProxy(): React.ReactElement {
+  const globalAppRegistry = useGlobalAppRegistry();


Same as above for direct usage of appRegistry in the connection-form

Also done in 42c22a6 👍

gribnoysup · 2024-08-27T12:08:59Z

...ection-form/src/components/advanced-options-tabs/ssh-tunnel-tab/proxy-and-ssh-tunnel-tab.tsx

 }: {
  errors: ConnectionFormError[];
  connectionStringUrl: ConnectionStringUrl;
  updateConnectionFormField: UpdateConnectionFormField;
  connectionOptions?: ConnectionOptions;
+  _showProxySettingsForTesting?: boolean;


Nit, I'd suggest to fallback to using special testing only props on components as a total last resort: should be possible to just pre-configure the preferences in test to match the logic we're expecting here, unless I'm missing something 🤔

Handled as a side-effect of the cleanup in 03fe506 👍

gribnoysup · 2024-08-27T12:20:01Z

packages/data-service/src/connect-mongo-client.ts

-  const [tunnel, socks5Options] = await openSshTunnel(
-    connectionOptions.sshTunnel,
-    logger
+  // (?. because this can be false-y in compass-web)


Concerns me a bit that this requires some knowledge about how compass-web build is configured and is not enforced by anything else, but this knowledge. I wonder if we can make sure that somehow every time anything from this library is used in compas monorepo 🤔 Is there a way for us to maybe extend this package types with a shared d.ts file in some way that would make every export optional and apply it everywhere through our shared tsconfig? Not a blocker, but maybe if it's low effort to try something like this, it might be worth it?

Yeah, fair enough. I think it's a bit cleaner to use the localPolyfill() variant in this case, it's not quite as "automatic" like making the types for a given package all-optional, but I don't think the latter can be implemented all that easily either

(done in fa14cef)

Yep, makes sense, even better probably, very obvious what the expectation is there now 👍

gribnoysup · 2024-08-27T12:23:13Z

configs/webpack-config-compass/src/index.ts

@@ -110,6 +111,11 @@ const sharedResolveOptions = (
  };
 };

+const providePlugin = new ProvidePlugin({


It worked! 😄

It did! 🙂

gribnoysup

Looks neat! TY for addressing the feedback!

feat: add per-connection proxy settings COMPASS-8142

608a86a

- Remove the `ssh-tunnel` package from Compass and its usage from data-service - Add relevant fields to the connection form and integrate them into data-service

github-actions bot added the feat label Aug 16, 2024

addaleax added the release notes label Aug 19, 2024

addaleax and others added 11 commits August 19, 2024 14:04

fixup: use Sergey's suggestion to use webpack ProvidePlugin to always…

b753b64

… specify whatwg-url

Merge remote-tracking branch 'origin/main' into 8142-dev

5d0f64c

fixup: check & fix shell

de59fb2

Merge remote-tracking branch 'origin/main' into 8142-dev

a476570

fixup: extraneous console.log()

4d07f95

Merge remote-tracking branch 'origin/main' into 8142-dev

8035440

fixup: small test & check fixes

0411a3c

fixup: update package-lock

2ddc686

fixup: bump devtools-connect, devtools-proxy-support, devtools-docker…

b3653fe

…-test-envs

Merge remote-tracking branch 'origin/main' into 8142-dev

d1bf511

fixup: add e2e tests

0d53e6c

addaleax marked this pull request as ready for review August 26, 2024 18:06

fixup: initialize safeStorage plain text encryption usage earlier

055f605

gribnoysup reviewed Aug 27, 2024

View reviewed changes

addaleax added 4 commits August 27, 2024 15:08

fixup: drill openSettingsModal through the connection form

42c22a6

fixup: address unrelated (?) ts errors

6f1cdc6

fixup: use localPolyfill for devtools-proxy-support

fa14cef

fixup: explicitly pass in the proxy feature flag to the connection form

03fe506

addaleax requested a review from gribnoysup August 27, 2024 13:30

fixup: lint

9621025

gribnoysup approved these changes Aug 27, 2024

View reviewed changes

addaleax merged commit 86a9451 into main Aug 27, 2024
27 checks passed

addaleax deleted the 8142-dev branch August 27, 2024 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add per-connection proxy settings COMPASS-8142 #6133

feat: add per-connection proxy settings COMPASS-8142 #6133

addaleax commented Aug 16, 2024

gribnoysup left a comment

gribnoysup Aug 27, 2024

addaleax Aug 27, 2024 •

edited

Loading

gribnoysup Aug 27, 2024

addaleax Aug 27, 2024

gribnoysup Aug 27, 2024

gribnoysup Aug 27, 2024

addaleax Aug 27, 2024

gribnoysup Aug 27, 2024

addaleax Aug 27, 2024

gribnoysup Aug 27, 2024

addaleax Aug 27, 2024

gribnoysup Aug 27, 2024

addaleax Aug 27, 2024

addaleax Aug 27, 2024

gribnoysup Aug 27, 2024

gribnoysup Aug 27, 2024

addaleax Aug 27, 2024

gribnoysup left a comment

feat: add per-connection proxy settings COMPASS-8142 #6133

feat: add per-connection proxy settings COMPASS-8142 #6133

Conversation

addaleax commented Aug 16, 2024

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

gribnoysup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Aug 27, 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

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

gribnoysup left a comment

Choose a reason for hiding this comment

addaleax Aug 27, 2024 •

edited

Loading