pulsar-resources-operator icon indicating copy to clipboard operation
pulsar-resources-operator copied to clipboard

feat: TLS authentication

Open tomjo opened this issue 2 years ago • 4 comments

(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

  • [x] 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)

  • [x] doc

    (If this PR contains doc changes)

tomjo avatar Aug 23 '23 16:08 tomjo

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

tomjo avatar Aug 23 '23 17:08 tomjo

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.

tomjo avatar Oct 13 '23 07:10 tomjo

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

ericsyh avatar Jan 25 '24 02:01 ericsyh

Hello. Any ETA to merge this? Thanks

brunodomenici avatar Sep 30 '24 09:09 brunodomenici

When will this get merged?

ThomasVerhoeven1998 avatar Feb 12 '25 12:02 ThomasVerhoeven1998

I no longer work at the company that owns the branch repository so I cannot update the PR with master/fix conflicts.

@ThomasVerhoeven1998 is anyone of you able to do this?

Otherwise I could make a new branch/PR but I will have to find some free time for this

tomjo avatar Feb 13 '25 10:02 tomjo

I no longer work at the company that owns the branch repository so I cannot update the PR with master/fix conflicts.

@ThomasVerhoeven1998 is anyone of you able to do this?

Otherwise I could make a new branch/PR but I will have to find some free time for this

@tomjo I assumed that the owners of the project, bring the PR up to date. But if not, we can see if we can do it ourselfs

cc: @FushuWang

ThomasVerhoeven1998 avatar Feb 19 '25 12:02 ThomasVerhoeven1998

I no longer work at the company that owns the branch repository so I cannot update the PR with master/fix conflicts. @ThomasVerhoeven1998 is anyone of you able to do this? Otherwise I could make a new branch/PR but I will have to find some free time for this

@tomjo I assumed that the owners of the project, bring the PR up to date. But if not, we can see if we can do it ourselfs

cc: @FushuWang

@ThomasVerhoeven1998 Please create a new PR.

lhotari avatar Apr 08 '25 08:04 lhotari

I no longer work at the company that owns the branch repository so I cannot update the PR with master/fix conflicts. @ThomasVerhoeven1998 is anyone of you able to do this? Otherwise I could make a new branch/PR but I will have to find some free time for this

@tomjo I assumed that the owners of the project, bring the PR up to date. But if not, we can see if we can do it ourselfs cc: @FushuWang

@ThomasVerhoeven1998 Please create a new PR.

New PR created

ThomasVerhoeven1998 avatar Apr 08 '25 09:04 ThomasVerhoeven1998

New PR is #301

lhotari avatar Apr 08 '25 13:04 lhotari