Skip to content

Conversation

@maltesander
Copy link
Member

@maltesander maltesander commented Dec 23, 2022

Description

New structure looks like this:

apiVersion: kafka.stackable.tech/v1alpha1
kind: KafkaCluster
metadata:
  name: simple-kafka
spec:
  image:
    productVersion: 3.3.1
    stackableVersion: 0.3.0
  clusterConfig:
    authentication:
      - authenticationClass: kafka-client-auth-tls
    tls:
      internalSecretClass: kafka-internal-tls
      serverSecretClass: tls
    zookeeperConfigMapName: simple-kafka-znode
  brokers:
....

fixes: #529

test: https://ci.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/kafka-operator-it-custom/29/

Review Checklist

  • Code contains useful comments
  • CRD change approved (or not applicable)
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)
  • Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@maltesander maltesander added release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. changelog/crd-change labels Dec 23, 2022
@maltesander maltesander requested a review from a team December 23, 2022 15:20
@maltesander maltesander self-assigned this Dec 23, 2022
@maltesander maltesander marked this pull request as ready for review December 23, 2022 15:52
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

I really like the refactoring, many thanks!

@maltesander maltesander requested a review from sbernauer January 2, 2023 09:45
@maltesander maltesander requested a review from sbernauer January 2, 2023 10:03
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Sorry, this should be it 🙈

Copy link
Contributor

@vsupalov vsupalov left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@maltesander maltesander requested a review from sbernauer January 2, 2023 13:12
@maltesander
Copy link
Member Author

bors merge

bors bot pushed a commit that referenced this pull request Jan 2, 2023
# Description

New structure looks like this:
```
apiVersion: kafka.stackable.tech/v1alpha1
kind: KafkaCluster
metadata:
  name: simple-kafka
spec:
  image:
    productVersion: 3.3.1
    stackableVersion: 0.3.0
  clusterConfig:
    authentication:
      - authenticationClass: kafka-client-auth-tls
    tls:
      internalSecretClass: kafka-internal-tls
      serverSecretClass: tls
    zookeeperConfigMapName: simple-kafka-znode
  brokers:
....
```

fixes: #529  

test: https://ci.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/kafka-operator-it-custom/29/



Co-authored-by: Malte Sander <malte.sander.it@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jan 2, 2023

Pull request successfully merged into main.

Build succeeded!

And happy new year! 🎉

@bors bors bot changed the title Consolidate TLS encryption and authentication [Merged by Bors] - Consolidate TLS encryption and authentication Jan 2, 2023
@bors bors bot closed this Jan 2, 2023
@bors bors bot deleted the consolidate-encryption-and-authentication branch January 2, 2023 13:57
@maltesander
Copy link
Member Author

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 5, 2023

Already running a review

@sbernauer
Copy link
Member

🤔

bors bot pushed a commit to stackabletech/druid-operator that referenced this pull request Jan 10, 2023
# Description

Fixes #365

Changes, heavily inspired by the [consolidation which recently happened for the kafka-operator](stackabletech/kafka-operator#532). Relates to stackabletech/issues#293

The new structure was guided by this snippet:
```
apiVersion: druid.stackable.tech/v1alpha1
kind: DruidCluster
metadata:
  name: derby-druid
spec:
  image:
    productVersion: 24.0.0
    stackableVersion: 0.3.0
  clusterConfig:
    authentication:
      - authenticationClass: druid-tls-authentication-class (tls) # String
      - authenticationClass: druid-ldap-authentication-class (ldap) # String
    authorization:
      opa:
        configMapName: test-opa
        package: druid
    zookeeperConfigMapName: druid-znode
    metadataStorageDatabase:
      dbType: derby
      connString: jdbc:derby://localhost:1527/var/druid/metadata.db;create=true
      host: localhost
      port: 1527
    deepStorage:
      hdfs:
        configMapName: druid-hdfs
        directory: /druid
    tls:
      serverSecretClass: secret_class # Option<String>. *In general* defaults to "tls"
      internalSecretClass: secret_class # Option<String>. *In general* defaults to "tls"
```

## Overview of introduced changes

While working on the main issue, adjacent and somewhat-related refactorings/changes were introduced as well:

* Prefer not to disable TLS for integration tests, where possible (justification: while the complexity is slightly higher, we are tested the recommended codepath more, as TLS is on by default)
* Introduce dedicated authorization and security rust files
* Adjustments to test helper scripts (mostly regarding uniformity and ergonomics)

## Highlight

Security-validation logic is well tested!



Co-authored-by: Vladislav Supalov <vladislav.supalov@stackable.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rework Kafka TLS / Auth structs

4 participants