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

test: add more test cases for bucket notfication #9169

Merged
merged 1 commit into from
Dec 9, 2021
Merged

test: add more test cases for bucket notfication #9169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions tests/framework/clients/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ import (
b64 "encoding/base64"
"fmt"

"github.com/aws/aws-sdk-go/service/s3"
bktv1alpha1 "github.com/kube-object-storage/lib-bucket-provisioner/pkg/apis/objectbucket.io/v1alpha1"
rgw "github.com/rook/rook/pkg/operator/ceph/object"
"github.com/rook/rook/tests/framework/installer"
"github.com/rook/rook/tests/framework/utils"
)
Expand Down Expand Up @@ -146,3 +148,33 @@ func (b *BucketOperation) CheckOBMaxObject(obcName, maxobject string) bool {
fetchMaxObject, _ := b.k8sh.GetResource("ob", obName, "--output", "jsonpath={.spec.endpoint.additionalConfig.maxObjects}")
return maxobject == fetchMaxObject
}

// Checks the bucket notifications set on RGW backend bucket
func (b *BucketOperation) CheckBucketNotificationSetonRGW(namespace, storeName, obcName, bucketname, notificationName, s3endpoint string) bool {
var s3client *rgw.S3Agent
s3AccessKey, _ := b.GetAccessKey(obcName)
s3SecretKey, _ := b.GetSecretKey(obcName)

//TODO : add TLS check
s3client, err := rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "", true, nil)
if err != nil {
logger.Errorf("S3 client creation failed with error %v", err)
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 return an error instead of logging and returning false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CheckBucketNotificationSetonRGW() is used with along with utils.Retry() and it expects the function to have only boolean, I can change the signature to include the error value as well, but it won't check properly in utils.Retry(). IMO code is more neat in this way

return false
}

notifications, err := s3client.Client.GetBucketNotificationConfiguration(&s3.GetBucketNotificationConfigurationRequest{
Bucket: &bucketname,
})
if err != nil {
logger.Infof("failed to fetch bucket notifications configuration due to %v", err)
return false
}
logger.Infof("%d bucket notifications found in: %+v", len(notifications.TopicConfigurations), notifications)
for _, notification := range notifications.TopicConfigurations {
if *notification.Id == notificationName {
return true
}
logger.Infof("bucket notifications name mismatch %q != %q", *notification.Id, notificationName)
}
return false
}
4 changes: 2 additions & 2 deletions tests/framework/clients/notification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

/*
Copyright 2021 The Rook Authors. All rights reserved.

Expand Down Expand Up @@ -46,7 +45,8 @@ func (n *NotificationOperation) UpdateNotification(notificationName string, topi
}

// CheckNotification if notification was set
func (t *NotificationOperation) CheckNotification(notificationName string) bool {
func (t *NotificationOperation) CheckNotificationCR(notificationName string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

This whole test isn't really useful. This merely verifies that kubernetes has received the request to create the cephbucketnotification resource and committed it to etcd. Things that are more critical to verify are (1) has Rook finished reconciling the notification, and (2) what is the status of the cephbucketnotification resource. I would suggest separate checks for each.

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, I agree. It will become sense if the notification status is present in the CR. will add as todo list and modify it after it is added

Copy link
Contributor

Choose a reason for hiding this comment

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

this is planned to be added in phase1 (once the refactoring+UT PR is merged). see: https://gist.github.com/yuvalif/c4c7aed3dbc956a65399865a77c6a929#enhanced-cephbucketnotification-status

// TODO: return result based on reconcile status of the CR
thotz marked this conversation as resolved.
Show resolved Hide resolved
const resourceName = "cephbucketnotification"
_, err := t.k8sh.GetResource(resourceName, notificationName)
if err != nil {
Expand Down
8 changes: 7 additions & 1 deletion tests/framework/utils/k8s_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"time"

"github.com/coreos/pkg/capnslog"
bktclient "github.com/kube-object-storage/lib-bucket-provisioner/pkg/client/clientset/versioned"
"github.com/pkg/errors"
rookclient "github.com/rook/rook/pkg/client/clientset/versioned"
"github.com/rook/rook/pkg/clusterd"
Expand All @@ -56,6 +57,7 @@ type K8sHelper struct {
remoteExecutor *exec.RemotePodCommandExecutor
Clientset *kubernetes.Clientset
RookClientset *rookclient.Clientset
BucketClientset *bktclient.Clientset
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 it is not okay to add bucketclient, I will remove it here and define in the test file

Copy link
Member

@BlaineEXE BlaineEXE Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I think it might be best to add this to the TestClient the same as the BucketClient. Not here. https://github.com/rook/rook/blob/master/tests/framework/clients/test_client.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it make more sense to add here than in TestClient{}, bucketclient is similar to rookclient to execute apis directly on OBC and it requires rest.Config

Copy link
Member

Choose a reason for hiding this comment

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

K8SHelper has all very low-level clients. What is appropriate for the K8SHelper, based on current context might be a low-level OBCClientset, but the overall of the BucketClientset I think should be in TestClient because it is much more than just a clientset. It has many more helper methods. I think adding the bucket clientset here would be good, and adding the core of BucketClientset with its helper methods to TestClient is the best thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already bucket client as part of TestClient, here it more like low-level k8s operations like updating labels which can be easily with bucket client apis

Copy link
Member

Choose a reason for hiding this comment

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

Unless this changed recently, I may have been misinterpreting something. This looks good here as BucketClientset *bktclient.Clientset with other stuff in TestClient.

RunningInCluster bool
T func() *testing.T
}
Expand Down Expand Up @@ -94,13 +96,17 @@ func CreateK8sHelper(t func() *testing.T) (*K8sHelper, error) {
if err != nil {
return nil, fmt.Errorf("failed to get rook clientset. %+v", err)
}
bucketClientset, err := bktclient.NewForConfig(config)
if err != nil {
return nil, fmt.Errorf("failed to get lib-bucket-provisioner clientset. %+v", err)
}

remoteExecutor := &exec.RemotePodCommandExecutor{
ClientSet: clientset,
RestClient: config,
}

h := &K8sHelper{executor: executor, Clientset: clientset, RookClientset: rookClientset, T: t, remoteExecutor: remoteExecutor}
h := &K8sHelper{executor: executor, Clientset: clientset, RookClientset: rookClientset, BucketClientset: bucketClientset, T: t, remoteExecutor: remoteExecutor}
if strings.Contains(config.Host, "//10.") {
h.RunningInCluster = true
}
Expand Down
Loading