Skip to content

Add workspaces_enabled toggle to gate the control-room workspaces step #364

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
stevenolen merged 1 commit into from
Aug 12, 2026
Merged

Add workspaces_enabled toggle to gate the control-room workspaces step #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 24 additions & 0 deletions lib/steps/workspaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package steps
import (
"context"
"fmt"
"log/slog"

"github.com/posit-dev/ptd/lib/helpers"
"github.com/posit-dev/ptd/lib/types"
)

Expand Down Expand Up @@ -38,6 +40,28 @@ func (s *WorkspacesStep) Run(ctx context.Context) error {
return fmt.Errorf("workspaces step can only be run on control room targets")
}

// The AWS WorkSpaces environment is opt-out per control room via the
// `workspaces_enabled` config toggle (default on). When a control room sets
// `workspaces_enabled: false`, the whole step is a no-op. This gate is loaded and
// evaluated before any credential fetch or Pulumi stack creation so it applies to
// BOTH the apply and destroy paths: a `--destroy` on a control room that has the
// toggle off is also a no-op (once the stack has been destroyed it stays gone).
// Config is loaded the same way runAWSInlineGo loads it (helpers.ConfigForTarget +
// type-assert to AWSControlRoomConfig), which is the only control-room config kind.
rawConfig, err := helpers.ConfigForTarget(s.DstTarget)

@claude claude Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

runAWSInlineGo (in workspaces_aws.go:49-56) does the exact same ConfigForTarget + type-assert, so the config is parsed twice on the apply path. The struct is cheap to parse, so this isn't a performance issue, but it's a consistency hazard: if the type-assert error message or the config-load error format ever diverges between the two call sites, debugging gets confusing.

Not blocking, but worth noting if this pattern gets copied further.

if err != nil {
return fmt.Errorf("workspaces: failed to load config: %w", err)
}
cfg, ok := rawConfig.(types.AWSControlRoomConfig)
if !ok {
return fmt.Errorf("workspaces: expected AWSControlRoomConfig, got %T", rawConfig)
}
if !cfg.WorkspacesIsEnabled() {
slog.Info("skipping workspaces step: workspaces_enabled is false for this control room",
"target", s.DstTarget.Name())
return nil
}

@claude claude Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The destroy path has a silent-orphan risk: if a control room already has an active WorkSpaces Pulumi stack (real AWS resources) and someone then sets workspaces_enabled: false, any subsequent ptd ensure --destroy will hit this early return and leave those resources in AWS forever — the stack is never touched again.

The comment says "once the stack has been destroyed it stays gone", which assumes the operator already tore down before toggling, but that ordering isn't enforced. Consider at minimum a warning log when Destroy is set in Options and the gate fires:

if !cfg.WorkspacesIsEnabled() {
    if s.Options.Destroy {
        slog.Warn("workspaces step disabled but --destroy was requested; existing WorkSpaces stack will NOT be destroyed",
            "target", s.DstTarget.Name())
    } else {
        slog.Info("skipping workspaces step: workspaces_enabled is false for this control room",
            "target", s.DstTarget.Name())
    }
    return nil
}


