diff --git a/.kportal.yaml b/.kportal.yaml index 9f28c90..b2101a1 100644 --- a/.kportal.yaml +++ b/.kportal.yaml @@ -22,6 +22,35 @@ reliability: retryOnStale: true # Automatically reconnect when stale connections detected (default: true) watchdogPeriod: "30s" # Goroutine watchdog check interval to detect hung workers (default: 30s) +# Every context below is forwarded by default. Pass --context to run only some +# of them, for example: +# +# kportal --context staging +# kportal --context production,staging # repeatable, or comma-separated +# +# Local-port conflicts are only reported between forwards that could run at the +# same time, so contexts you never select together may reuse the same +# localPort - handy when the same service exists in several clusters and you +# want it on a fixed local port in each: +# +# contexts: +# - name: development-team-a +# namespaces: +# - name: rate-service +# forwards: +# - resource: pod/rpc-server +# protocol: tcp +# port: 50051 +# localPort: 3004 # same port as team-b below +# - name: development-team-b +# namespaces: +# - name: rate-service +# forwards: +# - resource: pod/rpc-server +# protocol: tcp +# port: 50051 +# localPort: 3004 # only valid when one of them is selected + # Optional: TUI table configuration # Controls which columns are visible in the forwards table, in what order, # and how wide each one renders. Omit this section (or leave it empty) to diff --git a/README.md b/README.md index 93dbc69..26f851b 100644 --- a/README.md +++ b/README.md @@ -302,6 +302,65 @@ kportal --check kportal -c /path/to/config.yaml ``` +### Select Contexts + +By default every context in the config file is forwarded. `--context` limits the +run to the contexts you name, which is useful when the file describes several +clusters but you only work against one at a time: + +```bash +kportal --context development-team-a +``` + +The flag is repeatable and accepts comma-separated values, so these are +equivalent: + +```bash +kportal --context team-a --context team-b +kportal --context team-a,team-b +``` + +It composes with the other modes, and works well as a per-cluster shell alias: + +```bash +alias kp-a='kportal --context development-team-a' +kportal --context development-team-a -headless +``` + +An unknown context name is an error that lists the contexts the file defines, so +a typo fails loudly instead of silently forwarding nothing. + +#### Reusing local ports across contexts + +Local-port conflicts are only reported between forwards that could run at the +same time. Contexts that are never selected together may therefore reuse the +same `localPort`, so the same service keeps the same local port in every +cluster and the app you are developing never needs reconfiguring: + +```yaml +contexts: + - name: development-team-a + namespaces: + - name: rate-service + forwards: + - &rate-service + resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + alias: rate-service + + - name: development-team-b + namespaces: + - name: rate-service + forwards: + - <<: *rate-service # same local port, no conflict +``` + +This requires passing `--context`. Without it every context is active, so the +duplicate ports above would genuinely collide and are still reported as an +error - as is naming both contexts at once (`--context team-a,team-b`). + ### Generate Forwards from a Cluster The `generate` subcommand discovers services in a Kubernetes context and lets you @@ -449,6 +508,11 @@ kill -HUP $(pgrep kportal) kportal validates port availability at startup and during hot-reload, showing which process is using conflicting ports. +Conflicts are only reported between forwards that could be running together. +With `--context`, forwards in contexts outside the selection are ignored, which +is what lets each context reuse the same `localPort` values - see +[Reusing local ports across contexts](#reusing-local-ports-across-contexts). + ### Retry Strategy Exponential backoff: 1s → 2s → 4s → 8s → 10s (max). Retries continue indefinitely until connection succeeds. @@ -489,6 +553,11 @@ Context names containing `@`, `.`, `:`, or `/` (e.g. `admin@home`, `user@cluster.example.com`, GKE dotted names, EKS ARNs) are accepted by the config validator. +If `--context` reports `unknown context "..."`, the name does not match any +`contexts[].name` in the config file. The error lists the names that are +defined - note these are the contexts in your `.kportal.yaml`, which need not be +every context in your kubeconfig. + ## 🔧 Development ### Prerequisites diff --git a/cmd/kportal/main.go b/cmd/kportal/main.go index 77a824f..eb9c19f 100644 --- a/cmd/kportal/main.go +++ b/cmd/kportal/main.go @@ -51,6 +51,7 @@ type runOptions struct { logFormat string convertInput string convertOutput string + contexts []string verbose bool headless bool check bool @@ -58,6 +59,28 @@ type runOptions struct { checkUpdate bool } +// contextListFlag collects repeated -context values. Each value may itself be a +// comma-separated list, so "-context a -context b" and "-context a,b" are +// equivalent. +type contextListFlag []string + +func (c *contextListFlag) String() string { + if c == nil { + return "" + } + return strings.Join(*c, ",") +} + +func (c *contextListFlag) Set(value string) error { + // Reject an empty value rather than silently forwarding every context - + // "-context=$UNSET_VAR" in a script should fail loudly. + if strings.TrimSpace(value) == "" { + return fmt.Errorf("context name cannot be empty") + } + *c = append(*c, value) + return nil +} + // fprintf is a small wrapper that suppresses the io.Writer write error. We // route output to caller-provided writers (stdout / stderr / io.Discard / // bytes.Buffer in tests), and a write error on any of these is non-actionable @@ -166,13 +189,31 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. return code } - // Validate configuration (allow empty for newly created files). + // Validate the whole configuration - including contexts that -context + // excludes - so problems anywhere in the file are still reported. Only the + // checks that depend on what actually runs (local ports, mDNS hostnames) + // are scoped to the selection. validator := config.NewValidator() - if errs := validator.ValidateConfigWithOptions(cfg, configIsNew || cfg.IsEmpty()); len(errs) > 0 { + if errs := validator.ValidateConfigWithOpts(cfg, config.ValidateOptions{ + AllowEmpty: configIsNew || cfg.IsEmpty(), + ActiveContexts: opts.contexts, + }); len(errs) > 0 { fprint(stderr, config.FormatValidationErrors(errs)) return 1 } + // Narrow the config to the selected contexts for everything downstream. + // Skipped for a config with no contexts at all (a file just created above), + // where reporting an unknown context would be unhelpful. + if len(opts.contexts) > 0 && len(cfg.Contexts) > 0 { + selected, err := cfg.SelectContexts(opts.contexts) + if err != nil { + fprintf(stderr, "Error: %v\n", err) + return 1 + } + cfg = selected + } + if opts.check { fprintln(stdout, "Configuration is valid") return 0 @@ -192,9 +233,9 @@ func run(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io. switch { case opts.headless: - return runHeadless(ctx, opts, cfg, deps, validator, stderr) + return runHeadless(ctx, opts, cfg, deps, stderr) case opts.verbose: - return runVerboseTable(ctx, opts, cfg, deps, validator, stderr) + return runVerboseTable(ctx, opts, cfg, deps, stderr) default: return runInteractive(ctx, opts, cfg, deps, stderr) } @@ -208,6 +249,8 @@ func parseFlags(args []string, stderr io.Writer) (runOptions, int, bool) { fs.SetOutput(stderr) var opts runOptions + var contexts contextListFlag + fs.Var(&contexts, "context", "Only forward the named contexts from the config file (repeatable and comma-separated; default: all)") fs.StringVar(&opts.configFile, "c", defaultConfigFile, "Path to configuration file") fs.BoolVar(&opts.verbose, "v", false, "Enable verbose logging") fs.BoolVar(&opts.headless, "headless", false, "Run in headless mode (no UI, for background/daemon use)") @@ -224,6 +267,7 @@ func parseFlags(args []string, stderr io.Writer) (runOptions, int, bool) { } return opts, 2, true } + opts.contexts = config.NormalizeContextSelection(contexts) return opts, 0, false } @@ -335,6 +379,30 @@ func loadOrCreateConfig(configFile string, stdin io.Reader, stdout, stderr io.Wr return cfg, true, 0, false } +// reloadFromSignal re-reads the config after SIGHUP and applies it to the +// manager. Load, validation and the -context selection all live in +// config.LoadForRuntime, shared with the file watcher, so no reload path can +// forget the selection. Failures are logged and the previous config stays +// active. onApplied, when non-nil, runs only after a successful Reload. +func reloadFromSignal(opts runOptions, manager *forward.Manager, logIt bool, onApplied func(*config.Config)) { + newCfg, err := config.LoadForRuntime(opts.configFile, opts.contexts) + if err != nil { + if logIt { + log.Printf("Failed to reload config: %v", err) + } + return + } + if err := manager.Reload(newCfg); err != nil { + if logIt { + log.Printf("Failed to reload: %v", err) + } + return + } + if onApplied != nil { + onApplied(newCfg) + } +} + // runtimeDeps bundles the long-lived objects shared by all UI modes. type runtimeDeps struct { manager *forward.Manager @@ -355,6 +423,7 @@ func buildRuntimeDeps(opts runOptions, cfg *config.Config, stderr io.Writer) (*r } discovery := k8s.NewDiscovery(pool) mutator := config.NewMutator(opts.configFile) + mutator.SetActiveContexts(opts.contexts) manager, err := forward.NewManager(opts.verbose) if err != nil { @@ -429,7 +498,7 @@ func runConvert(input, output string, stdout, stderr io.Writer) int { // runHeadless runs the daemon-style mode: no UI, signal-driven SIGHUP reloads, // graceful shutdown on ctx.Done() (which is cancelled by SIGINT/SIGTERM). -func runHeadless(ctx context.Context, opts runOptions, cfg *config.Config, deps *runtimeDeps, validator *config.Validator, stderr io.Writer) int { +func runHeadless(ctx context.Context, opts runOptions, cfg *config.Config, deps *runtimeDeps, stderr io.Writer) int { if startErr := deps.manager.Start(cfg); startErr != nil { fprintf(stderr, "Error starting forwards: %v\n", startErr) return 1 @@ -442,7 +511,7 @@ func runHeadless(ctx context.Context, opts runOptions, cfg *config.Config, deps watcher, watcherErr := config.NewWatcher(opts.configFile, func(newCfg *config.Config) error { return deps.manager.Reload(newCfg) - }, opts.verbose) + }, opts.verbose, config.WithContextSelection(opts.contexts)) watcherStarted := false if watcherErr != nil { if opts.verbose { @@ -471,32 +540,14 @@ func runHeadless(ctx context.Context, opts runOptions, cfg *config.Config, deps if opts.verbose { log.Printf("Received SIGHUP, reloading configuration...") } - newCfg, loadErr := config.LoadConfig(opts.configFile) - if loadErr != nil { - if opts.verbose { - log.Printf("Failed to reload config: %v", loadErr) - } - continue - } - if errs := validator.ValidateConfig(newCfg); len(errs) > 0 { - if opts.verbose { - log.Printf("Config validation failed:") - log.Print(config.FormatValidationErrors(errs)) - } - continue - } - if reloadErr := deps.manager.Reload(newCfg); reloadErr != nil { - if opts.verbose { - log.Printf("Failed to reload: %v", reloadErr) - } - } + reloadFromSignal(opts, deps.manager, opts.verbose, nil) } } } // runVerboseTable runs the simple table UI with periodic redraws and SIGHUP // reload, exiting cleanly when ctx is cancelled. -func runVerboseTable(ctx context.Context, opts runOptions, cfg *config.Config, deps *runtimeDeps, validator *config.Validator, stderr io.Writer) int { +func runVerboseTable(ctx context.Context, opts runOptions, cfg *config.Config, deps *runtimeDeps, stderr io.Writer) int { tableUI := ui.NewTableUI(opts.verbose, cfg) deps.manager.SetStatusUI(tableUI) @@ -544,7 +595,7 @@ func runVerboseTable(ctx context.Context, opts runOptions, cfg *config.Config, d } tableUI.SetColumns(newCfg) return nil - }, opts.verbose) + }, opts.verbose, config.WithContextSelection(opts.contexts)) watcherActive := false if watchErr != nil { log.Printf("Warning: Failed to setup config watcher: %v", watchErr) @@ -568,21 +619,7 @@ func runVerboseTable(ctx context.Context, opts runOptions, cfg *config.Config, d return shutdownManager(ctx, deps.manager, opts.verbose) case <-sigChan: log.Printf("Received SIGHUP, reloading configuration...") - newCfg, loadErr := config.LoadConfig(opts.configFile) - if loadErr != nil { - log.Printf("Failed to reload config: %v", loadErr) - continue - } - if errs := validator.ValidateConfig(newCfg); len(errs) > 0 { - log.Printf("Config validation failed:") - log.Print(config.FormatValidationErrors(errs)) - continue - } - if reloadErr := deps.manager.Reload(newCfg); reloadErr != nil { - log.Printf("Failed to reload: %v", reloadErr) - continue - } - tableUI.SetColumns(newCfg) + reloadFromSignal(opts, deps.manager, true, tableUI.SetColumns) } } } @@ -598,6 +635,7 @@ func runInteractive(ctx context.Context, opts runOptions, cfg *config.Config, de }, appVersion) bubbleTeaUI.SetColumns(cfg) bubbleTeaUI.SetWizardDependencies(deps.discovery, deps.mutator, opts.configFile) + bubbleTeaUI.SetActiveContexts(opts.contexts) bubbleTeaUI.SetHTTPLogSubscriber(makeHTTPLogSubscriber(deps.manager)) go func() { @@ -623,7 +661,7 @@ func runInteractive(ctx context.Context, opts runOptions, cfg *config.Config, de } bubbleTeaUI.SetColumns(newCfg) return nil - }, opts.verbose) + }, opts.verbose, config.WithContextSelection(opts.contexts)) if err == nil { watcher.Start() } diff --git a/cmd/kportal/run_test.go b/cmd/kportal/run_test.go index 5aa79d0..5e6d266 100644 --- a/cmd/kportal/run_test.go +++ b/cmd/kportal/run_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/lukaszraczylo/kportal/internal/config" "github.com/lukaszraczylo/kportal/internal/forward" "github.com/lukaszraczylo/kportal/internal/ui" "github.com/lukaszraczylo/kportal/internal/version" @@ -545,6 +546,7 @@ func TestParseFlags_Defaults(t *testing.T) { assert.False(t, opts.verbose) assert.False(t, opts.headless) assert.Equal(t, "text", opts.logFormat) + assert.Empty(t, opts.contexts, "no -context means every context is forwarded") } func TestParseFlags_AllSet(t *testing.T) { @@ -651,3 +653,190 @@ func TestLoadOrCreateConfig_NotFound_AcceptCreates(t *testing.T) { require.NotNil(t, cfg) assert.FileExists(t, cfgPath) } + +// ---- -context selection ---- + +func TestParseFlags_ContextSelection(t *testing.T) { + tests := []struct { + name string + args []string + want []string + }{ + {name: "single", args: []string{"-context", "team-a"}, want: []string{"team-a"}}, + {name: "repeated", args: []string{"-context", "team-a", "-context", "team-b"}, want: []string{"team-a", "team-b"}}, + {name: "comma separated", args: []string{"-context", "team-a,team-b"}, want: []string{"team-a", "team-b"}}, + {name: "equals form", args: []string{"-context=team-a,team-b"}, want: []string{"team-a", "team-b"}}, + {name: "double dash", args: []string{"--context", "team-a"}, want: []string{"team-a"}}, + { + name: "repeated and comma mixed, de-duplicated", + args: []string{"-context", "team-a,team-b", "-context", "team-a", "-context", " team-c "}, + want: []string{"team-a", "team-b", "team-c"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var stderr bytes.Buffer + opts, code, handled := parseFlags(tt.args, &stderr) + require.False(t, handled) + assert.Equal(t, 0, code) + assert.Equal(t, tt.want, opts.contexts) + }) + } +} + +// TestParseFlags_EmptyContextRejected keeps "-context=$UNSET_VAR" in a script +// from silently degrading to "forward everything". +func TestParseFlags_EmptyContextRejected(t *testing.T) { + var stderr bytes.Buffer + _, code, handled := parseFlags([]string{"-context="}, &stderr) + assert.True(t, handled) + assert.Equal(t, 2, code) +} + +// sharedPortConfigBody is the layout from issue #80: the same service on the +// same local port in two per-team clusters. +const sharedPortConfigBody = `contexts: + - name: team-a + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + - name: team-b + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 +` + +func writeSharedPortConfig(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), ".kportal.yaml") + require.NoError(t, os.WriteFile(path, []byte(sharedPortConfigBody), 0600)) + return path +} + +// TestRun_CheckWithContextSelection is the end-to-end proof of issue #80: +// selecting a single context makes a config that reuses local ports valid. +func TestRun_CheckWithContextSelection(t *testing.T) { + path := writeSharedPortConfig(t) + + t.Run("without selection the shared port is a conflict", func(t *testing.T) { + var stdout, stderr bytes.Buffer + code := run(context.Background(), []string{"-c", path, "-check"}, strings.NewReader(""), &stdout, &stderr) + assert.Equal(t, 1, code) + assert.Contains(t, stderr.String(), "Duplicate local port 3004") + }) + + t.Run("selecting one context is valid", func(t *testing.T) { + var stdout, stderr bytes.Buffer + code := run(context.Background(), []string{"-c", path, "-check", "-context", "team-a"}, strings.NewReader(""), &stdout, &stderr) + assert.Equal(t, 0, code, "stderr: %s", stderr.String()) + assert.Contains(t, stdout.String(), "Configuration is valid") + }) + + t.Run("selecting both brings the conflict back", func(t *testing.T) { + var stdout, stderr bytes.Buffer + code := run(context.Background(), []string{"-c", path, "-check", "-context", "team-a,team-b"}, strings.NewReader(""), &stdout, &stderr) + assert.Equal(t, 1, code) + assert.Contains(t, stderr.String(), "Duplicate local port 3004") + }) +} + +// TestRun_CheckUnknownContext verifies the typo guard. +func TestRun_CheckUnknownContext(t *testing.T) { + path := writeSharedPortConfig(t) + + var stdout, stderr bytes.Buffer + code := run(context.Background(), []string{"-c", path, "-check", "-context", "team-z"}, strings.NewReader(""), &stdout, &stderr) + assert.Equal(t, 1, code) + assert.Contains(t, stderr.String(), `unknown context "team-z"`) + // The message should point at what does exist. + assert.Contains(t, stderr.String(), "team-a") + assert.Contains(t, stderr.String(), "team-b") +} + +// TestRun_CheckContextWithNewlyCreatedConfig makes sure -context does not turn +// the create-on-missing flow into an "unknown context" dead end. +func TestRun_CheckContextWithNewlyCreatedConfig(t *testing.T) { + path := filepath.Join(t.TempDir(), ".kportal.yaml") + + var stdout, stderr bytes.Buffer + code := run(context.Background(), []string{"-c", path, "-check", "-context", "team-a"}, strings.NewReader("y\n"), &stdout, &stderr) + assert.Equal(t, 0, code, "stderr: %s", stderr.String()) + assert.Contains(t, stdout.String(), "Configuration is valid") +} + +// TestRun_CheckValidatesNonSelectedContexts proves -check did not get weaker: +// a broken forward outside the selection is still reported. +func TestRun_CheckValidatesNonSelectedContexts(t *testing.T) { + path := filepath.Join(t.TempDir(), ".kportal.yaml") + body := `contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/ok + protocol: tcp + port: 80 + localPort: 8080 + - name: team-b + namespaces: + - name: default + forwards: + - resource: pod/bad + protocol: tcp + port: 80 + localPort: 999999 +` + require.NoError(t, os.WriteFile(path, []byte(body), 0600)) + + var stdout, stderr bytes.Buffer + code := run(context.Background(), []string{"-c", path, "-check", "-context", "team-a"}, strings.NewReader(""), &stdout, &stderr) + assert.Equal(t, 1, code) + assert.Contains(t, stderr.String(), "localPort") +} + +// ---- reloadFromSignal ---- + +func TestReloadFromSignal_OnAppliedRunsAfterSuccessfulReload(t *testing.T) { + mgr, err := forward.NewManager(false) + require.NoError(t, err) + t.Cleanup(mgr.Stop) + cfgPath := writeYAML(t, "r.yaml", `tui: + columns: + - name: alias +contexts: + - name: dev + namespaces: + - name: default + forwards: + - resource: pod/app + port: 8080 + localPort: 38080 +`) + + var got *config.Config + reloadFromSignal(runOptions{configFile: cfgPath}, mgr, false, func(c *config.Config) { got = c }) + + require.NotNil(t, got) + require.Len(t, got.GetTableColumns(), 1) + assert.Equal(t, "alias", got.GetTableColumns()[0].Name) +} + +func TestReloadFromSignal_OnAppliedSkippedWhenConfigInvalid(t *testing.T) { + mgr, err := forward.NewManager(false) + require.NoError(t, err) + cfgPath := writeYAML(t, "bad.yaml", "tui:\n columns:\n - name: bogus\ncontexts: []\n") + + called := false + reloadFromSignal(runOptions{configFile: cfgPath}, mgr, false, func(*config.Config) { called = true }) + + assert.False(t, called) +} diff --git a/docs/index.html b/docs/index.html index 7555f83..fcf8134 100644 --- a/docs/index.html +++ b/docs/index.html @@ -667,6 +667,14 @@

