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

[WIP] configcat #12695

Closed
wants to merge 2 commits into from
Closed

[WIP] configcat #12695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04460bd
[werft] add configcat key env into workspace template
mustard-mh Sep 7, 2022
9c1a5c1
test
mustard-mh Sep 7, 2022
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
8 changes: 5 additions & 3 deletions .werft/jobs/build/installer/installer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,8 @@ export class Installer {
this.configureSSHGateway(slice);
this.configurePublicAPIServer(slice);
this.configureUsage(slice);
this.configureConfigCat(slice);

this.configureDefaultTemplate(slice);
this.configureConfigCat(slice);

if (this.options.analytics) {
this.includeAnalytics(slice);
Expand Down Expand Up @@ -247,10 +246,13 @@ EOF`);

private configureConfigCat(slice: string) {
// This key is not a secret, it is a unique identifier of our ConfigCat application
const configcatKey = "WBLaCPtkjkqKHlHedziE9g/LEAOCNkbuUKiqUZAcVg7dw";
exec(
`yq w -i ${this.options.installerConfigPath} experimental.webapp.configcatKey "WBLaCPtkjkqKHlHedziE9g/LEAOCNkbuUKiqUZAcVg7dw"`,
`yq w -i ${this.options.installerConfigPath} experimental.webapp.configcatKey "${configcatKey}"`,
{ slice: slice },
);
exec(`yq w -i ${this.options.installerConfigPath} 'workspace.templates.default.spec.containers.(name==workspace).env[+].name' GITPOD_CONFIGCAT_SDK_KEY`);
exec(`yq w -i ${this.options.installerConfigPath} 'workspace.templates.default.spec.containers.(name==workspace).env.(name==GITPOD_CONFIGCAT_SDK_KEY).value' "${configcatKey}"`);
Comment on lines +254 to +255
Copy link
Member

Choose a reason for hiding this comment

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

what's the a reason to put the key in an env variable? I know the key is public but in general I think is not a good practice to put secrets in env variables and also pollutes the shell environment, is it just to avoid copying it for the different clients that may use it (vscode/jetbrains/supervisor)?

Copy link
Contributor Author

@mustard-mh mustard-mh Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is how we did with segment. (Even openvscode-server is using server API trackEvent)

gitpod/.werft/jobs/build/installer/installer.ts

Lines 263 to 264 in 867b11f

exec(`yq w -i ${this.options.installerConfigPath} 'workspace.templates.default.spec.containers.(name==workspace).env[+].name' GITPOD_ANALYTICS_SEGMENT_KEY`);
exec(`yq w -i ${this.options.installerConfigPath} 'workspace.templates.default.spec.containers.(name==workspace).env.(name==GITPOD_ANALYTICS_SEGMENT_KEY).value' "${this.options.analytics.token}"`);

Copy link
Member

Choose a reason for hiding this comment

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

yeah I see that, the same applies to it 🙂

Copy link
Contributor Author

@mustard-mh mustard-mh Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yeah I see that, the same applies to it 🙂

@jeanp413 I thought segment env var in workspace is not used by anyone. Maybe remove it after ask other teams if it affect something.

If we don't want configcat key to exists in env var, where should we put it and other secret in with third-party component?

  • 👎 Inside build process will affect Self-Hosted images
  • 👎 Inside env leak secret
  • 👎 Inside API only make it a little bit harder to leak
  • 👍 Ask server to provide API about configcat (experimental) like trackEvent API

Copy link
Contributor Author

@mustard-mh mustard-mh Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As the key is public, I thought it can be ok. ( I see dashboard just use it with a constant value here XD)

For segment's key GITPOD_ANALYTICS_SEGMENT_KEY, it's ok too because its value is refer to Production Untrusted project

gitpod/components/dashboard/src/experiments/client.ts

Lines 34 to 45 in 7dbfa49

// newProductionConfigCatClient constructs a new ConfigCat client with production configuration.
function newProductionConfigCatClient(): Client {
// clientKey is an identifier of our ConfigCat application. It is not a secret.
const clientKey = "WBLaCPtkjkqKHlHedziE9g/TwAe6YyftEGPnGxVRXd0Ig";
const client = configcat.createClient(clientKey, {
logger: configcat.createConsoleLogger(LogLevel.Error),
pollIntervalSeconds: 60 * 3, // 3 minutes
maxInitWaitTimeSeconds: 0,
});
return new ConfigCatClient(client);
}

Copy link
Contributor Author

@mustard-mh mustard-mh Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

👍 Ask server to provide API about configcat (experimental) like trackEvent API

Should we change plan to add it to server API? It benefits other components like JetBrains Gateway to use it easier. But will take long time to change related codes.

Maybe an issue labels with tech-debt is good for all teams too. (And let go this PR 🙈

cc @akosyakov @jeanp413

Copy link
Member

Choose a reason for hiding this comment

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

No we should not go through server for it, but used ConfigCat directly.

Copy link
Member

Choose a reason for hiding this comment

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

The good thing about env var is that it is available to all IDEs, so IDE can use it or fallback to non-production hardcoded inside. I think it is alright to continue with env var.

}

private includeAnalytics(slice: string): void {
Expand Down
2 changes: 1 addition & 1 deletion WORKSPACE.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ defaultArgs:
jbMarketplacePublishTrigger: "false"
publishToJBMarketplace: true
localAppVersion: unknown
codeCommit: 06783e74552ddefd74c7e02cdcce19054b294469
codeCommit: 5ea73bdb57dc8567bd03ed749f999964b2b6b7c4
codeQuality: stable
intellijDownloadUrl: "https://download.jetbrains.com/idea/ideaIU-2022.2.1.tar.gz"
golandDownloadUrl: "https://download.jetbrains.com/go/goland-2022.2.2.tar.gz"
Expand Down