Skip to content

Fix supervisor panic when tkns and publicapi Service is nil #15274

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 3 commits into from
Dec 12, 2022
Merged

Fix supervisor panic when tkns and publicapi Service is nil #15274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
448ac91
Revert "Revert "[supervisor] use public api in supervisor""
mustard-mh Dec 12, 2022
aaff95b
[supervisor] add global panic handler
akosyakov Dec 9, 2022
e081051
[supervisor] fix imagebuild failed when tkns is nil
mustard-mh Dec 9, 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
11 changes: 2 additions & 9 deletions components/common-go/experiments/configcat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@ package experiments

import (
"context"
"time"

configcat "github.com/configcat/go-sdk/v7"
"github.com/gitpod-io/gitpod/common-go/log"
"github.com/sirupsen/logrus"
)

Expand All @@ -21,14 +19,9 @@ const (
vscodeClientIDAttribute = "vscode_client_id"
)

func newConfigCatClient(sdkKey string) *configCatClient {
func newConfigCatClient(config configcat.Config) *configCatClient {
return &configCatClient{
client: configcat.NewCustomClient(configcat.Config{
SDKKey: sdkKey,
PollInterval: 3 * time.Minute,
HTTPTimeout: 3 * time.Second,
Logger: &configCatLogger{log.Log},
}),
client: configcat.NewCustomClient(config),
}
}

Expand Down
14 changes: 12 additions & 2 deletions components/common-go/experiments/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ package experiments
import "context"

const (
PersonalAccessTokensEnabledFlag = "personalAccessTokensEnabled"
OIDCServiceEnabledFlag = "oidcServiceEnabled"
PersonalAccessTokensEnabledFlag = "personalAccessTokensEnabled"
OIDCServiceEnabledFlag = "oidcServiceEnabled"
SupervisorPersistServerAPIChannelWhenStartFlag = "supervisor_persist_serverapi_channel_when_start"
SupervisorUsePublicAPIFlag = "supervisor_experimental_publicapi"
)

func IsPersonalAccessTokensEnabled(ctx context.Context, client Client, attributes Attributes) bool {
Expand All @@ -18,3 +20,11 @@ func IsPersonalAccessTokensEnabled(ctx context.Context, client Client, attribute
func IsOIDCServiceEnabled(ctx context.Context, client Client, attributes Attributes) bool {
return client.GetBoolValue(ctx, OIDCServiceEnabledFlag, false, attributes)
}

func SupervisorPersistServerAPIChannelWhenStart(ctx context.Context, client Client, attributes Attributes) bool {
return client.GetBoolValue(ctx, SupervisorPersistServerAPIChannelWhenStartFlag, true, attributes)
}

func SupervisorUsePublicAPI(ctx context.Context, client Client, attributes Attributes) bool {
return client.GetBoolValue(ctx, SupervisorUsePublicAPIFlag, false, attributes)
}
22 changes: 20 additions & 2 deletions components/common-go/experiments/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ package experiments

import (
"context"
"fmt"
"os"
"time"

configcat "github.com/configcat/go-sdk/v7"
"github.com/gitpod-io/gitpod/common-go/log"
)

type Client interface {
Expand All @@ -32,10 +37,23 @@ type Attributes struct {
// If the environment contains CONFIGCAT_SDK_KEY value, it vill be used to construct a ConfigCat client.
// Otherwise, it returns a client which always returns the default value. This client is used for Self-Hosted installations.
func NewClient() Client {
gitpodHost := os.Getenv("GITPOD_HOST")
Copy link
Member

Choose a reason for hiding this comment

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

@mustard-mh it is a internal secret of supervisor which is a subject to change, why don't pass it as an argument?

if gitpodHost != "" {
return newConfigCatClient(configcat.Config{
SDKKey: "gitpod",
BaseURL: fmt.Sprintf("%s%s", gitpodHost, "/configcat"),
PollInterval: 1 * time.Minute,
HTTPTimeout: 3 * time.Second,
})
}
sdkKey := os.Getenv("CONFIGCAT_SDK_KEY")
if sdkKey == "" {
return NewAlwaysReturningDefaultValueClient()
}

return newConfigCatClient(sdkKey)
return newConfigCatClient(configcat.Config{
SDKKey: sdkKey,
PollInterval: 3 * time.Minute,
HTTPTimeout: 3 * time.Second,
Logger: &configCatLogger{log.Log},
})
Comment on lines +42 to +58
Copy link
Member

Choose a reason for hiding this comment

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

there's duplication in the config that could be reduced.

}
1 change: 1 addition & 0 deletions components/supervisor/BUILD.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ packages:
- components/supervisor-api/go:lib
- components/ws-daemon-api/go:lib
- components/ide-metrics-api/go:lib
- components/public-api/go:lib
env:
- CGO_ENABLED=0
- GOOS=linux
Expand Down
9 changes: 7 additions & 2 deletions components/supervisor/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,19 @@ go 1.19
require (
github.com/Netflix/go-env v0.0.0-20220526054621-78278af1949d
github.com/c9s/goprocinfo v0.0.0-20210130143923-c95fcf8c64a8
github.com/cenkalti/backoff/v4 v4.1.3
github.com/cenkalti/backoff/v4 v4.2.0
github.com/creack/pty v1.1.11
github.com/fsnotify/fsnotify v1.4.9
github.com/gitpod-io/gitpod/common-go v0.0.0-00010101000000-000000000000
github.com/gitpod-io/gitpod/components/public-api/go v0.0.0-00010101000000-000000000000
github.com/gitpod-io/gitpod/content-service v0.0.0-00010101000000-000000000000
github.com/gitpod-io/gitpod/content-service/api v0.0.0-00010101000000-000000000000
github.com/gitpod-io/gitpod/gitpod-protocol v0.0.0-00010101000000-000000000000
github.com/gitpod-io/gitpod/ide-metrics-api v0.0.0-00010101000000-000000000000
github.com/gitpod-io/gitpod/supervisor/api v0.0.0-00010101000000-000000000000
github.com/gitpod-io/gitpod/ws-daemon/api v0.0.0-00010101000000-000000000000
github.com/golang/mock v1.6.0
github.com/google/go-cmp v0.5.8
github.com/google/go-cmp v0.5.9
github.com/google/uuid v1.3.0
github.com/gorilla/websocket v1.5.0
github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
Expand Down Expand Up @@ -69,8 +70,10 @@ require (
github.com/aws/aws-sdk-go-v2/service/sts v1.17.5 // indirect
github.com/aws/smithy-go v1.13.4 // indirect
github.com/beorn7/perks v1.0.1 // indirect
github.com/blang/semver v3.5.1+incompatible // indirect
github.com/cenkalti/backoff v2.2.1+incompatible // indirect
github.com/cespare/xxhash/v2 v2.1.2 // indirect
github.com/configcat/go-sdk/v7 v7.6.0 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/desertbit/timer v0.0.0-20180107155436-c41aec40b27f // indirect
github.com/dustin/go-humanize v1.0.0 // indirect
Expand Down Expand Up @@ -133,6 +136,8 @@ replace github.com/gitpod-io/gitpod/common-go => ../common-go // leeway

replace github.com/gitpod-io/gitpod/content-service => ../content-service // leeway

replace github.com/gitpod-io/gitpod/components/public-api/go => ../public-api/go // leeway

replace github.com/gitpod-io/gitpod/content-service/api => ../content-service-api/go // leeway

replace github.com/gitpod-io/gitpod/gitpod-protocol => ../gitpod-protocol/go // leeway
Expand Down
16 changes: 12 additions & 4 deletions components/supervisor/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions components/supervisor/pkg/metrics/reporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ func NewGrpcMetricsReporter(gitpodHost string) *GrpcMetricsReporter {
"grpc_server_handling_seconds": true,
"supervisor_ide_ready_duration_total": true,
"supervisor_initializer_bytes_second": true,
"grpc_client_started_total": true,
"grpc_client_handled_total": true,
"grpc_client_handling_seconds": true,
},
values: make(map[string]float64),
addCounter: func(name string, labels map[string]string, value uint64) {
Expand Down
23 changes: 12 additions & 11 deletions components/supervisor/pkg/ports/exposed-ports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
backoff "github.com/cenkalti/backoff/v4"
"github.com/gitpod-io/gitpod/common-go/log"
gitpod "github.com/gitpod-io/gitpod/gitpod-protocol"
"github.com/gitpod-io/gitpod/supervisor/pkg/serverapi"
)

// ExposedPort represents an exposed pprt
Expand Down Expand Up @@ -59,10 +60,10 @@ func (*NoopExposedPorts) Expose(ctx context.Context, local uint32, public bool)
// GitpodExposedPorts uses a connection to the Gitpod server to implement
// the ExposedPortsInterface.
type GitpodExposedPorts struct {
WorkspaceID string
InstanceID string
WorkspaceUrl string
C gitpod.APIInterface
WorkspaceID string
InstanceID string
WorkspaceUrl string
gitpodService serverapi.APIInterface

localExposedPort []uint32
localExposedNotice chan struct{}
Expand All @@ -78,12 +79,12 @@ type exposePortRequest struct {
}

// NewGitpodExposedPorts creates a new instance of GitpodExposedPorts
func NewGitpodExposedPorts(workspaceID string, instanceID string, workspaceUrl string, gitpodService gitpod.APIInterface) *GitpodExposedPorts {
func NewGitpodExposedPorts(workspaceID string, instanceID string, workspaceUrl string, gitpodService serverapi.APIInterface) *GitpodExposedPorts {
return &GitpodExposedPorts{
WorkspaceID: workspaceID,
InstanceID: instanceID,
WorkspaceUrl: workspaceUrl,
C: gitpodService,
WorkspaceID: workspaceID,
InstanceID: instanceID,
WorkspaceUrl: workspaceUrl,
gitpodService: gitpodService,

// allow clients to submit 30 expose requests without blocking
requests: make(chan *exposePortRequest, 30),
Expand Down Expand Up @@ -120,7 +121,7 @@ func (g *GitpodExposedPorts) Observe(ctx context.Context) (<-chan []ExposedPort,
defer close(reschan)
defer close(errchan)

updates, err := g.C.InstanceUpdates(ctx, g.InstanceID)
updates, err := g.gitpodService.InstanceUpdates(ctx, g.InstanceID, g.WorkspaceID)
if err != nil {
errchan <- err
return
Expand Down Expand Up @@ -204,7 +205,7 @@ func (g *GitpodExposedPorts) doExpose(req *exposePortRequest) {
exp.Reset()
attempt := 0
for {
_, err = g.C.OpenPort(req.ctx, g.WorkspaceID, req.port)
_, err = g.gitpodService.OpenPort(req.ctx, g.WorkspaceID, req.port)
if err == nil || req.ctx.Err() != nil || attempt == 5 {
return
}
Expand Down
4 changes: 1 addition & 3 deletions components/supervisor/pkg/ports/ports-config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,13 @@ type ConfigInterace interface {
type ConfigService struct {
workspaceID string
configService config.ConfigInterface
gitpodAPI gitpod.APIInterface
}

// NewConfigService creates a new instance of ConfigService.
func NewConfigService(workspaceID string, configService config.ConfigInterface, gitpodAPI gitpod.APIInterface) *ConfigService {
func NewConfigService(workspaceID string, configService config.ConfigInterface) *ConfigService {
return &ConfigService{
workspaceID: workspaceID,
configService: configService,
gitpodAPI: gitpodAPI,
}
}

Expand Down
4 changes: 1 addition & 3 deletions components/supervisor/pkg/ports/ports-config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,7 @@ func TestPortsConfig(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

gitpodAPI := gitpod.NewMockAPIInterface(ctrl)

service := NewConfigService(workspaceID, configService, gitpodAPI)
service := NewConfigService(workspaceID, configService)
updates, errors := service.Observe(context)

actual := &PortConfigTestExpectations{}
Expand Down
Loading