diff --git a/go.mod b/go.mod index 62e519c..7187cd6 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,7 @@ require ( github.com/coreos/go-oidc/v3 v3.19.0 github.com/fatih/color v1.19.0 github.com/gkampitakis/go-snaps v0.5.22 - github.com/gleanwork/api-client-go v0.12.0 + github.com/gleanwork/api-client-go v0.13.3 github.com/int128/oauth2cli v1.18.0 github.com/minio/selfupdate v0.6.0 github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c diff --git a/go.sum b/go.sum index 35789f2..7518e12 100644 --- a/go.sum +++ b/go.sum @@ -66,8 +66,8 @@ github.com/gkampitakis/ciinfo v0.3.4 h1:5eBSibVuSMbb/H6Elc0IIEFbkzCJi3lm94n0+U7Z github.com/gkampitakis/ciinfo v0.3.4/go.mod h1:1NIwaOcFChN4fa/B0hEBdAb6npDlFL8Bwx4dfRLRqAo= github.com/gkampitakis/go-snaps v0.5.22 h1:xg9omphRnbDnimMCl1KqznC4krlxOGpkB0vDSfX2P7M= github.com/gkampitakis/go-snaps v0.5.22/go.mod h1:uy3lVzCCRRsAwYqSocyw5fY8xRLCYEfqoOJNxr8HonM= -github.com/gleanwork/api-client-go v0.12.0 h1:umtm2Fa1rRqFcBKWDpJwqGbekAysvb7jjvPSs78CB/c= -github.com/gleanwork/api-client-go v0.12.0/go.mod h1:EL9EbKruPM+pb75cD1ivKVIaM86ysyro13Ssl/EDVTo= +github.com/gleanwork/api-client-go v0.13.3 h1:CT+jYZVw+q8zSWZBbmWXsYKFH0x/66aFoI3RvXce7Yc= +github.com/gleanwork/api-client-go v0.13.3/go.mod h1:EL9EbKruPM+pb75cD1ivKVIaM86ysyro13Ssl/EDVTo= github.com/go-jose/go-jose/v4 v4.1.4 h1:moDMcTHmvE6Groj34emNPLs/qtYXRVcd6S7NHbHz3kA= github.com/go-jose/go-jose/v4 v4.1.4/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM= diff --git a/internal/client/client.go b/internal/client/client.go index b4a30b0..7344571 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -110,6 +110,12 @@ func New(cfg *config.Config) (*glean.Glean, error) { opts := []glean.SDKOption{ glean.WithServerURL(cfg.GleanServerURL), glean.WithSecurity(token), + // Always opt in to experimental platform endpoints (/api/*). The CLI + // is platform-first: migrated commands call platform endpoints by + // default and fall back to the classic API when the tenant gate is + // off (see internal/platform). The X_GLEAN_INCLUDE_EXPERIMENTAL env + // var takes precedence over this option inside the SDK. + glean.WithIncludeExperimental(true), glean.WithClient(&http.Client{ Transport: httputil.NewTransport(http.DefaultTransport, httputil.WithHeader("X-Glean-Auth-Type", authType), diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 143b7d2..d87d35e 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -4,9 +4,11 @@ import ( "context" "io" "net/http" + "net/http/httptest" "strings" "testing" + "github.com/gleanwork/api-client-go/models/components" "github.com/gleanwork/glean-cli/internal/config" "github.com/gleanwork/glean-cli/internal/httputil" "github.com/stretchr/testify/assert" @@ -108,6 +110,30 @@ func TestNew_Success(t *testing.T) { assert.NotNil(t, client) } +// TestNew_SendsExperimentalHeader proves the SDK client built by New opts in +// to experimental platform endpoints on the wire: every request must carry +// X-Glean-Include-Experimental: true (via glean.WithIncludeExperimental). +func TestNew_SendsExperimentalHeader(t *testing.T) { + var captured http.Header + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captured = r.Header.Clone() + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"results":[],"has_more":false,"next_cursor":null,"request_id":"req-1"}`)) + })) + defer server.Close() + + cfg := &config.Config{GleanServerURL: server.URL, GleanToken: "valid-token"} + sdk, err := New(cfg) + require.NoError(t, err) + + _, err = sdk.Search.Query(context.Background(), components.PlatformSearchRequest{Query: "test"}) + require.NoError(t, err) + + require.NotNil(t, captured, "server should have received a request") + assert.Equal(t, "true", captured.Get("X-Glean-Include-Experimental")) + assert.Equal(t, "Bearer valid-token", captured.Get("Authorization")) +} + // TestNew_FullHostnames verifies that custom-shape Glean server URLs (vanity // domains, non-"-be" suffixes, obfuscated subdomains) all produce a working // SDK client. Covers the bugs originally reported in #102. diff --git a/internal/platform/platform.go b/internal/platform/platform.go new file mode 100644 index 0000000..d3051bb --- /dev/null +++ b/internal/platform/platform.go @@ -0,0 +1,101 @@ +// Package platform decides whether a command call is served by the new +// platform APIs (/api/*) or the classic client APIs (/rest/api/v1/*). +// +// Migrated commands are platform-first: they call the platform endpoint and +// fall back to the classic equivalent when the tenant has not enabled +// experimental platform endpoints, which surfaces as a hidden 404. Setting +// GLEAN_LEGACY_APIS=1 skips platform endpoints entirely. +package platform + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "os" + "strings" + "sync" + + "github.com/gleanwork/api-client-go/models/apierrors" +) + +// EnvLegacy is the environment variable that forces migrated commands onto +// the classic client APIs. +const EnvLegacy = "GLEAN_LEGACY_APIS" + +// Legacy reports whether the user has opted out of platform APIs. +func Legacy() bool { + v := os.Getenv(EnvLegacy) + return v == "1" || strings.EqualFold(v, "true") +} + +// IsGateClosed reports whether err looks like the hidden 404 returned when a +// tenant has not enabled experimental platform endpoints. The platform stack +// answers gated endpoints with 404 resource_not_found, which is +// indistinguishable from a genuinely missing resource — callers on GET-by-ID +// endpoints therefore accept one spurious fallback attempt. +func IsGateClosed(err error) bool { + var problem *apierrors.PlatformProblemDetailError + if errors.As(err, &problem) { + return problem.Status == http.StatusNotFound + } + var apiErr *apierrors.APIError + if errors.As(err, &apiErr) { + return apiErr.StatusCode == http.StatusNotFound + } + return false +} + +// GateClosedErr wraps a gate-closed error with guidance for call sites that +// cannot fall back automatically (user-authored request bodies are coupled to +// the endpoint's request shape, so silently replaying them against the legacy +// endpoint would mangle the request). +func GateClosedErr(endpoint string, err error) error { + return fmt.Errorf("platform API %s is unavailable on this instance; re-run with %s=1 and a legacy-shaped request body: %w", endpoint, EnvLegacy, err) +} + +var ( + warnMu sync.Mutex + warnedEndpoints = map[string]struct{}{} +) + +// warnFallback prints the fallback warning at most once per endpoint per +// process, so a process that falls back on several endpoints names each one +// exactly once. +func warnFallback(stderr io.Writer, endpoint string) { + warnMu.Lock() + defer warnMu.Unlock() + if _, ok := warnedEndpoints[endpoint]; ok { + return + } + warnedEndpoints[endpoint] = struct{}{} + fmt.Fprintf(stderr, "Warning: platform API %s is unavailable on this instance; falling back to the legacy API. Set %s=1 to skip platform endpoints.\n", endpoint, EnvLegacy) +} + +// ResetWarnings re-arms the warn-once guards. Only for use in tests. +func ResetWarnings() { + warnMu.Lock() + defer warnMu.Unlock() + warnedEndpoints = map[string]struct{}{} +} + +// Run executes platformFn, falling back to legacyFn when GLEAN_LEGACY_APIS is +// set (silently) or when the platform endpoint is gated off (with a one-time +// stderr warning naming endpoint). Non-gate errors from platformFn — auth +// failures, validation errors, server errors — are returned as-is: only the +// hidden 404 triggers a fallback. viaLegacy tells the caller which surface +// produced the result, since the two surfaces return different shapes. +func Run(ctx context.Context, stderr io.Writer, endpoint string, platformFn, legacyFn func(context.Context) (any, error)) (result any, viaLegacy bool, err error) { + if Legacy() { + result, err = legacyFn(ctx) + return result, true, err + } + result, err = platformFn(ctx) + if err == nil || !IsGateClosed(err) { + return result, false, err + } + warnFallback(stderr, endpoint) + result, err = legacyFn(ctx) + return result, true, err +} diff --git a/internal/platform/platform_test.go b/internal/platform/platform_test.go new file mode 100644 index 0000000..aa93c4a --- /dev/null +++ b/internal/platform/platform_test.go @@ -0,0 +1,153 @@ +package platform + +import ( + "bytes" + "context" + "errors" + "fmt" + "net/http" + "testing" + + "github.com/gleanwork/api-client-go/models/apierrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// legacyResult is the sentinel legacyFn return value in Run tests. +const legacyResult = "legacy" + +func TestLegacy(t *testing.T) { + tests := []struct { + value string + want bool + }{ + {"", false}, + {"0", false}, + {"false", false}, + {"1", true}, + {"true", true}, + {"TRUE", true}, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("%q", tt.value), func(t *testing.T) { + t.Setenv(EnvLegacy, tt.value) + assert.Equal(t, tt.want, Legacy()) + }) + } +} + +func TestIsGateClosed(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"plain error", errors.New("boom"), false}, + {"problem detail 404", &apierrors.PlatformProblemDetailError{Status: http.StatusNotFound}, true}, + {"problem detail 404 wrapped", fmt.Errorf("search failed: %w", &apierrors.PlatformProblemDetailError{Status: http.StatusNotFound}), true}, + {"problem detail 401", &apierrors.PlatformProblemDetailError{Status: http.StatusUnauthorized}, false}, + {"problem detail 500", &apierrors.PlatformProblemDetailError{Status: http.StatusInternalServerError}, false}, + {"api error 404", &apierrors.APIError{StatusCode: http.StatusNotFound}, true}, + {"api error 403", &apierrors.APIError{StatusCode: http.StatusForbidden}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, IsGateClosed(tt.err)) + }) + } +} + +func TestRun_PlatformSuccess(t *testing.T) { + ResetWarnings() + stderr := &bytes.Buffer{} + result, viaLegacy, err := Run(context.Background(), stderr, "/api/search", + func(context.Context) (any, error) { return "platform", nil }, + func(context.Context) (any, error) { t.Fatal("legacyFn must not run"); return nil, nil }, + ) + require.NoError(t, err) + assert.Equal(t, "platform", result) + assert.False(t, viaLegacy) + assert.Empty(t, stderr.String()) +} + +func TestRun_GateClosedFallsBack(t *testing.T) { + ResetWarnings() + stderr := &bytes.Buffer{} + gateErr := &apierrors.PlatformProblemDetailError{Status: http.StatusNotFound} + result, viaLegacy, err := Run(context.Background(), stderr, "/api/search", + func(context.Context) (any, error) { return nil, gateErr }, + func(context.Context) (any, error) { return legacyResult, nil }, + ) + require.NoError(t, err) + assert.Equal(t, legacyResult, result) + assert.True(t, viaLegacy) + assert.Contains(t, stderr.String(), "platform API /api/search is unavailable") + assert.Contains(t, stderr.String(), EnvLegacy) +} + +func TestRun_WarnsOnlyOnce(t *testing.T) { + ResetWarnings() + stderr := &bytes.Buffer{} + gateErr := &apierrors.APIError{StatusCode: http.StatusNotFound} + for range 2 { + _, _, err := Run(context.Background(), stderr, "/api/search", + func(context.Context) (any, error) { return nil, gateErr }, + func(context.Context) (any, error) { return legacyResult, nil }, + ) + require.NoError(t, err) + } + assert.Equal(t, 1, bytes.Count(stderr.Bytes(), []byte("Warning:"))) +} + +func TestRun_WarnsPerEndpoint(t *testing.T) { + ResetWarnings() + stderr := &bytes.Buffer{} + gateErr := &apierrors.APIError{StatusCode: http.StatusNotFound} + for _, endpoint := range []string{"/api/search", "/api/agents/search"} { + _, _, err := Run(context.Background(), stderr, endpoint, + func(context.Context) (any, error) { return nil, gateErr }, + func(context.Context) (any, error) { return legacyResult, nil }, + ) + require.NoError(t, err) + } + assert.Equal(t, 2, bytes.Count(stderr.Bytes(), []byte("Warning:")), "each endpoint warns once") +} + +func TestRun_NonGateErrorNoFallback(t *testing.T) { + ResetWarnings() + stderr := &bytes.Buffer{} + authErr := &apierrors.PlatformProblemDetailError{Status: http.StatusUnauthorized} + _, viaLegacy, err := Run(context.Background(), stderr, "/api/search", + func(context.Context) (any, error) { return nil, authErr }, + func(context.Context) (any, error) { + t.Fatal("legacyFn must not run on non-404 errors") + return nil, nil + }, + ) + require.Error(t, err) + assert.False(t, viaLegacy) + assert.Empty(t, stderr.String()) +} + +func TestRun_LegacyEnvBypassesPlatform(t *testing.T) { + ResetWarnings() + t.Setenv(EnvLegacy, "1") + stderr := &bytes.Buffer{} + result, viaLegacy, err := Run(context.Background(), stderr, "/api/search", + func(context.Context) (any, error) { t.Fatal("platformFn must not run in legacy mode"); return nil, nil }, + func(context.Context) (any, error) { return legacyResult, nil }, + ) + require.NoError(t, err) + assert.Equal(t, legacyResult, result) + assert.True(t, viaLegacy) + assert.Empty(t, stderr.String(), "legacy env opt-out is deliberate, no warning") +} + +func TestGateClosedErr(t *testing.T) { + base := &apierrors.PlatformProblemDetailError{Status: http.StatusNotFound} + err := GateClosedErr("/api/search", base) + assert.Contains(t, err.Error(), "/api/search") + assert.Contains(t, err.Error(), EnvLegacy) + assert.True(t, errors.Is(err, error(base)) || IsGateClosed(err), "must preserve the wrapped error") +}