[ws-proxy] only get username if workspace not managed by mk2 #19180

iQQBot · 2023-12-01T14:50:10Z

Description

[ws-proxy] only get username if workspace not managed by mk2

Summary generated by Copilot

`🤖[deprecated] Generated by Copilot at 3d5c859`

This pull request enhances the ws-proxy component to support workspaces managed by the new workspace controller mk2. It adds a new field IsManagedByMk2 to the WorkspaceInfo type and uses it to handle SSH connections and pod detection.

Related Issue(s)

Fixes ENG-1297

How to test

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

🏷️ Name - pd-ws-proxea4f3f5894
🔗 URL - pd-ws-proxea4f3f5894.preview.gitpod-dev.com/workspaces.
📚 Documentation - See our internal documentation for information on how to interact with your preview environment.
📦 Version - pd-ws-proxy-ssh-username-gha.21206
🗒️ Logs - GCP Logs Explorer

Build Options

Build

/werft with-werft
Run the build with werft instead of GHA
leeway-no-cache
/werft no-test
Run Leeway with --dont-test

Publish

/werft publish-to-npm
/werft publish-to-jb-marketplace

Installer

analytics=segment
with-dedicated-emulation
workspace-feature-flags
Add desired feature flags to the end of the line above, space separated

Preview Environment / Integration Tests

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-gce-vm
If enabled this will create the environment on GCE infra
/werft preemptible
Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
with-monitoring

/hold

kylos101

👋 sharing some initial feedback, waiting to add approval, will test now

kylos101 · 2023-12-01T16:13:08Z

components/ws-proxy/pkg/sshproxy/server.go

@@ -318,29 +318,30 @@ func (s *Server) HandleConn(c net.Conn) {
 		OwnerUserId: wsInfo.OwnerUserId,
 	}

