diff --git a/internal/client/factory.go b/internal/client/factory.go index 45e16d9..ad88d55 100644 --- a/internal/client/factory.go +++ b/internal/client/factory.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "time" "github.com/spf13/pflag" @@ -414,6 +415,12 @@ func (c *CustomConfigFlags) resolveScope(ctxEntry *datumconfig.DiscoveredContext return Scope{}, fmt.Errorf("exactly one of --project or --organization must be provided") } + if source == ScopeSourceFlag || source == ScopeSourceEnv { + if err := validateProjectName(projectID, source); err != nil { + return Scope{}, err + } + } + return Scope{ Project: projectID, Organization: organizationID, @@ -515,3 +522,23 @@ func NewDatumFactory(ctx context.Context) (*DatumCloudFactory, error) { ConfigFlags: configFlags, }, nil } + +// validateProjectName rejects a project given in the "org/project" form that +// 'datumctl ctx list' prints. Project names never contain "/". +func validateProjectName(project string, source ScopeSource) error { + if !strings.Contains(project, "/") { + return nil + } + name := "" + if parts := strings.Split(project, "/"); len(parts) == 2 && parts[1] != "" { + name = parts[1] + } + setting, use := "--project", "--project "+name + if source == ScopeSourceEnv { + setting, use = "DATUM_PROJECT", "DATUM_PROJECT="+name + } + return customerrors.NewUserErrorWithHint( + fmt.Sprintf("%s takes a project name, not org/project: %q", setting, project), + fmt.Sprintf("Use %s", use), + ) +} diff --git a/internal/client/scope_test.go b/internal/client/scope_test.go index fd0edc7..f96c015 100644 --- a/internal/client/scope_test.go +++ b/internal/client/scope_test.go @@ -1,9 +1,11 @@ package client import ( + "strings" "testing" "go.datum.net/datumctl/internal/datumconfig" + customerrors "go.datum.net/datumctl/internal/errors" ) func TestResolvedScopeDetails(t *testing.T) { @@ -93,3 +95,37 @@ func TestResolvedScopeDetails(t *testing.T) { }) } } + +func TestResolvedScopeRejectsOrgProjectForm(t *testing.T) { + tests := []struct { + name string + project string + envProject string + wantHint string + }{ + {name: "flag in the org/project form", project: "acme/web", wantHint: "Use --project web"}, + {name: "flag with an empty project segment", project: "acme/", wantHint: "Use --project "}, + {name: "flag with too many segments", project: "a/b/c", wantHint: "Use --project "}, + {name: "environment in the org/project form", envProject: "acme/web", wantHint: "Use DATUM_PROJECT=web"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := setupFactoryOverrideEnv(t) + t.Setenv("DATUM_PROJECT", tt.envProject) + t.Setenv("DATUM_ORGANIZATION", "") + *f.ConfigFlags.Project = tt.project + + _, err := f.ConfigFlags.ResolvedScopeDetails() + userErr, ok := customerrors.IsUserError(err) + if !ok { + t.Fatalf("err = %v, want a UserError", err) + } + if userErr.Hint != tt.wantHint { + t.Errorf("hint = %q, want %q", userErr.Hint, tt.wantHint) + } + if !strings.Contains(userErr.Message, "takes a project name, not org/project") { + t.Errorf("message = %q", userErr.Message) + } + }) + } +} diff --git a/internal/cmd/get_org_scope.go b/internal/cmd/get_org_scope.go new file mode 100644 index 0000000..47b483b --- /dev/null +++ b/internal/cmd/get_org_scope.go @@ -0,0 +1,49 @@ +package cmd + +import ( + "fmt" + "strings" + + "github.com/spf13/cobra" + + "go.datum.net/datumctl/internal/client" +) + +// getFatalHint is the hint for a Forbidden that the running get command +// expects. kubectl get reports its errors through cmdutil.CheckErr, which only +// passes the formatted message on, so the hint is recorded before the command +// runs and attached by FatalHint when the error is printed. +var getFatalHint string + +// FatalHint returns guidance to print after a fatal kubectl error message, or +// "" when there is none. +func FatalHint(msg string) string { + if getFatalHint == "" || !strings.Contains(msg, "(Forbidden)") { + return "" + } + return getFatalHint +} + +// orgScopeAllNamespacesHint returns the hint for a get across all namespaces +// at organization scope. The authorizer refuses a namespaced list with no +// namespace in an organization's control plane, whatever the caller's role, +// because an organization's resources live in a single namespace and project +// resources live in each project's own control plane. +func orgScopeAllNamespacesHint(c *cobra.Command, factory *client.DatumCloudFactory, args []string) string { + allNamespaces, err := c.Flags().GetBool("all-namespaces") + if err != nil || !allNamespaces { + return "" + } + scope, err := factory.ConfigFlags.ResolvedScopeDetails() + if err != nil || scope.Organization == "" { + return "" + } + kind := "" + if len(args) > 0 { + kind, _, _ = strings.Cut(args[0], "/") + } + return fmt.Sprintf(`Listing across all namespaces is not allowed at organization scope. List a project's resources instead: + datumctl get %s --project -A +To find project names, run: + datumctl get projects --organization %s`, kind, scope.Organization) +} diff --git a/internal/cmd/get_org_scope_test.go b/internal/cmd/get_org_scope_test.go new file mode 100644 index 0000000..4edb0bd --- /dev/null +++ b/internal/cmd/get_org_scope_test.go @@ -0,0 +1,136 @@ +package cmd + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/spf13/cobra" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/cli-runtime/pkg/genericclioptions" + cmdutil "k8s.io/kubectl/pkg/cmd/util" + + "go.datum.net/datumctl/internal/client" +) + +// newFakeGet stands in for kubectl get: the flags the wrapper reads, and a Run +// that fails the way kubectl does when the server refuses the list. +func newFakeGet(t *testing.T, runErr error) *cobra.Command { + t.Helper() + factory, err := client.NewDatumFactory(context.Background()) + if err != nil { + t.Fatal(err) + } + cmd := &cobra.Command{ + Use: "get", + Run: func(cmd *cobra.Command, args []string) { cmdutil.CheckErr(runErr) }, + } + cmd.Flags().BoolP("all-namespaces", "A", false, "") + cmd.Flags().StringP("output", "o", "", "") + factory.AddFlags(cmd.Flags()) + return WrapGetCommand(cmd, factory, genericclioptions.IOStreams{Out: &bytes.Buffer{}, ErrOut: &bytes.Buffer{}}) +} + +func captureFatal(t *testing.T) *string { + t.Helper() + var got string + cmdutil.BehaviorOnFatal(func(msg string, code int) { got = msg }) + t.Cleanup(cmdutil.DefaultBehaviorOnFatal) + t.Cleanup(func() { getFatalHint = "" }) + return &got +} + +func TestGetForbiddenAllNamespacesHint(t *testing.T) { + forbidden := apierrors.NewForbidden( + schema.GroupResource{Group: "dns.networking.miloapis.com", Resource: "dnszones"}, "", + errFake("cross-namespace queries not allowed for organization-scoped requests"), + ) + notFound := apierrors.NewNotFound(schema.GroupResource{Resource: "dnszones"}, "zone-a") + + tests := []struct { + name string + args []string + runErr error + wantHint []string + }{ + { + name: "organization scope from the context", + args: []string{"dnszones", "-A"}, + runErr: forbidden, + wantHint: []string{"datumctl get dnszones --project -A", "datumctl get projects --organization org-prod"}, + }, + { + name: "organization flag", + args: []string{"dnszones", "--all-namespaces", "--organization", "acme"}, + runErr: forbidden, + wantHint: []string{"datumctl get dnszones --project -A", "datumctl get projects --organization acme"}, + }, + { + name: "project scope gets no hint", + args: []string{"dnszones", "-A", "--project", "web"}, + runErr: forbidden, + }, + { + name: "organization scope without -A gets no hint", + args: []string{"dnszones", "--organization", "acme"}, + runErr: forbidden, + }, + { + name: "an error other than Forbidden gets no hint", + args: []string{"dnszones", "-A", "--organization", "acme"}, + runErr: notFound, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setupOverrideEnv(t) + fatal := captureFatal(t) + + cmd := newFakeGet(t, tt.runErr) + cmd.SetArgs(tt.args) + if err := cmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + if *fatal == "" { + t.Fatal("the wrapped get command never reported its error") + } + + hint := FatalHint(*fatal) + if len(tt.wantHint) == 0 { + if hint != "" { + t.Errorf("expected no hint, got %q", hint) + } + return + } + for _, want := range tt.wantHint { + if !strings.Contains(hint, want) { + t.Errorf("hint %q does not contain %q", hint, want) + } + } + }) + } +} + +type errFake string + +func (e errFake) Error() string { return string(e) } + +func TestGetExampleListsAllNamespacesInAProject(t *testing.T) { + setupOverrideEnv(t) + get, _, err := RootCmd().Find([]string{"get"}) + if err != nil { + t.Fatal(err) + } + for _, line := range strings.Split(get.Example, "\n") { + if strings.Contains(line, "--all-namespaces") || strings.Contains(line, " -A") { + if strings.Contains(line, "--organization") { + t.Errorf("example lists across all namespaces at organization scope, which the server refuses: %q", line) + } + } + } + if !strings.Contains(get.Example, "--project --all-namespaces") { + t.Error("example should show listing across all namespaces in a project") + } +} diff --git a/internal/cmd/get_organizations.go b/internal/cmd/get_organizations.go index bd77adb..75896b1 100644 --- a/internal/cmd/get_organizations.go +++ b/internal/cmd/get_organizations.go @@ -43,6 +43,8 @@ func WrapGetCommand(cmd *cobra.Command, factory *datumclient.DatumCloudFactory, factory.ConfigFlags.ForceUserControlPlane = true defer func() { factory.ConfigFlags.ForceUserControlPlane = prev }() _ = c.Flags().Set("all-namespaces", "true") + } else { + getFatalHint = orgScopeAllNamespacesHint(c, factory, args) } if origRunE != nil { diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 50ab137..008dea9 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -381,8 +381,8 @@ The 'organizations' shorthand also works with 'datumctl delete', 'datumctl edit' # List DNS zones in a project namespace datumctl get dnszones --project --namespace default - # List all resources of a type across all namespaces - datumctl get dnszones --organization --all-namespaces + # List all resources of a type across every namespace in a project + datumctl get dnszones --project --all-namespaces # Output as YAML datumctl get project my-project-id --organization -o yaml diff --git a/main.go b/main.go index e5590e5..802f56c 100644 --- a/main.go +++ b/main.go @@ -42,13 +42,17 @@ func main() { // re-encode the message inside the JSON/YAML envelope. util.BehaviorOnFatal(func(msg string, code int) { msg = strings.TrimSuffix(msg, "\n") + hint := cmd.FatalHint(msg) format := formatFor(rootCmd) if format == customerrors.FormatHuman { fmt.Fprintln(os.Stderr, msg) + if hint != "" { + fmt.Fprintln(os.Stderr, hint) + } os.Exit(code) } clean := strings.TrimPrefix(msg, "error: ") - customerrors.Format(os.Stderr, errors.New(clean), format, verbosity()) + customerrors.Format(os.Stderr, customerrors.NewUserErrorWithHint(clean, hint), format, verbosity()) os.Exit(code) })