pr comments

coder · sreya · Oct 25, 2024 · Oct 5, 2024 · Oct 5, 2024 · Oct 5, 2024
commit 87828a2f04476845eeea6c7b70b17d2aa09525d6
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -492,10 +492,7 @@ func New(options *Options) *API {
 	// Start a background process that rotates keys. We intentionally start this after the caches
 	// are created to force initial requests for a key to populate the caches. This helps catch
 	// bugs that may only occur when a key isn't precached in tests and the latency cost is minimal.
-	err = cryptokeys.StartRotator(ctx, options.Logger, options.Database)
-	if err != nil {
-		must(options.Logger, "failed to start key rotator", err)
-	}
+	cryptokeys.StartRotator(ctx, options.Logger, options.Database)
 
 	api := &API{
 		ctx:          ctx,
@@ -659,7 +656,7 @@ func New(options *Options) *API {
 		ResumeTokenProvider:     api.Options.CoordinatorResumeTokenProvider,
 	})
 	if err != nil {
-		must(api.Logger, "failed to initialize tailnet client service", err)
+		api.Logger.Fatal(context.Background(), "failed to initialize tailnet client service", slog.Error(err))
 	}
 
 	api.statsReporter = workspacestats.NewReporter(workspacestats.ReporterOptions{
@@ -1657,9 +1654,3 @@ func ReadExperiments(log slog.Logger, raw []string) codersdk.Experiments {
 	}
 	return exps
 }
-
-func must(logger slog.Logger, msg string, err error) {
-	if err != nil {
-		logger.Fatal(context.Background(), msg, slog.Error(err))
-	}
-}
diff --git a/coderd/cryptokeys/cache.go b/coderd/cryptokeys/cache.go
@@ -110,7 +110,7 @@ func NewSigningCache(ctx context.Context, logger slog.Logger, fetcher Fetcher,
 		return nil, xerrors.Errorf("invalid feature: %s", feature)
 	}
 	logger = logger.Named(fmt.Sprintf("%s_signing_keycache", feature))
-	return newCache(ctx, logger, fetcher, feature, opts...)
+	return newCache(ctx, logger, fetcher, feature, opts...), nil
 }
 
 func NewEncryptionCache(ctx context.Context, logger slog.Logger, fetcher Fetcher,
@@ -120,10 +120,10 @@ func NewEncryptionCache(ctx context.Context, logger slog.Logger, fetcher Fetcher
 		return nil, xerrors.Errorf("invalid feature: %s", feature)
 	}
 	logger = logger.Named(fmt.Sprintf("%s_encryption_keycache", feature))
-	return newCache(ctx, logger, fetcher, feature, opts...)
+	return newCache(ctx, logger, fetcher, feature, opts...), nil
 }
 
-func newCache(ctx context.Context, logger slog.Logger, fetcher Fetcher, feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (*cache, error) {
+func newCache(ctx context.Context, logger slog.Logger, fetcher Fetcher, feature codersdk.CryptoKeyFeature, opts ...func(*cache)) *cache {
 	cache := &cache{
 		clock:   quartz.NewReal(),
 		logger:  logger,
@@ -142,11 +142,10 @@ func newCache(ctx context.Context, logger slog.Logger, fetcher Fetcher, feature
 
 	keys, err := cache.cryptoKeys(cache.ctx)
 	if err != nil {
-		cache.cancel()
-		return nil, xerrors.Errorf("initial fetch: %w", err)
+		cache.logger.Critical(cache.ctx, "failed initial fetch", slog.Error(err))
 	}
 	cache.keys = keys
-	return cache, nil
+	return cache
 }
 
 func (c *cache) EncryptingKey(ctx context.Context) (string, interface{}, error) {

diff --git a/coderd/cryptokeys/rotate.go b/coderd/cryptokeys/rotate.go
@@ -54,7 +54,7 @@ func WithKeyDuration(keyDuration time.Duration) RotatorOption {
 // StartRotator starts a background process that rotates keys in the database.
 // It ensures there's at least one valid key per feature prior to returning.
 // Canceling the provided context will stop the background process.
-func StartRotator(ctx context.Context, logger slog.Logger, db database.Store, opts ...RotatorOption) error {
+func StartRotator(ctx context.Context, logger slog.Logger, db database.Store, opts ...RotatorOption) {
 	//nolint:gocritic // KeyRotator can only rotate crypto keys.
 	ctx = dbauthz.AsKeyRotator(ctx)
 	kr := &rotator{
@@ -71,12 +71,10 @@ func StartRotator(ctx context.Context, logger slog.Logger, db database.Store, op
 
 	err := kr.rotateKeys(ctx)
 	if err != nil {
-		return xerrors.Errorf("rotate keys: %w", err)
+		kr.logger.Critical(ctx, "failed to rotate keys", slog.Error(err))
 	}
 
 	go kr.start(ctx)
-
-	return nil
 }
 
 // start begins the process of rotating keys.

diff --git a/coderd/cryptokeys/rotate_test.go b/coderd/cryptokeys/rotate_test.go
@@ -34,8 +34,7 @@ func TestRotator(t *testing.T) {
 		require.NoError(t, err)
 		require.Len(t, dbkeys, 0)
 
-		err = cryptokeys.StartRotator(ctx, logger, db, cryptokeys.WithClock(clock))
-		require.NoError(t, err)
+		cryptokeys.StartRotator(ctx, logger, db, cryptokeys.WithClock(clock))
 
 		// Fetch the keys from the database and ensure they
 		// are as expected.
@@ -66,8 +65,7 @@ func TestRotator(t *testing.T) {
 		trap := clock.Trap().TickerFunc()
 		t.Cleanup(trap.Close)
 
-		err := cryptokeys.StartRotator(ctx, logger, db, cryptokeys.WithClock(clock))
-		require.NoError(t, err)
+		cryptokeys.StartRotator(ctx, logger, db, cryptokeys.WithClock(clock))
 
 		initialKeyLen := len(database.AllCryptoKeyFeatureValues())
 		// Fetch the keys from the database and ensure they

diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go
@@ -654,50 +654,19 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
 					appClient.SetSessionToken("")
 					u := *c.appURL
 					u.Path = path.Join(u.Path, "/test")
-					req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil)
-					require.NoError(t, err)
-
-					var resp *http.Response
-					resp, err = doWithRetries(t, appClient, req)
-					require.NoError(t, err)
-
-					if !assert.Equal(t, http.StatusSeeOther, resp.StatusCode) {
-						dump, err := httputil.DumpResponse(resp, true)
-						require.NoError(t, err)
-						t.Log(string(dump))
-					}
-					resp.Body.Close()
-
-					// Check that the Location is correct.
-					gotLocation, err := resp.Location()
-					require.NoError(t, err)
-					// This should always redirect to the primary access URL.
-					require.Equal(t, appDetails.SDKClient.URL.Host, gotLocation.Host)
-					require.Equal(t, "/api/v2/applications/auth-redirect", gotLocation.Path)
-					require.Equal(t, u.String(), gotLocation.Query().Get("redirect_uri"))
-
-					// Load the application auth-redirect endpoint.
-					resp, err = requestWithRetries(ctx, t, appDetails.SDKClient, http.MethodGet, "/api/v2/applications/auth-redirect", nil, codersdk.WithQueryParam(
-						"redirect_uri", u.String(),
-					))
-					require.NoError(t, err)
-					defer resp.Body.Close()
-
-					require.Equal(t, http.StatusSeeOther, resp.StatusCode)
-					gotLocation, err = resp.Location()
-					require.NoError(t, err)
-
 					badToken := generateBadJWE(t, workspaceapps.EncryptedAPIKeyPayload{
 						APIKey: currentKeyStr,
 					})
 
-					gotLocation.RawQuery = (url.Values{
+					u.RawQuery = (url.Values{
 						workspaceapps.SubdomainProxyAPIKeyParam: {badToken},
 					}).Encode()
 
-					req, err = http.NewRequestWithContext(ctx, "GET", gotLocation.String(), nil)
+					req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil)
 					require.NoError(t, err)
-					resp, err = appClient.HTTPClient.Do(req)
+
+					var resp *http.Response
+					resp, err = doWithRetries(t, appClient, req)
 					require.NoError(t, err)
 					defer resp.Body.Close()
 					require.Equal(t, http.StatusBadRequest, resp.StatusCode)