creds, err := s.DstTarget.Credentials(ctx)
if err != nil {
return err
Expand Down
63 changes: 63 additions & 0 deletions lib/steps/workspaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,20 @@ package steps

import (
"context"
"errors"
"os"
"path/filepath"
"sync"
"testing"

"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/pulumi/pulumi/sdk/v3/go/pulumi"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"

"github.com/posit-dev/ptd/lib/helpers"
"github.com/posit-dev/ptd/lib/types"
"github.com/posit-dev/ptd/lib/types/typestest"
)
Expand Down Expand Up @@ -457,3 +463,60 @@ func TestWorkspacesStepNotControlRoom(t *testing.T) {
err := step.Run(context.Background())
assert.ErrorContains(t, err, "control room")
}

// writeControlRoomConfig writes a control-room ptd.yaml with the given spec body
// into a temp targets dir and points viper at it for the duration of the test, so
// helpers.ConfigForTarget(target) loads it. specBody is the YAML under `spec:`.
func writeControlRoomConfig(t *testing.T, name, specBody string) {
t.Helper()
dir := t.TempDir()
crDir := filepath.Join(dir, helpers.CtrlDir, name)
require.NoError(t, os.MkdirAll(crDir, 0o755))
body := "apiVersion: ptd/v1\nkind: AWSControlRoomConfig\nspec:\n" + specBody
require.NoError(t, os.WriteFile(filepath.Join(crDir, "ptd.yaml"), []byte(body), 0o644))

orig := viper.GetString("targets_config_dir")
viper.Set("targets_config_dir", dir)
t.Cleanup(func() { viper.Set("targets_config_dir", orig) })
}

// TestWorkspacesStepDisabledNoOp verifies that a control room with
// workspaces_enabled: false cleanly no-ops: Run returns nil and never fetches
// credentials or creates a Pulumi stack. This gate covers both the apply and
// destroy paths, since it precedes any credential fetch or stack creation.
func TestWorkspacesStepDisabledNoOp(t *testing.T) {
writeControlRoomConfig(t, "main01-staging", " workspaces_enabled: false\n")

tgt := mockAWSControlRoomTarget("main01-staging")

step := &WorkspacesStep{}
step.Set(tgt, nil, StepOptions{})
err := step.Run(context.Background())

require.NoError(t, err)
// The gate must short-circuit before any credential fetch (and therefore
// before any Pulumi stack create/preview/destroy).
tgt.AssertNotCalled(t, "Credentials")
}

// TestWorkspacesStepEnabledGetsPastGate verifies that a control room with the
// toggle unset (default on) does NOT short-circuit: Run proceeds past the gate and
// attempts to fetch credentials. Credentials are stubbed to fail so the test does
// not need real cloud/Pulumi wiring; asserting Credentials is called proves the
// gate did not skip the step.
func TestWorkspacesStepEnabledGetsPastGate(t *testing.T) {
// No workspaces_enabled key => nil => WorkspacesIsEnabled() true (default on).
writeControlRoomConfig(t, "main01-staging", " region: us-east-1\n")

tgt := mockAWSControlRoomTarget("main01-staging")
tgt.On("Credentials", mock.Anything).
Return(typestest.DefaultCredentials(), errors.New("creds unavailable in unit test"))

step := &WorkspacesStep{}
step.Set(tgt, nil, StepOptions{})
err := step.Run(context.Background())

// It got past the gate and tried (and failed) to fetch credentials.
require.ErrorContains(t, err, "creds unavailable in unit test")
tgt.AssertCalled(t, "Credentials", mock.Anything)
}
18 changes: 18 additions & 0 deletions lib/types/controlroom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,24 @@ type AWSControlRoomConfig struct {
TraefikForwardAuthVersion string `json:"traefik_forward_auth_version" yaml:"traefik_forward_auth_version"`
TraefikVersion string `json:"traefik_version" yaml:"traefik_version"`
EbsCsiAddonVersion string `json:"ebs_csi_addon_version" yaml:"ebs_csi_addon_version"`
// WorkspacesEnabled toggles the AWS WorkSpaces (control-room `workspaces`) step.
// It is a pointer so an absent field can be distinguished from an explicit false.
// The historical behavior is always-on, so an unset value (nil) must resolve to
// true; only an explicit `workspaces_enabled: false` disables the step. Resolve
// via WorkspacesIsEnabled (nil → true).
WorkspacesEnabled *bool `json:"workspaces_enabled" yaml:"workspaces_enabled"`
}

// WorkspacesIsEnabled resolves the WorkspacesEnabled flag (default true). Returns
// true when the field is unset (nil) or explicitly true; returns false only when
// WorkspacesEnabled is explicitly set to false. The nil → true default preserves
// the historical always-on behavior of the AWS WorkSpaces step, mirroring how
// EKSAccessEntriesConfig.IsEnabled treats an absent flag as enabled.
func (c AWSControlRoomConfig) WorkspacesIsEnabled() bool {

@claude claude Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor inconsistency with EKSAccessEntriesConfig.IsEnabled() (line 21), which has a pointer receiver (c *EKSAccessEntriesConfig) that also handles c == nil. WorkspacesIsEnabled uses a value receiver (c AWSControlRoomConfig), which is fine since AWSControlRoomConfig is never used via pointer here, but the parallel drawn in the doc comment is slightly misleading — the nil-receiver guard in IsEnabled doesn't apply here.

if c.WorkspacesEnabled == nil {
return true
}
return *c.WorkspacesEnabled
}

// The following accessor methods return the value from config, or the Python
Expand Down
Loading