[installer, ws-manager]: Use installation shortname in URL templates #10152

andrew-farries · 2022-05-20T12:39:56Z

Description

ℹ️ ℹ️ ℹ️ ℹ️

This is a second attempt at #10127, which was reverted (#10143) as it broke preview environments. See this comment in #9875. The last commit on the branch is a workaround for the preview environment issue.

ℹ️ ℹ️ ℹ️ ℹ️

As described in #9875, failing image builds currently fail to produce any useful output. A possible cause, as described in this comment, is that the format of the ws-manager WorkspaceURLTemplate strings is wrong.

When we deployed with the old helm chart, these values were set in the relevant configmap like this:

gitpod/chart/templates/ws-manager-configmap.yaml

Lines 74 to 75 in 2f988c9

    
           "urlTemplate":     "https://{{"{{ .Prefix }}"}}.ws{{- if $.Values.installation.shortname -}}-{{ $.Values.installation.shortname }}{{- end -}}.{{ $.Values.hostname }}", 
        
           "portUrlTemplate": "https://{{"{{ .WorkspacePort }}"}}-{{"{{ .Prefix }}"}}.ws{{- if $.Values.installation.shortname -}}-{{ $.Values.installation.shortname }}{{- end -}}.{{ $.Values.hostname }}",

ie, they included a suffix after the ws.

This PR tailors the installer output to produce these URLs in the same way the old helm chart did. Installation short name suffixes like eu-02 are set here:

https://github.com/gitpod-io/ops/blob/main/deploy/production/meta-eu02/app/installer-config.yaml#L6

so the URLs will now look like https://{{ .Prefix }}.ws-eu02.example.com.

Related Issue(s)

Fixes #9875

How to test

I've tested this change in staging by editing the ws-manager configmap in staging to contain the correct URLs, restarting ws-manager and running an image build. I was able to see image build logs as expected:

Release Notes

[installer] Use installation shortname when constructing ws-manager URL templates

Documentation

None

andrew-farries · 2022-05-20T12:40:06Z

/hold

sagor999 · 2022-05-20T18:01:59Z

@meysholdt how do you deploy gitpod in preview env?
Andrew placed a workaround to make sure this change doesn't break preview, but I feel like we should instead fix the way we are deploying in preview (remove default as shortname)

sagor999 · 2022-05-23T22:57:37Z

@gitpod-io/platform for comment above. can you provide insight before we will merge this.

mads-hartmann · 2022-05-24T07:30:47Z

@andrew-farries @sagor999 The Werft job uses the Gitpod Installer to generate a config for the installer and then modifies it quite heavily for preview environments. The implementation is here.

The metadata.shortname is set to default by the installer when generating the config. I verified by running the Installer from my workspace off main.

gitpod /workspace/gitpod/install/installer (main) $ go run main.go  init | grep -6 metadata
  inCluster: true
database:
  inCluster: true
disableDefinitelyGp: true
domain: ""
kind: Full
metadata:
  region: local
  shortname: default
objectStorage:
  inCluster: true
observability:
  logLevel: info

So the change you want to make (if you just want to remove metadata.shortname from the config used for preview environments) is something like this

