Skip to content

Add initial ACL support to consul-rs#58

Merged
kushudai merged 18 commits intoRoblox:mainfrom
x0rw:feature/acl-access-control-list
Jun 24, 2025
Merged

Add initial ACL support to consul-rs#58
kushudai merged 18 commits intoRoblox:mainfrom
x0rw:feature/acl-access-control-list

Conversation

@x0rw
Copy link
Contributor

@x0rw x0rw commented May 8, 2025

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
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

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

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
- 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
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 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.

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

Alright, I will review the comments.

Co-authored-by: Kushagra Udai <[email protected]>
@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.

@x0rw
Copy link
Contributor Author

x0rw commented May 29, 2025

Here is the output of cargo public-api --all-features diff main..HEAD -sss
the -sss flag omits auto traits, blanket impls and auto derived impls for cleaner output.

Removed items from the public API
=================================
(none)

Changed items in the public API
===============================
(none)

Added items to the public API
=============================

+pub mod rs_consul::acl
+pub mod rs_consul::acl_types

+pub struct rs_consul::acl_types::ACLPolicy
+pub rs_consul::acl_types::ACLPolicy::create_index: u32
+pub rs_consul::acl_types::ACLPolicy::datacenters: core::option::Option<alloc::vec::Vec<alloc::string::String>>
+pub rs_consul::acl_types::ACLPolicy::description: alloc::string::String
+pub rs_consul::acl_types::ACLPolicy::hash: alloc::string::String
+pub rs_consul::acl_types::ACLPolicy::id: alloc::string::String
+pub rs_consul::acl_types::ACLPolicy::modify_index: u32
+pub rs_consul::acl_types::ACLPolicy::name: alloc::string::String

+pub struct rs_consul::acl_types::ACLToken
+pub rs_consul::acl_types::ACLToken::accessor_id: alloc::string::String
+pub rs_consul::acl_types::ACLToken::create_index: u64
+pub rs_consul::acl_types::ACLToken::create_time: alloc::string::String
+pub rs_consul::acl_types::ACLToken::description: alloc::string::String
+pub rs_consul::acl_types::ACLToken::hash: alloc::string::String
+pub rs_consul::acl_types::ACLToken::local: bool
+pub rs_consul::acl_types::ACLToken::modify_index: i64
+pub rs_consul::acl_types::ACLToken::policies: alloc::vec::Vec<rs_consul::ACLTokenPolicyLink>
+pub rs_consul::acl_types::ACLToken::secret_id: alloc::string::String

+pub struct rs_consul::acl_types::ACLTokenPolicyLink
+pub rs_consul::acl_types::ACLTokenPolicyLink::id: core::option::Option<alloc::string::String>
+pub rs_consul::acl_types::ACLTokenPolicyLink::name: core::option::Option<alloc::string::String>

+pub struct rs_consul::acl_types::CreateACLPolicyRequest
+pub rs_consul::acl_types::CreateACLPolicyRequest::description: core::option::Option<alloc::string::String>
+pub rs_consul::acl_types::CreateACLPolicyRequest::name: alloc::string::String
+pub rs_consul::acl_types::CreateACLPolicyRequest::rules: core::option::Option<alloc::string::String>

+pub struct rs_consul::acl_types::CreateACLTokenPayload
+pub rs_consul::acl_types::CreateACLTokenPayload::accessor_id: core::option::Option<alloc::string::String>
+pub rs_consul::acl_types::CreateACLTokenPayload::create_time: core::option::Option<alloc::string::String>
+pub rs_consul::acl_types::CreateACLTokenPayload::description: core::option::Option<alloc::string::String>
+pub rs_consul::acl_types::CreateACLTokenPayload::expiration_time: core::option::Option<alloc::string::String>
+pub rs_consul::acl_types::CreateACLTokenPayload::hash: core::option::Option<alloc::string::String>
+pub rs_consul::acl_types::CreateACLTokenPayload::local: bool
+pub rs_consul::acl_types::CreateACLTokenPayload::policies: alloc::vec::Vec<rs_consul::ACLTokenPolicyLink>
+pub rs_consul::acl_types::CreateACLTokenPayload::secret_id: core::option::Option<alloc::string::String>

+pub rs_consul::ConsulError::TokenDeleteFailed

+pub rs_consul::Function::CreateACLPolicy
+pub rs_consul::Function::DeleteACLPolicy
+pub rs_consul::Function::DeleteACLToken
+pub rs_consul::Function::GetACLPolicies
+pub rs_consul::Function::GetAclTokens
+pub rs_consul::Function::ReadACLPolicies
+pub rs_consul::Function::ReadACLToken

+pub struct rs_consul::ACLPolicy
+pub rs_consul::ACLPolicy::create_index: u32
+pub rs_consul::ACLPolicy::datacenters: core::option::Option<alloc::vec::Vec<alloc::string::String>>
+pub rs_consul::ACLPolicy::description: alloc::string::String
+pub rs_consul::ACLPolicy::hash: alloc::string::String
+pub rs_consul::ACLPolicy::id: alloc::string::String
+pub rs_consul::ACLPolicy::modify_index: u32
+pub rs_consul::ACLPolicy::name: alloc::string::String

