[server] Switch from Env to Config #4982

geropl · 2021-07-28T09:49:47Z

Companion PR: https://github.com/gitpod-com/gitpod/pull/5862

TL;DR

This replaces Env (derived from env variables and relying on manual de-/serialization) with Config derived from a ConfigMap whose structure (mostly) matches the values.yaml. To ensure we don't break stuff on deployment this comes with a restrictive safety check.

About

This PR does away with the old Env/EnvEE classes and replaces it with a Config shape which can be loaded from a config file of that exact same shape in server-configmap.yaml. In consequence a lot of the de-/serializing logic in Env got superfluous, and (nearly) all defaults moved to values.yaml:components.server.

For the initial deployment server on startups compares old and new config to make sure we didn't introduce an unwanted config drift.

Code-wise the most important parts are in:
- server-configmap.yaml
- server-deployment.yaml
- values.yaml
- config.ts

Besides this, changes fall into these categories:

name normalizations so values.yaml and Config names map directly
grouping of some values where it made sense
removal of dead code and config
validation that checks that old and new config are identical

ToDo

see if Config/-Serialized should be reversed
make integration tests work again
compare and validate devstaging config
compare and validate staging config
adjust staging/production values
- Companion PR: https://github.com/gitpod-com/gitpod/pull/5862
- deploy to staging and check config

Future work

derive schema from shape and use that for validation

jankeromnes · 2021-08-13T13:11:16Z

Removing the automatic review requests as this is still Draft. 😊

jankeromnes

Wow, quite a big change! 💪 I took a quick look, and didn't notice anything obviously broken.

What is the testing strategy to deploy this without accidentally breaking something subtle?

I like the old/new comparison check (did you try it by introducing some temporary small change?)
But I think we should also test this on staging really well (e.g. workspace backups, workspace downloads, prebuilds, the GitHub App, payments all could seem somewhat at risk and should be well tested)

Also, there are several import { Env } and import { EnvEE } left in various files. I know some of them are for comparison, but I'm not sure all remaining imports are still needed. Please double-check.

jankeromnes · 2021-08-31T09:56:37Z

components/server/src/branding-parser.ts

@@ -9,19 +9,20 @@ import { Branding } from "@gitpod/gitpod-protocol";
 export namespace BrandingParser {


Do we still use this BrandingParser? Can we remove it?

see next comment.

jankeromnes · 2021-08-31T09:57:49Z

components/server/src/config.ts

    disableDynamicAuthProviderLogin: boolean;

-    // TODO(gpl) Can we remove this?
    brandingConfig: Branding;


Did you decide to keep it?

Yes. There were some places where we use two fields. And I did not want to increase the complexity of this PR even more by a) making the decision here, and
b) changing that code/API as well.

components/server/src/github/github-context-parser.spec.ts

components/server/src/server.ts

geropl · 2021-08-31T11:43:11Z

Wow, quite a big change!

This is actually the 2nd, bigger PR. The Config shape was already introduced before.

What is the testing strategy to deploy this without accidentally breaking something subtle?

There are two things we can check:

Manually

As Config has already been introduced, current prod/staging versions print there Config shape (the one derived from Env) to stdout on startup. Once we deployed the new version, we should compare those manually. But there have been a few additions, and also fixes to getConfigFromOldEnv, so this is not perfect.

==> This helps to ensure that there is no drift in the values files (we have to slightly adjust those as well).

Automatically

There are three things we have to verify (in this review) to make this work:

env vars in server-deployment were not changed semantically
env.ts was not changed semantically
the getConfigFromOldEnv in config.ts is correct (mostly straight forward, should be easy to verify)

==> This ensures that at least given the same input (chart), there is no difference between Env and Config.

I like the old/new comparison check (did you try it by introducing some temporary small change?)

Not sure what you mean with "temporary small changes"? I used this to find all discrepancies, so the "negative" as well as "positive" case works if that is what you mean.

But I think we should also test this on staging really well (e.g. workspace backups, workspace downloads, prebuilds, the GitHub App, payments all could seem somewhat at risk and should be well tested)

If we do the two checks above really thoroughly I don't think this is necessary.

Also, there are several import { Env } and import { EnvEE } left in various files. I know some of them are for comparison, but I'm not sure all remaining imports are still needed. Please double-check.

Did that, it's all for the comparison to work. 👍

JanKoehnlein · 2021-09-01T12:23:21Z

