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

rgw: inject tls certs for bucket notification and topic operations #9565

Merged
merged 1 commit into from Jan 24, 2022
Merged

rgw: inject tls certs for bucket notification and topic operations #9565

merged 1 commit into from Jan 24, 2022

Conversation

thotz
Copy link
Contributor

@thotz thotz commented Jan 12, 2022

Description of your changes:
The certs for accessing TLS enabled RGW is saved as secrets and inject
them if controllers for notification and topics if the request is sent to
TLS enabled RGW endpoint.

Signed-off-by: Jiffin Tony Thottan thottanjiffin@gmail.com

Which issue is resolved by this Pull Request:
Resolves #

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 requested a review from yuvalif January 12, 2022 11:46
@thotz thotz added backport-release-1.8 object Object protocol - S3 labels Jan 12, 2022
@thotz thotz changed the title rgw: Inject TLS certs for bucket notification and topic operations rgw: inject tls certs for bucket notification and topic operations Jan 12, 2022
@thotz thotz force-pushed the rgw-tls-cert-fix-bucket-notifications branch from 4bd8fbe to 84ca546 Compare January 12, 2022 11:54
@yuvalif yuvalif mentioned this pull request Jan 12, 2022
Merged
leseb
leseb requested changes Jan 12, 2022
View reviewed changes
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Neat

pkg/operator/ceph/object/notification/provisioner.go Outdated Show resolved Hide resolved
pkg/operator/ceph/object/topic/provisioner.go Outdated Show resolved Hide resolved
BlaineEXE
BlaineEXE reviewed Jan 12, 2022
View reviewed changes
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 think this looks good, being pretty straightforward, but I have a couple questions...

I don't see any changes to unit tests. Should we add some validation to the existing unit tests for this to make sure the TLS connections are working as we expect them to?

Should we update any documentation for users about how to use certs?

@thotz
Copy link
Contributor Author

thotz commented Jan 13, 2022 •
edited by BlaineEXE

I think this looks good, being pretty straightforward, but I have a couple questions...

I don't see any changes to unit tests. Should we add some validation to the existing unit tests for this to make sure the TLS connections are working as we expect them to?

@BlaineEXE :
I guess it is better to add cases in the integration tests of SSL objectstore than units tests, we check this cephobjectstore user, obc via integration test than unit tests.

Should we update any documentation for users about how to use certs?

Not really, this is internal between Rook and RGW server. If the RGW server is configured with TLS, then Rook as a s3 client should not fail operations sent to the RGW server.

@thotz thotz force-pushed the rgw-tls-cert-fix-bucket-notifications branch from 84ca546 to 11cd3ec Compare January 13, 2022 07:42
@thotz thotz requested review from leseb and BlaineEXE January 13, 2022 07:43
@yuvalif
Copy link
Contributor

yuvalif commented Jan 13, 2022

I don't see any changes to unit tests. Should we add some validation to the existing unit tests for this to make sure the TLS connections are working as we expect them to?

we do not have any unit tests that cover the session creation between the operator and the RGW.
currently, this is covered in the integration tests, we should probably add that there.

@thotz thotz force-pushed the rgw-tls-cert-fix-bucket-notifications branch from 11cd3ec to 1d967d5 Compare January 13, 2022 11:38
@yuvalif
Copy link
Contributor

yuvalif commented Jan 17, 2022

GetBucketNotificationConfiguration test in failing:
https://github.com/rook/rook/runs/4806381551?check_suite_focus=true#step:4:1371

@thotz thotz force-pushed the rgw-tls-cert-fix-bucket-notifications branch 7 times, most recently from f2203fd to 6e1f01e Compare January 20, 2022 05:11
@thotz thotz requested review from yuvalif and leseb January 21, 2022 05:15
leseb
leseb reviewed Jan 21, 2022
View reviewed changes
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

nit

pkg/operator/ceph/object/notification/provisioner.go Outdated Show resolved Hide resolved
@thotz
rgw: inject tls certs for bucket notification and topic operations
a97747c 
The certs for accessing TLS enabled RGW is saved as secrets and inject
them if controllers for notification and topics if request is sent to
TLS enabled RGW endpoint.

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
Signed-off-by: Jiffin Tony Thottan <jthottan@redhat.com>
@thotz thotz force-pushed the rgw-tls-cert-fix-bucket-notifications branch from 6e1f01e to a97747c Compare January 24, 2022 17:10
@thotz thotz requested a review from leseb January 24, 2022 17:10
leseb
leseb approved these changes Jan 24, 2022
View reviewed changes
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

LGTM!

@BlaineEXE BlaineEXE merged commit 18c1397 into rook:master Jan 24, 2022
@mergify mergify bot mentioned this pull request Jan 24, 2022
Merged
mergify bot added a commit that referenced this pull request Jan 24, 2022
@mergify
Merge pull request #9641 from rook/mergify/bp/release-1.8/pr-9565
ae489d7 
rgw: inject tls certs for bucket notification and topic operations (backport #9565)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leseb leseb leseb approved these changes

@yuvalif yuvalif yuvalif approved these changes

@BlaineEXE BlaineEXE BlaineEXE approved these changes

Assignees
No one assigned
Labels
object Object protocol - S3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@thotz @yuvalif @leseb @BlaineEXE