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

[Merged by Bors] - Consolidate global cluster config #612

Closed
wants to merge 34 commits into from

Conversation

maltesander
Copy link
Member

@maltesander maltesander commented Dec 19, 2022

Description

The new structure now looks like:

apiVersion: zookeeper.stackable.tech/v1alpha1
kind: ZookeeperCluster
metadata:
  name: simple-zk
spec:
  image:
    productVersion: 3.8.0
    stackableVersion: 0.8.0
  clusterConfig:
    authentication:
      - authenticationClass: zookeeper-tls-authentication-class (with provider tls) # String
    tls:
      serverSecretClass: tls
      quorumSecretClass: tls
    logging:
      vectorAggregatorConfigMapName: vector-aggregator-discovery
  servers:
....

fixes #596

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

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/2023-01 release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. changelog/crd-change labels Dec 19, 2022
@maltesander maltesander self-assigned this Dec 19, 2022
@maltesander maltesander requested a review from a team December 19, 2022 14:22
@maltesander maltesander marked this pull request as ready for review December 19, 2022 14:44
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.

Just some comment, no complete review.

Any particular reason you did go with

    tls:
      server:
        secretClass: tls
      quorum:
        secretClass: tls

instead of

    tls:
      serverSecretClass: secret_class # Option<String>. *In general* defaults to "tls"
      internalSecretClass: secret_class # Option<String>. *In general* defaults to "tls"

rust/crd/src/lib.rs Outdated Show resolved Hide resolved
rust/crd/src/lib.rs Show resolved Hide resolved
@maltesander
Copy link
Member Author

maltesander commented Dec 19, 2022

Just some comment, no complete review.

Any particular reason you did go with

    tls:
      server:
        secretClass: tls
      quorum:
        secretClass: tls

instead of

    tls:
      serverSecretClass: secret_class # Option<String>. *In general* defaults to "tls"
      internalSecretClass: secret_class # Option<String>. *In general* defaults to "tls"

It basically was like this for server, i did the same for quorum. I thought about just using the String and like it better as well. Happy to change this.

Copy link
Member

@razvan razvan 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
Copy link
Member Author

Copy link
Member

@razvan razvan left a comment

Choose a reason for hiding this comment

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

lgtm

@fhennig
Copy link
Member

fhennig commented Dec 20, 2022

Can you test if the getting started guide still works?

@sbernauer
Copy link
Member

For the record: Spoke to Malte and we agreed that the authentication´section needs to be changed to a Vec<> to match the description of the PR. We should include all braking changes in this PR to not have another breaking change shortly afterwards

@maltesander
Copy link
Member Author

@maltesander
Copy link
Member Author

bors merge

bors bot pushed a commit that referenced this pull request Dec 21, 2022
# Description

The new structure now looks like:
```
apiVersion: zookeeper.stackable.tech/v1alpha1
kind: ZookeeperCluster
metadata:
  name: simple-zk
spec:
  image:
    productVersion: 3.8.0
    stackableVersion: 0.8.0
  clusterConfig:
    authentication:
      - authenticationClass: zookeeper-tls-authentication-class (with provider tls) # String
    tls:
      serverSecretClass: tls
      quorumSecretClass: tls
    logging:
      vectorAggregatorConfigMapName: vector-aggregator-discovery
  servers:
....
```

fixes #596

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



Co-authored-by: Malte Sander <malte.sander.it@gmail.com>
Co-authored-by: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Dec 21, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Consolidate global cluster config [Merged by Bors] - Consolidate global cluster config Dec 21, 2022
@bors bors bot closed this Dec 21, 2022
@bors bors bot deleted the consolidate-global-cluster-config branch December 21, 2022 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/2023-01 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: ZooKeeper (split encryption and authentication according to the decision)
4 participants