+pub struct rs_consul::ACLToken
+pub rs_consul::ACLToken::accessor_id: alloc::string::String
+pub rs_consul::ACLToken::create_index: u64
+pub rs_consul::ACLToken::create_time: alloc::string::String
+pub rs_consul::ACLToken::description: alloc::string::String
+pub rs_consul::ACLToken::hash: alloc::string::String
+pub rs_consul::ACLToken::local: bool
+pub rs_consul::ACLToken::modify_index: i64
+pub rs_consul::ACLToken::policies: alloc::vec::Vec<rs_consul::ACLTokenPolicyLink>
+pub rs_consul::ACLToken::secret_id: alloc::string::String

+pub struct rs_consul::ACLTokenPolicyLink
+pub rs_consul::ACLTokenPolicyLink::id: core::option::Option<alloc::string::String>
+pub rs_consul::ACLTokenPolicyLink::name: core::option::Option<alloc::string::String>

+impl rs_consul::Consul
+pub async fn rs_consul::Consul::create_acl_policy(&self, payload: &rs_consul::CreateACLPolicyRequest) -> core::result::Result<rs_consul::ACLPolicy, rs_consul::ConsulError>
+pub async fn rs_consul::Consul::create_acl_token(&self, payload: &rs_consul::CreateACLTokenPayload) -> core::result::Result<rs_consul::ACLToken, rs_consul::ConsulError>
+pub async fn rs_consul::Consul::delete_acl_policy(&self, id: alloc::string::String) -> core::result::Result<(), rs_consul::ConsulError>
+pub async fn rs_consul::Consul::delete_acl_token(&self, token: alloc::string::String) -> core::result::Result<(), rs_consul::ConsulError>
+pub async fn rs_consul::Consul::get_acl_policies(&self) -> core::result::Result<alloc::vec::Vec<rs_consul::ACLPolicy>, rs_consul::ConsulError>
+pub async fn rs_consul::Consul::get_acl_tokens(&self) -> core::result::Result<alloc::vec::Vec<rs_consul::ACLToken>, rs_consul::ConsulError>
+pub async fn rs_consul::Consul::read_acl_token(&self, accessor_id: alloc::string::String) -> core::result::Result<rs_consul::ACLToken, rs_consul::ConsulError>

+pub struct rs_consul::CreateACLPolicyRequest
+pub rs_consul::CreateACLPolicyRequest::description: core::option::Option<alloc::string::String>
+pub rs_consul::CreateACLPolicyRequest::name: alloc::string::String
+pub rs_consul::CreateACLPolicyRequest::rules: core::option::Option<alloc::string::String>

+pub struct rs_consul::CreateACLTokenPayload
+pub rs_consul::CreateACLTokenPayload::accessor_id: core::option::Option<alloc::string::String>
+pub rs_consul::CreateACLTokenPayload::create_time: core::option::Option<alloc::string::String>
+pub rs_consul::CreateACLTokenPayload::description: core::option::Option<alloc::string::String>
+pub rs_consul::CreateACLTokenPayload::expiration_time: core::option::Option<alloc::string::String>
+pub rs_consul::CreateACLTokenPayload::hash: core::option::Option<alloc::string::String>
+pub rs_consul::CreateACLTokenPayload::local: bool
+pub rs_consul::CreateACLTokenPayload::policies: alloc::vec::Vec<rs_consul::ACLTokenPolicyLink>
+pub rs_consul::CreateACLTokenPayload::secret_id: core::option::Option<alloc::string::String>

@kushudai
Copy link
Contributor

Hi @x0rw This mostly looks good to me. Lets get rid of the acl feature and expose it directly.
We generally don't put functionality behind features if they do not entail new dependencies.

@x0rw
Copy link
Contributor Author

x0rw commented Jun 18, 2025

Alright, sounds good, let me know if there is any issue with /examples dir.

Comment on lines +123 to +124
// TODO: Make the rules strongly typed
pub rules: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be a breaking change.
Are you okay with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we can avoid breaking changes if we want and have a forward and backward compatible api, There are two approaches i could think of,
The first one is to define a

struct HclRules{
      raw: String, 
      //rules: Vec<HclRule> // (this field can be added later without breaking existing code that interact with raw field.)
}
impl HclRules{
    fn from_string(s: String)-> Self{
       Self{raw: s}
    }
}

,and then our rules field becomes "pub rules: HclRules," for now, and the users can call HclRules::from_string(..) to fill his raw string rules. then later when a proper builder/parser is introduced we can seamlessly integrate it(by adding the new field), and keep the old raw option available too for flexebility and if HCL specs were changed, the user can fallback to the raw option without having to wait for this crate to change or be forced to update it.

The pther approach is to keep it as a Vec and later when we introduce the hcl builder/parse we could basically build it into a string at the end.

impl HclRules{
...
    fn build()-> String {...}
}

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm perfectly okay with breaking changes - but I don't have an immediate use case for ACL so I assumed that would affect you if/when we get around to fixing this TODO :)

I would actually prefer leaving it as is, as opposed to exposing a type without it's internals being visible since that diverges a bit from how we expose types from this crates today.

So if you're okay with dealing with breaking changes down the line, I think we can merge this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i'm okay with breaking changes.

@kushudai kushudai merged commit ec01078 into Roblox:main Jun 24, 2025
4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants