Annotate a resource with the namespace of the operator handling it (#…

…1594)

* Annotate a resource with the namespace of the operator handling it

This will help users to find the operator configured to reconcile a
resource, and log a warning if it looks like multiple operators are
watching the same namespace.

* Refuse to reconcile a resource owned by another operator

This is safer in the case of operators misconfigured to have
overlapping namespaces. We'll be guaranteed that only one operator
will be trying to manage a resource, and the other operator will log
events complaining that it's also supposed to manage the resource but
isn't.

controllers/async_controller.go

@@ -26,10 +26,11 @@ import (
 )
 
 const (
-	finalizerName    string        = "azure.microsoft.com/finalizer"
-	requeueDuration  time.Duration = time.Second * 20
-	successMsg       string        = "successfully provisioned"
-	reconcileTimeout time.Duration = time.Minute * 5
+	finalizerName       string        = "azure.microsoft.com/finalizer"
+	namespaceAnnotation string        = "azure.microsoft.com/operator-namespace"
+	requeueDuration     time.Duration = time.Second * 20
+	successMsg          string        = "successfully provisioned"
+	reconcileTimeout    time.Duration = time.Minute * 5
 )
 
 // AsyncReconciler is a generic reconciler for Azure resources.
@@ -99,6 +100,31 @@ func (r *AsyncReconciler) Reconcile(req ctrl.Request, obj runtime.Object) (resul
 		return ctrl.Result{}, r.Update(ctx, obj)
 	}
 