diff --git a/.werft/jobs/build/installer/installer.ts b/.werft/jobs/build/installer/installer.ts
index 52edde6f6..ea2a83671 100644
--- a/.werft/jobs/build/installer/installer.ts
+++ b/.werft/jobs/build/installer/installer.ts
@@ -58,6 +58,7 @@ export class Installer {
         this.options.werft.log(slice, "Adding extra configuration");
         try {
             this.getDevCustomValues(slice)
+            this.configureMetadata(slice)
             this.configureContainerRegistry(slice)
             this.configureDomain(slice)
             this.configureWorkspaces(slice)
@@ -92,6 +93,10 @@ export class Installer {
         exec(`yq m -i ${this.options.installerConfigPath} ${WORKSPACE_SIZE_CONFIG_PATH}`, { slice: slice });
     }
 
+    private configureMetadata(slice: string): void {
+        exec(`yq d -i ${this.options.installerConfigPath} metadata.shortname `, { slice: slice });
+    }
+
     private configureContainerRegistry(slice: string): void {
         exec(`yq w -i ${this.options.installerConfigPath} certificate.name ${this.options.proxySecretName}`, { slice: slice });
         exec(`yq w -i ${this.options.installerConfigPath} containerRegistry.inCluster false`, { slice: slice });

andrew-farries · 2022-05-24T08:35:23Z

Thanks @mads-hartmann. I'm more concerned about the consequences of removing the installation shortname rather than the mechanics of how to do it, given the comment here from @geropl:

gitpod/install/installer/pkg/config/v1/config.go

Line 50 in 53c9935

    
           cfg.Metadata.InstallationShortname = "default" // TODO(gpl): we're tied to "default" here because that's what we put into static bridges in the past

But I guess we can leave it as the default in the installer and remove it specifically for preview environments.

mads-hartmann · 2022-05-24T08:39:53Z

But I guess we can leave it as the default in the installer and remove it specifically for preview environments.

Yeah I think that's the best option, and that is what the TypeScript diff above should do ☺️ I don't know what consequences of not having the shortname is for preview environments though.

andrew-farries · 2022-05-24T13:16:01Z

I've removed the installation.shortname when deploying to a preview environment. Previews still appear to work.

sagor999 · 2022-05-24T18:09:48Z

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-af-installer-append-shortname-to-url-template.17
(with .werft/ from main)

sagor999 · 2022-05-24T18:24:25Z

@andrew-farries can you rebase your PR? There is a breaking change without which build fails now :(

andrew-farries · 2022-05-25T08:57:44Z

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-af-installer-append-shortname-to-url-template.19
(with .werft/ from main)

andrew-farries · 2022-05-25T09:45:53Z

Tests failing due to this: #10243.

Will rebase again when that's resolved.

kylos101 · 2022-05-25T09:52:44Z

@sagor999 could you test the installer asset produced by this build in an ephemeral cluster? I'm worried this could impact our post processing for ws-proxy.

kylos101 · 2022-05-25T09:54:48Z

@sagor999 could you test the installer asset produced by this build in an ephemeral cluster? I'm worried this could impact our post processing for ws-proxy.

@Furisto wanted to give you a heads up on this, as it could impact our ability to deploy workspace clusters.

sagor999 · 2022-05-25T17:53:55Z

@kylos101 tested ephemeral cluster creation using installer from this branch. All worked fine. Was able to start workspace in that cluster.

geropl · 2022-05-27T12:23:24Z

install/installer/pkg/components/ws-manager/configmap_test.go

+			Domain:                           "example.com",
+			InstallationShortname:            "eu02",
+			ExpectedWorkspaceUrlTemplate:     "https://{{ .Prefix }}.ws-eu02.example.com",
+			ExpectedWorkspacePortURLTemplate: "https://{{ .WorkspacePort }}-{{ .Prefix }}.ws-eu02.example.com",


geropl · 2022-05-27T12:23:35Z

install/installer/pkg/components/ws-manager/configmap_test.go

+			Domain:                           "example.com",
+			InstallationShortname:            "",
+			ExpectedWorkspaceUrlTemplate:     "https://{{ .Prefix }}.ws.example.com",
+			ExpectedWorkspacePortURLTemplate: "https://{{ .WorkspacePort }}-{{ .Prefix }}.ws.example.com",


@andrew-farries We need a test case "with 'default' as installation shortname" to cover all existing self-hosted installations out there I think. That will lead us to the comment above I think.

geropl · 2022-05-27T12:40:36Z

I'm more concerned about the consequences of removing the installation shortname rather than the mechanics of how to do it, given the comment here from @geropl:

@andrew-farries I added that line when making installationShortname editable (again) and kept the "default" instead of moving to "" (empty string), because I did not want to run into migration trouble with existing self-hosted installations.

The problem is that whatever we put into installationShortname is what new workspaceInstances get set as "region". And for the lifetime of a workspace, we need to make sure there is a ws-manager registered for exactly that "region"/"cluster".

But I guess we can leave it as the default in the installer and remove it specifically for preview environments.

💯 This is what I would have done as well.

geropl · 2022-05-27T12:41:24Z

install/installer/pkg/components/ws-manager/configmap.go

@@ -113,6 +113,11 @@ func configmap(ctx *common.RenderContext) ([]runtime.Object, error) {
 		return nil, err
 	}

+	installationShortNameSuffix := ""
+	if ctx.Config.Metadata.InstallationShortname != "" {


@andrew-farries IMO we need to check for `!= "" && != "default", no? (cmp test cases below)

geropl

https://github.com/gitpod-io/gitpod/pull/10152/files#r883578267

This matches how these URLs were constructed in the old helm chart: https://github.com/gitpod-io/gitpod/blob/2f988c93499926bd34d6728defbd9d9c0964797e/chart/templates/ws-manager-configmap.yaml#L74-L75

Use `yq -m` to merge an inline config rather than `yq -w` because the old version of `yq` we have installed does not support setting fields to the empty string literal.

geropl

Fixes (my own) complaint with this commit.

geropl · 2022-05-27T14:36:44Z

/unhold

andrew-farries requested review from geropl and a team May 20, 2022 12:39

roboquat added the release-note label May 20, 2022

roboquat added do-not-merge/hold size/M labels May 20, 2022

github-actions bot added the team: workspace Issue belongs to the Workspace team label May 20, 2022

mads-hartmann self-assigned this May 24, 2022

andrew-farries added the do-not-merge/work-in-progress label May 24, 2022

andrew-farries force-pushed the af/installer-append-shortname-to-url-templates branch 3 times, most recently from 52a10c2 to f4fa140 Compare May 24, 2022 11:05

andrew-farries requested a review from a team May 24, 2022 11:05

github-actions bot added the team: delivery Issue belongs to the self-hosted team label May 24, 2022

andrew-farries force-pushed the af/installer-append-shortname-to-url-templates branch 4 times, most recently from a3fb0aa to 1800f7b Compare May 24, 2022 13:14

andrew-farries removed the do-not-merge/work-in-progress label May 24, 2022

andrew-farries force-pushed the af/installer-append-shortname-to-url-templates branch from 1800f7b to e9b4b40 Compare May 25, 2022 08:38

corneliusludmann approved these changes May 25, 2022

View reviewed changes

andrew-farries force-pushed the af/installer-append-shortname-to-url-templates branch from e9b4b40 to d0a8c58 Compare May 25, 2022 10:18

sagor999 approved these changes May 25, 2022

View reviewed changes

geropl reviewed May 27, 2022

View reviewed changes

geropl requested changes May 27, 2022

View reviewed changes

roboquat added size/L and removed size/M labels May 27, 2022

Andrew Farries and others added 6 commits May 27, 2022 14:19

Append installation shortname to url templates

fb1e69d

This matches how these URLs were constructed in the old helm chart: https://github.com/gitpod-io/gitpod/blob/2f988c93499926bd34d6728defbd9d9c0964797e/chart/templates/ws-manager-configmap.yaml#L74-L75

Add test for workspace URL templates

a14d383

Remove shortname from preview installations

b5f422a

Use `yq -m` to merge an inline config rather than `yq -w` because the old version of `yq` we have installed does not support setting fields to the empty string literal.

Make InstallationShortname not required

390b79d

[installer] Treat InstallationShortname "" and "default" as equivalent

8823016

[werft] Print installer config on validation error

3bd7592

geropl force-pushed the af/installer-append-shortname-to-url-templates branch from 14e2287 to 3bd7592 Compare May 27, 2022 14:19

geropl approved these changes May 27, 2022

View reviewed changes

roboquat removed the do-not-merge/hold label May 27, 2022

roboquat merged commit 6c5ef9a into main May 27, 2022

roboquat deleted the af/installer-append-shortname-to-url-templates branch May 27, 2022 14:37

	"urlTemplate": "https://{{"{{ .Prefix }}"}}.ws{{- if $.Values.installation.shortname -}}-{{ $.Values.installation.shortname }}{{- end -}}.{{ $.Values.hostname }}",
	"portUrlTemplate": "https://{{"{{ .WorkspacePort }}"}}-{{"{{ .Prefix }}"}}.ws{{- if $.Values.installation.shortname -}}-{{ $.Values.installation.shortname }}{{- end -}}.{{ $.Values.hostname }}",

[installer, ws-manager]: Use installation shortname in URL templates #10152

[installer, ws-manager]: Use installation shortname in URL templates #10152

Uh oh!

Conversation

andrew-farries commented May 20, 2022

Description

Related Issue(s)

How to test

Release Notes

Documentation

Uh oh!

andrew-farries commented May 20, 2022

Uh oh!

sagor999 commented May 20, 2022

Uh oh!

sagor999 commented May 23, 2022

Uh oh!

mads-hartmann commented May 24, 2022

Uh oh!

andrew-farries commented May 24, 2022

Uh oh!

mads-hartmann commented May 24, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrew-farries commented May 24, 2022

Uh oh!

sagor999 commented May 24, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sagor999 commented May 24, 2022

Uh oh!

andrew-farries commented May 25, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrew-farries commented May 25, 2022

Uh oh!

kylos101 commented May 25, 2022

Uh oh!

kylos101 commented May 25, 2022

Uh oh!

sagor999 commented May 25, 2022

Uh oh!

geropl May 27, 2022

Choose a reason for hiding this comment

Uh oh!

geropl May 27, 2022

Choose a reason for hiding this comment

Uh oh!

geropl May 27, 2022

Choose a reason for hiding this comment

Uh oh!

geropl commented May 27, 2022

Uh oh!

geropl May 27, 2022

Choose a reason for hiding this comment

Uh oh!

geropl left a comment

Choose a reason for hiding this comment

Uh oh!

geropl left a comment

Choose a reason for hiding this comment

Uh oh!

geropl commented May 27, 2022

Uh oh!

Uh oh!

mads-hartmann commented May 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

sagor999 commented May 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

andrew-farries commented May 25, 2022 •

edited by werft-gitpod-dev-com bot

Loading