Merge pull request noobaa#1936 from VershaAgrawal/fix-bucketclass-quota

Fixed BucketClass quota ignore during OBC update: Improved bucket class resolution for existing bucket operations by using the claim’s namespace when available and applying consistent by-name lookup semantics .

Author: VershaAgrawal

pkg/obc/obc_suite_test.go

@@ -1,4 +1,4 @@
-package obc_test
+package obc
 
 import (
 	"testing"

pkg/obc/obc_test.go

@@ -4,6 +4,7 @@ import (
 	nbv1 "github.com/noobaa/noobaa-operator/v5/pkg/apis/noobaa/v1alpha1"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -160,6 +161,103 @@ var _ = Describe("OBC referenced BucketClass", func() {
 	})
 })
 
+var _ = Describe("getBucketClassByName", func() {
+	Context("When a bucketclass exists in the same namespace that of OBC", func() {
+		It("should return the BucketClass with namespace to be the same as that of OBC", func() {
+			obcNS := "obc-ns"
+			systemNS := "test"
+
+			bc, ok := getBucketClassByName(
+				"bc1",
+				obcNS,
+				systemNS,
+				func(o client.Object) bool {
+					return o.GetNamespace() == obcNS
+				},
+			)
+
+			Expect(ok).To(BeTrue())
+			Expect(bc.GetNamespace()).To(Equal(obcNS))
+		})
+	})
+
+	Context("When a bucketclass exists in the system namespace", func() {
+		It("should return the BucketClass's namespace to be the same as that of system", func() {
+			obcNS := "obc-ns"
+			systemNS := "test"
+
+			bc, ok := getBucketClassByName(
+				"bc1",
+				obcNS,
+				systemNS,
+				func(o client.Object) bool {
+					return o.GetNamespace() == systemNS
+				},
+			)
+
+			Expect(ok).To(BeTrue())
+			Expect(bc.GetNamespace()).To(Equal(systemNS))
+		})
+	})
+
+	Context("When a bucketclass exists in the system namespace as well as OBC namespace", func() {
+		It("should return the BucketClass's namespace to be the same as that of OBC", func() {
+			obcNS := "obc-ns"
+			systemNS := "test"
+
+			bc, ok := getBucketClassByName(
+				"bc1",
+				obcNS,
+				systemNS,
+				func(o client.Object) bool {
+					return o.GetNamespace() == systemNS || o.GetNamespace() == obcNS
+				},
+			)
+
+			Expect(ok).To(BeTrue())
+			Expect(bc.GetNamespace()).To(Equal(obcNS))
+		})
+	})
+
+	Context("When a bucketclass does not exists in system namespace or OBC namespace", func() {
+		It("should return the BucketClass's namespace to be the same as that of system namespace with \"ok\" set to false", func() {
+			obcNS := "obc-ns"
+			systemNS := "test"
+
+			bc, ok := getBucketClassByName(
+				"bc1",
+				obcNS,
+				systemNS,
+				func(o client.Object) bool {
+					return false
+				},
+			)
+
+			Expect(ok).To(BeFalse())
+			Expect(bc.GetNamespace()).To(Equal(systemNS))
+		})
+	})
+
+	Context("When a bucketclass name is empty", func() {
+		It("should return empty string for namespace and \"exists\" set to false", func() {
+			obcNS := "obc-ns"
+			systemNS := "test"
+
+			bc, ok := getBucketClassByName(
+				"",
+				obcNS,
+				systemNS,
+				func(o client.Object) bool {
+					return false
+				},
+			)
+
+			Expect(ok).To(BeFalse())
+			Expect(bc.GetNamespace()).To(Equal(""))
+		})
+	})
+})
+
 var _ = Describe("getExternalDNSDetails", func() {
 	noobaaWithExternalDNS := func(s3URLs, vectorsURLs []string) *nbv1.NooBaa {
 		return &nbv1.NooBaa{
@@ -249,3 +347,23 @@ var _ = Describe("getExternalDNSDetails", func() {
 		}
 	})
 })
+
+var _ = Describe("GetQuotaConfig", func() {
+	It("uses the minimum maxObjects when BucketClass and OBC both specify a limit", func() {
+		bcSpec := &nbv1.BucketClassSpec{
+			Quota: &nbv1.Quota{
+				MaxObjects: "1000",
+			},
+		}
+		obConfig := map[string]string{
+			"maxObjects": "2000",
+		}
+		log := logrus.NewEntry(logrus.New())
+
+		quota, err := GetQuotaConfig("test-bucket", bcSpec, obConfig, log)
+
+		Expect(err).NotTo(HaveOccurred())
+		Expect(quota.Quantity).NotTo(BeNil())
+		Expect(quota.Quantity.Value).To(Equal(1000))
+	})
+})

pkg/obc/provisioner.go

@@ -454,7 +454,11 @@ func NewBucketRequest(
 		r.AccountName = ob.Spec.AdditionalState["account"]
 		bucketClassName := ob.Spec.AdditionalState["bucketclass"]
 
-		bucketClass, exists := getBucketClass(r.OBC, bucketOptions, p.Namespace, util.KubeCheck)
+		obcNamespace := ""
+		if ob.Spec.ClaimRef != nil {
+			obcNamespace = ob.Spec.ClaimRef.Namespace
+		}
+		bucketClass, exists := getBucketClassByName(bucketClassName, obcNamespace, p.Namespace, util.KubeCheck)
 		if !exists {
 			p.Logger.Warnf("BucketClass %q not found in namespace %q", bucketClassName, p.Namespace)
 		}
@@ -910,16 +914,10 @@ func (r *BucketRequest) updateReplicationPolicy(ob *nbv1.ObjectBucket) error {
 	return nil
 }
 
-// getBucketClass takes an OBC, bucketoptions and provisioner namespace and returns the bucketClass
-//
-// If BucketClass name is not specified in the OBC, then the empty string is returned with exists=false
-// If BucketClass name is specified in the OBC, then:
-// - if the bucketclass is found in the obc namespace, then that bucketclass is returned
-// with exists=true
-// - if the bucketclass is found in the provisioner namespace, then that buckeclass is
-// returned with exists=true
-// - if the bucketclass is not found in the obc namespace or the provisioner namespace, then the
-// bucketclass with namespace set to provisioner namespace is returned with exists=false
+// getBucketClass extracts the bucket class name from an OBC (or StorageClass parameters)
+// and delegates to getBucketClassByName. Used by the create/provision path where a full
+// ObjectBucketClaim is available. On the update path (ObjectBucket only), callers should
+// use getBucketClassByName directly with values from AdditionalState and ClaimRef.
 func getBucketClass(
 	obc *nbv1.ObjectBucketClaim,
 	bucketOptions *obAPI.BucketOptions,
@@ -942,16 +940,40 @@ func getBucketClass(
 	if bucketclassName == "" && bucketOptions != nil {
 		bucketclassName = bucketOptions.Parameters["bucketclass"]
 	}
-	if bucketclassName == "" {
+	return getBucketClassByName(bucketclassName, obc.Namespace, provisionerNS, checkExists)
+}
+
+// getBucketClassByName looks up a BucketClass by name in the OBC namespace first,
+// then in the provisioner (NooBaa system) namespace.
+//
+// If bucketClassName is empty, returns exists=false.
+// If found in obcNamespace, returns that BucketClass with exists=true.
+// If found in provisionerNS, returns that BucketClass with exists=true.
+// If not found, returns a BucketClass with namespace set to provisionerNS and exists=false.
+func getBucketClassByName(
+	bucketClassName, obcNamespace, provisionerNS string,
+	checkExists func(client.Object) bool,
+) (bc *nbv1.BucketClass, exists bool) {
+	bucketClass := &nbv1.BucketClass{
+		TypeMeta: metav1.TypeMeta{Kind: "BucketClass"},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "",
+			Namespace: "",
+		},
+	}
+
+	if bucketClassName == "" {
 		return bucketClass, false
 	}
 
-	bucketClass.SetName(bucketclassName)
+	bucketClass.SetName(bucketClassName)
 
 	// Find the bucketclass in the same namespace as the OBC
-	bucketClass.SetNamespace(obc.Namespace)
-	if checkExists(bucketClass) {
-		return bucketClass, true
+	if obcNamespace != "" {
+		bucketClass.SetNamespace(obcNamespace)
+		if checkExists(bucketClass) {
+			return bucketClass, true
+		}
 	}
 
 	// Find the bucketclass in the provisioner namespace