Skip to content

Allow users to supply ACL credentials for provided ZK ensembles to configure Solr to set ACLs on znodes#253

Merged
thelabdude merged 4 commits intoapache:mainfrom
thelabdude:zk-acl-provided
Apr 9, 2021
Merged

Allow users to supply ACL credentials for provided ZK ensembles to configure Solr to set ACLs on znodes#253
thelabdude merged 4 commits intoapache:mainfrom
thelabdude:zk-acl-provided

Conversation

@thelabdude
Copy link
Contributor

@thelabdude thelabdude commented Apr 8, 2021

This fixes #252 .

In addition to unit tests, I added the following config to my SolrCloud CRD YAML to enable basic auth and configure my provided ZK ensemble with ACLs:

  solrSecurity:
    authenticationType: Basic
   ...
  zookeeperRef:
    provided:
      acl:
        secret: dev-zk-admin-acl
        passwordKey: password
        usernameKey: username
      readOnlyAcl:
        secret: dev-zk-readonly-acl
        passwordKey: password
        usernameKey: username

Verified the /security.json was bootstrapped with the correct ACLs.

@thelabdude thelabdude requested a review from HoustonPutman April 9, 2021 13:48
HoustonPutman
HoustonPutman requested changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@HoustonPutman HoustonPutman left a comment

Choose a reason for hiding this comment

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

Awesome addition and especially with tests AND docs 😉

Just a few minor comments

api/v1beta1/solrcloud_types.go
allACL = ref.ProvidedZookeeper.AllACL
readOnlyACL = ref.ProvidedZookeeper.ReadOnlyACL
}
return
Copy link
Contributor

@HoustonPutman HoustonPutman Apr 9, 2021

Choose a reason for hiding this comment

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

very cool, did not know about this syntax

controllers/util/zk_util.go
}

if zkSpec.ZookeeperPod.Env != nil {
zkCluster.Spec.Pod.Env = zkSpec.ZookeeperPod.Env
Copy link
Contributor

@HoustonPutman HoustonPutman Apr 9, 2021

Choose a reason for hiding this comment

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

Think you need to add this field to CopyZookeeperClusterFields

Copy link
Contributor Author

@thelabdude thelabdude Apr 9, 2021

Choose a reason for hiding this comment

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

Good catch! Thanks ... always seem to forget about these. I added a basic test for this but only looking at the env ... maybe I should bite the bullet and test all of those fields?

Copy link
Contributor

@HoustonPutman HoustonPutman Apr 9, 2021

Choose a reason for hiding this comment

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

Nah, there's already another issue that relates to that. We can tackle it separately and handle all objects at the same time. (They all have the same issue)

HoustonPutman
HoustonPutman approved these changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@HoustonPutman HoustonPutman left a comment

Choose a reason for hiding this comment

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

I think this is good to go! Great work as always

@thelabdude thelabdude merged commit 1548258 into apache:main Apr 9, 2021
@HoustonPutman HoustonPutman added this to the v0.3.0 milestone Apr 14, 2021
jward-bw pushed a commit to BrandwatchLtd/solr-operator that referenced this pull request Apr 27, 2021
@thelabdude @jward-bw
Allow users to supply ACL credentials for provided ZK ensembles to co…
d929a78 
…nfigure Solr to set ACLs on znodes (apache#253)
@HoustonPutman HoustonPutman linked an issue Sep 24, 2021 that may be closed by this pull request
Add support for customizing ZookeeperCluster properties #148
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HoustonPutman HoustonPutman HoustonPutman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.3.0

Development

Successfully merging this pull request may close these issues.

Provided Zookeeper config should also allow setting SOLR_ZK_CREDS_AND_ACLS for Solr Add support for customizing ZookeeperCluster properties

2 participants

@thelabdude @HoustonPutman