Implement access control for cross-namespace backup restoration (#400)

Restored backups from other namespaces are only allowed if the source namespace has the authorized access label.

Change-Id: I409b4ffb32a9c94a335597eff0a5c0c46d3ba962

Author: jiaalloydb

oracle/config/rbac/role.yaml

@@ -126,6 +126,14 @@ rules:
   - get
   - list
   - update
+- apiGroups:
+  - ""
+  resources:
+  - namespaces
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:

oracle/controllers/instancecontroller/instance_controller.go

@@ -82,6 +82,7 @@ func (r *InstanceReconciler) Scheme() *runtime.Scheme {
 // +kubebuilder:rbac:groups=storage.k8s.io,resources=storageclasses,verbs=get;list;watch
 // +kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;list;create;update
 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch
 
 // +kubebuilder:rbac:groups=oracle.db.anthosapis.com,resources=databases,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=oracle.db.anthosapis.com,resources=databases/status,verbs=get;update;patch

oracle/controllers/instancecontroller/instance_controller_restore.go

@@ -37,6 +37,8 @@ import (
 	"github.com/GoogleCloudPlatform/elcarro-oracle-operator/oracle/pkg/k8s"
 )
 
+const crossNSBackupRefAccessLabel = "allow-cross-namespace-backup-reference"
+
 // Reconciler for restore logic.
 // Invoked when Spec.Restore is present.
 // State transition:
@@ -389,6 +391,15 @@ func (r *InstanceReconciler) findBackupForRestore(ctx context.Context, inst v1al
 		if inst.Spec.Restore.BackupID != "" || inst.Spec.Restore.PITRRestore != nil {
 			return nil, fmt.Errorf("preflight check: specify only one of BackupID/BackupRef/PITRRestore")
 		}
+
+		/*
+			For cross-namespace backup requests, ensure the target namespace permits
+			cross-namespace references; otherwise, fail the request and set the status condition to False
+		*/
+		if err := r.checkCrossNSPermission(ctx, inst); err != nil {
+			return nil, err
+		}
+
 		// find backup based on BackupRef
 		if err := r.Get(ctx, types.NamespacedName{Name: backupRef.Name, Namespace: backupRef.Namespace}, &backup); err != nil {
 			return nil, fmt.Errorf("preflight check: failed to get backup for a restore: %v, backupRef: %v", err, backupRef)
@@ -425,6 +436,24 @@ func (r *InstanceReconciler) findBackupForRestore(ctx context.Context, inst v1al
 	return &backup, nil
 }
 
+func (r *InstanceReconciler) checkCrossNSPermission(ctx context.Context, inst v1alpha1.Instance) error {
+	if inst.Namespace == inst.Spec.Restore.BackupRef.Namespace {
+		return nil
+	}
+
+	namespace := inst.Spec.Restore.BackupRef.Namespace
+	ns := &corev1.Namespace{}
+	if err := r.Client.Get(ctx, types.NamespacedName{Name: namespace}, ns); err != nil {
+		return fmt.Errorf("failed to get namespace %q: %w", namespace, err)
+
+	}
+
+	if val, ok := ns.Labels[crossNSBackupRefAccessLabel]; !ok || val != "true" {
+		return fmt.Errorf("namespace %q does not allow cross-namespace backup references (label '%s' must be set to 'true')", namespace, crossNSBackupRefAccessLabel)
+	}
+	return nil
+}
+
 // restorePhysical runs the pre-flight checks and if all is good
 // it makes a gRPC call to a PhysicalRestore.
 func (r *InstanceReconciler) restorePhysical(ctx context.Context, inst v1alpha1.Instance, backup *v1alpha1.Backup, req ctrl.Request, log logr.Logger) (*lropb.Operation, error) {

oracle/controllers/instancecontroller/instance_controller_restore_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/go-logr/logr"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -28,18 +29,20 @@ func testInstanceRestore() {
 		interval = time.Millisecond * 15
 	)
 	var (
-		Namespace    string
-		InstanceName string
-		BackupName   string
-		BackupID     string
-		ObjKey       client.ObjectKey
+		Namespace            string
+		CrossNamespacePrefix string
+		InstanceName         string
+		BackupName           string
+		BackupID             string
+		ObjKey               client.ObjectKey
 	)
 
 	var fakeDatabaseClient *testhelpers.FakeDatabaseClient
 	oldPreflightFunc := restorePhysicalPreflightCheck
 
 	BeforeEach(func() {
 		Namespace = "default"
+		CrossNamespacePrefix = "cross-namespace-"
 		InstanceName = testhelpers.RandName("test-instance-restore")
 		BackupName = testhelpers.RandName("test-backup")
 		BackupID = testhelpers.RandName("test-backup-id")
@@ -67,6 +70,23 @@ func testInstanceRestore() {
 	ctx := context.Background()
 	restoreRequestTime := metav1.Now()
 
+	createNamespace := func(ctx context.Context, k8sClient client.Client, name string, crossNSLabel bool) {
+		// 1. Define the Namespace object
+		ns := &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+		}
+
+		if crossNSLabel {
+			ns.ObjectMeta.Labels = map[string]string{
+				crossNSBackupRefAccessLabel: "true",
+			}
+		}
+
+		testhelpers.K8sCreateWithRetry(k8sClient, ctx, ns)
+	}
+
 	createInstanceAndStartRestore := func(mode testhelpers.FakeOperationStatus) (*v1alpha1.Instance, *v1alpha1.Backup) {
 		instance := createSimpleInstance(ctx, InstanceName, Namespace, timeout, interval)
 		backup := createSimpleRMANBackup(ctx, InstanceName, BackupName, BackupID, Namespace)
@@ -91,6 +111,36 @@ func testInstanceRestore() {
 		return instance, backup
 	}
 
+	createInstanceAndStartRestoreWithBackupRef := func(mode testhelpers.FakeOperationStatus, backupNS string) (*v1alpha1.Instance, *v1alpha1.Backup) {
+		instance := createSimpleInstance(ctx, InstanceName, Namespace, timeout, interval)
+		var backup *v1alpha1.Backup
+		if backupNS != "" {
+			backup = createSimpleRMANBackup(ctx, InstanceName, BackupName, BackupID, backupNS)
+
+		}
+		By("invoking RMAN restore for the Instance with backupRef")
+
+		// configure fakeDatabaseClient to be in requested mode
+		fakeDatabaseClient.SetNextGetOperationStatus(mode)
+		Expect(retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			instance.Spec.Restore = &v1alpha1.RestoreSpec{
+				BackupRef: &v1alpha1.BackupReference{
+					Namespace: backupNS,
+					Name:      BackupName,
+				},
+				BackupType:  "Physical",
+				Force:       true,
+				RequestTime: restoreRequestTime,
+			}
+			return k8sClient.Update(ctx, instance)
+		})).Should(Succeed())
+
+		return instance, backup
+	}
+
 	// extract happy path restore test part into a function for reuse
 	// in multiple tests
 	testCaseHappyPathLRORestore := func() (*v1alpha1.Instance, *v1alpha1.Backup) {
@@ -405,6 +455,164 @@ func testInstanceRestore() {
 		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, ObjKey, instance)
 		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, client.ObjectKey{Namespace: Namespace, Name: BackupName}, backup)
 	})
+
+	It("it should restore successfully with backupRef of the same namespace", func() {
+		fakeDatabaseClient.SetAsyncPhysicalRestore(false)
+		instance, backup := createInstanceAndStartRestoreWithBackupRef(testhelpers.StatusDone, Namespace)
+
+		By("checking that instance status is Ready")
+		Eventually(func() (metav1.ConditionStatus, error) {
+			return getConditionStatus(ctx, ObjKey, k8s.Ready)
+		}, timeout, interval).Should(Equal(metav1.ConditionTrue))
+
+		Expect(fakeDatabaseClient.DeleteOperationCalledCnt()).Should(Equal(0))
+
+		By("checking that instance Restore section is deleted")
+		Eventually(func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			if instance.Spec.Restore != nil {
+				return fmt.Errorf("expected update has not yet happened")
+			}
+			return nil
+		}, timeout, interval).Should(Succeed())
+
+		By("checking that instance Status.Description is updated")
+		Eventually(func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			if !strings.HasPrefix(instance.Status.Description, "Restored on") {
+				return fmt.Errorf("%q does not have expected prefix", instance.Status.Description)
+			}
+			return nil
+		}, timeout, interval).Should(Succeed())
+
+		By("checking that instance maintenance lock is released")
+		// Instance object should be fresh at this point, no need to retry
+		Expect(instance.Status.LockedByController).Should(Equal(""))
+
+		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, ObjKey, instance)
+		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, client.ObjectKey{Namespace: Namespace, Name: BackupName}, backup)
+	})
+
+	It("it should restore fail with cross-namespace backupRef if the cross-namespace backupRef specifies a namespace that does not exist.", func() {
+		fakeDatabaseClient.SetAsyncPhysicalRestore(false)
+		instance, _ := createInstanceAndStartRestoreWithBackupRef(testhelpers.StatusDone, "")
+
+		By("checking that instance Restore section is deleted")
+		Eventually(func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			if instance.Spec.Restore != nil {
+				return fmt.Errorf("expected update has not yet happened")
+			}
+			return nil
+		}, timeout, interval).Should(Succeed())
+
+		By("checking that instance Status.Description is updated")
+		Eventually(func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			cond := k8s.FindCondition(instance.Status.Conditions, k8s.Ready)
+			if !strings.HasPrefix(cond.Message, "Could not find a matching backup") {
+				return fmt.Errorf("%q does not have expected prefix in the condition", cond.Message)
+			}
+			return nil
+		}, timeout, interval).Should(Succeed())
+
+		By("checking that instance maintenance lock is released")
+		// Instance object should be fresh at this point, no need to retry
+		Expect(instance.Status.LockedByController).Should(Equal(""))
+
+		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, ObjKey, instance)
+	})
+
+	It("it should restore fail with cross-namespace backupRef without cross-namespace backupRef access label", func() {
+		fakeDatabaseClient.SetAsyncPhysicalRestore(false)
+		// Create a different namespace
+		crossNS := CrossNamespacePrefix + "1"
+		createNamespace(ctx, k8sClient, crossNS, false)
+
+		instance, backup := createInstanceAndStartRestoreWithBackupRef(testhelpers.StatusDone, crossNS)
+
+		By("checking that instance Restore section is deleted")
+		Eventually(func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			if instance.Spec.Restore != nil {
+				return fmt.Errorf("expected update has not yet happened")
+			}
+			return nil
+		}, timeout, interval).Should(Succeed())
+
+		By("checking that instance Status.Description is updated")
+		Eventually(func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			cond := k8s.FindCondition(instance.Status.Conditions, k8s.Ready)
+			if !strings.HasPrefix(cond.Message, "Could not find a matching backup") {
+				return fmt.Errorf("%q does not have expected prefix in the condition", cond.Message)
+			}
+			return nil
+		}, timeout, interval).Should(Succeed())
+
+		By("checking that instance maintenance lock is released")
+		// Instance object should be fresh at this point, no need to retry
+		Expect(instance.Status.LockedByController).Should(Equal(""))
+
+		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, ObjKey, instance)
+		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, client.ObjectKey{Namespace: crossNS, Name: BackupName}, backup)
+	})
+
+	It("it should restore successfully with cross-namespace backupRef if the cross namespace is labeled with allow-cross-namespace-backup-reference", func() {
+		fakeDatabaseClient.SetAsyncPhysicalRestore(false)
+		// Create a different namespace
+		crossNS := CrossNamespacePrefix + "2"
+		createNamespace(ctx, k8sClient, crossNS, true)
+
+		instance, backup := createInstanceAndStartRestoreWithBackupRef(testhelpers.StatusDone, crossNS)
+		By("checking that instance status is Ready")
+		Eventually(func() (metav1.ConditionStatus, error) {
+			return getConditionStatus(ctx, ObjKey, k8s.Ready)
+		}, timeout, interval).Should(Equal(metav1.ConditionTrue))
+
+		Expect(fakeDatabaseClient.DeleteOperationCalledCnt()).Should(Equal(0))
+
+		By("checking that instance Restore section is deleted")
+		Eventually(func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			if instance.Spec.Restore != nil {
+				return fmt.Errorf("expected update has not yet happened")
+			}
+			return nil
+		}, timeout, interval).Should(Succeed())
+
+		By("checking that instance Status.Description is updated")
+		Eventually(func() error {
+			if err := k8sClient.Get(ctx, ObjKey, instance); err != nil {
+				return err
+			}
+			if !strings.HasPrefix(instance.Status.Description, "Restored on") {
+				return fmt.Errorf("%q does not have expected prefix", instance.Status.Description)
+			}
+			return nil
+		}, timeout, interval).Should(Succeed())
+
+		By("checking that instance maintenance lock is released")
+		// Instance object should be fresh at this point, no need to retry
+		Expect(instance.Status.LockedByController).Should(Equal(""))
+
+		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, ObjKey, instance)
+		testhelpers.K8sDeleteWithRetry(k8sClient, ctx, client.ObjectKey{Namespace: crossNS, Name: BackupName}, backup)
+	})
 }
 
 func createSimpleRMANBackup(ctx context.Context, instanceName string, backupName string, backupID string, namespace string) *v1alpha1.Backup {

oracle/operator.yaml

@@ -3865,6 +3865,14 @@ rules:
   - get
   - list
   - update
+- apiGroups:
+  - ""
+  resources:
+  - namespaces
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources: