internal/dryrun/transport: simplify (#2085)

Author: s-urbaniak

internal/controller/atlas/provider.go

@@ -14,7 +14,6 @@ import (
 	"go.mongodb.org/atlas/mongodbatlas"
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/api"
@@ -48,7 +47,7 @@ type ProductionProvider struct {
 	k8sClient       client.Client
 	domain          string
 	globalSecretRef client.ObjectKey
-	dryRunRecorder  record.EventRecorder
+	dryRun          bool
 }
 
 type credentialsSecret struct {
@@ -57,12 +56,12 @@ type credentialsSecret struct {
 	PrivateKey string
 }
 
-func NewProductionProvider(atlasDomain string, globalSecretRef client.ObjectKey, k8sClient client.Client, dryRunRecorder record.EventRecorder) *ProductionProvider {
+func NewProductionProvider(atlasDomain string, globalSecretRef client.ObjectKey, k8sClient client.Client, dryRun bool) *ProductionProvider {
 	return &ProductionProvider{
 		k8sClient:       k8sClient,
 		domain:          atlasDomain,
 		globalSecretRef: globalSecretRef,
-		dryRunRecorder:  dryRunRecorder,
+		dryRun:          dryRun,
 	}
 }
 
@@ -171,11 +170,11 @@ func (p *ProductionProvider) SdkClientSet(ctx context.Context, secretRef *client
 }
 
 func (p *ProductionProvider) newDryRunTransport(delegate http.RoundTripper) http.RoundTripper {
-	if p.dryRunRecorder == nil {
-		return delegate
+	if p.dryRun {
+		return dryrun.NewDryRunTransport(delegate)
 	}
 
-	return dryrun.NewDryRunTransport(p.dryRunRecorder, delegate)
+	return delegate
 }
 
 func getSecrets(ctx context.Context, k8sClient client.Client, secretRef, fallbackRef *client.ObjectKey) (*credentialsSecret, error) {

internal/controller/atlas/provider_test.go

@@ -43,7 +43,7 @@ func TestProvider_Client(t *testing.T) {
 		Build()
 
 	t.Run("should return Atlas API client and organization id using global secret", func(t *testing.T) {
-		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{Name: "api-secret", Namespace: "default"}, k8sClient, nil)
+		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{Name: "api-secret", Namespace: "default"}, k8sClient, false)
 
 		c, id, err := p.Client(context.Background(), nil, zaptest.NewLogger(t).Sugar())
 		assert.NoError(t, err)
@@ -52,7 +52,7 @@ func TestProvider_Client(t *testing.T) {
 	})
 
 	t.Run("should return Atlas API client and organization id using connection secret", func(t *testing.T) {
-		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{Name: "global-secret", Namespace: "default"}, k8sClient, nil)
+		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{Name: "global-secret", Namespace: "default"}, k8sClient, false)
 
 		c, id, err := p.Client(context.Background(), &client.ObjectKey{Name: "api-secret", Namespace: "default"}, zaptest.NewLogger(t).Sugar())
 		assert.NoError(t, err)
@@ -63,17 +63,17 @@ func TestProvider_Client(t *testing.T) {
 
 func TestProvider_IsCloudGov(t *testing.T) {
 	t.Run("should return false for invalid domain", func(t *testing.T) {
-		p := NewProductionProvider("http://x:namedport", client.ObjectKey{}, nil, nil)
+		p := NewProductionProvider("http://x:namedport", client.ObjectKey{}, nil, false)
 		assert.False(t, p.IsCloudGov())
 	})
 
 	t.Run("should return false for commercial Atlas domain", func(t *testing.T) {
-		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{}, nil, nil)
+		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{}, nil, false)
 		assert.False(t, p.IsCloudGov())
 	})
 
 	t.Run("should return true for Atlas for government domain", func(t *testing.T) {
-		p := NewProductionProvider("https://cloud.mongodbgov.com/", client.ObjectKey{}, nil, nil)
+		p := NewProductionProvider("https://cloud.mongodbgov.com/", client.ObjectKey{}, nil, false)
 		assert.True(t, p.IsCloudGov())
 	})
 }
@@ -173,7 +173,7 @@ func TestProvider_IsResourceSupported(t *testing.T) {
 
 	for desc, data := range dataProvider {
 		t.Run(desc, func(t *testing.T) {
-			p := NewProductionProvider(data.domain, client.ObjectKey{}, nil, nil)
+			p := NewProductionProvider(data.domain, client.ObjectKey{}, nil, false)
 			assert.Equal(t, data.expectation, p.IsResourceSupported(data.resource))
 		})
 	}

internal/controller/workflow/context.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/api"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/internal/controller/atlas"
-	"github.com/mongodb/mongodb-atlas-kubernetes/v2/internal/dryrun"
 )
 
 // Context is a container for some information that is needed on all levels of function calls during reconciliation.
@@ -50,7 +49,7 @@ func NewContext(log *zap.SugaredLogger, conditions []api.Condition, context cont
 	return &Context{
 		status:  NewStatus(conditions),
 		Log:     log,
-		Context: dryrun.WithRuntimeObject(context, obj),
+		Context: context,
 	}
 }
 

internal/dryrun/context.go

@@ -1,12 +1,7 @@
 package dryrun
 
 import (
-	"errors"
 	"net/http"
-
-	v1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/tools/record"
 )
 
 const DryRunReason = "DryRun"
@@ -19,13 +14,11 @@ var verbMap = map[string]string{
 }
 
 type DryRunTransport struct {
-	Recorder record.EventRecorder
 	Delegate http.RoundTripper
 }
 
-func NewDryRunTransport(recorder record.EventRecorder, delegate http.RoundTripper) *DryRunTransport {
+func NewDryRunTransport(delegate http.RoundTripper) *DryRunTransport {
 	return &DryRunTransport{
-		Recorder: recorder,
 		Delegate: delegate,
 	}
 }
@@ -37,24 +30,13 @@ func (t *DryRunTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	case http.MethodTrace:
 	case http.MethodHead:
 	default:
-		obj, ok := runtimeObjectFrom(req.Context())
-		if !ok {
-			return nil, errors.New("no object present in context: cannot record dry run without an object")
-		}
-
-		meta, ok := obj.(metav1.ObjectMetaAccessor)
-		if !ok {
-			return nil, errors.New("object does not implement ObjectMetaAccessor")
-		}
-
 		verb, ok := verbMap[req.Method]
 		if !ok {
 			verb = "execute " + req.Method
 		}
 		msg := "Would %v %v"
-		t.Recorder.Eventf(obj, v1.EventTypeNormal, DryRunReason, msg, verb, req.URL.Path)
 
-		return nil, NewDryRunError(obj.GetObjectKind(), meta, v1.EventTypeNormal, DryRunReason, msg, verb, req.URL.Path)
+		return nil, NewDryRunError(nil, nil, "", "", msg, verb, req.URL.Path)
 	}
 
 	return t.Delegate.RoundTrip(req)

internal/dryrun/transport.go

@@ -1,21 +1,11 @@
 package dryrun
 
 import (
-	"context"
 	"net/http"
 	"net/url"
 	"testing"
-	"time"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/util/wait"
-	"k8s.io/client-go/tools/record"
-
-	akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/api/v1"
 )
 
 type RoundTripperFunc func(*http.Request) (*http.Response, error)
@@ -26,103 +16,44 @@ func (fn RoundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error)
 
 func TestDryRunTransport(t *testing.T) {
 	for _, tc := range []struct {
-		name       string
-		req        *http.Request
-		obj        runtime.Object
-		wantEvents []string
-		wantErr    string
+		name    string
+		req     *http.Request
+		wantErr string
 	}{
 		{
 			name: "GET request",
 			req: &http.Request{
 				Method: http.MethodGet,
 			},
 		},
-		{
-			name: "no object",
-			req: &http.Request{
-				Method: http.MethodPost,
-			},
-			wantErr: "no object present in context: cannot record dry run without an object",
-		},
 		{
 			name: "unknown verb",
 			req: &http.Request{
 				Method: "UNKNOWN",
 				URL:    &url.URL{Path: "/test"},
 			},
-			obj: &akov2.AtlasProject{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "foo",
-					Namespace: "bar",
-				},
-			},
-			wantErr: "DryRun event GVK=atlas.mongodb.com/v1, Kind=AtlasProject, Namespace=bar, Name=foo, EventType=Normal, Reason=DryRun, Message=Would execute UNKNOWN /test",
-			wantEvents: []string{
-				"Normal DryRun Would execute UNKNOWN /test",
-			},
+			wantErr: "DryRun event GVK=unknown, Namespace=unknown, Name=unknown, EventType=, Reason=, Message=Would execute UNKNOWN /test",
 		},
 		{
 			name: "POST request",
 			req: &http.Request{
 				Method: http.MethodPost,
 				URL:    &url.URL{Path: "/test"},
 			},
-			obj:     &akov2.AtlasProject{},
-			wantErr: "DryRun event GVK=atlas.mongodb.com/v1, Kind=AtlasProject, Namespace=, Name=, EventType=Normal, Reason=DryRun, Message=Would create (POST) /test",
-			wantEvents: []string{
-				"Normal DryRun Would create (POST) /test",
-			},
+			wantErr: "DryRun event GVK=unknown, Namespace=unknown, Name=unknown, EventType=, Reason=, Message=Would create (POST) /test",
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			rec := record.NewFakeRecorder(100)
 			nopDelegate := RoundTripperFunc(func(req *http.Request) (*http.Response, error) {
 				return nil, nil
 			})
-			transport := NewDryRunTransport(rec, nopDelegate)
-
-			if tc.obj != nil {
-				scheme := runtime.NewScheme()
-				assert.NoError(t, akov2.AddToScheme(scheme))
-				gvks, _, err := scheme.ObjectKinds(tc.obj)
-				require.NoError(t, err)
-				meta := tc.obj.(schema.ObjectKind)
-				meta.SetGroupVersionKind(gvks[0])
-			}
-
-			req := tc.req.WithContext(WithRuntimeObject(context.Background(), tc.obj))
-			_, err := transport.RoundTrip(req)
+			transport := NewDryRunTransport(nopDelegate)
+			_, err := transport.RoundTrip(tc.req)
 			gotErr := ""
 			if err != nil {
 				gotErr = err.Error()
 			}
 			require.Equal(t, tc.wantErr, gotErr)
-			assertEqualEvents(t, tc.wantEvents, rec.Events)
 		})
 	}
 }
-
-func assertEqualEvents(t *testing.T, expected []string, actual <-chan string) {
-	c := time.After(wait.ForeverTestTimeout)
-	for _, e := range expected {
-		select {
-		case a := <-actual:
-			if e != a {
-				t.Errorf("Expected event %q, got %q", e, a)
-				return
-			}
-		case <-c:
-			t.Errorf("Expected event %q, got nothing", e)
-			// continue iterating to print all expected events
-		}
-	}
-	for {
-		select {
-		case a := <-actual:
-			t.Errorf("Unexpected event: %q", a)
-		default:
-			return // No more events, as expected.
-		}
-	}
-}

internal/dryrun/transport_test.go

@@ -208,7 +208,7 @@ func (b *Builder) Build(ctx context.Context) (cluster.Cluster, error) {
 	}
 
 	if b.atlasProvider == nil {
-		b.atlasProvider = atlas.NewProductionProvider(b.atlasDomain, b.apiSecret, mgr.GetClient(), nil)
+		b.atlasProvider = atlas.NewProductionProvider(b.atlasDomain, b.apiSecret, mgr.GetClient(), false)
 	}
 
 	if err := controllerRegistry.RegisterWithManager(mgr, b.skipNameValidation, b.atlasProvider); err != nil {