components/server/src/auth/gitpod-cookie.ts


 @injectable()
 export class GitpodCookie {
-    @inject(Env) protected readonly env: Env;
+    @inject(Config) protected readonly env: Config;


You called it config everywhere else. Why not here?

JanKoehnlein · 2021-09-01T12:28:34Z

components/server/src/user/user-deletion-service.ts


 @injectable()
 export class UserDeletionService {
-    @inject(Env) protected readonly env: Env;
+    @inject(Config) protected readonly config: Config;


Not used here. Should we push it down to subclass?

JanKoehnlein · 2021-09-01T12:31:40Z

components/server/src/workspace/workspace-deletion-service.ts

 import { TraceContext } from "@gitpod/gitpod-protocol/lib/util/tracing";

 @injectable()
 export class WorkspaceDeletionService {
    @inject(TracedWorkspaceDB) protected readonly db: DBWithTracing<WorkspaceDB>;
    @inject(StorageClient) protected readonly storageClient: StorageClient;
-    @inject(Env) protected readonly env: Env;
+    @inject(Config) protected readonly config: Config;


Not used here, either. Push down to subclass?

JanKoehnlein · 2021-09-01T12:37:30Z

components/server/src/server.ts

    @inject(Config) protected readonly config: Config;
+    @inject(Env) protected readonly env: Env;
    @inject(SessionHandlerProvider) protected sessionHandlerProvider: SessionHandlerProvider;
    @inject(Authenticator) protected authenticator: Authenticator;
    @inject(UserController) protected readonly userController: UserController;
    @inject(EnforcementController) protected readonly enforcementController: EnforcementController;


Why did you remove TheiaPluginController in this PR?

We don't use it anymore.

JanKoehnlein · 2021-09-01T12:39:32Z

Apart from some nitty comments

/lgtm

roboquat · 2021-09-01T12:39:36Z

LGTM label has been added.

Git tree hash: c6fa5afedfe24a0016883c22f118fd4dc854fd8f

roboquat · 2021-09-01T12:39:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JanKoehnlein

Associated issue: #4650

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JanKoehnlein]
~~components/gitpod-protocol/OWNERS~~ [JanKoehnlein]
~~components/server/OWNERS~~ [JanKoehnlein]

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

roboquat added the do-not-merge/work-in-progress label Jul 28, 2021

roboquat requested review from AlexTugarev and jankeromnes July 28, 2021 09:49

roboquat added approved size/XXL labels Jul 28, 2021

geropl force-pushed the gpl/4650-server-config-2 branch 7 times, most recently from f967e1a to c517b70 Compare July 30, 2021 05:09

jankeromnes removed request for jankeromnes and AlexTugarev August 13, 2021 13:11

geropl force-pushed the gpl/4650-server-config-2 branch from c517b70 to 81dc340 Compare August 31, 2021 09:02

roboquat removed the approved label Aug 31, 2021

[server] Introduce Config and use it everywhere

3d9dcec

geropl force-pushed the gpl/4650-server-config-2 branch from 81dc340 to 35a2926 Compare August 31, 2021 09:24

geropl marked this pull request as ready for review August 31, 2021 09:45

roboquat removed the do-not-merge/work-in-progress label Aug 31, 2021

jankeromnes reviewed Aug 31, 2021

View reviewed changes

[server] Make Env and Config comparable

3c829c6

geropl force-pushed the gpl/4650-server-config-2 branch from 35a2926 to 3c829c6 Compare August 31, 2021 11:50

geropl added 3 commits August 31, 2021 13:19

[server] Config: Unify github.app to githubApp

f1c2a95

[server] Fix triallicense and chargebeeOptionsFile

06c7fda

[server] Remove serverProxyApiKey

e7082e0

JanKoehnlein reviewed Sep 1, 2021

View reviewed changes

roboquat assigned JanKoehnlein Sep 1, 2021

roboquat added the lgtm label Sep 1, 2021

roboquat added the approved label Sep 1, 2021

roboquat merged commit e2b931a into main Sep 1, 2021

roboquat deleted the gpl/4650-server-config-2 branch September 1, 2021 12:40

This was referenced Sep 1, 2021

[server, paymentendpoint] Fix config (parsing) #5482

Merged

[server] Add/propagate missing defaults #5483

Merged

[server] Fix temporary config validation #5485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[server] Switch from Env to Config #4982

[server] Switch from Env to Config #4982

geropl commented Jul 28, 2021 •

edited

Loading

jankeromnes commented Aug 13, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes left a comment

jankeromnes Aug 31, 2021

geropl Aug 31, 2021

jankeromnes Aug 31, 2021

geropl Aug 31, 2021

geropl commented Aug 31, 2021

JanKoehnlein Sep 1, 2021

JanKoehnlein Sep 1, 2021

JanKoehnlein Sep 1, 2021

JanKoehnlein Sep 1, 2021

geropl Sep 1, 2021

JanKoehnlein commented Sep 1, 2021

roboquat commented Sep 1, 2021

roboquat commented Sep 1, 2021

		@@ -9,19 +9,20 @@ import { Branding } from "@gitpod/gitpod-protocol";
		export namespace BrandingParser {

[server] Switch from Env to Config #4982

[server] Switch from Env to Config #4982

Conversation

geropl commented Jul 28, 2021 • edited Loading

TL;DR

About

ToDo

Future work

jankeromnes commented Aug 13, 2021 • edited by werft-gitpod-dev-com bot Loading

jankeromnes left a 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

geropl commented Aug 31, 2021

Manually

Automatically

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

JanKoehnlein commented Sep 1, 2021

roboquat commented Sep 1, 2021

roboquat commented Sep 1, 2021

geropl commented Jul 28, 2021 •

edited

Loading

jankeromnes commented Aug 13, 2021 •

edited by werft-gitpod-dev-com bot

Loading