diff --git a/e2e/embedded_integration_test.go b/e2e/embedded_integration_test.go index 021eed8..96e930f 100644 --- a/e2e/embedded_integration_test.go +++ b/e2e/embedded_integration_test.go @@ -14,7 +14,6 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" diff --git a/e2e/proxy_test.go b/e2e/proxy_test.go index bb7723f..f124b3e 100644 --- a/e2e/proxy_test.go +++ b/e2e/proxy_test.go @@ -9,7 +9,6 @@ import ( "strings" "time" - v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/samber/lo" @@ -29,6 +28,8 @@ import ( "k8s.io/client-go/tools/clientcmd" "k8s.io/utils/pointer" + v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" + "github.com/authzed/spicedb-kubeapi-proxy/pkg/authz/distributedtx" "github.com/authzed/spicedb-kubeapi-proxy/pkg/config/proxyrule" "github.com/authzed/spicedb-kubeapi-proxy/pkg/failpoints" diff --git a/e2e/util_test.go b/e2e/util_test.go index 26259ca..f059afa 100644 --- a/e2e/util_test.go +++ b/e2e/util_test.go @@ -9,8 +9,6 @@ import ( "os" goruntime "runtime" - v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" - "github.com/authzed/spicedb/pkg/tuple" "github.com/go-logr/logr" . "github.com/onsi/gomega" "github.com/samber/lo" @@ -20,6 +18,9 @@ import ( "sigs.k8s.io/controller-runtime/tools/setup-envtest/store" "sigs.k8s.io/controller-runtime/tools/setup-envtest/versions" "sigs.k8s.io/controller-runtime/tools/setup-envtest/workflows" + + v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" + "github.com/authzed/spicedb/pkg/tuple" ) // GetAllTuples collects all tuples matching the filter from SpiceDB diff --git a/magefiles/test.go b/magefiles/test.go index 0dae0cd..0e7cd13 100644 --- a/magefiles/test.go +++ b/magefiles/test.go @@ -18,10 +18,19 @@ func (t Test) All() error { // Unit runs the unit tests. func (t Test) Unit() error { - // TODO add -race - args := []string{"test", "-count=1"} - args = append(args, []string{"-coverpkg=./...", "-covermode=atomic", "-coverprofile=coverageunit.txt"}...) - args = append(args, "./...") + args := []string{ + "test", + // bypass cache + "-count=1", + // check for data races + "-race", + // coverage flags + "-coverpkg=./...", + "-covermode=atomic", + "-coverprofile=coverageunit.txt", + // package + "./...", + } return RunSh("go", WithV())(args...) } diff --git a/magefiles/util.go b/magefiles/util.go index 59b44d1..a1a0ad4 100644 --- a/magefiles/util.go +++ b/magefiles/util.go @@ -24,7 +24,6 @@ import ( "github.com/onsi/gomega/gexec" "golang.org/x/exp/slices" "sigs.k8s.io/kind/pkg/apis/config/v1alpha4" - kind "sigs.k8s.io/kind/pkg/cluster" "sigs.k8s.io/kind/pkg/cluster/nodeutils" "sigs.k8s.io/kind/pkg/cmd" diff --git a/pkg/proxy/authn_test.go b/pkg/proxy/authn_test.go index e297b50..af64f37 100644 --- a/pkg/proxy/authn_test.go +++ b/pkg/proxy/authn_test.go @@ -34,7 +34,7 @@ import ( ) func TestRequestHeaderAuth(t *testing.T) { - ctx, cancel := context.WithCancel(t.Context()) + ctx, cancel := context.WithCancel(testContext(t)) t.Cleanup(cancel) userInfo := runProxyRequest(t, ctx, map[string][]string{ @@ -96,6 +96,7 @@ func runProxyRequest(t testing.TB, ctx context.Context, headers map[string][]str } opts := NewOptions(WithEmbeddedSpiceDBEndpoint) + opts.SkipLoggerSetupForTesting = true opts.RestConfigFunc = func() (*rest.Config, http.RoundTripper, error) { ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -184,7 +185,7 @@ func runProxyRequest(t testing.TB, ctx context.Context, headers map[string][]str return rules.NewResolveInputFromHttp(req) }) - c, err := opts.Complete(t.Context()) + c, err := opts.Complete(testContext(t)) require.NoError(t, err) require.NotNil(t, c) @@ -192,7 +193,7 @@ func runProxyRequest(t testing.TB, ctx context.Context, headers map[string][]str require.NoError(t, err) // Start the server in a separate context that won't be cancelled until we're done - serverCtx, serverCancel := context.WithCancel(t.Context()) + serverCtx, serverCancel := context.WithCancel(testContext(t)) t.Cleanup(serverCancel) serverReady := make(chan struct{}) diff --git a/pkg/proxy/embedded_test.go b/pkg/proxy/embedded_test.go index 4c0389c..e6cae17 100644 --- a/pkg/proxy/embedded_test.go +++ b/pkg/proxy/embedded_test.go @@ -10,17 +10,30 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/endpoints/request" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" - logsv1 "k8s.io/component-base/logs/api/v1" + "k8s.io/klog/v2" + "k8s.io/klog/v2/ktesting" "github.com/authzed/spicedb-kubeapi-proxy/pkg/rules" ) +// testContext returns a context whose klog logger writes to t.Log. Proxy and +// embedded-SpiceDB output is then captured per-test by the testing framework and +// only printed when the test fails (or under -v), so successful runs are quiet. +// +// The logger is injected via the context rather than klog.SetLogger so that it +// never mutates klog's global logger; mutating that global races with background +// server goroutines from earlier tests under -race (which is why the options keep +// SkipLoggerSetupForTesting set). +func testContext(t testing.TB) context.Context { + t.Helper() + logger := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.Verbosity(3))) + return klog.NewContext(t.Context(), logger) +} + func TestEmbeddedMode(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - ctx := t.Context() + ctx := testContext(t) opts := createEmbeddedTestOptions(t) completedConfig, err := opts.Complete(ctx) @@ -69,9 +82,7 @@ func TestEmbeddedMode(t *testing.T) { } func TestEmbeddedModeCustomHeaders(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - - ctx, cancel := context.WithCancel(t.Context()) + ctx, cancel := context.WithCancel(testContext(t)) t.Cleanup(cancel) // Create a proxy with custom header names @@ -112,9 +123,7 @@ func TestEmbeddedModeCustomHeaders(t *testing.T) { } func TestEmbeddedModeAuthenticationConfiguration(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - - ctx, cancel := context.WithCancel(t.Context()) + ctx, cancel := context.WithCancel(testContext(t)) t.Cleanup(cancel) // Create one proxy with multiple header configuration for all tests @@ -196,14 +205,13 @@ func TestEmbeddedModeAuthenticationConfiguration(t *testing.T) { } func TestEmbeddedModeDefaults(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - - ctx, cancel := context.WithCancel(t.Context()) + ctx, cancel := context.WithCancel(testContext(t)) t.Cleanup(cancel) // Create embedded proxy with no explicit header configuration to test defaults opts := NewOptions(WithEmbeddedProxy, WithEmbeddedSpiceDBEndpoint) opts.Authentication.Embedded.Enabled = true + opts.SkipLoggerSetupForTesting = true // Configure mock upstream server opts.RestConfigFunc = func() (*rest.Config, http.RoundTripper, error) { @@ -259,9 +267,7 @@ func TestEmbeddedModeDefaults(t *testing.T) { } func TestEmbeddedClientFunctionalOptions(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - - ctx, cancel := context.WithCancel(t.Context()) + ctx, cancel := context.WithCancel(testContext(t)) t.Cleanup(cancel) // Create one proxy server for all subtests to avoid logging config issues @@ -350,9 +356,7 @@ func TestEmbeddedClientFunctionalOptions(t *testing.T) { } func TestEmbeddedClientCustomHeaderConfig(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - - ctx, cancel := context.WithCancel(t.Context()) + ctx, cancel := context.WithCancel(testContext(t)) t.Cleanup(cancel) // Create proxy with custom header names @@ -402,6 +406,7 @@ func createEmbeddedTestOptions(t *testing.T) *Options { opts := NewOptions(WithEmbeddedProxy, WithEmbeddedSpiceDBEndpoint) opts.Authentication.Embedded.Enabled = true + opts.SkipLoggerSetupForTesting = true // Configure mock upstream server opts.RestConfigFunc = func() (*rest.Config, http.RoundTripper, error) { diff --git a/pkg/proxy/options_test.go b/pkg/proxy/options_test.go index 7b07788..8a8ca5c 100644 --- a/pkg/proxy/options_test.go +++ b/pkg/proxy/options_test.go @@ -12,9 +12,7 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" "k8s.io/apiserver/pkg/endpoints/request" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/tools/clientcmd" - logsv1 "k8s.io/component-base/logs/api/v1" v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" "github.com/authzed/grpcutil" @@ -24,34 +22,29 @@ import ( ) func TestKubeConfig(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - opts := optionsForTesting(t, WithEmbeddedSpiceDBEndpoint) require.Empty(t, opts.Validate()) - c, err := opts.Complete(t.Context()) + c, err := opts.Complete(testContext(t)) require.NoError(t, err) require.NotNil(t, c) - require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) opts = optionsForTesting(t) opts.BackendKubeconfigPath = uuid.NewString() - c, err = opts.Complete(t.Context()) + c, err = opts.Complete(testContext(t)) require.ErrorContains(t, err, "couldn't load kubeconfig") require.ErrorContains(t, err, opts.BackendKubeconfigPath) require.Nil(t, c, "expected nil config on error") } func TestInClusterConfig(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - opts := optionsForTesting(t, WithEmbeddedSpiceDBEndpoint) opts.BackendKubeconfigPath = "" opts.UseInClusterConfig = true require.Empty(t, opts.Validate()) - c, err := opts.Complete(t.Context()) + c, err := opts.Complete(testContext(t)) require.NoError(t, err) require.NotNil(t, c) require.NotNil(t, opts.RestConfigFunc, "missing kube client REST config") @@ -64,7 +57,7 @@ func TestEmbeddedSpiceDB(t *testing.T) { opts := optionsForTesting(t, WithEmbeddedSpiceDBEndpoint) require.Empty(t, opts.Validate()) - c, err := opts.Complete(t.Context()) + c, err := opts.Complete(testContext(t)) require.NoError(t, err) require.NotNil(t, c) @@ -74,7 +67,7 @@ func TestEmbeddedSpiceDB(t *testing.T) { } func TestRemoteSpiceDB(t *testing.T) { - ctx, cancel := context.WithCancel(t.Context()) + ctx, cancel := context.WithCancel(testContext(t)) defer cancel() srv, addr := newTCPSpiceDB(t, ctx) @@ -90,7 +83,7 @@ func TestRemoteSpiceDB(t *testing.T) { opts.SpiceDBOptions.SecureSpiceDBTokensBySpace = "foobar" require.Empty(t, opts.Validate()) - c, err := opts.Complete(t.Context()) + c, err := opts.Complete(testContext(t)) require.NoError(t, err) require.NotNil(t, c) @@ -109,7 +102,7 @@ func TestRemoteSpiceDBCerts(t *testing.T) { opts.SpiceDBOptions.SpicedbCAPath = "test" require.Empty(t, opts.Validate()) - _, err := opts.Complete(t.Context()) + _, err := opts.Complete(testContext(t)) require.ErrorContains(t, err, "unable to load custom certificates") } @@ -117,7 +110,7 @@ func TestRuleConfig(t *testing.T) { opts := optionsForTesting(t, WithEmbeddedSpiceDBEndpoint) require.Empty(t, opts.Validate()) - c, err := opts.Complete(t.Context()) + c, err := opts.Complete(testContext(t)) require.NoError(t, err) require.NotNil(t, c) @@ -132,7 +125,6 @@ func TestRuleConfig(t *testing.T) { require.Empty(t, rules[0].Checks) require.Nil(t, rules[0].Update) - require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) errConfigBytes := []byte(` apiVersion: authzed.com/v1alpha1 kind: ProxyRule @@ -152,15 +144,17 @@ prefilter: opts.RuleConfigFile = errConfigFile require.Empty(t, opts.Validate()) - _, err = opts.Complete(t.Context()) + _, err = opts.Complete(testContext(t)) require.ErrorContains(t, err, "expected") } func optionsForTesting(t *testing.T, opts ...setOpt) *Options { t.Helper() - require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) options := NewOptions(opts...) + // Without this option, you get data races as the different tests + // touch the k8s logger at the same time. + options.SkipLoggerSetupForTesting = true options.SecureServing.BindPort = getFreePort(t, "127.0.0.1") options.SecureServing.BindAddress = net.ParseIP("127.0.0.1") options.BackendKubeconfigPath = kubeConfigForTest(t) @@ -216,9 +210,7 @@ relationships: | } func TestWithEmbeddedSpiceDBBootstrapIntegration(t *testing.T) { - defer require.NoError(t, logsv1.ResetForTest(utilfeature.DefaultFeatureGate)) - - ctx, cancel := context.WithCancel(t.Context()) + ctx, cancel := context.WithCancel(testContext(t)) t.Cleanup(cancel) // Create custom bootstrap content