Skip to content

Add initial ACL support to consul-rs #58

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add initial ACL support to consul-rs #58

wants to merge 15 commits into from
+834 −43

Conversation

x0rw
Copy link
Contributor

@x0rw x0rw commented May 8, 2025 •
edited
Loading

Hey 👋.

This PR Adds

Initial ACL support with:

  • Core ACLToken/Policy types.
  • Initial Policy/Token management functions.
    (Future PRs will expand this with full token/policy/role management!)

Minor changes:

  • Moved get_session() from lib.rs to lock.rs.
  • Added examples.
  • Static initial_managment token for testing (get_acl_tokens.rs requires token with read permission).
  • get_privileged_client() a privillaged consul client for testing.

Use case:

  • Token and policy management in a client library allows end-to-end automation of ACL lifecycles within CI/CD. pipelines or orchestration tools.
  • Automated token rotation and revocation(great for security and compliance).
  • Generate scoped tokens for specific tasks.
  • ...

Checks

Please check these off before promoting the pull request to non-draft status.

  • All CI checks are green.
  • I have reviewed the proposed changes myself.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@x0rw x0rw marked this pull request as draft May 8, 2025 23:16
@x0rw
Copy link
Contributor Author

x0rw commented May 12, 2025

There is so much redundancy that can be solved by defining base structs with common fields then flattening(#[serde(flatten)]) them, and introducing builders too, and possibly do local verifications before sending the structs.

@x0rw
Copy link
Contributor Author

x0rw commented May 18, 2025

Hey @kushudai, Can you trigger the workflow once for this?

@kushudai
Copy link
Contributor

kushudai commented May 18, 2025 •
edited
Loading

Hey @kushudai, Can you trigger the workflow once for this?

Hello, I cannot unfortunately, you will need to resolve conflicts first 😅

EDIT: I went ahead and did that, hope you don't mind!

@x0rw
Copy link
Contributor Author

x0rw commented May 18, 2025 •
edited
Loading

EDIT: I went ahead and did that, hope you don't mind!

Thanks, i saw what caused the ci failure.

x0rw added 12 commits May 18, 2025 22:02
@x0rw
Implement ACL token types and listing functionality
afaf492 
- Add ACLToken, ACLTokenPolicyLink and CreateACLTokenRequest types with proper serde derives
- Implement list_acl_tokens() function with Consul API integration
- Create example usage in examples/get_acl_tokens.rs
@x0rw
Add lock_key.rs example
dd86d9b
@x0rw
Add predefined initial_managment token to config.hcl for testing
2afc662
@x0rw
Add list_acl_policies(), create_acl_policy(), and examples
00494ae
@x0rw
Add create_acl_token() read_acl_token(), 2 examples
736027b
@x0rw
Add comments, test
94beb14
@x0rw
Add more tests, organize acl tests under the same module
d0ee676
@x0rw
Add acl smoke test, fix optional types
55391fe
@x0rw
feature-gate ACL functionality and related tests
033f74c
@x0rw
Separate ACL types, update workflow
b9683da 
Updated workflows to build and test with feature flags.
Updated the CI/CD workflow consul config file.
Moved ACL-related types into a new acl_types.rs to make them easier to feature-gate.
@x0rw
Fix CI issues
cd8cf4c
@x0rw
Fix test tokens collision
cfe8e51
@x0rw x0rw force-pushed the feature/acl-access-control-list branch from 9d039ae to cfe8e51 Compare May 18, 2025 21:29
@x0rw x0rw marked this pull request as ready for review May 22, 2025 20:26
@x0rw
Copy link
Contributor Author

x0rw commented May 22, 2025

Hey @kushudai, I think this is ready for review now, let me know if you have any suggestions.

kushudai
kushudai reviewed May 28, 2025
View reviewed changes
Copy link
Contributor

@kushudai kushudai left a comment

Choose a reason for hiding this comment

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

Hi @x0rw Apologies, I am on vacation for the next few weeks so I'm unable to find time to thoroughly review this but I took a cursory look.

I don't quite see the point of the acl feature. Any reason we'd want to gate this behind a feature?

Also, an output from cargo public api for such a large change would be appreciated!
At some point, I should just have github actions leave that output as a comment on all PRs

@x0rw @kushudai
Apply suggestions from code review
ce76e51 
Alright, I will review the comments.

Co-authored-by: Kushagra Udai <112903599+kushudai@users.noreply.github.com>
@x0rw
Copy link
Contributor Author

x0rw commented May 29, 2025

Hi @kushudai, Thank you for taking the time to look at this.

The reason I added the acl behind a feature flag is to keep it opt-in for users who don't need ACL management, but if you think ACL is core enough to be enabled by default or not gated at all, I'm happy to remove the flag and integrate it directly.
Also, I may have overdone it in the examples/ directory. I’m considering consolidating them into two or three smoke test–style examples.

Enjoy the rest of your vacation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kushudai kushudai kushudai left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@x0rw @kushudai