Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

object: remove default value us-east-1 as region from s3 client #9754

Closed
wants to merge 2 commits into from
Closed

object: remove default value us-east-1 as region from s3 client #9754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f0cf1a
object: remove default value us-east-1 as region from s3 client
thotz Feb 16, 2022
5a2f5ad
docs: explain about region in storage class for OBC
thotz Jan 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 64 additions & 3 deletions Documentation/ceph-object-bucket-claim.md
View file
Open in desktop
Expand Up @@ -102,18 +102,79 @@ metadata:
labels:
aws-s3/object [1]
provisioner: rook-ceph.ceph.rook.io/bucket [2]
parameters: [3]
parameters:
objectStoreName: my-store
objectStoreNamespace: rook-ceph
region: us-west-1
region: us-west-1 [3]
bucketName: ceph-bucket [4]
reclaimPolicy: Delete [5]
```
1. `label`(optional) here associates this `StorageClass` to a specific provisioner.
1. `provisioner` responsible for handling `OBCs` referencing this `StorageClass`.
1. **all** `parameter` required.
thotz marked this conversation as resolved.
Show resolved Hide resolved
1. `region`(optional) defines the region in which the bucket should be created. For RGW, this maps to [zonegroup](https://docs.ceph.com/en/latest/radosgw/multisite/#zone-groups) of the server. If user does not define `region`, Rook will fill this value accordingly. Please check example at the end of the doc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we mention the default here? Otherwise, I'm not sure what it means for Rook to "fill this value".

Suggested change
1. `region`(optional) defines the region in which the bucket should be created. For RGW, this maps to [zonegroup](https://docs.ceph.com/en/latest/radosgw/multisite/#zone-groups) of the server. If user does not define `region`, Rook will fill this value accordingly. Please check example at the end of the doc.
1. `region`(optional) defines the region in which the bucket should be created. For RGW, this maps to [zonegroup](https://docs.ceph.com/en/latest/radosgw/multisite/#zone-groups) of the server. If user does not define `region`, the default is `us-east-1`. Please check example at the end of the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisn : I don't what I missed earlier if the user does not define the region, Rook will fill it from Zonegroup from ceph object store context and don't use us-east-1 at all(at least that was my first intention of PR).

us-east-1 is not the default value from RGW(at least not in RGW code base). It is a value that always works mysteriously. Maybe because a lot of s3 clients use this as default for region, but again not defined as default by RGW

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, does that mean rook can just take care of this internally? Why do we need to expose the region setting at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisn : The region defined in storage class of libbucket provisioner. Not sure whether Rook can hide it from user, maybe we can ignore the value. But this was available earlier release not sure whether it right to remove the variable. AFAIR I have sent #8766 as per request from the user, he wants to set the custom region for his use case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. `region`(optional) defines the region in which the bucket should be created. For RGW, this maps to [zonegroup](https://docs.ceph.com/en/latest/radosgw/multisite/#zone-groups) of the server. If user does not define `region`, Rook will fill this value accordingly. Please check example at the end of the doc.
1. `region`(optional) defines the region in which the bucket should be created. For RGW, this maps to the [zone group](https://docs.ceph.com/en/latest/radosgw/multisite/#zone-groups) of the server. If user does not define `region`, Rook will fill this value accordingly. Please check example at the end of the doc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not very clear. What is the region set to if the user doesn't define it? "Accordingly" isn't clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BlaineEXE : earlier version was Rook Operato fill this value from object store context internally. But @travisn suggested it better not to give internal code details for the end-user. Hence changed like this. I can revert back the change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even the text "fill this value from object store context internally" doesn't explain to the user what the value will be. In order for the user to understand when they need to modify the value, they must understand what the default value is. In words good for a user, what value will be the default?

This is also assuming that region is even a necessary parameter to have still.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay will update it again next push

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a link to the example in the doc for users.

1. `bucketName` is required for access to existing buckets but is omitted when provisioning new buckets.
Unlike greenfield provisioning, the brownfield bucket name appears in the `StorageClass`, not the `OBC`.
1. rook-ceph provisioner decides how to treat the `reclaimPolicy` when an `OBC` is deleted for the bucket. See explanation as [specified in Kubernetes](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#retain)
+ _Delete_ = physically delete the bucket.
+ _Retain_ = do not physically delete the bucket.

##### Example: Custom Region
If user is providing the `region` value other than `us-east-1` then additional CRDs for zonegroup/zone/realm need to be created. For example, here user is providing `region` value as `us-west-1`. So make sure following [multisite related CRDs](ceph-object-multisite-crd.md) are defined as well. The Rook Operator will create required `zonegroup` for RGW server.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If user is providing the `region` value other than `us-east-1` then additional CRDs for zonegroup/zone/realm need to be created. For example, here user is providing `region` value as `us-west-1`. So make sure following [multisite related CRDs](ceph-object-multisite-crd.md) are defined as well. The Rook Operator will create required `zonegroup` for RGW server.
To specify a `region` other than `us-east-1`, a custom zone group must be created that matches the desired region name. In the following example, the `region` is set to `us-west-1`. For more details, see the [related multisite CRDs](ceph-object-multisite-crd.md).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do users need to create all of the resources, or only zone group?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly zonegroup is not an independent resource. Rooks mandates to have realm for zonegroup and in RGW does not make sense to have zonegroup without zones (zonegroup is collection of zones and zone is collection of RGW servers in short)

```yaml
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: rook-ceph-delete-bucket
provisioner: rook-ceph.ceph.rook.io/bucket
reclaimPolicy: Delete
parameters:
objectStoreName: my-store
objectStoreNamespace: rook-ceph
region: us-west-1
---
apiVersion: ceph.rook.io/v1
kind: CephObjectRealm
metadata:
name: us-west-realm
namespace: rook-ceph
---
apiVersion: ceph.rook.io/v1
kind: CephObjectZoneGroup
metadata:
name: us-west-1
namespace: rook-ceph
spec:
realm: us-west-realm
---
apiVersion: ceph.rook.io/v1
kind: CephObjectZone
metadata:
name: us-west-1a
namespace: rook-ceph
spec:
zoneGroup: us-west-1
metadataPool:
failureDomain: host
replicated:
size: 1
requireSafeReplicaSize: false
dataPool:
failureDomain: host
replicated:
size: 1
requireSafeReplicaSize: false
parameters:
compression_mode: none
---
apiVersion: ceph.rook.io/v1
kind: CephObjectStore
metadata:
name: my-store
namespace: rook-ceph
spec:
gateway:
port: 80
instances: 1
zone:
name: us-west-1a
```
2 changes: 1 addition & 1 deletion deploy/examples/storageclass-bucket-delete.yaml
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@ reclaimPolicy: Delete
parameters:
objectStoreName: my-store
objectStoreNamespace: rook-ceph # namespace:cluster
region: us-east-1
# region: us-east-1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this needs to match with the zone group, perhaps we should add documentation here in the example and also in the official docs to help users set this value correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BlaineEXE : I will try to add #9556 with this change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thotz Since the default is us-east-1, should we just leave this default uncommented?

Suggested change
# region: us-east-1
region: us-east-1

# To accommodate brownfield cases reference the existing bucket name here instead
# of in the ObjectBucketClaim (OBC). In this case the provisioner will grant
# access to the bucket by creating a new user, attaching it to the bucket, and
Expand Down
2 changes: 1 addition & 1 deletion deploy/examples/storageclass-bucket-retain.yaml
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ reclaimPolicy: Retain
parameters:
objectStoreName: my-store # port 80 assumed
objectStoreNamespace: rook-ceph # namespace:cluster
region: us-east-1
# region: us-east-1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# region: us-east-1
region: us-east-1

# To accommodate brownfield cases reference the existing bucket name here instead
# of in the ObjectBucketClaim (OBC). In this case the provisioner will grant
# access to the bucket by creating a new user, attaching it to the bucket, and
Expand Down
10 changes: 8 additions & 2 deletions pkg/operator/ceph/object/bucket/provisioner.go
View file
Open in desktop
Expand Up @@ -337,13 +337,13 @@ func (p *Provisioner) initializeCreateOrGrant(options *apibkt.BucketOptions) err
}

p.setObjectStoreName(sc)
p.setRegion(sc)
p.setAdditionalConfigData(obc.Spec.AdditionalConfig)
p.setEndpoint(sc)
err = p.setObjectContext()
if err != nil {
return err
}
p.setRegion(sc)

// If an endpoint is declared let's use it
err = p.populateDomainAndPort(sc)
Expand Down Expand Up @@ -513,7 +513,13 @@ func (p *Provisioner) setEndpoint(sc *storagev1.StorageClass) {

func (p *Provisioner) setRegion(sc *storagev1.StorageClass) {
const key = "region"
p.region = sc.Parameters[key]
if len(sc.Parameters[key]) > 0 {
p.region = sc.Parameters[key]
} else {
// If user does not define, then set region to Zonegroup, since RGW internally maps
// aws region to ZoneGroup
p.region = p.objectContext.ZoneGroup
thotz marked this conversation as resolved.
Show resolved Hide resolved
}
Comment on lines +516 to +522
Copy link
Member

@BlaineEXE BlaineEXE Mar 7, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit confused. What is the point of overriding region if the default is mapped to ZoneGroup and if the user has to create a ZoneGroup resource if they want to override region? Would it be simpler to just remove this parameter and rely on users to create a ZoneGroup instead?

Is this only useful if there are multiple zone groups? What happens in the case where there are multiple zone groups? Then what is the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BlaineEXE : region is an optional value for storage-class. If it is not defined then we need to fill up with Zonegroup from object store context, This value changes in different scenarios:
1.) normal cluster, it is name of object store
2.) external cluster it is default
3.) if multiple zones are configured then it depends on value set in object-store CR.

If the user wants to use a custom region then he defines Zonegroup that's different scenario. Hope this avoids the confusion

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all three of those cases, I don't think we need to have the region parameter.

  1. If external cluster with no zone group defined, use default
  2. If external cluster with external zone group defined, use the zone group
  3. If internal cluster with no zone group defined, use name of object store
  4. If internal cluster with zone group defined, use the zone group

Is this not correct? Are there any cases missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BlaineEXE : Please see #9754 (comment) why the region is needed.
Above cases are right, but please note 2.) is not support in Rook atm

}

func (p Provisioner) getObjectStoreEndpoint() string {
Expand Down
2 changes: 1 addition & 1 deletion pkg/operator/ceph/object/health.go
View file
Open in desktop
Expand Up @@ -165,7 +165,7 @@ func (c *bucketChecker) checkObjectStoreHealth() error {

// Initiate s3 agent
logger.Debugf("initializing s3 connection for object store %q", c.namespacedName.Name)
s3client, err := NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "", false)
s3client, err := NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, c.objContext.ZoneGroup, false)
if err != nil {
return errors.Wrap(err, "failed to initialize s3 connection")
}
Expand Down
7 changes: 1 addition & 6 deletions pkg/operator/ceph/object/s3-handlers.go
View file
Open in desktop
Expand Up @@ -45,11 +45,6 @@ func NewInsecureS3Agent(accessKey, secretKey, endpoint, region string, debug boo
}

func newS3Agent(accessKey, secretKey, endpoint, region string, debug bool, tlsCert []byte, insecure bool) (*S3Agent, error) {
var cephRegion = "us-east-1"
if region != "" {
cephRegion = region
}

logLevel := aws.LogOff
if debug {
logLevel = aws.LogDebug
Expand All @@ -64,7 +59,7 @@ func newS3Agent(accessKey, secretKey, endpoint, region string, debug bool, tlsCe
}
session, err := awssession.NewSession(
aws.NewConfig().
WithRegion(cephRegion).
WithRegion(region).
WithCredentials(credentials.NewStaticCredentials(accessKey, secretKey, "")).
WithEndpoint(endpoint).
WithS3ForcePathStyle(true).
Expand Down
4 changes: 2 additions & 2 deletions tests/framework/clients/bucket.go
View file
Open in desktop
Expand Up @@ -157,9 +157,9 @@ func (b *BucketOperation) CheckBucketNotificationSetonRGW(namespace, storeName,
s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName)
s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName)
if tlsEnabled {
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "", true)
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a different region is specified in the storage class, will this client connection work with the store name? Or does it need to match the storage class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisn : I wondered magic behind us-east-1 as region value. The actual value for s3 client needs to storeName and ideally we should have filled same value for storage class. The us-east-1 works for RGW, I am guessing it skips checks(or maybe bug)

Copy link
Contributor Author

@thotz thotz Feb 22, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisn : [Another update]: us-east-1 as a region no longer valid in master, it works with pacific though. So IMO we might see a similar issue when we start using new ceph versions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we remove region from the default examples in rook, it will work with ceph pacific and master? Or will we need different examples for pacific vs quincy? And to be clear, does the ceph master change also affect quincy, or just post-quincy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it will work, this fills internal by Rook. Earlier we are using us-east-1 from this patch we add the actual zone of the rgw server that's the difference.
The us-east-1 worked in quincy as well, but I don't see much code difference in that path for master and Quincy. Need to have a bit of time to understand that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documented examples we are working on in the other comments only apply to Quincy and newer, correct? So what is the behavior for region in Pacific? The docs need to give that detail, especially since Pacific is still the default version with Rook.

Copy link
Contributor Author

@thotz thotz Feb 24, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisn : Sorry if I created confusion before for RGW in any versions, region is the zone group but (again true for all versions) if we give region as us-east-1 it will bypass some checks in RGW and request succeed. I felt it is kind of a hack. But without knowing much about this context Rook was setting us-east-1 for the internal aws s3 SDK and this was working fine for our use case. But this may create confusion for users when they use us-east-1 works but other region's requests are failing. By this PR we are avoiding hack and using the required value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still confused about how users can understand the region. If us-east-1 is a workaround and a hack, maybe we should just keep that for the simple, local rgw case. What does the user gain by using a different region? Seems like it should only be important for external or multi-site where we need the region name in order to connect. But if it's just the local cluster, can we hide the region from the user so they don't need to worry about it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's find a time to discuss this next week since I'm having such a hard time. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisn: Sure :). Sounds good to me

} else {
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "", true, nil)
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true, nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the storeName used here, but the zonegroup is used in the example for using a different region? Shouldn't this use the zonegroup as well? Or in the test, the storeName and zonegroup are the same anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this should be zone group, since we testing only internal ceph cluster without multisite, I just set directly storeName which is value of zonegroup as well

}
if err != nil {
logger.Infof("failed to s3client due to %v", err)
Expand Down
29 changes: 29 additions & 0 deletions tests/framework/clients/object_user.go
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@ package clients

import (
"context"
b64 "encoding/base64"
"fmt"
"strings"

Expand All @@ -34,6 +35,11 @@ type ObjectUserOperation struct {
manifests installer.CephManifests
}

var (
// #nosec G101 since this is not leaking any hardcoded credentials, it's just prefix for the secret name
objectStoreUserSecretPrefix = "rook-ceph-object-user-"
)

// CreateObjectUserOperation creates new rook object user client
func CreateObjectUserOperation(k8sh *utils.K8sHelper, manifests installer.CephManifests) *ObjectUserOperation {
return &ObjectUserOperation{k8sh, manifests}
Expand Down Expand Up @@ -91,3 +97,26 @@ func (o *ObjectUserOperation) Delete(namespace string, userid string) error {
}
return nil
}

// Fetch SecretKey, AccessKey for s3 client.
func (o *ObjectUserOperation) GetAccessKey(namespace, store, userid string) (string, error) {
SecretName := objectStoreUserSecretPrefix + store + "-" + userid
args := []string{"-n", namespace, "get", "secret", SecretName, "-o", "jsonpath={@.data.AccessKey}"}
AccessKey, err := o.k8sh.Kubectl(args...)
if err != nil {
return "", fmt.Errorf("Unable to find access key -- %s", err)
}
decode, _ := b64.StdEncoding.DecodeString(AccessKey)
return string(decode), nil
}

func (o *ObjectUserOperation) GetSecretKey(namespace, store, userid string) (string, error) {
SecretName := objectStoreUserSecretPrefix + store + "-" + userid
args := []string{"-n", namespace, "get", "secret", SecretName, "-o", "jsonpath={@.data.SecretKey}"}
SecretKey, err := o.k8sh.Kubectl(args...)
if err != nil {
return "", fmt.Errorf("Unable to find secret key-- %s", err)
}
decode, _ := b64.StdEncoding.DecodeString(SecretKey)
return string(decode), nil
}
2 changes: 1 addition & 1 deletion tests/framework/installer/ceph_helm_installer.go
View file
Open in desktop
Expand Up @@ -276,7 +276,7 @@ func (h *CephInstaller) CreateObjectStoreConfiguration(values map[string]interfa
return err
}

storageClassBytes := []byte(h.Manifests.GetBucketStorageClass(name, scName, "Delete", "us-east-1"))
storageClassBytes := []byte(h.Manifests.GetBucketStorageClass(name, scName, "Delete", name))
var testObjectStoreSC map[string]interface{}
if err := yaml.Unmarshal(storageClassBytes, &testObjectStoreSC); err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/ceph_base_object_test.go
View file
Open in desktop
Expand Up @@ -53,13 +53,13 @@ var (
ObjectKey4 = "rookObj4"
contentType = "plain/text"
obcName = "smoke-delete-bucket"
region = "us-east-1"
maxObject = "2"
newMaxObject = "3"
bucketStorageClassName = "rook-smoke-delete-bucket"
maxBucket = 1
maxSize = "100000"
userCap = "read"
userBucket = "user-bkt"
)

// Test Object StoreCreation on Rook that was installed via helm
Expand Down
12 changes: 6 additions & 6 deletions tests/integration/ceph_bucket_notification_test.go
View file
Open in desktop
Expand Up @@ -90,7 +90,7 @@ func testBucketNotifications(s suite.Suite, helper *clients.TestClient, k8sh *ut

t.Run("create ObjectBucketClaim", func(t *testing.T) {
logger.Infof("create OBC %q with storageclass %q and notification %q", obcName, bucketStorageClassName, notificationName)
cobErr := helper.BucketClient.CreateBucketStorageClass(namespace, storeName, bucketStorageClassName, "Delete", region)
cobErr := helper.BucketClient.CreateBucketStorageClass(namespace, storeName, bucketStorageClassName, "Delete", storeName)
assert.Nil(t, cobErr)
cobcErr := helper.BucketClient.CreateObcNotification(obcName, bucketStorageClassName, bucketname, notificationName, true)
assert.Nil(t, cobcErr)
Expand Down Expand Up @@ -124,9 +124,9 @@ func testBucketNotifications(s suite.Suite, helper *clients.TestClient, k8sh *ut
s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName)
s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName)
if objectStore.Spec.IsTLSEnabled() {
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true)
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true)
} else {
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true, nil)
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true, nil)
}

assert.Nil(t, err)
Expand Down Expand Up @@ -228,9 +228,9 @@ func testBucketNotifications(s suite.Suite, helper *clients.TestClient, k8sh *ut
s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName)
s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName)
if objectStore.Spec.IsTLSEnabled() {
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true)
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true)
} else {
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true, nil)
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true, nil)
}

assert.Nil(t, err)
Expand Down Expand Up @@ -427,7 +427,7 @@ func testBucketNotifications(s suite.Suite, helper *clients.TestClient, k8sh *ut
assert.NotEqual(t, 4, i)
assert.Equal(t, rgwErr, rgw.RGWErrorNotFound)

dobErr := helper.BucketClient.DeleteBucketStorageClass(namespace, storeName, bucketStorageClassName, "Delete", region)
dobErr := helper.BucketClient.DeleteBucketStorageClass(namespace, storeName, bucketStorageClassName, "Delete", storeName)
assert.Nil(t, dobErr)
})

Expand Down
56 changes: 46 additions & 10 deletions tests/integration/ceph_object_test.go
View file
Open in desktop
Expand Up @@ -163,8 +163,13 @@ func testObjectStoreOperations(s suite.Suite, helper *clients.TestClient, k8sh *
ctx := context.TODO()
clusterInfo := client.AdminTestClusterInfo(namespace)
t := s.T()
context := k8sh.MakeContext()
objectStore, err := k8sh.RookClientset.CephV1().CephObjectStores(namespace).Get(ctx, storeName, metav1.GetOptions{})
assert.Nil(t, err)
rgwcontext, err := rgw.NewMultisiteContext(context, clusterInfo, objectStore)
assert.Nil(t, err)
logger.Infof("Testing Object Store Operations on %s", storeName)

logger.Infof("Testing Object Operations on %s", storeName)
t.Run("create CephObjectStoreUser", func(t *testing.T) {
createCephObjectUser(s, helper, k8sh, namespace, storeName, userid, true, true)
i := 0
Expand All @@ -178,14 +183,44 @@ func testObjectStoreOperations(s suite.Suite, helper *clients.TestClient, k8sh *
assert.NotEqual(t, 4, i)
})

context := k8sh.MakeContext()
objectStore, err := k8sh.RookClientset.CephV1().CephObjectStores(namespace).Get(ctx, storeName, metav1.GetOptions{})
assert.Nil(t, err)
rgwcontext, err := rgw.NewMultisiteContext(context, clusterInfo, objectStore)
assert.Nil(t, err)
t.Run("S3 access for Ceph Object Store User", func(t *testing.T) {
var s3client *rgw.S3Agent
s3endpoint, _ := helper.ObjectClient.GetEndPointUrl(namespace, storeName)
s3AccessKey, _ := helper.ObjectUserClient.GetAccessKey(namespace, storeName, userid)
s3SecretKey, _ := helper.ObjectUserClient.GetSecretKey(namespace, storeName, userid)
if objectStore.Spec.IsTLSEnabled() {
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true)
} else {
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true, nil)
}

assert.Nil(t, err)
logger.Infof("endpoint (%s) Accesskey (%s) secret (%s)", s3endpoint, s3AccessKey, s3SecretKey)

t.Run("create bucket", func(t *testing.T) {
err = s3client.CreateBucket(userBucket)
assert.Nil(t, err)
})

t.Run("delete bucket", func(t *testing.T) {
_, err = s3client.DeleteBucket(userBucket)
assert.Nil(t, err)
})

t.Run("create bucket with invalid region", func(t *testing.T) {
if objectStore.Spec.IsTLSEnabled() {
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "invalid-region", true)
} else {
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "invalid-region", true, nil)
}
err = s3client.CreateBucket(userBucket)
assert.Error(t, err)
})
})

t.Run("create ObjectBucketClaim", func(t *testing.T) {
logger.Infof("create OBC %q with storageclass %q - using reclaim policy 'delete' so buckets don't block deletion", obcName, bucketStorageClassName)
cobErr := helper.BucketClient.CreateBucketStorageClass(namespace, storeName, bucketStorageClassName, "Delete", region)
cobErr := helper.BucketClient.CreateBucketStorageClass(namespace, storeName, bucketStorageClassName, "Delete", storeName)
assert.Nil(t, cobErr)
cobcErr := helper.BucketClient.CreateObc(obcName, bucketStorageClassName, bucketname, maxObject, true)
assert.Nil(t, cobcErr)
Expand Down Expand Up @@ -219,9 +254,9 @@ func testObjectStoreOperations(s suite.Suite, helper *clients.TestClient, k8sh *
s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName)
s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName)
if objectStore.Spec.IsTLSEnabled() {
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true)
s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true)
} else {
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true, nil)
s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, storeName, true, nil)
}

assert.Nil(t, err)
Expand Down Expand Up @@ -269,6 +304,7 @@ func testObjectStoreOperations(s suite.Suite, helper *clients.TestClient, k8sh *
assert.Nil(t, delobjErr)
logger.Info("Objects deleted on bucket successfully")
})

})

t.Run("Regression check: OBC does not revert to Pending phase", func(t *testing.T) {
Expand Down Expand Up @@ -342,7 +378,7 @@ func testObjectStoreOperations(s suite.Suite, helper *clients.TestClient, k8sh *
assert.NotEqual(t, 4, i)
assert.Equal(t, rgwErr, rgw.RGWErrorNotFound)

dobErr := helper.BucketClient.DeleteBucketStorageClass(namespace, storeName, bucketStorageClassName, "Delete", region)
dobErr := helper.BucketClient.DeleteBucketStorageClass(namespace, storeName, bucketStorageClassName, "Delete", storeName)
assert.Nil(t, dobErr)
})

Expand Down