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.

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

ceph: add options for cephobjectstore user #8211

Merged
merged 1 commit into from Sep 8, 2021

Conversation

thotz
Copy link
Contributor

@thotz thotz commented Jun 28, 2021

Description of your changes:
Adding options for quota, bucket limit, caps for the
cephobjectstoreuser.
Signed-off-by: Jiffin Tony Thottan thottanjiffin@gmail.com

Which issue is resolved by this Pull Request:
Resolves #7906

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
  • Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
  • Pending release notes updated with breaking and/or notable changes, if necessary.
  • Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
  • Code generation (make codegen) has been run to update object specifications, if necessary.

@thotz thotz added do-not-merge DO NOT MERGE :) WIP Work in Progress labels Jun 28, 2021
@mergify mergify bot added the ceph main ceph tag label Jun 28, 2021
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

It will be great to have these settings on the object users! Maybe you're already planning on it, but please do add:

  • Documentation updates to ceph-object-store-user-crd.md
  • Example settings in object-user.yaml, perhaps commented out

pkg/apis/ceph.rook.io/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/ceph.rook.io/v1/types.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
Documentation/ceph-object-store-user-crd.md Outdated Show resolved Hide resolved
Documentation/ceph-object-store-user-crd.md Outdated Show resolved Hide resolved
Documentation/ceph-object-store-user-crd.md Outdated Show resolved Hide resolved
PendingReleaseNotes.md Outdated Show resolved Hide resolved
pkg/apis/ceph.rook.io/v1/types.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
@thotz thotz requested a review from travisn August 11, 2021 10:16
@thotz thotz removed do-not-merge DO NOT MERGE :) WIP Work in Progress labels Aug 11, 2021
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

a couple small suggestions...

pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller_test.go Outdated Show resolved Hide resolved
@thotz thotz force-pushed the enhancecephuser branch 3 times, most recently from 1a639d7 to b82b353 Compare August 12, 2021 09:05
@thotz thotz requested a review from travisn August 12, 2021 09:06
@travisn
Copy link
Member

travisn commented Aug 12, 2021

The code changes look good and the new integration test will be very nice, thanks. The upgrade and smoke suites just seem to be failing if you could take a look. For example, this failure was seeing from the upgrade suite:

2021-08-12 09:20:18.517555 I | integrationTest: pool upgradepool is deleted
    suite.go:63: test panicked: runtime error: index out of range [0] with length 0
        goroutine 125 [running]:
        runtime/debug.Stack(0xc000879458, 0x1c48dc0, 0xc00070aac8)
        	/opt/hostedtoolcache/go/1.16.7/x64/src/runtime/debug/stack.go:24 +0x9f
        github.com/stretchr/testify/suite.failOnPanic(0xc0002d8c00)
        	/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:63 +0x5b
        panic(0x1c48dc0, 0xc00070aac8)
        	/opt/hostedtoolcache/go/1.16.7/x64/src/runtime/panic.go:965 +0x1b9
        github.com/rook/rook/tests/integration.checkCephObjectUser(0xc0007e3190, 0xc0007e31a0, 0xc0002d8c00, 0xc000944730, 0xc0005e0510, 0x1d73c9e, 0xa, 0x1d7ac45, 0xf, 0x1d77c33, ...)
        	/home/runner/work/rook/rook/tests/integration/ceph_base_object_test.go:155 +0x98f

@thotz
Copy link
Contributor Author

thotz commented Aug 12, 2021

The code changes look good and the new integration test will be very nice, thanks. The upgrade and smoke suites just seem to be failing if you could take a look. For example, this failure was seeing from the upgrade suite:

