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

feat: TLS authentication #143

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

tomjo
Copy link
Contributor

@tomjo tomjo commented Aug 23, 2023

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #108

Motivation

As mentioned in issue, we would like to make use of TLS authentication since pulsar supports this.

Modifications

This PR:

  • Allows only configuring the secure url properties (adminServiceSecureURL & brokerServiceSecureURL) - before it was mandatory to configure adminServiceURL
  • When using secure urls (TLS) allows configuring host key verification / allowInsecureConnection
  • Adds support for TLS authentication
  • Add support to helm chart for volumes/volumeMounts so certificates can be specified

Would you prefer seperate PRs for these? Then I can split em up.

Verifying this change

  • Make sure that the change passes the CI checks.

No tests were added yet, e2e tests would probably be the best to add. I won't be able to run the github actions and have the test setup so it would be a bit hard to add them. My idea was to add a test similar to resources_test but creates a connection with tls authentication parameters and tries to get/create/delete a resource. Certificates will also need to be created and also passed to the pulsar-broker setup, I wasn't sure what the best way to do this is. I am hoping that one of the other contributors or maintainers who have access to an easy e2e test setup could help with this.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

Check the box below.

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@tomjo tomjo requested a review from a team as a code owner August 23, 2023 16:53
@github-actions github-actions bot added the doc This pr contains a document label Aug 23, 2023
@tomjo
Copy link
Contributor Author

tomjo commented Aug 23, 2023

Was not sure what version to set for the helm changes, chose v0.5.0 for now

@tomjo
Copy link
Contributor Author

tomjo commented Oct 13, 2023

I have removed the helm chart changes as I see these are usually done in separate PRs. These changes added support for adding volumes to the helm chart to provide the necessary certificates + update CRD.

I have been thinking, would it perhaps be more preferable to allow configuring secrets for the certificates and have the operator look them up instead of being available as volumes to the operator? Then they could be in any namespace together with the pulsar resources they belong to (when giving the operator the necessary RBAC permissions).

Also still looking for some help on how to do the test setup.

We're running a version of this in production and it is working fine for us but it would be nice to have this feature upstreamed.

pkg/admin/interface.go Outdated Show resolved Hide resolved
zerbitx
zerbitx previously approved these changes Dec 20, 2023
Copy link
Contributor

@zerbitx zerbitx left a comment

Choose a reason for hiding this comment

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

LGTM

@tomjo tomjo requested a review from a team as a code owner December 22, 2023 09:12
@ericsyh
Copy link
Member

ericsyh commented Jan 25, 2024

@labuladong Could you also help take a review on this PR?

@ericsyh ericsyh removed this from the 0.5.0 milestone Jun 24, 2024
@brunodomenici
Copy link

Hello. Any ETA to merge this? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc This pr contains a document
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TLS authentication
4 participants