Restore safetyValve and singleUser when not using LDAP or OIDC

cetic / helm-nifi

Helm Chart for Apache Nifi

Apache License 2.0

215 stars 228 forks source link

Restore safetyValve and singleUser when not using LDAP or OIDC #212

Closed wknickless closed 2 years ago

wknickless commented 2 years ago

What this PR does / why we need it:

Fixes the NiFi Helm chart when LDAP and OIDC are not enabled, which they are not by default.

Which issue this PR fixes

fixes #208
fixes #211
fixes #213

Checklist

[x] DCO signed
[x] Fix NOTES.txt suitable for the default ClusterIP service
[x] Add test to confirm persistent storage works (with possible fixes)
[x] Add test to confirm LDAP works (with possible fixes)
[x] Add test to confirm OIDC works (with possible fixes)
[x] Adjust github workflow definition to run in master branch too
[x] Chart Version bumped

wknickless commented 2 years ago

@banzo @zakaria2905 I have not yet tested whether this breaks LDAP and OIDC, or whether it actually fixes persistent storage, so you might want to hold off on merging this until you get a chance to test it or I get those tests merged.

leshibily commented 2 years ago

@banzo can we expect the release this week?

banzo commented 2 years ago

@wknickless amazing work with the tests, thank you very much!

@leshibily probably not, the oidc feature still need to be tested and we will not have time to take care of it this week.

leshibily commented 2 years ago

@banzo thank you.

@wknickless forgot to mention, great work, mate.

wknickless commented 2 years ago

Thanks for the positive feedback!

Didn't get the OIDC test written today. As of 971dea20159b58fc65ebde2f74492e84a481feaf OIDC works. I tested it manually, driving Firefox, using Keycloak as the OpenID Provider. The Keycloak setup artifacts are in the tests/ directory.

Looks like it should be straightforward to write a simple login-succeeds and login-fails test pair in Puppeteer driving browserless, but I guess I'll find out shortly.

wknickless commented 2 years ago

@banzo This PR passes the tests and resolves #208, #211 and #213. But @leshibily is probably correct that it won't support scaling beyond a single node.

How would you like to proceed? If you merge and release as-is then people can use it for single NiFi node clusters. Alternatively, we could open another issue about scaling to multiple nodes and update the scope of this PR.

Also, would you entertain a dependency on cert-manager to manage the TLS keys within the NiFi cluster when scaling beyond a single node, and for the UI? Or would you prefer to stick with the NiFi toolkit?

leshibily commented 2 years ago

@wknickless Thank you for the quick fixes and thanks for verifying the bug. Why isn't a Nifi cluster created even though properties.isNode is set to true in the values.yaml file? Also, how long would it take to fix the clustering issue? This is a blocker for us using Nifi in production.

Appreciate the efforts again.

banzo commented 2 years ago

@wknickless

How would you like to proceed? If you merge and release as-is then people can use it for single NiFi node clusters. Alternatively, we could open another issue about scaling to multiple nodes and update the scope of this PR.

I think we should merge as is, multi-node is broken anyway in the current latest release. And then fix SSL multi-node in another PR.

Also, would you entertain a dependency on cert-manager to manage the TLS keys within the NiFi cluster when scaling beyond a single node, and for the UI? Or would you prefer to stick with the NiFi toolkit?

cert-manager seems to be a good fit to handle that issue. We are trying it for Nifi UI in some deployments.

wknickless commented 2 years ago

@banzo Thank you for the guidance. I've opened #218 for the cert-manager feature that will also allow secure clusters.