Skip to content

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

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

Merged
merged 4 commits into from
Dec 1, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3d5c859
[ws-proxy] only get username if workspace not managed by mk2
iQQBot Dec 1, 2023
ab1ff4a
remove ssh key from infoprovider
iQQBot Dec 1, 2023
a1430a8
improve logs
iQQBot Dec 1, 2023
b395010
Update components/ws-proxy/pkg/sshproxy/server.go
iQQBot Dec 1, 2023
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
4 changes: 1 addition & 3 deletions components/ws-proxy/pkg/common/infoprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (

"github.com/gitpod-io/gitpod/ws-manager/api"
wsapi "github.com/gitpod-io/gitpod/ws-manager/api"
workspacev1 "github.com/gitpod-io/gitpod/ws-manager/api/crd/v1"
)

const (
Expand Down Expand Up @@ -65,7 +64,6 @@ type WorkspaceInfo struct {
SSHPublicKeys []string
IsRunning bool

SSHKey *workspacev1.SSHKey

IsEnabledSSHCA bool
IsManagedByMk2 bool
}
8 changes: 7 additions & 1 deletion components/ws-proxy/pkg/proxy/infoprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

wsk8s "github.com/gitpod-io/gitpod/common-go/kubernetes"
"github.com/gitpod-io/gitpod/common-go/log"
wsapi "github.com/gitpod-io/gitpod/ws-manager/api"
workspacev1 "github.com/gitpod-io/gitpod/ws-manager/api/crd/v1"
Expand Down Expand Up @@ -137,6 +138,11 @@ func (r *CRDWorkspaceInfoProvider) Reconcile(ctx context.Context, req ctrl.Reque
if ws.Spec.Admission.Level == workspacev1.AdmissionLevelEveryone {
admission = wsapi.AdmissionLevel_ADMIT_EVERYONE
}
managedByMk2 := true
if managedBy, ok := ws.Labels[wsk8s.WorkspaceManagedByLabel]; ok && managedBy != "ws-manager-mk2" {
managedByMk2 = false
}

wsinfo := &common.WorkspaceInfo{
WorkspaceID: ws.Spec.Ownership.WorkspaceID,
InstanceID: ws.Name,
Expand All @@ -150,9 +156,9 @@ func (r *CRDWorkspaceInfoProvider) Reconcile(ctx context.Context, req ctrl.Reque
StartedAt: ws.CreationTimestamp.Time,
OwnerUserId: ws.Spec.Ownership.Owner,
SSHPublicKeys: ws.Spec.SshPublicKeys,
SSHKey: ws.Spec.SSHKey,
IsRunning: ws.Status.Phase == workspacev1.WorkspacePhaseRunning,
IsEnabledSSHCA: ws.Spec.SSHGatewayCAPublicKey != "",
IsManagedByMk2: managedByMk2,
}

r.store.Update(req.Name, wsinfo)
Expand Down
30 changes: 17 additions & 13 deletions components/ws-proxy/pkg/sshproxy/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ func (s *Server) HandleConn(c net.Conn) {
log.WithField("workspaceId", workspaceId).WithError(err).Error("failed to get workspace info")
return
}
log := log.WithField("instanceId", wsInfo.InstanceID).WithField("isMk2", wsInfo.IsManagedByMk2)
ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
supervisorPort := "22999"
if debugWorkspace {
Expand All @@ -318,37 +319,40 @@ func (s *Server) HandleConn(c net.Conn) {
OwnerUserId: wsInfo.OwnerUserId,
}

if wsInfo.SSHKey != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Was nextgen the only one using wsInfo.SSHKey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

key, err = ssh.ParsePrivateKey([]byte(wsInfo.SSHKey.Private))
if err != nil {
if !wsInfo.IsManagedByMk2 {
if s.caKey == nil || !wsInfo.IsEnabledSSHCA {
err = xerrors.Errorf("workspace not managed by mk2, but didn't have SSH CA enabled")
s.TrackSSHConnection(wsInfo, "connect", ErrCreateSSHKey)
ReportSSHAttemptMetrics(ErrCreateSSHKey)
log.WithError(err).Error("failed to generate ssh cert")
cancel()
return
}

session.WorkspacePrivateKey = key

// obtain the SSH username from workspacekit.
workspacekitPort := "22998"
userName, err = workspaceSSHUsername(ctx, wsInfo.IPAddress, workspacekitPort)
if err != nil {
log.WithField("instanceId", wsInfo.InstanceID).WithError(err).Warn("failed to retrieve the SSH username. Using the default.")
log.WithError(err).Warn("failed to retrieve the SSH username. Using the default.")
}
} else if s.caKey != nil && wsInfo.IsEnabledSSHCA {
}

if s.caKey != nil && wsInfo.IsEnabledSSHCA {
key, err = s.GenerateSSHCert(ctx, userName)
if err != nil {
log.WithField("workspaceId", workspaceId).WithError(err).Error("failed to generate ssh cert")
s.TrackSSHConnection(wsInfo, "connect", ErrCreateSSHKey)
ReportSSHAttemptMetrics(ErrCreateSSHKey)
log.WithError(err).Error("failed to generate ssh cert")
cancel()
return
}

session.WorkspacePrivateKey = key
} else {
key, userName, err = s.GetWorkspaceSSHKey(ctx, wsInfo.IPAddress, supervisorPort)
if err != nil {
cancel()
s.TrackSSHConnection(wsInfo, "connect", ErrCreateSSHKey)
ReportSSHAttemptMetrics(ErrCreateSSHKey)
log.WithField("instanceId", wsInfo.InstanceID).WithError(err).Error("failed to create private pair in workspace")
log.WithError(err).Error("failed to create private pair in workspace")
return
}

Expand All @@ -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("dial failed")
return
}
defer conn.Close()
Expand All @@ -384,7 +388,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("connect failed")
log.WithField("workspaceIP", wsInfo.IPAddress).WithError(err).Error("connect failed")
return
}
s.Heartbeater.SendHeartbeat(wsInfo.InstanceID, false, true)
Expand Down