Use a custom configuration file instead of .kportal.yaml

+
+

Select Contexts

+
+ kportal --context development-team-a +
+
+

Forward only the contexts you name instead of all of them. Repeatable and comma-separated. Contexts never selected together can reuse the same local ports.

+

Headless Mode

diff --git a/internal/complete/completion.go b/internal/complete/completion.go index 1e134a9..1a4d6f7 100644 --- a/internal/complete/completion.go +++ b/internal/complete/completion.go @@ -231,7 +231,7 @@ _kportal() # Top-level options if [[ "$cur" == -* ]]; then - COMPREPLY=( $(compgen -W "-c -v --check --headless --log-format --version --update --convert --convert-output" -- "$cur") ) + COMPREPLY=( $(compgen -W "-c -v --check --context --headless --log-format --version --update --convert --convert-output" -- "$cur") ) return fi @@ -258,6 +258,7 @@ func generateZsh() (string, error) { // these are literals (single source of truth — keep in sync with the CLI flags). flagDescs := []string{ `'-c[Path to configuration file]:config file:_files -g "*.yaml"'`, + `'*--context[Only forward the named contexts from the config file]:context:($(kubectl config get-contexts -o name 2>/dev/null))'`, `'-v[Enable verbose logging]'`, `'--version[Show version and exit]'`, `'--update[Check for updates]'`, @@ -355,6 +356,7 @@ complete -c kportal -s v -d 'Enable verbose logging' complete -c kportal -l version -d 'Show version and exit' complete -c kportal -l update -d 'Check for updates' complete -c kportal -l check -d 'Validate configuration' +complete -c kportal -n 'not __fish_seen_subcommand_from generate completion' -l context -r -d 'Only forward the named contexts' -a '(kubectl config get-contexts -o name 2>/dev/null)' -f complete -c kportal -l headless -d 'Run without UI' complete -c kportal -l log-format -d 'Log format' -a 'text json' -f complete -c kportal -l convert -r -f -a '( __fish_complete_suffix .json )' -d 'Convert kftray config' diff --git a/internal/complete/completion_test.go b/internal/complete/completion_test.go index b05eae9..cf0f45d 100644 --- a/internal/complete/completion_test.go +++ b/internal/complete/completion_test.go @@ -251,6 +251,7 @@ func TestAllFlagsPresent(t *testing.T) { "--version", "--update", "--check", + "--context", "--headless", "--log-format", "--convert", diff --git a/internal/config/mutator.go b/internal/config/mutator.go index b8f3f97..978add5 100644 --- a/internal/config/mutator.go +++ b/internal/config/mutator.go @@ -13,8 +13,9 @@ import ( // All operations use atomic file writes (write to temp, then rename) to prevent // corruption and ensure the file watcher picks up changes. type Mutator struct { - configPath string - mu sync.Mutex // Ensure only one mutation at a time + configPath string + activeContexts []string + mu sync.Mutex // Ensure only one mutation at a time } // NewMutator creates a new configuration mutator for the given config file path. @@ -24,6 +25,24 @@ func NewMutator(configPath string) *Mutator { } } +// SetActiveContexts records which contexts are currently being forwarded, as +// selected with --context. Local-port conflicts are then only reported between +// forwards that could actually run at the same time, so editing a forward is +// not blocked by a port that is only used by a context you are not running. +// +// Validation still covers the whole file, and the whole file is still written +// back, so a config saved here always reloads cleanly. +func (m *Mutator) SetActiveContexts(names []string) { + m.mu.Lock() + defer m.mu.Unlock() + m.activeContexts = names +} + +// activeContextsLocked returns the active selection. Callers must hold m.mu. +func (m *Mutator) activeContextsLocked() []string { + return m.activeContexts +} + // findOrCreateContext finds an existing context or creates a new one func (m *Mutator) findOrCreateContext(cfg *Config, contextName string) *Context { for i := range cfg.Contexts { @@ -82,20 +101,28 @@ func (m *Mutator) AddForward(contextName, namespaceName string, fwd Forward) err // Set context/namespace on the forward for validation fwd.SetContext(contextName, namespaceName) - // Check for duplicate local port + // Check for a duplicate local port among forwards that could run alongside + // this one. A port used only by a context that is never active at the same + // time is free to reuse. + active := m.activeContextsLocked() allForwards := cfg.GetAllForwards() for _, existing := range allForwards { - if existing.LocalPort == fwd.LocalPort { - return fmt.Errorf("port %d is already in use by %s", fwd.LocalPort, existing.String()) + if existing.LocalPort != fwd.LocalPort { + continue } + if !PortsCanConflict(existing.GetContext(), contextName, active) { + continue + } + return fmt.Errorf("port %d is already in use by %s", fwd.LocalPort, existing.String()) } // Add the forward targetNamespace.Forwards = append(targetNamespace.Forwards, fwd) - // Validate the new configuration + // Validate the whole configuration - including contexts that are not + // currently active - so nothing invalid is ever written to disk. validator := NewValidator() - if errs := validator.ValidateConfig(cfg); len(errs) > 0 { + if errs := validator.ValidateConfigWithOpts(cfg, ValidateOptions{ActiveContexts: active}); len(errs) > 0 { return fmt.Errorf("validation failed: %s", FormatValidationErrors(errs)) } @@ -150,7 +177,7 @@ func (m *Mutator) RemoveForwards(predicate func(ctx, ns string, fwd Forward) boo // Validate the new configuration validator := NewValidator() - if errs := validator.ValidateConfig(cfg); len(errs) > 0 { + if errs := validator.ValidateConfigWithOpts(cfg, ValidateOptions{ActiveContexts: m.activeContextsLocked()}); len(errs) > 0 { return fmt.Errorf("validation failed: %s", FormatValidationErrors(errs)) } @@ -159,8 +186,23 @@ func (m *Mutator) RemoveForwards(predicate func(ctx, ns string, fwd Forward) boo } // RemoveForwardByID removes a specific forward by its ID. +// +// Forward.ID() is only "alias:localPort" when an alias is set, which is not +// unique across contexts once contexts are allowed to reuse local ports. +// Prefer RemoveForwardByIDInContext, which cannot match a forward in a context +// the caller did not mean. func (m *Mutator) RemoveForwardByID(id string) error { + return m.RemoveForwardByIDInContext("", id) +} + +// RemoveForwardByIDInContext removes a forward by ID from a single context. +// An empty contextName matches the ID in every context, preserving the +// behaviour of RemoveForwardByID. +func (m *Mutator) RemoveForwardByIDInContext(contextName, id string) error { return m.RemoveForwards(func(ctx, ns string, fwd Forward) bool { + if contextName != "" && ctx != contextName { + return false + } return fwd.ID() == id }) } @@ -170,6 +212,17 @@ func (m *Mutator) RemoveForwardByID(id string) error { // If the old forward doesn't exist, returns an error. // If the new forward validation fails, the operation is rolled back (old forward remains). func (m *Mutator) UpdateForward(oldID, newContextName, newNamespaceName string, newFwd Forward) error { + return m.UpdateForwardInContext("", oldID, newContextName, newNamespaceName, newFwd) +} + +// UpdateForwardInContext is UpdateForward scoped to the context the edited +// forward came from. An empty oldContextName matches oldID in every context, +// preserving the behaviour of UpdateForward. +// +// Scoping matters because Forward.ID() is only "alias:localPort" when an alias +// is set: without a context, editing one forward would collapse every +// same-alias forward across contexts into a single entry. +func (m *Mutator) UpdateForwardInContext(oldContextName, oldID, newContextName, newNamespaceName string, newFwd Forward) error { m.mu.Lock() defer m.mu.Unlock() @@ -192,7 +245,8 @@ func (m *Mutator) UpdateForward(oldID, newContextName, newNamespaceName string, // CRITICAL: Set context/namespace so fwd.ID() generates correct ID fwd.SetContext(ctx.Name, ns.Name) - if fwd.ID() == oldID { + matchesContext := oldContextName == "" || ctx.Name == oldContextName + if matchesContext && fwd.ID() == oldID { oldForwardFound = true // Skip this forward (remove it) continue @@ -218,20 +272,27 @@ func (m *Mutator) UpdateForward(oldID, newContextName, newNamespaceName string, // Set context/namespace on the forward for validation newFwd.SetContext(newContextName, newNamespaceName) - // Check for duplicate local port (excluding the one we just removed) + // Check for a duplicate local port among forwards that could run alongside + // this one (excluding the one we just removed). + active := m.activeContextsLocked() allForwards := cfg.GetAllForwards() for _, existing := range allForwards { - if existing.LocalPort == newFwd.LocalPort && existing.ID() != oldID { - return fmt.Errorf("port %d is already in use by %s", newFwd.LocalPort, existing.String()) + if existing.LocalPort != newFwd.LocalPort || existing.ID() == oldID { + continue } + if !PortsCanConflict(existing.GetContext(), newContextName, active) { + continue + } + return fmt.Errorf("port %d is already in use by %s", newFwd.LocalPort, existing.String()) } // Add the new forward targetNamespace.Forwards = append(targetNamespace.Forwards, newFwd) - // Validate the new configuration + // Validate the whole configuration - including contexts that are not + // currently active - so nothing invalid is ever written to disk. validator := NewValidator() - if errs := validator.ValidateConfig(cfg); len(errs) > 0 { + if errs := validator.ValidateConfigWithOpts(cfg, ValidateOptions{ActiveContexts: active}); len(errs) > 0 { return fmt.Errorf("validation failed: %s", FormatValidationErrors(errs)) } diff --git a/internal/config/mutator_test.go b/internal/config/mutator_test.go index 8265d82..d0282be 100644 --- a/internal/config/mutator_test.go +++ b/internal/config/mutator_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -662,3 +663,237 @@ func TestMutator_Concurrent(t *testing.T) { require.NoError(t, err) require.NotNil(t, cfg) } + +// mutatorWithSharedPorts writes the issue #80 layout - the same alias and local +// port in two contexts - and returns a mutator scoped to team-a. +func mutatorWithSharedPorts(t *testing.T, active []string) (*Mutator, string) { + t.Helper() + + path := filepath.Join(t.TempDir(), ".kportal.yaml") + body := `contexts: + - name: team-a + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + alias: rate-service + - resource: pod/keeper + protocol: tcp + port: 9000 + localPort: 3009 + alias: keeper-a + - name: team-b + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + alias: rate-service + - resource: pod/keeper + protocol: tcp + port: 9000 + localPort: 3009 + alias: keeper-b +` + if err := os.WriteFile(path, []byte(body), 0600); err != nil { + t.Fatalf("failed to write config: %v", err) + } + + m := NewMutator(path) + m.SetActiveContexts(active) + return m, path +} + +func forwardsForContext(t *testing.T, path, contextName string) []Forward { + t.Helper() + + cfg, err := LoadConfig(path) + if err != nil { + t.Fatalf("failed to reload config: %v", err) + } + var out []Forward + for _, fwd := range cfg.GetAllForwards() { + if fwd.GetContext() == contextName { + out = append(out, fwd) + } + } + return out +} + +// TestMutator_RemoveForwardByIDInContext_OnlyTouchesOneContext is the +// regression test for the data-loss case that --context makes reachable: +// Forward.ID() is just "alias:localPort" when an alias is set, so an unscoped +// delete would strip the forward from every context at once. +func TestMutator_RemoveForwardByIDInContext_OnlyTouchesOneContext(t *testing.T) { + m, path := mutatorWithSharedPorts(t, []string{"team-a"}) + + if err := m.RemoveForwardByIDInContext("team-a", "rate-service:3004"); err != nil { + t.Fatalf("RemoveForwardByIDInContext() failed: %v", err) + } + + // Only the shared-alias forward goes; team-a keeps its other one and + // team-b is untouched entirely. + teamA := forwardsForContext(t, path, "team-a") + if len(teamA) != 1 || teamA[0].Alias != "keeper-a" { + t.Errorf("team-a should keep only keeper-a, got %+v", teamA) + } + if got := forwardsForContext(t, path, "team-b"); len(got) != 2 { + t.Errorf("team-b must be untouched, got %d forwards", len(got)) + } +} + +// TestMutator_UpdateForwardInContext_OnlyTouchesOneContext guards the same +// identity problem on the edit path. +func TestMutator_UpdateForwardInContext_OnlyTouchesOneContext(t *testing.T) { + m, path := mutatorWithSharedPorts(t, []string{"team-a"}) + + updated := Forward{ + Resource: "pod/rpc-server", + Protocol: "tcp", + Port: 50052, + LocalPort: 3004, + Alias: "rate-service", + } + if err := m.UpdateForwardInContext("team-a", "rate-service:3004", "team-a", "rate-service", updated); err != nil { + t.Fatalf("UpdateForwardInContext() failed: %v", err) + } + + teamA := forwardsForContext(t, path, "team-a") + if len(teamA) != 2 { + t.Fatalf("team-a should still have two forwards, got %d", len(teamA)) + } + + teamB := forwardsForContext(t, path, "team-b") + if len(teamB) != 2 { + t.Fatalf("team-b must keep both forwards, got %d", len(teamB)) + } + for _, fwd := range teamB { + if fwd.Alias == "rate-service" && fwd.Port != 50051 { + t.Errorf("team-b remote port = %d, want 50051 (unchanged)", fwd.Port) + } + } +} + +// TestMutator_AddForward_PortFreeInNonActiveContext covers the TUI add path for +// a config that reuses ports across contexts. +func TestMutator_AddForward_PortFreeInNonActiveContext(t *testing.T) { + m, path := mutatorWithSharedPorts(t, []string{"team-a"}) + + // 3005 is unused anywhere; adding it to the active context must succeed + // even though team-b exists with a colliding 3004 entry. + fwd := Forward{Resource: "pod/other", Protocol: "tcp", Port: 8080, LocalPort: 3005, Alias: "other"} + if err := m.AddForward("team-a", "rate-service", fwd); err != nil { + t.Fatalf("AddForward() failed: %v", err) + } + + // The file must still hold both contexts and reload cleanly. + cfg, err := LoadConfig(path) + if err != nil { + t.Fatalf("failed to reload written config: %v", err) + } + if len(cfg.Contexts) != 2 { + t.Errorf("written config lost a context: got %d, want 2", len(cfg.Contexts)) + } + if got := len(forwardsForContext(t, path, "team-a")); got != 3 { + t.Errorf("team-a should now have three forwards, got %d", got) + } +} + +// TestMutator_AddForward_PortTakenInActiveContext keeps the guard rail: a real +// clash inside the running selection is still refused. +func TestMutator_AddForward_PortTakenInActiveContext(t *testing.T) { + m, _ := mutatorWithSharedPorts(t, []string{"team-a"}) + + fwd := Forward{Resource: "pod/other", Protocol: "tcp", Port: 8080, LocalPort: 3004, Alias: "other"} + err := m.AddForward("team-a", "rate-service", fwd) + if err == nil { + t.Fatal("AddForward() should reject a port already used in the active context") + } + if !strings.Contains(err.Error(), "3004") { + t.Errorf("error should name the conflicting port, got %q", err) + } +} + +// TestMutator_AddForward_ValidatesNonActiveContexts closes the hole where +// validating only the active selection would let an invalid forward reach disk. +func TestMutator_AddForward_ValidatesNonActiveContexts(t *testing.T) { + m, path := mutatorWithSharedPorts(t, []string{"team-a"}) + + // The wizard lets the user pick any kubeconfig context, including one that + // is not currently being forwarded. It must still be validated. + bad := Forward{Resource: "not-a-valid-resource-format", Protocol: "tcp", Port: 8080, LocalPort: 4000} + if err := m.AddForward("team-c", "default", bad); err == nil { + t.Fatal("AddForward() must validate forwards added to non-active contexts") + } + + // Nothing should have been written. + cfg, err := LoadConfig(path) + if err != nil { + t.Fatalf("failed to reload config: %v", err) + } + if len(cfg.Contexts) != 2 { + t.Errorf("rejected add must not modify the file, got %d contexts", len(cfg.Contexts)) + } +} + +// TestMutator_RemoveForwardByID_UnscopedStillMatchesEveryContext documents that +// the original entry point keeps its behaviour for callers that have no context. +// The fixture uses distinct ports so it is valid with no selection at all. +func TestMutator_RemoveForwardByID_UnscopedStillMatchesEveryContext(t *testing.T) { + path := filepath.Join(t.TempDir(), ".kportal.yaml") + body := `contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + alias: shared + - resource: pod/keeper + protocol: tcp + port: 9000 + localPort: 3009 + alias: keeper-a + - name: team-b + namespaces: + - name: default + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + alias: shared + - resource: pod/keeper + protocol: tcp + port: 9000 + localPort: 3010 + alias: keeper-b +` + if err := os.WriteFile(path, []byte(body), 0600); err != nil { + t.Fatalf("failed to write config: %v", err) + } + m := NewMutator(path) + // Scope to team-a so the shared 3004 entries are legal; the point of the + // test is that the unscoped remove still matches both of them. + m.SetActiveContexts([]string{"team-a"}) + + if err := m.RemoveForwardByID("shared:3004"); err != nil { + t.Fatalf("RemoveForwardByID() failed: %v", err) + } + + cfg, err := LoadConfig(path) + if err != nil { + t.Fatalf("failed to reload config: %v", err) + } + // Both rate-service entries go; only the two keeper forwards remain. + if got := len(cfg.GetAllForwards()); got != 2 { + t.Errorf("unscoped remove should match both contexts, %d forwards left", got) + } +} diff --git a/internal/config/select.go b/internal/config/select.go new file mode 100644 index 0000000..e80655e --- /dev/null +++ b/internal/config/select.go @@ -0,0 +1,184 @@ +package config + +import ( + "fmt" + "strings" + + "github.com/lukaszraczylo/kportal/internal/logger" +) + +// NormalizeContextSelection turns raw --context flag values into a clean +// selection: comma-separated entries are split, surrounding whitespace is +// trimmed, empty segments are dropped, and duplicates are removed while +// preserving first-seen order. +// +// Duplicates matter beyond tidiness: a repeated name would select the same +// context twice, producing duplicate forward IDs and spurious port conflicts. +func NormalizeContextSelection(names []string) []string { + var ( + out []string + seen = make(map[string]struct{}) + ) + + for _, raw := range names { + for _, part := range strings.Split(raw, ",") { + name := strings.TrimSpace(part) + if name == "" { + continue + } + if _, dup := seen[name]; dup { + continue + } + seen[name] = struct{}{} + out = append(out, name) + } + } + + return out +} + +// contextIsActive reports whether name is part of the active selection. An +// empty selection means every context is active, which is kportal's default. +func contextIsActive(name string, activeContexts []string) bool { + if len(activeContexts) == 0 { + return true + } + for _, active := range activeContexts { + if active == name { + return true + } + } + return false +} + +// PortsCanConflict reports whether forwards living in contextA and contextB can +// ever run at the same time and therefore compete for the same local port. +// +// Two forwards collide when they share a context, or when both of their +// contexts are active simultaneously. Forwards in contexts that are never +// started together are free to reuse local ports - which is the whole point of +// selecting contexts: the same service can keep the same local port in every +// cluster. +func PortsCanConflict(contextA, contextB string, activeContexts []string) bool { + if contextA == contextB { + return true + } + return contextIsActive(contextA, activeContexts) && contextIsActive(contextB, activeContexts) +} + +// SelectContexts returns a shallow copy of c containing only the named +// contexts, keeping the order they appear in the config file so the UI's row +// order stays stable regardless of the order names were passed on the command +// line. An empty selection returns c unchanged. +// +// Unknown names are an error listing the contexts that do exist, so a typo +// surfaces immediately instead of silently forwarding nothing. +// +// The returned config shares backing arrays with c and must never be written +// back to disk - use the original config for that. +func (c *Config) SelectContexts(names []string) (*Config, error) { + selected, missing := c.selectContexts(names) + if len(missing) > 0 { + return nil, fmt.Errorf("unknown context %s in --context (available: %s)", + quoteAll(missing), availableContexts(c)) + } + return selected, nil +} + +// selectContextsLenient is SelectContexts without the error: unknown names are +// dropped and returned to the caller. Reload paths use this so that renaming a +// context in the config file does not freeze hot-reload for the rest of the +// session. +func (c *Config) selectContextsLenient(names []string) (*Config, []string) { + return c.selectContexts(names) +} + +func (c *Config) selectContexts(names []string) (*Config, []string) { + if len(names) == 0 { + return c, nil + } + + wanted := make(map[string]bool, len(names)) + for _, name := range names { + wanted[name] = false + } + + // Iterate the config rather than the selection so file order is preserved. + filtered := make([]Context, 0, len(names)) + for _, ctx := range c.Contexts { + if _, ok := wanted[ctx.Name]; !ok { + continue + } + wanted[ctx.Name] = true + filtered = append(filtered, ctx) + } + + var missing []string + for _, name := range names { + if !wanted[name] { + missing = append(missing, name) + } + } + + selected := *c + selected.Contexts = filtered + return &selected, missing +} + +// availableContexts renders the context names present in the config, for use +// in error messages. +func availableContexts(c *Config) string { + if len(c.Contexts) == 0 { + return "none defined in the config file" + } + names := make([]string, 0, len(c.Contexts)) + for _, ctx := range c.Contexts { + names = append(names, ctx.Name) + } + return quoteAll(names) +} + +func quoteAll(names []string) string { + quoted := make([]string, 0, len(names)) + for _, name := range names { + quoted = append(quoted, fmt.Sprintf("%q", name)) + } + return strings.Join(quoted, ", ") +} + +// LoadForRuntime loads, validates and filters the configuration for the +// forward manager. Validation runs against the *whole* file - so a mistake in a +// context you are not currently forwarding is still reported - while the +// uniqueness checks that only matter for running forwards (local ports, mDNS +// hostnames) are scoped to activeContexts. +// +// Every path that re-reads the config at runtime goes through here, so the +// selection cannot be forgotten in one of them. +func LoadForRuntime(path string, activeContexts []string) (*Config, error) { + cfg, err := LoadConfig(path) + if err != nil { + return nil, err + } + + validator := NewValidator() + if errs := validator.ValidateConfigWithOpts(cfg, ValidateOptions{ + ActiveContexts: activeContexts, + }); len(errs) > 0 { + return nil, fmt.Errorf("configuration is invalid:\n%s", FormatValidationErrors(errs)) + } + + selected, missing := cfg.selectContextsLenient(activeContexts) + // Keep the running config when every selected context vanished: an empty + // result would stop all forwards after a single rename in the file. + if len(selected.Contexts) == 0 && len(missing) > 0 { + return nil, fmt.Errorf("none of the selected contexts %s exist in the config file (available: %s)", + quoteAll(missing), availableContexts(cfg)) + } + if len(missing) > 0 { + logger.Info("Ignoring selected contexts that are no longer in the config file", map[string]interface{}{ + "contexts": strings.Join(missing, ", "), + }) + } + + return selected, nil +} diff --git a/internal/config/select_test.go b/internal/config/select_test.go new file mode 100644 index 0000000..3c0d7e0 --- /dev/null +++ b/internal/config/select_test.go @@ -0,0 +1,306 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNormalizeContextSelection(t *testing.T) { + tests := []struct { + name string + input []string + want []string + }{ + {name: "nil input", input: nil, want: nil}, + {name: "single name", input: []string{"team-a"}, want: []string{"team-a"}}, + {name: "repeated flag", input: []string{"team-a", "team-b"}, want: []string{"team-a", "team-b"}}, + {name: "comma separated", input: []string{"team-a,team-b"}, want: []string{"team-a", "team-b"}}, + {name: "repeated and comma mixed", input: []string{"team-a,team-b", "team-c"}, want: []string{"team-a", "team-b", "team-c"}}, + {name: "whitespace trimmed", input: []string{" team-a , team-b "}, want: []string{"team-a", "team-b"}}, + {name: "empty segments dropped", input: []string{"team-a,,team-b"}, want: []string{"team-a", "team-b"}}, + {name: "duplicates removed", input: []string{"team-a,team-a", "team-a"}, want: []string{"team-a"}}, + {name: "only separators", input: []string{",,"}, want: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, NormalizeContextSelection(tt.input)) + }) + } +} + +func TestPortsCanConflict(t *testing.T) { + tests := []struct { + name string + contextA string + contextB string + active []string + want bool + }{ + { + name: "same context always conflicts", + contextA: "team-a", contextB: "team-a", active: []string{"team-b"}, + want: true, + }, + { + name: "no selection means everything runs together", + contextA: "team-a", contextB: "team-b", active: nil, + want: true, + }, + { + name: "both selected conflict", + contextA: "team-a", contextB: "team-b", active: []string{"team-a", "team-b"}, + want: true, + }, + { + name: "only one selected does not conflict", + contextA: "team-a", contextB: "team-b", active: []string{"team-a"}, + want: false, + }, + { + name: "neither selected does not conflict", + contextA: "team-a", contextB: "team-b", active: []string{"team-c"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, PortsCanConflict(tt.contextA, tt.contextB, tt.active)) + }) + } +} + +// threeContextConfig builds a config with three contexts, each holding one +// forward on the same local port - the layout this feature exists to enable. +func threeContextConfig(t *testing.T) *Config { + t.Helper() + cfg, err := ParseConfig([]byte(` +mdns: + enabled: false +healthCheck: + interval: "5s" +contexts: + - name: team-a + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + alias: rate-service-a + - name: team-b + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + alias: rate-service-b + - name: team-c + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + alias: rate-service-c +`)) + require.NoError(t, err) + return cfg +} + +func contextNames(cfg *Config) []string { + names := make([]string, 0, len(cfg.Contexts)) + for _, ctx := range cfg.Contexts { + names = append(names, ctx.Name) + } + return names +} + +func TestConfig_SelectContexts(t *testing.T) { + tests := []struct { + name string + wantErr string + selection []string + want []string + }{ + {name: "empty selection keeps everything", selection: nil, want: []string{"team-a", "team-b", "team-c"}}, + {name: "single context", selection: []string{"team-b"}, want: []string{"team-b"}}, + {name: "multiple contexts", selection: []string{"team-a", "team-c"}, want: []string{"team-a", "team-c"}}, + { + name: "config order wins over flag order", + selection: []string{"team-c", "team-a"}, + want: []string{"team-a", "team-c"}, + }, + { + name: "unknown context errors and lists what exists", + selection: []string{"team-z"}, + wantErr: `unknown context "team-z" in --context (available: "team-a", "team-b", "team-c")`, + }, + { + name: "one unknown among known still errors", + selection: []string{"team-a", "team-z"}, + wantErr: `unknown context "team-z"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := threeContextConfig(t) + + got, err := cfg.SelectContexts(tt.selection) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.want, contextNames(got)) + // The original config must not be modified. + assert.Equal(t, []string{"team-a", "team-b", "team-c"}, contextNames(cfg)) + }) + } +} + +func TestConfig_SelectContexts_PreservesTopLevelSettings(t *testing.T) { + cfg := threeContextConfig(t) + + selected, err := cfg.SelectContexts([]string{"team-a"}) + require.NoError(t, err) + + require.NotNil(t, selected.HealthCheck) + assert.Equal(t, "5s", selected.HealthCheck.Interval) + assert.Equal(t, cfg.MDNS, selected.MDNS) + assert.Len(t, selected.GetAllForwards(), 1) +} + +func TestConfig_SelectContexts_EmptyConfig(t *testing.T) { + cfg := &Config{} + + // An empty selection is a no-op even with nothing to select. + same, err := cfg.SelectContexts(nil) + require.NoError(t, err) + assert.Same(t, cfg, same) + + _, err = cfg.SelectContexts([]string{"team-a"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "none defined in the config file") +} + +func TestConfig_SelectContexts_ContextWithNoForwards(t *testing.T) { + cfg := &Config{Contexts: []Context{{Name: "empty"}}} + + selected, err := cfg.SelectContexts([]string{"empty"}) + require.NoError(t, err) + assert.Equal(t, []string{"empty"}, contextNames(selected)) + assert.Empty(t, selected.GetAllForwards()) +} + +func writeConfig(t *testing.T, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), ".kportal.yaml") + require.NoError(t, os.WriteFile(path, []byte(body), 0600)) + return path +} + +// sharedPortConfig is the reporter's layout from issue #80: the same service on +// the same local port in every cluster. +const sharedPortConfig = `contexts: + - name: team-a + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + - name: team-b + namespaces: + - name: rate-service + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 +` + +func TestLoadForRuntime_SharedPortsAcrossNonSelectedContexts(t *testing.T) { + path := writeConfig(t, sharedPortConfig) + + // Without a selection both contexts run together, so the shared port is a + // genuine conflict. + _, err := LoadForRuntime(path, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "Duplicate local port 3004") + + // Selecting one context makes the layout legal and narrows what runs. + cfg, err := LoadForRuntime(path, []string{"team-a"}) + require.NoError(t, err) + assert.Equal(t, []string{"team-a"}, contextNames(cfg)) + require.Len(t, cfg.GetAllForwards(), 1) + + // Selecting both brings the conflict back - they really would collide. + _, err = LoadForRuntime(path, []string{"team-a", "team-b"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "Duplicate local port 3004") +} + +func TestLoadForRuntime_UnknownSelectedContextIsDropped(t *testing.T) { + path := writeConfig(t, sharedPortConfig) + + // A context renamed in the file must not freeze hot-reload for the session. + cfg, err := LoadForRuntime(path, []string{"team-a", "renamed-away"}) + require.NoError(t, err) + assert.Equal(t, []string{"team-a"}, contextNames(cfg)) +} + +func TestLoadForRuntime_SelectionReducingToNothingIsAnError(t *testing.T) { + path := writeConfig(t, sharedPortConfig) + + cfg, err := LoadForRuntime(path, []string{"gone"}) + require.Error(t, err) + assert.Nil(t, cfg) + assert.Contains(t, err.Error(), `"gone"`) + assert.Contains(t, err.Error(), `"team-a"`) +} + +func TestLoadForRuntime_ReportsErrorsInNonSelectedContexts(t *testing.T) { + // A broken forward in a context we are not forwarding is still a broken + // config file, and must be reported. + path := writeConfig(t, `contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/ok + protocol: tcp + port: 80 + localPort: 8080 + - name: team-b + namespaces: + - name: default + forwards: + - resource: pod/bad + protocol: tcp + port: 80 + localPort: 999999 +`) + + _, err := LoadForRuntime(path, []string{"team-a"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "localPort") +} + +func TestLoadForRuntime_MissingFile(t *testing.T) { + _, err := LoadForRuntime(filepath.Join(t.TempDir(), "absent.yaml"), nil) + assert.ErrorIs(t, err, ErrConfigNotFound) +} diff --git a/internal/config/validator.go b/internal/config/validator.go index 8cf7b21..7e783de 100644 --- a/internal/config/validator.go +++ b/internal/config/validator.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "regexp" + "sort" "strings" "time" ) @@ -64,6 +65,20 @@ func NewValidator() *Validator { return &Validator{} } +// ValidateOptions tunes how a configuration is validated. +type ValidateOptions struct { + // ActiveContexts names the contexts that will actually be forwarded, as + // selected with --context. Checks for resources that can only clash while + // running - local ports and mDNS hostnames - are scoped to these contexts, + // so contexts that are never started together may reuse the same values. + // An empty slice means every context is active, which is the default. + ActiveContexts []string + + // AllowEmpty permits configurations with no contexts or forwards. Used for + // newly created config files where the user will add forwards via the TUI. + AllowEmpty bool +} + // ValidateConfig validates the entire configuration and returns all errors found. func (v *Validator) ValidateConfig(cfg *Config) []ValidationError { return v.ValidateConfigWithOptions(cfg, false) @@ -73,7 +88,13 @@ func (v *Validator) ValidateConfig(cfg *Config) []ValidationError { // When allowEmpty is true, empty configurations (no contexts/forwards) are allowed. // This is useful for newly created config files where the user will add forwards via the TUI. func (v *Validator) ValidateConfigWithOptions(cfg *Config, allowEmpty bool) []ValidationError { + return v.ValidateConfigWithOpts(cfg, ValidateOptions{AllowEmpty: allowEmpty}) +} + +// ValidateConfigWithOpts validates configuration against the given options. +func (v *Validator) ValidateConfigWithOpts(cfg *Config, opts ValidateOptions) []ValidationError { var errs []ValidationError + allowEmpty := opts.AllowEmpty if cfg == nil { return []ValidationError{{ @@ -103,11 +124,11 @@ func (v *Validator) ValidateConfigWithOptions(cfg *Config, allowEmpty bool) []Va } // Check for duplicate local ports - errs = append(errs, v.validateDuplicatePorts(cfg)...) + errs = append(errs, v.validateDuplicatePorts(cfg, opts.ActiveContexts)...) // Validate mDNS configuration if cfg.IsMDNSEnabled() { - errs = append(errs, v.validateMDNS(cfg)...) + errs = append(errs, v.validateMDNS(cfg, opts.ActiveContexts)...) } // Validate duration fields in specs @@ -307,28 +328,58 @@ func (v *Validator) validateResource(fwd *Forward) []ValidationError { return errs } -// validateDuplicatePorts checks for duplicate local ports across all forwards. -func (v *Validator) validateDuplicatePorts(cfg *Config) []ValidationError { +// conflictScope returns the key that groups forwards which can run at the same +// time. Active contexts all share one scope because they run together; each +// inactive context gets its own, so it may reuse the ports and hostnames of +// contexts it is never started alongside. +func conflictScope(contextName string, activeContexts []string) string { + if contextIsActive(contextName, activeContexts) { + return "" // the shared "currently running" scope + } + return contextName +} + +// validateDuplicatePorts checks for local ports claimed by more than one +// forward that could be running simultaneously. Forwards in contexts that are +// never active together are allowed to share a local port. +func (v *Validator) validateDuplicatePorts(cfg *Config, activeContexts []string) []ValidationError { var errs []ValidationError - portMap := make(map[int][]string) // port -> list of forward IDs + type portKey struct { + scope string + port int + } + portMap := make(map[portKey][]string) // port within a scope -> list of forward IDs for _, ctx := range cfg.Contexts { for _, ns := range ctx.Namespaces { for _, fwd := range ns.Forwards { - portMap[fwd.LocalPort] = append(portMap[fwd.LocalPort], fwd.ID()) + key := portKey{scope: conflictScope(ctx.Name, activeContexts), port: fwd.LocalPort} + portMap[key] = append(portMap[key], fwd.ID()) } } } - // Find duplicates - for port, forwards := range portMap { + // Find duplicates. Sort the keys so the reported errors are deterministic. + keys := make([]portKey, 0, len(portMap)) + for key := range portMap { + keys = append(keys, key) + } + sort.Slice(keys, func(i, j int) bool { + if keys[i].port != keys[j].port { + return keys[i].port < keys[j].port + } + return keys[i].scope < keys[j].scope + }) + + for _, key := range keys { + forwards := portMap[key] if len(forwards) > 1 { errs = append(errs, ValidationError{ Field: "localPort", - Message: fmt.Sprintf("Duplicate local port %d used by multiple forwards", port), + Message: fmt.Sprintf("Duplicate local port %d used by multiple forwards", key.port), Context: map[string]string{ - "port": fmt.Sprintf("%d", port), + "port": fmt.Sprintf("%d", key.port), "forwards": strings.Join(forwards, ", "), }, }) @@ -467,10 +518,14 @@ func FormatValidationErrors(errs []ValidationError) string { // validateMDNS validates mDNS configuration when enabled. // It checks that aliases used for mDNS hostnames are valid and unique. // This includes both explicit aliases and auto-generated ones from resource names. -func (v *Validator) validateMDNS(cfg *Config) []ValidationError { +func (v *Validator) validateMDNS(cfg *Config, activeContexts []string) []ValidationError { var errs []ValidationError - aliasMap := make(map[string][]string) // alias -> list of forward IDs using it + type aliasKey struct { + scope string + alias string + } + aliasMap := make(map[aliasKey][]string) // alias within a scope -> list of forward IDs using it for _, ctx := range cfg.Contexts { for _, ns := range ctx.Namespaces { @@ -490,19 +545,33 @@ func (v *Validator) validateMDNS(cfg *Config) []ValidationError { }) } - aliasMap[mdnsAlias] = append(aliasMap[mdnsAlias], fwd.ID()) + key := aliasKey{scope: conflictScope(ctx.Name, activeContexts), alias: mdnsAlias} + aliasMap[key] = append(aliasMap[key], fwd.ID()) } } } - // Check for duplicate aliases (would cause mDNS conflicts) - for alias, forwards := range aliasMap { + // Check for duplicate aliases among forwards that can run together (would + // cause mDNS conflicts). Sort the keys so reported errors are deterministic. + keys := make([]aliasKey, 0, len(aliasMap)) + for key := range aliasMap { + keys = append(keys, key) + } + sort.Slice(keys, func(i, j int) bool { + if keys[i].alias != keys[j].alias { + return keys[i].alias < keys[j].alias + } + return keys[i].scope < keys[j].scope + }) + + for _, key := range keys { + forwards := aliasMap[key] if len(forwards) > 1 { errs = append(errs, ValidationError{ Field: "alias", - Message: fmt.Sprintf("Duplicate mDNS hostname '%s' used by multiple forwards (would cause conflict)", alias), + Message: fmt.Sprintf("Duplicate mDNS hostname '%s' used by multiple forwards (would cause conflict)", key.alias), Context: map[string]string{ - "alias": alias, + "alias": key.alias, "forwards": strings.Join(forwards, ", "), }, }) diff --git a/internal/config/validator_test.go b/internal/config/validator_test.go index b9891a6..0b502c8 100644 --- a/internal/config/validator_test.go +++ b/internal/config/validator_test.go @@ -526,7 +526,7 @@ func TestValidator_CheckDuplicatePorts(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - errs := validator.validateDuplicatePorts(tt.config) + errs := validator.validateDuplicatePorts(tt.config, nil) if tt.expectErrors { assert.NotEmpty(t, errs, "expected validation errors") @@ -2184,3 +2184,176 @@ func TestValidator_ValidateTUI(t *testing.T) { }) } } + +// TestValidator_DuplicatePortsScopedToActiveContexts covers the rule that makes +// --context useful: forwards in contexts that are never running at the same +// time may reuse a local port, while forwards that would genuinely collide are +// still rejected. +func TestValidator_DuplicatePortsScopedToActiveContexts(t *testing.T) { + validator := NewValidator() + + buildConfig := func() *Config { + cfg, err := ParseConfig([]byte(`contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 + - name: team-b + namespaces: + - name: default + forwards: + - resource: pod/rpc-server + protocol: tcp + port: 50051 + localPort: 3004 +`)) + if err != nil { + t.Fatalf("ParseConfig() failed: %v", err) + } + return cfg + } + + tests := []struct { + name string + active []string + expectErrors bool + }{ + {name: "no selection keeps the global check", active: nil, expectErrors: true}, + {name: "one active context allows the shared port", active: []string{"team-a"}, expectErrors: false}, + {name: "other active context allows it too", active: []string{"team-b"}, expectErrors: false}, + {name: "both active reinstates the conflict", active: []string{"team-a", "team-b"}, expectErrors: true}, + {name: "neither active leaves them isolated", active: []string{"team-c"}, expectErrors: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errs := validator.ValidateConfigWithOpts(buildConfig(), ValidateOptions{ActiveContexts: tt.active}) + + var portErrs []ValidationError + for _, err := range errs { + if err.Field == "localPort" { + portErrs = append(portErrs, err) + } + } + + if tt.expectErrors { + assert.NotEmpty(t, portErrs, "expected a duplicate local port error") + } else { + assert.Empty(t, portErrs, "shared port should be allowed: %v", portErrs) + } + }) + } +} + +// TestValidator_DuplicatePortsWithinContextAlwaysConflict guards the case a +// scope key must never hide: two forwards in the SAME context always collide. +func TestValidator_DuplicatePortsWithinContextAlwaysConflict(t *testing.T) { + cfg, err := ParseConfig([]byte(`contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/one + protocol: tcp + port: 80 + localPort: 8080 + - resource: pod/two + protocol: tcp + port: 81 + localPort: 8080 +`)) + if err != nil { + t.Fatalf("ParseConfig() failed: %v", err) + } + + for _, active := range [][]string{nil, {"team-a"}, {"team-b"}} { + errs := NewValidator().ValidateConfigWithOpts(cfg, ValidateOptions{ActiveContexts: active}) + found := false + for _, e := range errs { + if e.Field == "localPort" { + found = true + } + } + assert.True(t, found, "same-context duplicate must conflict (active=%v)", active) + } +} + +// TestValidator_StructuralErrorsNotHiddenBySelection proves --check did not get +// weaker: a problem in a context that is not being forwarded is still reported. +func TestValidator_StructuralErrorsNotHiddenBySelection(t *testing.T) { + cfg, err := ParseConfig([]byte(`contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/ok + protocol: tcp + port: 80 + localPort: 8080 + - name: team-b + namespaces: + - name: default + forwards: + - resource: "" + protocol: tcp + port: 80 + localPort: 9090 +`)) + if err != nil { + t.Fatalf("ParseConfig() failed: %v", err) + } + + errs := NewValidator().ValidateConfigWithOpts(cfg, ValidateOptions{ActiveContexts: []string{"team-a"}}) + assert.NotEmpty(t, errs, "an invalid forward in a non-selected context must still be reported") +} + +// TestValidator_DuplicateMDNSAliasScopedToActiveContexts mirrors the local-port +// rule for mDNS hostnames, which can only clash while forwards are running. +func TestValidator_DuplicateMDNSAliasScopedToActiveContexts(t *testing.T) { + buildConfig := func() *Config { + cfg, err := ParseConfig([]byte(`mdns: + enabled: true +contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: service/api + protocol: tcp + port: 80 + localPort: 8080 + alias: api + - name: team-b + namespaces: + - name: default + forwards: + - resource: service/api + protocol: tcp + port: 80 + localPort: 8080 + alias: api +`)) + if err != nil { + t.Fatalf("ParseConfig() failed: %v", err) + } + return cfg + } + + aliasErrs := func(active []string) []ValidationError { + var out []ValidationError + for _, e := range NewValidator().ValidateConfigWithOpts(buildConfig(), ValidateOptions{ActiveContexts: active}) { + if e.Field == "alias" { + out = append(out, e) + } + } + return out + } + + assert.NotEmpty(t, aliasErrs(nil), "duplicate hostname must conflict when everything runs") + assert.Empty(t, aliasErrs([]string{"team-a"}), "duplicate hostname is fine when only one context runs") + assert.NotEmpty(t, aliasErrs([]string{"team-a", "team-b"}), "both active means a real hostname clash") +} diff --git a/internal/config/watcher.go b/internal/config/watcher.go index e2c52db..86f38ee 100644 --- a/internal/config/watcher.go +++ b/internal/config/watcher.go @@ -16,17 +16,30 @@ type ReloadCallback func(*Config) error // Watcher watches a configuration file for changes and triggers hot-reload. type Watcher struct { - callback ReloadCallback - watcher *fsnotify.Watcher - done chan struct{} - configPath string - wg sync.WaitGroup - stopOnce sync.Once - verbose bool + callback ReloadCallback + watcher *fsnotify.Watcher + done chan struct{} + configPath string + activeContexts []string + wg sync.WaitGroup + stopOnce sync.Once + verbose bool +} + +// WatcherOption configures a Watcher at construction time. +type WatcherOption func(*Watcher) + +// WithContextSelection restricts hot-reload to the given contexts, matching the +// --context selection the process started with. Without it a reload would +// resurrect every context in the file. +func WithContextSelection(names []string) WatcherOption { + return func(w *Watcher) { + w.activeContexts = names + } } // NewWatcher creates a new file watcher for the given config file. -func NewWatcher(configPath string, callback ReloadCallback, verbose bool) (*Watcher, error) { +func NewWatcher(configPath string, callback ReloadCallback, verbose bool, opts ...WatcherOption) (*Watcher, error) { watcher, err := fsnotify.NewWatcher() if err != nil { return nil, fmt.Errorf("failed to create file watcher: %w", err) @@ -46,13 +59,17 @@ func NewWatcher(configPath string, callback ReloadCallback, verbose bool) (*Watc return nil, fmt.Errorf("failed to watch directory %s: %w", dir, err) } - return &Watcher{ + w := &Watcher{ configPath: absPath, callback: callback, watcher: watcher, done: make(chan struct{}), verbose: verbose, - }, nil + } + for _, opt := range opts { + opt(w) + } + return w, nil } // Start begins watching the configuration file for changes. @@ -121,8 +138,9 @@ func (w *Watcher) watch() { // handleReload loads and validates the new configuration, then calls the callback. func (w *Watcher) handleReload() { - // Load new configuration - newCfg, err := LoadConfig(w.configPath) + // Load, validate and apply the --context selection in one step, so the + // selection cannot drift from the other reload paths. + newCfg, err := LoadForRuntime(w.configPath, w.activeContexts) if err != nil { logger.Error("Failed to load configuration during hot-reload", map[string]interface{}{ "config_path": w.configPath, @@ -132,17 +150,6 @@ func (w *Watcher) handleReload() { return } - // Validate new configuration - validator := NewValidator() - if errs := validator.ValidateConfig(newCfg); len(errs) > 0 { - logger.Error("Configuration validation failed during hot-reload", map[string]interface{}{ - "config_path": w.configPath, - "validation_errors": len(errs), - }) - logger.Info("Keeping previous configuration active", nil) - return - } - // Call reload callback if err := w.callback(newCfg); err != nil { logger.Error("Failed to apply new configuration", map[string]interface{}{ diff --git a/internal/config/watcher_test.go b/internal/config/watcher_test.go index d9299bc..1801b51 100644 --- a/internal/config/watcher_test.go +++ b/internal/config/watcher_test.go @@ -504,3 +504,124 @@ func TestWatcher_StopWithoutStart(t *testing.T) { t.Fatal("Stop without start timed out") } } + +// TestWatcher_WithContextSelection verifies that hot-reload honours the +// --context selection: a config whose non-selected contexts reuse local ports +// must reload cleanly, and the callback must only receive the selection. +func TestWatcher_WithContextSelection(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, ".kportal.yaml") + + initial := `contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/app + port: 8080 + localPort: 3004 + - name: team-b + namespaces: + - name: default + forwards: + - resource: pod/app + port: 8080 + localPort: 3004 +` + require.NoError(t, os.WriteFile(configPath, []byte(initial), 0600)) + + var mu sync.Mutex + var receivedConfig *Config + + callback := func(cfg *Config) error { + mu.Lock() + defer mu.Unlock() + receivedConfig = cfg + return nil + } + + watcher, err := NewWatcher(configPath, callback, false, WithContextSelection([]string{"team-a"})) + require.NoError(t, err) + defer watcher.Stop() + + watcher.Start() + time.Sleep(100 * time.Millisecond) + + updated := `contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/app + port: 8080 + localPort: 3004 + - resource: pod/extra + port: 9090 + localPort: 3005 + - name: team-b + namespaces: + - name: default + forwards: + - resource: pod/app + port: 8080 + localPort: 3004 +` + require.NoError(t, os.WriteFile(configPath, []byte(updated), 0600)) + + require.Eventually(t, func() bool { + mu.Lock() + defer mu.Unlock() + return receivedConfig != nil + }, 3*time.Second, 50*time.Millisecond, "watcher should reload a config with ports shared across non-selected contexts") + + mu.Lock() + defer mu.Unlock() + require.Len(t, receivedConfig.Contexts, 1, "callback should only see the selected context") + assert.Equal(t, "team-a", receivedConfig.Contexts[0].Name) + assert.Len(t, receivedConfig.GetAllForwards(), 2) +} + +// TestWatcher_WithContextSelection_UnknownNameDoesNotFreezeReload covers a +// context renamed in the file: reload must keep working for the rest. +func TestWatcher_WithContextSelection_UnknownNameDoesNotFreezeReload(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, ".kportal.yaml") + + initial := `contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/app + port: 8080 + localPort: 3004 +` + require.NoError(t, os.WriteFile(configPath, []byte(initial), 0600)) + + var mu sync.Mutex + var called bool + + watcher, err := NewWatcher(configPath, func(cfg *Config) error { + mu.Lock() + defer mu.Unlock() + called = true + return nil + }, false, WithContextSelection([]string{"team-a", "since-renamed"})) + require.NoError(t, err) + defer watcher.Stop() + + watcher.Start() + time.Sleep(100 * time.Millisecond) + + updated := initial + ` - resource: pod/extra + port: 9090 + localPort: 3005 +` + require.NoError(t, os.WriteFile(configPath, []byte(updated), 0600)) + + require.Eventually(t, func() bool { + mu.Lock() + defer mu.Unlock() + return called + }, 3*time.Second, 50*time.Millisecond, "an unknown selected context must not freeze hot-reload") +} diff --git a/internal/ui/bubbletea_ui.go b/internal/ui/bubbletea_ui.go index 851f219..8759870 100644 --- a/internal/ui/bubbletea_ui.go +++ b/internal/ui/bubbletea_ui.go @@ -69,33 +69,35 @@ type HTTPLogSubscriber func(forwardID string, callback func(entry HTTPLogEntry)) // BubbleTeaUI is a bubbletea-based terminal UI type BubbleTeaUI struct { - discovery *k8s.Discovery - program *tea.Program - forwards map[string]*ForwardStatus - columns []ResolvedColumn - benchmarkState *BenchmarkState - httpLogSubscriber HTTPLogSubscriber - disabledMap map[string]bool - toggleCallback func(id string, enable bool) - httpLogCleanup func() - httpLogState *HTTPLogState - errors map[string]string - mutator *config.Mutator - removeWizard *RemoveWizardState - addWizard *AddWizardState - updateVersion string - updateURL string - configPath string - deleteConfirmID string - deleteConfirmAlias string - version string - forwardOrder []string - viewMode ViewMode - deleteConfirmCursor int - selectedIndex int - mu sync.RWMutex - deleteConfirming bool - updateAvailable bool + discovery *k8s.Discovery + program *tea.Program + forwards map[string]*ForwardStatus + benchmarkState *BenchmarkState + httpLogSubscriber HTTPLogSubscriber + disabledMap map[string]bool + toggleCallback func(id string, enable bool) + httpLogCleanup func() + httpLogState *HTTPLogState + errors map[string]string + mutator *config.Mutator + removeWizard *RemoveWizardState + addWizard *AddWizardState + updateVersion string + updateURL string + configPath string + deleteConfirmID string + deleteConfirmContext string + deleteConfirmAlias string + version string + forwardOrder []string + columns []ResolvedColumn + activeContexts []string + viewMode ViewMode + deleteConfirmCursor int + selectedIndex int + mu sync.RWMutex + deleteConfirming bool + updateAvailable bool } // bubbletea model @@ -140,6 +142,16 @@ func (ui *BubbleTeaUI) SetWizardDependencies(discovery *k8s.Discovery, mutator * ui.configPath = configPath } +// SetActiveContexts records the contexts selected with --context so the add +// and edit wizards only report local-port conflicts against forwards that +// could actually be running. An empty selection means every context is active. +func (ui *BubbleTeaUI) SetActiveContexts(names []string) { + ui.mu.Lock() + defer ui.mu.Unlock() + + ui.activeContexts = names +} + // SetHTTPLogSubscriber sets the function to subscribe to HTTP logs func (ui *BubbleTeaUI) SetHTTPLogSubscriber(subscriber HTTPLogSubscriber) { ui.mu.Lock() @@ -900,6 +912,7 @@ func (ui *BubbleTeaUI) moveSelection(delta int) { func (ui *BubbleTeaUI) resetDeleteConfirmation() { ui.deleteConfirming = false ui.deleteConfirmID = "" + ui.deleteConfirmContext = "" ui.deleteConfirmAlias = "" ui.deleteConfirmCursor = 0 } diff --git a/internal/ui/commands_test.go b/internal/ui/commands_test.go index 910eced..38a2f3d 100644 --- a/internal/ui/commands_test.go +++ b/internal/ui/commands_test.go @@ -162,7 +162,7 @@ func TestCheckPortCmd_PortAvailability(t *testing.T) { require.NoError(t, err) // Test checking a random high port that should be available - cmd := checkPortCmd(59999, configPath, "") + cmd := checkPortCmd(59999, configPath, "", "test-ctx", nil) msg := cmd() portMsg, ok := msg.(PortCheckedMsg) @@ -192,7 +192,7 @@ func TestCheckPortCmd_ConfigConflict(t *testing.T) { require.NoError(t, err) // Test checking port that's already in config - cmd := checkPortCmd(8080, configPath, "") + cmd := checkPortCmd(8080, configPath, "", "test-ctx", nil) msg := cmd() portMsg, ok := msg.(PortCheckedMsg) @@ -224,7 +224,7 @@ func TestCheckPortCmd_ExcludeID_AllowsKeepingOwnPort(t *testing.T) { // The forward's ID format is "//:". excludeID := "test-ctx/default/pod/my-app:8080" - cmd := checkPortCmd(8080, configPath, excludeID) + cmd := checkPortCmd(8080, configPath, excludeID, "test-ctx", nil) msg := cmd() portMsg, ok := msg.(PortCheckedMsg) @@ -241,7 +241,7 @@ func TestCheckPortCmd_ExcludeID_AllowsKeepingOwnPort(t *testing.T) { // TestCheckPortCmd_InvalidConfig tests behavior with invalid config file func TestCheckPortCmd_InvalidConfig(t *testing.T) { // Use a non-existent config path - cmd := checkPortCmd(59998, "/nonexistent/path/.kportal.yaml", "") + cmd := checkPortCmd(59998, "/nonexistent/path/.kportal.yaml", "", "test-ctx", nil) msg := cmd() portMsg, ok := msg.(PortCheckedMsg) @@ -420,3 +420,76 @@ func TestHTTPLogSubscriberType(t *testing.T) { cleanup() assert.Equal(t, 1, mock.CleanupCalls) } + +// TestCheckPortCmd_ContextScoping verifies the add/edit wizard does not report a +// port as taken when the only forward using it lives in a context that is not +// being forwarded. +func TestCheckPortCmd_ContextScoping(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, ".kportal.yaml") + + configContent := `contexts: + - name: team-a + namespaces: + - name: default + forwards: + - resource: pod/my-app + port: 80 + localPort: 8080 + - name: team-b + namespaces: + - name: default + forwards: + - resource: pod/other-app + port: 80 + localPort: 9090 +` + require.NoError(t, os.WriteFile(configPath, []byte(configContent), 0600)) + + tests := []struct { + name string + targetContext string + activeContexts []string + port int + wantAssignedMsg bool + }{ + { + name: "port used only by a non-active context is free", + port: 9090, targetContext: "team-a", activeContexts: []string{"team-a"}, + wantAssignedMsg: false, + }, + { + name: "port used by the target context is taken", + port: 8080, targetContext: "team-a", activeContexts: []string{"team-a"}, + wantAssignedMsg: true, + }, + { + name: "both contexts active means the port is taken", + port: 9090, targetContext: "team-a", activeContexts: []string{"team-a", "team-b"}, + wantAssignedMsg: true, + }, + { + name: "no selection keeps the global check", + port: 9090, targetContext: "team-a", activeContexts: nil, + wantAssignedMsg: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + msg := checkPortCmd(tt.port, configPath, "", tt.targetContext, tt.activeContexts)() + + portMsg, ok := msg.(PortCheckedMsg) + require.True(t, ok, "Expected PortCheckedMsg") + assert.Equal(t, tt.port, portMsg.port) + + if tt.wantAssignedMsg { + assert.Contains(t, portMsg.message, "already assigned") + } else { + // The OS-level check still runs, so only assert that the + // in-config conflict path did not fire. + assert.NotContains(t, portMsg.message, "already assigned") + } + }) + } +} diff --git a/internal/ui/generate_test.go b/internal/ui/generate_test.go index 1b49e00..92b3d56 100644 --- a/internal/ui/generate_test.go +++ b/internal/ui/generate_test.go @@ -36,6 +36,12 @@ func (f *fakeMutator) RemoveForwards(predicate func(ctx, ns string, fwd config.F return nil } func (f *fakeMutator) RemoveForwardByID(id string) error { return nil } +func (f *fakeMutator) RemoveForwardByIDInContext(contextName, id string) error { + return nil +} +func (f *fakeMutator) UpdateForwardInContext(oldCtx, oldID, newCtx, newNS string, newFwd config.Forward) error { + return nil +} func (f *fakeMutator) UpdateForward(oldID, newCtx, newNS string, newFwd config.Forward) error { return nil } diff --git a/internal/ui/interfaces.go b/internal/ui/interfaces.go index 0d81ca0..525a1f2 100644 --- a/internal/ui/interfaces.go +++ b/internal/ui/interfaces.go @@ -24,7 +24,9 @@ type MutatorInterface interface { AddForward(contextName, namespaceName string, fwd config.Forward) error RemoveForwards(predicate func(ctx, ns string, fwd config.Forward) bool) error RemoveForwardByID(id string) error + RemoveForwardByIDInContext(contextName, id string) error UpdateForward(oldID, newContextName, newNamespaceName string, newFwd config.Forward) error + UpdateForwardInContext(oldContextName, oldID, newContextName, newNamespaceName string, newFwd config.Forward) error } // Compile-time checks to ensure real types implement interfaces diff --git a/internal/ui/mocks_test.go b/internal/ui/mocks_test.go index 943681e..909dee8 100644 --- a/internal/ui/mocks_test.go +++ b/internal/ui/mocks_test.go @@ -105,6 +105,8 @@ type MockMutator struct { LastOldID string LastNamespaceName string LastRemovedID string + LastRemovedContext string + LastOldContext string Forwards []struct { Context string Namespace string @@ -157,6 +159,22 @@ func (m *MockMutator) RemoveForwardByID(id string) error { return m.RemoveForwardByIDErr } +func (m *MockMutator) RemoveForwardByIDInContext(contextName, id string) error { + m.mu.Lock() + defer m.mu.Unlock() + m.RemoveForwardByIDCalls++ + m.LastRemovedContext = contextName + m.LastRemovedID = id + return m.RemoveForwardByIDErr +} + +func (m *MockMutator) UpdateForwardInContext(oldContextName, oldID, newContextName, newNamespaceName string, newFwd config.Forward) error { + m.mu.Lock() + m.LastOldContext = oldContextName + m.mu.Unlock() + return m.UpdateForward(oldID, newContextName, newNamespaceName, newFwd) +} + func (m *MockMutator) UpdateForward(oldID, newContextName, newNamespaceName string, newFwd config.Forward) error { m.mu.Lock() defer m.mu.Unlock() diff --git a/internal/ui/wizard_commands.go b/internal/ui/wizard_commands.go index 8ad46c6..2ac66a1 100644 --- a/internal/ui/wizard_commands.go +++ b/internal/ui/wizard_commands.go @@ -149,7 +149,11 @@ func validateSelectorCmd(discovery *k8s.Discovery, contextName, namespace, selec // excludeID, when non-empty, is the ID of a forward to ignore during the // in-config conflict scan. Used in edit mode so the wizard does not flag the // forward being edited as conflicting with itself. -func checkPortCmd(port int, configPath, excludeID string) tea.Cmd { +// targetContext is the context the forward is being added to or edited in, and +// activeContexts the --context selection; together they decide which existing +// forwards could actually compete for the port. A forward in a context that is +// never running alongside targetContext does not conflict. +func checkPortCmd(port int, configPath, excludeID, targetContext string, activeContexts []string) tea.Cmd { return func() tea.Msg { // First check if port is already in the configuration cfg, err := config.LoadConfig(configPath) @@ -163,6 +167,9 @@ func checkPortCmd(port int, configPath, excludeID string) tea.Cmd { if excludeID != "" && fwd.ID() == excludeID { continue } + if !config.PortsCanConflict(fwd.GetContext(), targetContext, activeContexts) { + continue + } return PortCheckedMsg{ port: port, available: false, @@ -203,9 +210,9 @@ func saveForwardCmd(mutator *config.Mutator, contextName, namespace string, fwd } // updateForwardCmd atomically updates an existing forward (used in edit mode) -func updateForwardCmd(mutator *config.Mutator, oldID, contextName, namespace string, fwd config.Forward) tea.Cmd { +func updateForwardCmd(mutator *config.Mutator, oldContextName, oldID, contextName, namespace string, fwd config.Forward) tea.Cmd { return func() tea.Msg { - err := mutator.UpdateForward(oldID, contextName, namespace, fwd) + err := mutator.UpdateForwardInContext(oldContextName, oldID, contextName, namespace, fwd) return ForwardSavedMsg{ success: err == nil, err: err, @@ -216,15 +223,18 @@ func updateForwardCmd(mutator *config.Mutator, oldID, contextName, namespace str // removeForwardsCmd removes selected forwards from the configuration file func removeForwardsCmd(mutator *config.Mutator, forwards []RemovableForward) tea.Cmd { return func() tea.Msg { - // Create a map of IDs to remove - idsToRemove := make(map[string]bool) + // Key on context as well as ID: Forward.ID() is only "alias:localPort" + // when an alias is set, which is not unique across contexts once they + // are allowed to reuse local ports. + type forwardKey struct{ contextName, id string } + idsToRemove := make(map[forwardKey]bool) for _, fwd := range forwards { - idsToRemove[fwd.ID] = true + idsToRemove[forwardKey{contextName: fwd.Context, id: fwd.ID}] = true } // Remove forwards matching the IDs err := mutator.RemoveForwards(func(ctx, ns string, fwd config.Forward) bool { - return idsToRemove[fwd.ID()] + return idsToRemove[forwardKey{contextName: ctx, id: fwd.ID()}] }) return ForwardsRemovedMsg{ @@ -236,9 +246,9 @@ func removeForwardsCmd(mutator *config.Mutator, forwards []RemovableForward) tea } // removeForwardByIDCmd removes a single forward by its ID -func removeForwardByIDCmd(mutator *config.Mutator, id string) tea.Cmd { +func removeForwardByIDCmd(mutator *config.Mutator, contextName, id string) tea.Cmd { return func() tea.Msg { - err := mutator.RemoveForwardByID(id) + err := mutator.RemoveForwardByIDInContext(contextName, id) return ForwardsRemovedMsg{ success: err == nil, count: 1, diff --git a/internal/ui/wizard_handlers.go b/internal/ui/wizard_handlers.go index 9cd72a8..0ac99c0 100644 --- a/internal/ui/wizard_handlers.go +++ b/internal/ui/wizard_handlers.go @@ -135,6 +135,7 @@ func (m model) handleMainViewKeys(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // Mark as edit mode and store original ID m.ui.addWizard.isEditing = true m.ui.addWizard.originalID = selectedID + m.ui.addWizard.originalContext = selectedForward.Context // Start at the remote port step (skip context/namespace/resource selection) m.ui.addWizard.step = StepEnterRemotePort @@ -187,6 +188,7 @@ func (m model) handleMainViewKeys(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // Show confirmation dialog m.ui.deleteConfirming = true m.ui.deleteConfirmID = selectedID + m.ui.deleteConfirmContext = selectedForward.Context m.ui.deleteConfirmAlias = selectedForward.Alias m.ui.deleteConfirmCursor = 1 // Default to "No" for safety @@ -312,9 +314,10 @@ func (m model) handleDeleteConfirmation(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // Confirm deletion (either Enter on Yes or pressing 'y') if m.ui.deleteConfirmCursor == 0 || msg.String() == "y" { id := m.ui.deleteConfirmID + contextName := m.ui.deleteConfirmContext m.ui.resetDeleteConfirmation() m.ui.mu.Unlock() - return m, removeForwardByIDCmd(m.ui.mutator, id) + return m, removeForwardByIDCmd(m.ui.mutator, contextName, id) } // Enter on No = cancel m.ui.resetDeleteConfirmation() @@ -691,7 +694,7 @@ func (m model) handleAddWizardEnter() (tea.Model, tea.Cmd) { if wizard.isEditing { excludeID = wizard.originalID } - return m, checkPortCmd(port, m.ui.configPath, excludeID) + return m, checkPortCmd(port, m.ui.configPath, excludeID, wizard.selectedContext, m.ui.activeContexts) } case StepConfirmation: @@ -749,7 +752,7 @@ func (m model) handleAddWizardEnter() (tea.Model, tea.Cmd) { // If editing, use atomic update operation if wizard.isEditing { - return m, updateForwardCmd(m.ui.mutator, wizard.originalID, wizard.selectedContext, wizard.selectedNamespace, fwd) + return m, updateForwardCmd(m.ui.mutator, wizard.originalContext, wizard.originalID, wizard.selectedContext, wizard.selectedNamespace, fwd) } return m, saveForwardCmd(m.ui.mutator, wizard.selectedContext, wizard.selectedNamespace, fwd) diff --git a/internal/ui/wizard_handlers_extended_test.go b/internal/ui/wizard_handlers_extended_test.go index bedb25d..9036926 100644 --- a/internal/ui/wizard_handlers_extended_test.go +++ b/internal/ui/wizard_handlers_extended_test.go @@ -1685,7 +1685,7 @@ func TestRemoveForwardByIDCmd_Success(t *testing.T) { require.NoError(t, mutator.AddForward("ctx", "ns", fwdKeep)) id := "ctx/ns/pod/app:18083" - cmd := removeForwardByIDCmd(mutator, id) + cmd := removeForwardByIDCmd(mutator, "", id) msg := cmd() removedMsg, ok := msg.(ForwardsRemovedMsg) require.True(t, ok) diff --git a/internal/ui/wizard_state.go b/internal/ui/wizard_state.go index a013580..41fd69b 100644 --- a/internal/ui/wizard_state.go +++ b/internal/ui/wizard_state.go @@ -115,6 +115,7 @@ type AddWizardState struct { httpLogOriginal *config.HTTPLogSpec resourceValue string originalID string + originalContext string portCheckMsg string alias string textInput string