-	if wsInfo.SSHKey != nil {


question, blocking:
@iQQBot what scenario is wsInfo.SSHKey != nil for? Is it still needed? If no longer needed, can we remove in this PR too? Asking because if we don't do now, it might "linger".

This is the old method, we can indeed remove it here because next-gen has not been officially put into use, especially the ssh part.

But I'm not planning to remove it from the workspace CRD, I don't know if this will cause other problems.

Should we wait to land this PR until Monday? If we land it today, it could ship Monday. This would not be ideal, because pods config could apply after pods, falling back to ssh host.

When this PR lands, do we also need a ws-manager-mk2 change to land? Otherwise ws-manager-mk2 has the same risk.

This PR only prepared for next-gen, nothing change for regular workspace

Was nextgen the only one using wsInfo.SSHKey?

kylos101 · 2023-12-01T16:26:32Z

components/ws-proxy/pkg/sshproxy/server.go

+			err = xerrors.Errorf("workspace not managed by mk2, but didn't have SSH CA enabled")
+			s.TrackSSHConnection(wsInfo, "connect", ErrCreateSSHKey)
+			ReportSSHAttemptMetrics(ErrCreateSSHKey)
+			log.WithField("instanceId", wsInfo.InstanceID).WithError(err).Error("failed to generate ssh cert")


Question, non-blocking:
Should we also include wsInfo.IsManagedByMk2 as a field? I ask because I'm unsure if a nextgen cluster will support and have both managers at the same time. I assume it will.

kylos101 · 2023-12-01T16:28:11Z

components/ws-proxy/pkg/sshproxy/server.go

 		key, err = s.GenerateSSHCert(ctx, userName)
 		if err != nil {
-			log.WithField("workspaceId", workspaceId).WithError(err).Error("failed to generate ssh cert")
+			log.WithField("instanceId", wsInfo.InstanceID).WithError(err).Error("failed to generate ssh cert")


Question, blocking:
Should we do s.TrackSSHConnection and ReportSSHAttemptMetrics for this failure, too?

Question, non-blocking:
Also, should we include wsInfo.IsEnabledSSHCA as a field? Seems like could help when debugging.

Also, should we include wsInfo.IsEnabledSSHCA as a field? Seems like could help when debugging.

I assume you mean isMk2, if so that's make sense, wsInfo.IsEnabledSSHCA is always true if you enter this

components/ws-proxy/pkg/sshproxy/server.go

kylos101 · 2023-12-01T17:20:18Z

👋 sharing some initial feedback, waiting to add approval, will test now

I was able to connect to a workspace with SSH, and validate it is using ssh ca.

gitpod@kylos101-dotfiles-ah83qjcn6hs:/workspace/dotfiles$ printenv | grep GITPOD_SSH | wc
      1       3     120

# also, ~/.ssh/authorized_keys did not exist

kylos101 · 2023-12-01T17:20:41Z

@iQQBot let me know if you have any questions, and when you'd like a follow-up review

iQQBot · 2023-12-01T18:47:23Z

@kylos101 I have addressed your feedback, you can review it again.

kylos101

Thank you , @iQQBot , a couple follow-ups

kylos101 · 2023-12-01T19:06:34Z

components/ws-proxy/pkg/sshproxy/server.go

@@ -366,7 +370,7 @@ func (s *Server) HandleConn(c net.Conn) {
 	if err != nil {
 		s.TrackSSHConnection(wsInfo, "connect", ErrConnFailed)
 		ReportSSHAttemptMetrics(ErrConnFailed)
-		log.WithField("instanceId", wsInfo.InstanceID).WithField("workspaceIP", wsInfo.IPAddress).WithError(err).Error("dail failed")
+		log.WithField("workspaceIP", wsInfo.IPAddress).WithError(err).Error("dail failed")


nit: dial failed

kylos101 · 2023-12-01T19:09:14Z

components/ws-proxy/pkg/sshproxy/server.go

@@ -318,29 +318,30 @@ func (s *Server) HandleConn(c net.Conn) {
 		OwnerUserId: wsInfo.OwnerUserId,
 	}

-	if wsInfo.SSHKey != nil {


Should we wait to land this PR until Monday? If we land it today, it could ship Monday. This would not be ideal, because pods config could apply after pods, falling back to ssh host.

When this PR lands, do we also need a ws-manager-mk2 change to land? Otherwise ws-manager-mk2 has the same risk.

kylos101

💪 you rock, @iQQBot !🤗

components/ws-proxy/pkg/sshproxy/server.go

iQQBot · 2023-12-01T19:32:24Z

/unhold

[ws-proxy] only get username if workspace not managed by mk2

3d5c859

iQQBot requested a review from a team as a code owner December 1, 2023 14:50

roboquat added do-not-merge/hold size/S labels Dec 1, 2023

github-actions bot added the team: team-engine label Dec 1, 2023

kylos101 reviewed Dec 1, 2023

View reviewed changes

iQQBot added 2 commits December 1, 2023 18:46

remove ssh key from infoprovider

ab1ff4a

improve logs

a1430a8

roboquat added size/M and removed size/S labels Dec 1, 2023

kylos101 reviewed Dec 1, 2023

View reviewed changes

kylos101 approved these changes Dec 1, 2023

View reviewed changes

iQQBot commented Dec 1, 2023

View reviewed changes

components/ws-proxy/pkg/sshproxy/server.go Outdated Show resolved Hide resolved

Update components/ws-proxy/pkg/sshproxy/server.go

b395010

roboquat removed the do-not-merge/hold label Dec 1, 2023

roboquat merged commit 7f83e2f into main Dec 1, 2023

roboquat deleted the pd/ws-proxy-ssh-username branch December 1, 2023 19:41

[ws-proxy] only get username if workspace not managed by mk2 #19180

[ws-proxy] only get username if workspace not managed by mk2 #19180

Uh oh!

Conversation

iQQBot commented Dec 1, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

🤖[deprecated] Generated by Copilot at 3d5c859

Related Issue(s)

How to test

Documentation

Preview status

Build Options

Uh oh!

kylos101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kylos101 commented Dec 1, 2023

Uh oh!

kylos101 commented Dec 1, 2023

Uh oh!

iQQBot commented Dec 1, 2023

Uh oh!

kylos101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylos101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iQQBot commented Dec 1, 2023

Uh oh!

Uh oh!

iQQBot commented Dec 1, 2023 •

edited by github-actions bot

Loading

`🤖[deprecated] Generated by Copilot at 3d5c859`