2021-08-12 09:20:18.517555 I | integrationTest: pool upgradepool is deleted
    suite.go:63: test panicked: runtime error: index out of range [0] with length 0
        goroutine 125 [running]:
        runtime/debug.Stack(0xc000879458, 0x1c48dc0, 0xc00070aac8)
        	/opt/hostedtoolcache/go/1.16.7/x64/src/runtime/debug/stack.go:24 +0x9f
        github.com/stretchr/testify/suite.failOnPanic(0xc0002d8c00)
        	/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:63 +0x5b
        panic(0x1c48dc0, 0xc00070aac8)
        	/opt/hostedtoolcache/go/1.16.7/x64/src/runtime/panic.go:965 +0x1b9
        github.com/rook/rook/tests/integration.checkCephObjectUser(0xc0007e3190, 0xc0007e31a0, 0xc0002d8c00, 0xc000944730, 0xc0005e0510, 0x1d73c9e, 0xa, 0x1d7ac45, 0xf, 0x1d77c33, ...)
        	/home/runner/work/rook/rook/tests/integration/ceph_base_object_test.go:155 +0x98f

@travisn : I hope above failures

@travisn
Copy link
Member

travisn commented Aug 12, 2021

The smoke suite is passing. Now the upgrade test is just failing. Since the settings are not applied before the upgrade, looks like we just need to check for different values in that case.

   ceph_upgrade_test.go:197: 
        	Error Trace:	ceph_base_object_test.go:157
        	            				ceph_upgrade_test.go:197
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 1000
        	Test:       	TestCephUpgradeSuite/TestUpgradeToMaster
    ceph_upgrade_test.go:197: 
        	Error Trace:	ceph_base_object_test.go:158
        	            				ceph_upgrade_test.go:197
        	Error:      	Not equal: 
        	            	expected: 2
        	            	actual  : 0
        	Test:       	TestCephUpgradeSuite/TestUpgradeToMaster
    ceph_upgrade_test.go:197: 
        	Error Trace:	ceph_base_object_test.go:159
        	            				ceph_upgrade_test.go:197
        	Error:      	Not equal: 
        	            	expected: 100000
        	            	actual  : 0
        	Test:       	TestCephUpgradeSuite/TestUpgradeToMaster

@thotz
Copy link
Contributor Author

thotz commented Aug 13, 2021

The smoke suite is passing. Now the upgrade test is just failing. Since the settings are not applied before the upgrade, looks like we just need to check for different values in that case.

   ceph_upgrade_test.go:197: 
        	Error Trace:	ceph_base_object_test.go:157
        	            				ceph_upgrade_test.go:197
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 1000
        	Test:       	TestCephUpgradeSuite/TestUpgradeToMaster
    ceph_upgrade_test.go:197: 
        	Error Trace:	ceph_base_object_test.go:158
        	            				ceph_upgrade_test.go:197
        	Error:      	Not equal: 
        	            	expected: 2
        	            	actual  : 0
        	Test:       	TestCephUpgradeSuite/TestUpgradeToMaster
    ceph_upgrade_test.go:197: 
        	Error Trace:	ceph_base_object_test.go:159
        	            				ceph_upgrade_test.go:197
        	Error:      	Not equal: 
        	            	expected: 100000
        	            	actual  : 0
        	Test:       	TestCephUpgradeSuite/TestUpgradeToMaster

Done. PTAL @travisn

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Great to see the tests passing, just one more suggestion...

