From bc4ae532617dd920f7015f4f073c2e18007c6ab4 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Mon, 29 Jan 2024 12:17:31 +0400 Subject: [PATCH] chore: refactor Appearance to an interface callable by AGPL code (#11769) The new Agent API needs an interface for ServiceBanners, so this PR creates it and refactors the AGPL and Enterprise code to achieve it. Before we depended on the fact that the HTTP endpoint was missing to serve an empty ServiceBanner on AGPL deployments, but that won't work with dRPC, so we need a real interface to call. --- coderd/appearance/appearance.go | 39 ++++++++++++++++++++ coderd/coderd.go | 37 ++++++++++--------- enterprise/coderd/appearance.go | 53 ++++++++++----------------- enterprise/coderd/appearance_test.go | 18 ++++----- enterprise/coderd/coderd.go | 15 +++++++- site/site.go | 55 +++++++++++++++------------- 6 files changed, 130 insertions(+), 87 deletions(-) create mode 100644 coderd/appearance/appearance.go diff --git a/coderd/appearance/appearance.go b/coderd/appearance/appearance.go new file mode 100644 index 0000000000..1ac61dea21 --- /dev/null +++ b/coderd/appearance/appearance.go @@ -0,0 +1,39 @@ +package appearance + +import ( + "context" + + "github.com/coder/coder/v2/codersdk" +) + +type Fetcher interface { + Fetch(ctx context.Context) (codersdk.AppearanceConfig, error) +} + +var DefaultSupportLinks = []codersdk.LinkConfig{ + { + Name: "Documentation", + Target: "https://coder.com/docs/coder-oss", + Icon: "docs", + }, + { + Name: "Report a bug", + Target: "https://github.com/coder/coder/issues/new?labels=needs+grooming&body={CODER_BUILD_INFO}", + Icon: "bug", + }, + { + Name: "Join the Coder Discord", + Target: "https://coder.com/chat?utm_source=coder&utm_medium=coder&utm_campaign=server-footer", + Icon: "chat", + }, +} + +type AGPLFetcher struct{} + +func (AGPLFetcher) Fetch(context.Context) (codersdk.AppearanceConfig, error) { + return codersdk.AppearanceConfig{ + SupportLinks: DefaultSupportLinks, + }, nil +} + +var DefaultFetcher Fetcher = AGPLFetcher{} diff --git a/coderd/coderd.go b/coderd/coderd.go index 7afbbc1f44..7eddb20a91 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -17,10 +17,6 @@ import ( "sync/atomic" "time" - "github.com/coder/coder/v2/coderd/prometheusmetrics" - - agentproto "github.com/coder/coder/v2/agent/proto" - "github.com/andybalholm/brotli" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" @@ -40,11 +36,12 @@ import ( "tailscale.com/util/singleflight" "cdr.dev/slog" + + agentproto "github.com/coder/coder/v2/agent/proto" "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/cli/clibase" - - // Used for swagger docs. - _ "github.com/coder/coder/v2/coderd/apidoc" + _ "github.com/coder/coder/v2/coderd/apidoc" // Used for swagger docs. + "github.com/coder/coder/v2/coderd/appearance" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/awsidentity" "github.com/coder/coder/v2/coderd/batchstats" @@ -59,6 +56,7 @@ import ( "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/metricscache" + "github.com/coder/coder/v2/coderd/prometheusmetrics" "github.com/coder/coder/v2/coderd/provisionerdserver" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/schedule" @@ -357,16 +355,6 @@ func New(options *Options) *API { OIDC: options.OIDCConfig, } - staticHandler := site.New(&site.Options{ - BinFS: binFS, - BinHashes: binHashes, - Database: options.Database, - SiteFS: site.FS(), - OAuth2Configs: oauthConfigs, - DocsURL: options.DeploymentValues.DocsURL.String(), - }) - staticHandler.Experiments.Store(&experiments) - ctx, cancel := context.WithCancel(context.Background()) r := chi.NewRouter() @@ -383,7 +371,6 @@ func New(options *Options) *API { ID: uuid.New(), Options: options, RootHandler: r, - SiteHandler: staticHandler, HTTPAuth: &HTTPAuthorizer{ Authorizer: options.Authorizer, Logger: options.Logger, @@ -412,6 +399,19 @@ func New(options *Options) *API { options.Database, options.Pubsub), } + + api.AppearanceFetcher.Store(&appearance.DefaultFetcher) + api.SiteHandler = site.New(&site.Options{ + BinFS: binFS, + BinHashes: binHashes, + Database: options.Database, + SiteFS: site.FS(), + OAuth2Configs: oauthConfigs, + DocsURL: options.DeploymentValues.DocsURL.String(), + AppearanceFetcher: &api.AppearanceFetcher, + }) + api.SiteHandler.Experiments.Store(&experiments) + if options.UpdateCheckOptions != nil { api.updateChecker = updatecheck.New( options.Database, @@ -1089,6 +1089,7 @@ type API struct { TailnetCoordinator atomic.Pointer[tailnet.Coordinator] TailnetClientService *tailnet.ClientService QuotaCommitter atomic.Pointer[proto.QuotaCommitter] + AppearanceFetcher atomic.Pointer[appearance.Fetcher] // WorkspaceProxyHostsFn returns the hosts of healthy workspace proxies // for header reasons. WorkspaceProxyHostsFn atomic.Pointer[func() []string] diff --git a/enterprise/coderd/appearance.go b/enterprise/coderd/appearance.go index aceeec9ef1..70ef238d60 100644 --- a/enterprise/coderd/appearance.go +++ b/enterprise/coderd/appearance.go @@ -11,29 +11,13 @@ import ( "golang.org/x/sync/errgroup" "golang.org/x/xerrors" + agpl "github.com/coder/coder/v2/coderd/appearance" + "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" ) -var DefaultSupportLinks = []codersdk.LinkConfig{ - { - Name: "Documentation", - Target: "https://coder.com/docs/coder-oss", - Icon: "docs", - }, - { - Name: "Report a bug", - Target: "https://github.com/coder/coder/issues/new?labels=needs+grooming&body={CODER_BUILD_INFO}", - Icon: "bug", - }, - { - Name: "Join the Coder Discord", - Target: "https://coder.com/chat?utm_source=coder&utm_medium=coder&utm_campaign=server-footer", - Icon: "chat", - }, -} - // @Summary Get appearance // @ID get-appearance // @Security CoderSessionToken @@ -42,7 +26,8 @@ var DefaultSupportLinks = []codersdk.LinkConfig{ // @Success 200 {object} codersdk.AppearanceConfig // @Router /appearance [get] func (api *API) appearance(rw http.ResponseWriter, r *http.Request) { - cfg, err := api.fetchAppearanceConfig(r.Context()) + af := *api.AGPL.AppearanceFetcher.Load() + cfg, err := af.Fetch(r.Context()) if err != nil { httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{ Message: "Failed to fetch appearance config.", @@ -54,37 +39,39 @@ func (api *API) appearance(rw http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), rw, http.StatusOK, cfg) } -func (api *API) fetchAppearanceConfig(ctx context.Context) (codersdk.AppearanceConfig, error) { - api.entitlementsMu.RLock() - isEntitled := api.entitlements.Features[codersdk.FeatureAppearance].Entitlement == codersdk.EntitlementEntitled - api.entitlementsMu.RUnlock() +type appearanceFetcher struct { + database database.Store + supportLinks []codersdk.LinkConfig +} - if !isEntitled { - return codersdk.AppearanceConfig{ - SupportLinks: DefaultSupportLinks, - }, nil +func newAppearanceFetcher(store database.Store, links []codersdk.LinkConfig) agpl.Fetcher { + return &appearanceFetcher{ + database: store, + supportLinks: links, } +} +func (f *appearanceFetcher) Fetch(ctx context.Context) (codersdk.AppearanceConfig, error) { var eg errgroup.Group var applicationName string var logoURL string var serviceBannerJSON string eg.Go(func() (err error) { - applicationName, err = api.Database.GetApplicationName(ctx) + applicationName, err = f.database.GetApplicationName(ctx) if err != nil && !errors.Is(err, sql.ErrNoRows) { return xerrors.Errorf("get application name: %w", err) } return nil }) eg.Go(func() (err error) { - logoURL, err = api.Database.GetLogoURL(ctx) + logoURL, err = f.database.GetLogoURL(ctx) if err != nil && !errors.Is(err, sql.ErrNoRows) { return xerrors.Errorf("get logo url: %w", err) } return nil }) eg.Go(func() (err error) { - serviceBannerJSON, err = api.Database.GetServiceBanner(ctx) + serviceBannerJSON, err = f.database.GetServiceBanner(ctx) if err != nil && !errors.Is(err, sql.ErrNoRows) { return xerrors.Errorf("get service banner: %w", err) } @@ -108,10 +95,10 @@ func (api *API) fetchAppearanceConfig(ctx context.Context) (codersdk.AppearanceC } } - if len(api.DeploymentValues.Support.Links.Value) == 0 { - cfg.SupportLinks = DefaultSupportLinks + if len(f.supportLinks) == 0 { + cfg.SupportLinks = agpl.DefaultSupportLinks } else { - cfg.SupportLinks = api.DeploymentValues.Support.Links.Value + cfg.SupportLinks = f.supportLinks } return cfg, nil diff --git a/enterprise/coderd/appearance_test.go b/enterprise/coderd/appearance_test.go index 493bfd7cc5..bb15a700ce 100644 --- a/enterprise/coderd/appearance_test.go +++ b/enterprise/coderd/appearance_test.go @@ -6,19 +6,17 @@ import ( "net/http" "testing" - "github.com/coder/coder/v2/coderd/database/dbtestutil" - - "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbfake" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/cli/clibase" + "github.com/coder/coder/v2/coderd/appearance" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbfake" + "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/agentsdk" - "github.com/coder/coder/v2/enterprise/coderd" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" "github.com/coder/coder/v2/enterprise/coderd/license" "github.com/coder/coder/v2/testutil" @@ -214,9 +212,9 @@ func TestCustomSupportLinks(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) defer cancel() - appearance, err := anotherClient.Appearance(ctx) + appr, err := anotherClient.Appearance(ctx) require.NoError(t, err) - require.Equal(t, supportLinks, appearance.SupportLinks) + require.Equal(t, supportLinks, appr.SupportLinks) } func TestDefaultSupportLinks(t *testing.T) { @@ -229,7 +227,7 @@ func TestDefaultSupportLinks(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) defer cancel() - appearance, err := anotherClient.Appearance(ctx) + appr, err := anotherClient.Appearance(ctx) require.NoError(t, err) - require.Equal(t, coderd.DefaultSupportLinks, appearance.SupportLinks) + require.Equal(t, appearance.DefaultSupportLinks, appr.SupportLinks) } diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index af56626a8d..584f4627b8 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -14,6 +14,8 @@ import ( "sync" "time" + "github.com/coder/coder/v2/coderd/appearance" + "golang.org/x/xerrors" "tailscale.com/tailcfg" @@ -118,7 +120,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { } api.AGPL.Options.SetUserGroups = api.setUserGroups api.AGPL.Options.SetUserSiteRoles = api.setUserSiteRoles - api.AGPL.SiteHandler.AppearanceFetcher = api.fetchAppearanceConfig api.AGPL.SiteHandler.RegionsFetcher = func(ctx context.Context) (any, error) { // If the user can read the workspace proxy resource, return that. // If not, always default to the regions. @@ -677,6 +678,18 @@ func (api *API) updateEntitlements(ctx context.Context) error { api.AGPL.AccessControlStore.Store(&acs) } + if initial, changed, enabled := featureChanged(codersdk.FeatureAppearance); shouldUpdate(initial, changed, enabled) { + if enabled { + f := newAppearanceFetcher( + api.Database, + api.DeploymentValues.Support.Links.Value, + ) + api.AGPL.AppearanceFetcher.Store(&f) + } else { + api.AGPL.AppearanceFetcher.Store(&appearance.DefaultFetcher) + } + } + // External token encryption is soft-enforced featureExternalTokenEncryption := entitlements.Features[codersdk.FeatureExternalTokenEncryption] featureExternalTokenEncryption.Enabled = len(api.ExternalTokenEncryption) > 0 diff --git a/site/site.go b/site/site.go index 7e39dbeab1..ba6004fc3c 100644 --- a/site/site.go +++ b/site/site.go @@ -35,6 +35,7 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/v2/buildinfo" + "github.com/coder/coder/v2/coderd/appearance" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" @@ -61,15 +62,21 @@ func init() { } type Options struct { - BinFS http.FileSystem - BinHashes map[string]string - Database database.Store - SiteFS fs.FS - OAuth2Configs *httpmw.OAuth2Configs - DocsURL string + BinFS http.FileSystem + BinHashes map[string]string + Database database.Store + SiteFS fs.FS + OAuth2Configs *httpmw.OAuth2Configs + DocsURL string + AppearanceFetcher *atomic.Pointer[appearance.Fetcher] } func New(opts *Options) *Handler { + if opts.AppearanceFetcher == nil { + daf := atomic.Pointer[appearance.Fetcher]{} + daf.Store(&appearance.DefaultFetcher) + opts.AppearanceFetcher = &daf + } handler := &Handler{ opts: opts, secureHeaders: secureHeaders(), @@ -147,7 +154,6 @@ type Handler struct { buildInfoJSON string - AppearanceFetcher func(ctx context.Context) (codersdk.AppearanceConfig, error) // RegionsFetcher will attempt to fetch the more detailed WorkspaceProxy data, but will fall back to the // regions if the user does not have the correct permissions. RegionsFetcher func(ctx context.Context) (any, error) @@ -291,6 +297,7 @@ func execTmpl(tmpl *template.Template, state htmlState) ([]byte, error) { // renderWithState will render the file using the given nonce if the file exists // as a template. If it does not, it will return an error. func (h *Handler) renderHTMLWithState(r *http.Request, filePath string, state htmlState) ([]byte, error) { + af := *(h.opts.AppearanceFetcher.Load()) if filePath == "" { filePath = "index.html" } @@ -317,11 +324,9 @@ func (h *Handler) renderHTMLWithState(r *http.Request, filePath string, state ht }) if !ok || apiKey == nil || actor == nil { var cfg codersdk.AppearanceConfig - if h.AppearanceFetcher != nil { - // nolint:gocritic // User is not expected to be signed in. - ctx := dbauthz.AsSystemRestricted(r.Context()) - cfg, _ = h.AppearanceFetcher(ctx) - } + // nolint:gocritic // User is not expected to be signed in. + ctx := dbauthz.AsSystemRestricted(r.Context()) + cfg, _ = af.Fetch(ctx) state.ApplicationName = applicationNameOrDefault(cfg) state.LogoURL = cfg.LogoURL return execTmpl(tmpl, state) @@ -370,21 +375,21 @@ func (h *Handler) renderHTMLWithState(r *http.Request, filePath string, state ht } }() } - if h.AppearanceFetcher != nil { - wg.Add(1) - go func() { - defer wg.Done() - cfg, err := h.AppearanceFetcher(ctx) + + wg.Add(1) + go func() { + defer wg.Done() + cfg, err := af.Fetch(ctx) + if err == nil { + appr, err := json.Marshal(cfg) if err == nil { - appearance, err := json.Marshal(cfg) - if err == nil { - state.Appearance = html.EscapeString(string(appearance)) - state.ApplicationName = applicationNameOrDefault(cfg) - state.LogoURL = cfg.LogoURL - } + state.Appearance = html.EscapeString(string(appr)) + state.ApplicationName = applicationNameOrDefault(cfg) + state.LogoURL = cfg.LogoURL } - }() - } + } + }() + if h.RegionsFetcher != nil { wg.Add(1) go func() {