Feature : ACL integration features #287
Conversation
uds5501
force-pushed
the
feat/acl-integration-gopay
branch
from
c500933 to
9f9b540
Compare
| @@ -95,6 +96,9 @@ | |||
| (defn ssl-config [] | |||
| (get-in config [:ziggurat :ssl])) | |||
|
|
|||
| (defn sasl-config [] | |||
| (get-in config [:ziggurat :sasl])) | |||
There was a problem hiding this comment.
The problem here with this approach of having a single sasl config is that we cannot have seperate ACL for each streams.
This needs to be implemented. The acl should be per stream as each stream can have different topics its reading from hence having separate ACL.
There was a problem hiding this comment.
- As discussed in the thread we are currently okay with keeping actor-level changes.
- To support per-stream changes, (when required) the following plan should be done:
- Update
streams-config-mapping-tableto start setting ACL properties as well. (Currently it only picks up auto-offset reset configs) - not accommodating as part of this PR. - Update the
add-sasl-propertiesto putIfAbsent with actor level ACL if stream ACL is missing - updating this in next commit
project.clj
Outdated
| [cambium/cambium.logback.json "0.4.4"] | ||
| [ch.qos.logback/logback-classic "1.2.9"] | ||
| [cambium/cambium.logback.json "0.4.4" :exclusions [com.fasterxml.jackson.core/jackson-annotations com.fasterxml.jackson.core/jackson-databind]] | ||
| [ch.qos.logback/logback-classic "1.2.9" :exclusions [org.slf4j/slf4j-api]] | ||
| [ch.qos.logback.contrib/logback-json-classic "0.1.5"] |
There was a problem hiding this comment.
why are these excluded ?
There was a problem hiding this comment.
These exclusions were compiler's suggestions. can revert these.
|
@Vruttant1403 please help re-review? |
uds5501
force-pushed
the
feat/acl-integration-gopay
branch
from
b9e182b to
cf9594d
Compare
| volumes: | ||
| - /tmp/ziggurat_kafka_cluster_data/zookeeper/data:/data |
There was a problem hiding this comment.
why have we removed these. ?
There was a problem hiding this comment.
seemed redundant to mount this volume, will add back
| jaas-config (get ssl-config-map :jaas) | ||
| mechanism (get ssl-config-map :mechanism) | ||
| protocol (get ssl-config-map :protocol) | ||
| login-callback-handler (get ssl-config-map :login-callback-handler)] |
There was a problem hiding this comment.
please add what this is doing in the comments above here for context.
| @@ -448,3 +487,34 @@ | |||
| (testing "should return REPLACE_THREAD" | |||
| (let [r (handle-uncaught-exception :replace-thread t)] | |||
| (is (= r StreamsUncaughtExceptionHandler$StreamThreadExceptionResponse/REPLACE_THREAD)))))) | |||
|
|
|||
| (deftest start-streams-test-with-sasl-config | |||
There was a problem hiding this comment.
valid config test is only there for SASL_PLAINTEXT. have we checked if this works with SASL_SSL also as that is your usecase I believe right ?
There was a problem hiding this comment.
- We have verified this in the staging setups. As part of this MR we wanted to verify if SASL configs are being consumed correctly or not.
- To setup TLS for this will be another tedious task. For the testing suite a SASL_PLAINTEXT combination should be okay.
Supporting SASL authentication