pkg/operator/ceph/object/user.go Outdated Show resolved Hide resolved
Documentation/ceph-object-store-user-crd.md Outdated Show resolved Hide resolved
Documentation/ceph-object-store-user-crd.md Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
Comment on lines +364 to +390
if user.Spec.Capabilities != nil {
if user.Spec.Capabilities.User != "" {
userConfig.UserCaps += fmt.Sprintf("users=%s;", user.Spec.Capabilities.User)
}
if user.Spec.Capabilities.Bucket != "" {
userConfig.UserCaps += fmt.Sprintf("buckets=%s;", user.Spec.Capabilities.Bucket)
}
if user.Spec.Capabilities.MetaData != "" {
userConfig.UserCaps += fmt.Sprintf("metadata=%s;", user.Spec.Capabilities.MetaData)
}
if user.Spec.Capabilities.Usage != "" {
userConfig.UserCaps += fmt.Sprintf("usage=%s;", user.Spec.Capabilities.Usage)
}
if user.Spec.Capabilities.Zone != "" {
userConfig.UserCaps += fmt.Sprintf("zone=%s;", user.Spec.Capabilities.Zone)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Similarly to my question about removing quotas, does this allow caps to be revoked? What if a CephObjectStoreUser needs to have some permissions revoked after being given permissions (like the admin made an initial mistake)? Should we always set capabilities even if user.Spec.Capabilities is nil so that permissions can be easily revoked?

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 : Apparently, it looks like either bug in go-ceph or rgw(most likely) or updating caps is not working atm. It can be performed only while CreateUser API() not with ModifyUser(), even though the request is filled with caps details. I have mentioned the same in ceph-object-user-crd.md

pkg/apis/ceph.rook.io/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/ceph.rook.io/v1/types.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
Comment on lines 360 to 372
if user.Spec.Quotas != nil && user.Spec.Quotas.MaxBuckets != 0 {
userConfig.MaxBuckets = &user.Spec.Quotas.MaxBuckets
}
Copy link
Member

Choose a reason for hiding this comment

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

admin.User has a UserQuota spec inside of it which is the same thing set in the above code. Why are we setting bucket quotas at a different point in the code from the rest of the quotas? Is it not better to just set everything all at once upon user creation? (i.e., move the other quota code here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how RGW adminOps apis works, maxBucket is not part of quota Spec. And we can set quota(I mean object and size) only after successful user creation, not during the CreateUser(). Hence I moved to setquota() after the success of user creation. So I cannot move the quota code here. The setting MaxBuckets can be moved to another quota code part, but it requires another additional ModifyUser() api call. So I kept the MaxBuckets during the creation of User and other quotas after user creation.

@thotz thotz force-pushed the enhancecephuser branch 3 times, most recently from f0755e0 to 73d332b Compare September 3, 2021 13:35
@thotz thotz requested a review from BlaineEXE September 3, 2021 13:37
@thotz thotz force-pushed the enhancecephuser branch 2 times, most recently from 1e6a35e to e6db4ef Compare September 3, 2021 14:43
// Maximum bucket limit for the ceph user
// +optional
// +nullable
MaxBuckets *int `json:"maxBuckets,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set MaxBuckets to *int and MaxObjects into int64* only based on admin.User Struct form go ceph

Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

Thanks for your patience and paying close attention to this.

It's really unfortunate that capabilities can't be modified or removed once they are set due to a Ceph (or go-ceph) bug. Thanks for noting and documenting that.

From a feature perspective, I'm trying to understand what are the necessary steps to take after this PR merges. In my opinion, modifying and removing caps is required for us to call this feature complete. Users expect that of a declarative configuration system like Rook. Some questions to help me:

  • Is this for a BZ feature request? If so, we should make sure we have deletion done before that BZ is closed, or we should document the workaround with the docs team
  • Is there a tracker in Ceph to fix the issue from the Ceph side so we can follow up later with update/removal features in Rook?

pkg/operator/ceph/object/user/controller_test.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/user/controller.go Outdated Show resolved Hide resolved
@thotz
Copy link
Contributor Author

thotz commented Sep 3, 2021

Thanks for your patience and paying close attention to this.

It's really unfortunate that capabilities can't be modified or removed once they are set due to a Ceph (or go-ceph) bug. Thanks for noting and documenting that.

From a feature perspective, I'm trying to understand what are the necessary steps to take after this PR merges. In my opinion, modifying and removing caps is required for us to call this feature complete. Users expect that of a declarative configuration system like Rook. Some questions to help me:

I completely agree.

* Is this for a BZ feature request? If so, we should make sure we have deletion done before that BZ is closed, or we should document the workaround with the docs team

* Is there a tracker in Ceph to fix the issue from the Ceph side so we can follow up later with update/removal features in Rook?

Quick look didn't give any bugs, I will try to debug further to know whether issue with go-ceph or on rgw. At least on rasdowgw-admin cli modifying caps works fine. I will open bz/github/ceph tracker issue accordingly after getting a better idea and will open separate issue in Rook as well

@thotz
Copy link
Contributor Author

thotz commented Sep 6, 2021

Thanks for your patience and paying close attention to this.
It's really unfortunate that capabilities can't be modified or removed once they are set due to a Ceph (or go-ceph) bug. Thanks for noting and documenting that.
From a feature perspective, I'm trying to understand what are the necessary steps to take after this PR merges. In my opinion, modifying and removing caps is required for us to call this feature complete. Users expect that of a declarative configuration system like Rook. Some questions to help me:

I completely agree.

* Is this for a BZ feature request? If so, we should make sure we have deletion done before that BZ is closed, or we should document the workaround with the docs team

* Is there a tracker in Ceph to fix the issue from the Ceph side so we can follow up later with update/removal features in Rook?

Quick look didn't give any bugs, I will try to debug further to know whether issue with go-ceph or on rgw. At least on rasdowgw-admin cli modifying caps works fine. I will open bz/github/ceph tracker issue accordingly after getting a better idea and will open separate issue in Rook as well

Create https://tracker.ceph.com/issues/52521 in RGW to track this issue. Currently, RGW code does not handle the caps in the modifyuserAPI request().

@thotz
Copy link
Contributor Author

thotz commented Sep 7, 2021

Thanks for your patience and paying close attention to this.
It's really unfortunate that capabilities can't be modified or removed once they are set due to a Ceph (or go-ceph) bug. Thanks for noting and documenting that.
From a feature perspective, I'm trying to understand what are the necessary steps to take after this PR merges. In my opinion, modifying and removing caps is required for us to call this feature complete. Users expect that of a declarative configuration system like Rook. Some questions to help me:

I completely agree.

* Is this for a BZ feature request? If so, we should make sure we have deletion done before that BZ is closed, or we should document the workaround with the docs team

* Is there a tracker in Ceph to fix the issue from the Ceph side so we can follow up later with update/removal features in Rook?

Quick look didn't give any bugs, I will try to debug further to know whether issue with go-ceph or on rgw. At least on rasdowgw-admin cli modifying caps works fine. I will open bz/github/ceph tracker issue accordingly after getting a better idea and will open separate issue in Rook as well

Create https://tracker.ceph.com/issues/52521 in RGW to track this issue. Currently, RGW code does not handle the caps in the modifyuserAPI request().

Thanks for your patience and paying close attention to this.
It's really unfortunate that capabilities can't be modified or removed once they are set due to a Ceph (or go-ceph) bug. Thanks for noting and documenting that.
From a feature perspective, I'm trying to understand what are the necessary steps to take after this PR merges. In my opinion, modifying and removing caps is required for us to call this feature complete. Users expect that of a declarative configuration system like Rook. Some questions to help me:

I completely agree.

* Is this for a BZ feature request? If so, we should make sure we have deletion done before that BZ is closed, or we should document the workaround with the docs team

* Is there a tracker in Ceph to fix the issue from the Ceph side so we can follow up later with update/removal features in Rook?

Quick look didn't give any bugs, I will try to debug further to know whether issue with go-ceph or on rgw. At least on rasdowgw-admin cli modifying caps works fine. I will open bz/github/ceph tracker issue accordingly after getting a better idea and will open separate issue in Rook as well

We can use different approach here as well, instead of ModifyUser() API. We can have apis adding/removing for Caps() similar to quota, but it does not exist in go-ceph or not mentioned in RGW upstream adminOps docs. But the supported code exists in ceph-rgw

@mergify
Copy link

mergify bot commented Sep 7, 2021

This pull request has merge conflicts that must be resolved before it can be merged. @thotz please rebase it. https://rook.io/docs/rook/master/development-flow.html#updating-your-fork

Adding options for quota, bucket limit, caps for the
`cephobjectstoreuser`.

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

I am approving this PR with some nit comments to update docs and comments. Feel free to update the nits and merge the PR afterwards, or you can create a new PR to address the nits if you prefer.

@@ -281,13 +281,44 @@ func (r *ReconcileObjectStoreUser) createorUpdateCephUser(u *cephv1.CephObjectSt
} else {
return errors.Wrapf(err, "failed to get details from ceph object user %q", u.Name)
}
} else if *user.MaxBuckets != *r.userConfig.MaxBuckets {
// TODO handle update for user capabilities
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Let's link to the Ceph issue tracker here so we understand what is blocking Rook currently.

https://tracker.ceph.com/issues/52521

@@ -149,6 +153,17 @@ func checkCephObjectUser(
assert.NoError(s.T(), err)
assert.Equal(s.T(), k8sutil.ReadyStatus, phase)
}
if checkQuotaAndCaps {
// following fields in CephObjectStoreUser CRD doesn't exist before Rook v1.7
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
// following fields in CephObjectStoreUser CRD doesn't exist before Rook v1.7
// following fields in CephObjectStoreUser CRD doesn't exist before Rook v1.7.3

* `maxBuckets`: The maximum bucket limit for the user.
* `maxSize`: Maximum size limit of all objects across all the user's buckets.
* `maxObjects`: Maximum number of objects across all the user's buckets.
* `capabilities`: Ceph allows users to be given additional permissions(support added from onwards v1.7.3).
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
* `capabilities`: Ceph allows users to be given additional permissions(support added from onwards v1.7.3).
* `capabilities`: Ceph allows users to be given additional permissions (supported in Rook v1.7.3 and up).

* `maxSize`: Maximum size limit of all objects across all the user's buckets.
* `maxObjects`: Maximum number of objects across all the user's buckets.
* `capabilities`: Ceph allows users to be given additional permissions(support added from onwards v1.7.3).
P.S this setting can used only during the creation of the object store user, not afterwards.
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
P.S this setting can used only during the creation of the object store user, not afterwards.
Due to a bug in Ceph, this setting can currently only be used during the creation of the object store user. If a user's capabilities need modified, the user must be deleted and re-created.

@@ -33,3 +40,17 @@ spec:

* `store`: The object store in which the user will be created. This matches the name of the objectstore CRD.
* `displayName`: The display name which will be passed to the `radosgw-admin user create` command.
* `quotas`: This represents quota limitation can be set on the user(support added from onwards v1.7.3).
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
* `quotas`: This represents quota limitation can be set on the user(support added from onwards v1.7.3).
* `quotas`: Quota limitations to be set on the user (supported in Rook v1.7.3 and up).

@thotz
Copy link
Contributor Author

thotz commented Sep 8, 2021

I am approving this PR with some nit comments to update docs and comments. Feel free to update the nits and merge the PR afterwards, or you can create a new PR to address the nits if you prefer.

@BlaineEXE : May I address the nits after the merge of this PR as a separate one?

@BlaineEXE BlaineEXE merged commit 5b0785d into rook:master Sep 8, 2021
thotz added a commit to thotz/rook that referenced this pull request Sep 9, 2021
Addressing remaining nits from the PR rook#8211

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
@thotz thotz mentioned this pull request Sep 9, 2021
10 tasks
mergify bot added a commit that referenced this pull request Sep 9, 2021
ceph: add options for cephobjectstore user (backport #8211)
BlaineEXE added a commit that referenced this pull request Sep 9, 2021
mergify bot pushed a commit that referenced this pull request Sep 9, 2021
Addressing remaining nits from the PR #8211

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
(cherry picked from commit 50ecff8)
BlaineEXE added a commit that referenced this pull request Sep 9, 2021
subhamkrai pushed a commit to subhamkrai/rook that referenced this pull request Oct 1, 2021
Addressing remaining nits from the PR rook#8211

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ceph main ceph tag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set object storage quota per namespace/project via a ResourceQuota resource
3 participants