+	// Ensure the resource is tagged with the operator's namespace.
+	reconcilerNamespace := annotations[namespaceAnnotation]
+	podNamespace := config.PodNamespace()
+	if reconcilerNamespace != podNamespace && reconcilerNamespace != "" {
+		// We don't want to get into a fight with another operator -
+		// so treat some other operator's annotation in a very similar
+		// way as the skip annotation above. This will do the right
+		// thing in the case of two operators trying to manage the
+		// same namespace. It makes moving objects between namespaces
+		// or changing which operator owns a namespace fiddlier (since
+		// you'd need to remove the annotation) but those operations
+		// are likely to be rare.
+		message := fmt.Sprintf("Operators in %q and %q are both configured to manage this resource", podNamespace, reconcilerNamespace)
+		r.Recorder.Event(obj, corev1.EventTypeWarning, "Overlap", message)
+		return ctrl.Result{}, r.Update(ctx, obj)
+	} else if reconcilerNamespace == "" {
+		// Set the annotation to this operator's namespace and go around again.
+		if annotations == nil {
+			annotations = make(map[string]string)
+		}
+		annotations[namespaceAnnotation] = podNamespace
+		res.SetAnnotations(annotations)
+		return ctrl.Result{}, r.Update(ctx, obj)
+	}
+
 	var configOptions []resourcemanager.ConfigOption
 	if res.GetDeletionTimestamp().IsZero() {
 		if !HasFinalizer(res, finalizerName) {

controllers/targetnamespace_test.go

@@ -14,7 +14,9 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	v1alpha1 "github.com/Azure/azure-service-operator/api/v1alpha1"
 	"github.com/Azure/azure-service-operator/pkg/helpers"
@@ -57,6 +59,11 @@ func TestTargetNamespaces(t *testing.T) {
 
 	EnsureInstance(ctx, t, tc, &instanceDefault)
 
+	require := require.New(t)
+
+	// Check that the instance is annotated with the operator namespace.
+	checkNamespaceAnnotation(require, &instanceDefault, "azureoperator-system")
+
 	// The watched namespace is also reconciled.
 	instanceWatched := instanceDefault
 	instanceWatched.ObjectMeta = metav1.ObjectMeta{
@@ -65,26 +72,38 @@ func TestTargetNamespaces(t *testing.T) {
 	}
 	EnsureInstance(ctx, t, tc, &instanceWatched)
 
+	checkNamespaceAnnotation(require, &instanceWatched, "azureoperator-system")
+
 	// But the unwatched namespace isn't...
+	unwatchedName := newName()
 	instanceUnwatched := instanceDefault
 	instanceUnwatched.ObjectMeta = metav1.ObjectMeta{
-		Name:      newName(),
+		Name:      unwatchedName,
 		Namespace: "unwatched",
 	}
-	require := require.New(t)
 	err := tc.k8sClient.Create(ctx, &instanceUnwatched)
 	require.Equal(nil, err)
 
-	res, err := meta.Accessor(&instanceUnwatched)
-	require.Equal(nil, err)
-	names := types.NamespacedName{Name: res.GetName(), Namespace: res.GetNamespace()}
+	names := types.NamespacedName{Name: unwatchedName, Namespace: "unwatched"}
 
 	gotFinalizer := func() bool {
-		err := tc.k8sClient.Get(ctx, names, &instanceUnwatched)
+		var instance v1alpha1.StorageAccount
+		err := tc.k8sClient.Get(ctx, names, &instance)
+		require.Equal(nil, err)
+		res, err := meta.Accessor(&instance)
 		require.Equal(nil, err)
 		return HasFinalizer(res, finalizerName)
 	}
 
+	gotNamespaceAnnotation := func() bool {
+		var instance v1alpha1.StorageAccount
+		err := tc.k8sClient.Get(ctx, names, &instance)
+		require.Equal(nil, err)
+		res, err := meta.Accessor(&instance)
+		require.Equal(nil, err)
+		return res.GetAnnotations()[namespaceAnnotation] == "azureoperator-system"
+	}
+
 	if configuredNamespaces == "" {
 		// The operator should be watching all namespaces.
 		require.Eventually(
@@ -93,6 +112,13 @@ func TestTargetNamespaces(t *testing.T) {
 			tc.retry,
 			"instance in some namespace never got a finalizer",
 		)
+		// And there should also be a namespace annotation.
+		require.Eventually(
+			gotNamespaceAnnotation,
+			tc.timeoutFast,
+			tc.retry,
+			"instance in some namespace never got an operator namespace annotation",
+		)
 	} else {
 		// We can tell that the resource isn't being reconciled if it
 		// never gets a finalizer.
@@ -102,13 +128,108 @@ func TestTargetNamespaces(t *testing.T) {
 			time.Second,
 			"instance in unwatched namespace got finalizer",
 		)
+		// There also shouldn't be a namespace annotation.
+		checkNoNamespaceAnnotation(require, &instanceUnwatched)
 	}
 
 	EnsureDelete(ctx, t, tc, &instanceDefault)
 	EnsureDelete(ctx, t, tc, &instanceWatched)
 	EnsureDelete(ctx, t, tc, &instanceUnwatched)
 }
 
+func TestOperatorNamespacePreventsReconciling(t *testing.T) {
+	t.Parallel()
+	defer PanicRecover(t)
+	ctx := context.Background()
+
+	// If a resource has a different operator's namespace it won't be
+	// reconciled.
+	notMine := v1alpha1.StorageAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "storageacct" + helpers.RandomString(6),
+			Namespace: "default",
+			Annotations: map[string]string{
+				namespaceAnnotation: "hard-times",
+			},
+		},
+		Spec: v1alpha1.StorageAccountSpec{
+			Kind:          "BlobStorage",
+			Location:      tc.resourceGroupLocation,
+			ResourceGroup: tc.resourceGroupName,
+			Sku: v1alpha1.StorageAccountSku{
+				Name: "Standard_LRS",
+			},
+			AccessTier:             "Hot",
+			EnableHTTPSTrafficOnly: to.BoolPtr(true),
+		},
+	}
+
+	require := require.New(t)
+	err := tc.k8sClient.Create(ctx, &notMine)
+	require.Equal(nil, err)
+	defer EnsureDelete(ctx, t, tc, &notMine)
+
+	names := types.NamespacedName{
+		Name:      notMine.ObjectMeta.Name,
+		Namespace: "default",
+	}
+
+	gotFinalizer := func() bool {
+		var instance v1alpha1.StorageAccount
+		err := tc.k8sClient.Get(ctx, names, &instance)
+		require.Equal(nil, err)
+		res, err := meta.Accessor(&instance)
+		require.Equal(nil, err)
+		return HasFinalizer(res, finalizerName)
+	}
+
+	require.Never(
+		gotFinalizer,
+		20*time.Second,
+		time.Second,
+		"instance claimed by some other operator got finalizer",
+	)
+
+	var events corev1.EventList
+	err = tc.k8sClient.List(ctx, &events, &client.ListOptions{
+		FieldSelector: fields.ParseSelectorOrDie("involvedObject.name=" + notMine.ObjectMeta.Name),
+		Namespace:     "default",
+	})
+	require.Equal(nil, err)
+	require.Len(events.Items, 1)
+	event := events.Items[0]
+	require.Equal(event.Type, "Warning")
+	require.Equal(event.Reason, "Overlap")
+	require.Equal(event.Message, `Operators in "azureoperator-system" and "hard-times" are both configured to manage this resource`)
+
+	// But an instance that I've claimed gets reconciled fine.
+	mine := notMine
+	mine.ObjectMeta = metav1.ObjectMeta{
+		Name:      "storaceacct" + helpers.RandomString(6),
+		Namespace: "default",
+		Annotations: map[string]string{
+			namespaceAnnotation: "azureoperator-system",
+		},
+	}
+	EnsureInstance(ctx, t, tc, &mine)
+	EnsureDelete(ctx, t, tc, &mine)
+}
+
+func checkNoNamespaceAnnotation(r *require.Assertions, instance metav1.Object) {
+	res, err := meta.Accessor(instance)
+	r.Equal(nil, err)
+	_, found := res.GetAnnotations()[namespaceAnnotation]
+	r.Equal(false, found)
+}
+
+func checkNamespaceAnnotation(r *require.Assertions, instance metav1.Object, expected string) {
+	res, err := meta.Accessor(instance)
+	r.Equal(nil, err)
+	actual, found := res.GetAnnotations()[namespaceAnnotation]
+	r.Equal(true, found)
+	r.Equal(expected, actual)
+}
+
 func createNamespaces(ctx context.Context, t *testing.T, names ...string) {
 	for _, name := range names {
 		err := tc.k8sClient.Create(ctx, &corev1.Namespace{