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

Jump to bottom

mgr/dashboard: add server side encryption to rgw/s3 #47495

Merged
merged 1 commit into from Oct 11, 2022
Merged

mgr/dashboard: add server side encryption to rgw/s3 #47495

merged 1 commit into from Oct 11, 2022

Conversation

aaSharma14
Copy link
Contributor

@aaSharma14 aaSharma14 commented Aug 8, 2022 •
edited

Fixes: https://tracker.ceph.com/issues/57826
Signed-off-by: Aashish Sharma aasharma@redhat.com

Screenshot from 2022-09-15 16-50-02
Screenshot from 2022-09-26 18-45-48
Screenshot from 2022-09-26 18-46-08
Screenshot from 2022-09-26 18-51-07
Screenshot from 2022-09-26 18-47-18

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

epuertat
epuertat reviewed Aug 22, 2022
View reviewed changes
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Good WIP! Some notes on code and styling.

src/pybind/mgr/dashboard/services/rgw_client.py Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/services/rgw_client.py Show resolved Hide resolved
src/pybind/mgr/dashboard/services/rgw_client.py Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/services/rgw_client.py Outdated
'SSEAlgorithm')
sse_algo_element.text = sseAlgorithm

if sseAlgorithm == 'aws:kms':
Copy link
Member

@epuertat epuertat Aug 22, 2022 •
edited

Choose a reason for hiding this comment

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

Enum members are in UPPERCASE:

class SSEAlgorithms(Enum, str):
  NO_ENCRYPTION = 'No Encryption'
  AWS_KMS = 'aws:kms'
Suggested change
if sseAlgorithm == 'aws:kms':
if sse_algorithm == SSEAlgorithms.AWS_KMS:

@github-actions github-actions bot added this to In progress in Dashboard Aug 23, 2022
@aaSharma14 aaSharma14 force-pushed the rgw-s3-encryption branch 2 times, most recently from 0eda28a to 442ba3b Compare August 30, 2022 06:39
@aaSharma14 aaSharma14 marked this pull request as ready for review September 15, 2022 11:13
@aaSharma14 aaSharma14 requested a review from a team as a code owner September 15, 2022 11:13
@aaSharma14 aaSharma14 requested review from torchiaf, Pegonzal, cbodley, mattbenjamin, mdw-at-linuxbox and a team and removed request for a team September 15, 2022 11:13
@aaSharma14
Copy link
Contributor Author

