Added support for digest auth and ACLs

[abarabash-sift]

• Added username and password optional fields into the provider configuration. Only digest auth is supported.
• Added support for ACLs in ZNodes, sequential ZNodes and in data source ZNodes
• Updated docs

[abarabash-sift]

Hi @detro can I get a review, please?

While waiting, I published the updated version to the registry: https://registry.terraform.io/providers/SiftScience/zookeeper/latest

[detro]

I don't know how, but I have never received a notification from GitHub about this PR: so sorry @abarabash-sift for having to wait on this.

I think I might have messed up my notification config, because I only got it now that you mentioned my handle.

I'll look into this asap.

[detro]

Please provide an implementation for reading the username/password from ENV variables as well.

I suggest:

• ZOOKEEPER_USERNAME
• ZOOKEEPER_PASSWORD

Being those credentials, it's quite likely that env vars are how a production system would provide those anyway.

[abarabash-sift]

I added reading ZOOKEEPER_USERNAME and ZOOKEEPER_PASSWORD environment variables to set up provider and client for testing (in the NewClientFromEnv() function)

[abarabash-sift]

@detro thank you for the review! Please take a look at the updates, no rush :)

By the way, when I'm typing your nickname here, the popup suggestion shows that you're busy. Maybe that's why you didn't get the notification. I attached the screenshot.

[detro]

Thank you @abarabash-sift. I think we are really close now, but it needs a few things:

1. I have prepared a new section for a new release: please add the appropriate infor in CHANGELOG.md. There is a document next to it explaining how to do that.
2. Needs to be rebased
3. The build is complaining about a change after generate is run

Thank you so much - very excited to release this.

[abarabash-sift]

@detro the PR is updated, could you run the workflow, please?

[abarabash-sift]

Also, I didn't find the document explaining the proper way of updating the CHANGELOG.md file, please let me know if my changes are okay

[detro]

Unfortunately it seems tests are failing.
Can you take a look?

[detro]

Also, I didn't find the document explaining the proper way of updating the CHANGELOG.md file, please let me know if my changes are okay

Sorry, I think I misspoke and confused for another repo.

Anyway, yours are new features and should be in a new features section.

[detro]

So, I tried to test this code (#51) and sorted out the connectivity errors visible in the workflow execution, to see if it was the cause of the issue.
ZooKeeper clients, even when being told to connect to 3 nodes but only 1 actually exist, was able to operate.

The issue has to be with the new functionality. It's not clear to me what is going wrong, but I haven't spend a lot of time digging back into the tests.

I appreciate the additional features that this brings, but until I can see a clean set of tests I won't be able to pick this up.

[abarabash-sift]

The problem is the connection pool exhaustion. You can see the corresponding logs in the Stop Containers section of the workflow:

2024-10-02 19:52:04,940 [myid:1] - WARN  [NIOServerCxnFactory.AcceptThread:/0.0.0.0:2181:RateLogger@37] - [6 times] Error accepting new connection: Too many connections from /172.18.0.1 - max is 60

Funny enough, the problem appeared after I added more tests to the suite. I also tested it on your main branch, by cloning the same provider tests a couple of times. I assume it happens because each test creates a new provider without closing its connection right after the test is done. Another interesting point is that tests work locally because they are run against the zookeeper ensemble (3 instances) versus a single node in the GitHub Actions.

Here is how to reproduce it locally on your main branch:

1. Run a single instance zookeeper cluster: docker run -p 2181:2181 --name zk zookeeper:3.5
2. Add more tests. I just cloned TestAccDataSourceZNode test 10 times by adding a new name.
3. Run tests against a single instance: ZOOKEEPER_SERVERS=localhost:2181 TF_ACC=1 make test
4. Tests will fail. See zk: could not connect to a server Go errors and Error accepting new connection: Too many connections from ... in Zookeeper logs.
5. Increase max connections in zookeeper: docker run -p 2181:2181 --name zk -e ZOO_MAX_CLIENT_CNXNS=200 zookeeper:3.5
6. Rerun tests: ZOOKEEPER_SERVERS=localhost:2181 TF_ACC=1 make test
7. Now they succeed.

To summarize, I added a workaround to allow Zookeeper to accept more connections and also zkClient.Close() statement to the confirmAllZNodeDestroyed function to help with the connection issue in tests. We still might need to implement a proper fix, but it's up to @detro to decide :)

[detro]

@abarabash-sift I just wanted to leave a note to say "I'm on it".
I have a bit of personal stuff ongoing, so I can't pay the proper attention to this, but I'll look at this soon.

Thanks for digging so much and finding the root cause of this: maybe you even identified an actual bug here, that is great!

[detro]

You did an amazing job nailing down what the problem actually is. Thank you so much @abarabash-sift.

I'm going to merge this and work on fixing the underlying problem insite the Provider: I'm sure it is possible to handle connection closure correctly in the Provider.