@cbodley @mattbenjamin @mdw-at-linuxbox , In this PR we have introduced the functionality to set the rgw config values for kms encryption(rgw crypt s3 kms backend, rgw crypt vault addr etc.) through UI as mentioned here in this doc (https://docs.ceph.com/en/latest/radosgw/vault/). Further we are taking the keyId as user input and sending that keyId across the PutBucketEncryption API to set the default encryption for that bucket. Can you guys please confirm if this the correct implementation from dashboard side. Thanks in advance.

@cbodley
Copy link
Contributor

cbodley commented Sep 15, 2022

does the dashboard allow you to enable/disable encryption policy on existing buckets, or is this just during bucket creation?

i don't really like the idea of the dashboard overriding config variables this way. what if they're already set on some/all rgws?
i'd much rather see the orchestrator deploy/configure an intregation like this. maybe the dashboard should just grey out the encryption checkbox if the rgw it's talking to doesn't have vault configured?

@aaSharma14
Copy link
Contributor Author

does the dashboard allow you to enable/disable encryption policy on existing buckets, or is this just during bucket creation?

Thanks Casey, we can allow setting/disabling encryption on creation of a bucket and on existing buckets as well.

i don't really like the idea of the dashboard overriding config variables this way. what if they're already set on some/all rgws? i'd much rather see the orchestrator deploy/configure an intregation like this. maybe the dashboard should just grey out the encryption checkbox if the rgw it's talking to doesn't have vault configured?

I think we can check if the config values are already set, then we can go forward with the default values and if they are not set we can allow the user to set them through dashboard. Shouldn't this be the case?

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@github-actions github-actions bot added the mgr label Sep 26, 2022
nizamial09
nizamial09 reviewed Sep 27, 2022
View reviewed changes
Copy link
Member

@nizamial09 nizamial09 left a comment

Choose a reason for hiding this comment

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

initial reviews

...ind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-config-modal/rgw-config-modal.component.html Outdated Show resolved Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-config-modal/rgw-config-modal.component.ts Outdated Show resolved Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-config-modal/rgw-config-modal.component.ts Outdated Show resolved Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-config-modal/rgw-config-modal.component.ts Outdated Show resolved Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.scss Outdated Show resolved Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.scss Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts Outdated Show resolved Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-config-modal/rgw-config-modal.component.ts Outdated Show resolved Hide resolved
@aaSharma14 aaSharma14 force-pushed the rgw-s3-encryption branch 2 times, most recently from e7321cf to 8731591 Compare September 27, 2022 06:31
nizamial09
nizamial09 reviewed Sep 27, 2022
View reviewed changes
Copy link
Member

@nizamial09 nizamial09 left a comment

Choose a reason for hiding this comment

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

I saw an rgw-encryption file in the models which is empty. that should be removed too i guess

...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts Outdated Show resolved Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/models/rgw-bucket-encryption.ts Show resolved Hide resolved
...ybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-config-modal/rgw-config-modal.component.ts Show resolved Hide resolved
...ind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-config-modal/rgw-config-modal.component.html Outdated Show resolved Hide resolved
@aaSharma14
Copy link
Contributor Author

jenkins test dashboard cephadm

@aaSharma14
Copy link
Contributor Author

jenkins test api

1 similar comment
@aaSharma14
Copy link
Contributor Author

jenkins test api

@aaSharma14
Copy link
Contributor Author

jenkins test dashboard

@aaSharma14
Copy link
Contributor Author

jenkins test make check

1 similar comment
@aaSharma14
Copy link
Contributor Author

jenkins test make check

@aaSharma14
Copy link
Contributor Author

jenkins test api

@aaSharma14 aaSharma14 force-pushed the rgw-s3-encryption branch 4 times, most recently from b79adca to 67d3b60 Compare September 29, 2022 11:09
@aaSharma14
Copy link
Contributor Author

jenkins test dashboard

@aaSharma14
Copy link
Contributor Author

jenkins test make check arm64

1 similar comment
@aaSharma14
Copy link
Contributor Author

jenkins test make check arm64

@aaSharma14
Copy link
Contributor Author

@cbodley, I have updated the PR as per your comments, If you can please re-check that would be great. Thanks in advance.

@aaSharma14
Copy link
Contributor Author

jenkins test dashboard cephadm

pereman2
pereman2 approved these changes Oct 10, 2022
View reviewed changes
src/pybind/mgr/dashboard/controllers/rgw.py Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/controllers/rgw.py Show resolved Hide resolved
src/pybind/mgr/dashboard/controllers/rgw.py Outdated Show resolved Hide resolved
@nizamial09
Copy link
Member

jenkins test make check

@nizamial09
Copy link
Member

jenkins test dashboard cephadm

@nizamial09
Copy link
Member

jenkins test dashboard

@aaSharma14
mgr/dashboard: add server side encryption to rgw/s3
42aa228 
Fixes:https://tracker.ceph.com/issues/57826
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
@nizamial09 nizamial09 merged commit 43919b4 into ceph:main Oct 11, 2022
13 checks passed
Dashboard automation moved this from Reviewer approved to Done Oct 11, 2022
@nizamial09 nizamial09 deleted the rgw-s3-encryption branch October 11, 2022 07:18
@aaSharma14 aaSharma14 mentioned this pull request Oct 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pereman2 pereman2 pereman2 approved these changes

@nizamial09 nizamial09 nizamial09 approved these changes

@torchiaf torchiaf Awaiting requested review from torchiaf torchiaf is a code owner automatically assigned from ceph/dashboard

@Pegonzal Pegonzal Awaiting requested review from Pegonzal Pegonzal is a code owner automatically assigned from ceph/dashboard

@cbodley cbodley Awaiting requested review from cbodley

@mattbenjamin mattbenjamin Awaiting requested review from mattbenjamin

@mdw-at-linuxbox mdw-at-linuxbox Awaiting requested review from mdw-at-linuxbox

@epuertat epuertat Awaiting requested review from epuertat

Assignees
No one assigned
Labels
api-change dashboard mgr pybind
Projects
Ceph-Dashboard
Archived in project
Dashboard
  
Done
Milestone
No milestone
5 participants
@aaSharma14 @cbodley @nizamial09 @pereman2 @epuertat