Skip to content

policyfile: add AttrConfig support to ACLDetails #29

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rajsinghtech
Copy link

Adds missing AttrConfig field to ACLDetails struct to expose attribute configuration functionality already present in the internal policy package.

tailscale/tailscale#16167

Copy link

review-ai-agent bot commented Jun 2, 2025

Pull Request Revisions

RevisionDescription
r8
Updated ACLAttrConfig documentation commentsEnhanced documentation for ACLAttrConfig struct, adding more descriptive comments about Type and BroadcastToPeers fields
r7
Updated ACL attribute configuration typesModified ACLAttrConfig struct with clearer type definitions, added comments, and expanded test/example configurations for custom device attributes
r6
Added ACL attribute configuration structIntroduced new ACLAttrConfig struct and added AttrConfig map to ACL struct for enhanced attribute configuration
r5
Removed ACLs external management fieldsDeleted ACLsExternallyManagedOn and ACLsExternalLink fields from TailnetSettings and UpdateTailnetSettingsRequest structs
r4No changes since last revision
r3
Added external ACL management fieldsIntroduced new ACL-related fields in TailnetSettings and UpdateTailnetSettingsRequest, including external management and link options
r2
Added ACL attribute configuration structIntroduced new ACLAttrConfig struct and added AttrConfig map to ACL struct for attribute configuration
r1
Added ACL attribute configuration structIntroduced new ACLAttrConfig struct with type, node set permission, and peer broadcast fields

☑️ AI review skipped for r8
Help React with emojis to give feedback on AI-generated reviews:
  • 👍 means the feedback was helpful and actionable
  • 👎 means the feedback was incorrect or unhelpful
💬 Replying to feedback with a comment helps us improve the system. Your input also contributes to shaping future interactions with the AI reviewer.

We'd love to hear from you—reach out anytime at [email protected].

@bradfitz
Copy link
Member

bradfitz commented Jun 3, 2025

No merge commits in the history. Can you rebase to exactly one commit?

@rajsinghtech rajsinghtech force-pushed the rajsinghtech/attrconfig branch 4 times, most recently from e215d2b to 258e6aa Compare June 3, 2025 06:07
@rajsinghtech
Copy link
Author

sorry, done @bradfitz !

Copy link
Member

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

There is some existing unit test coverage for parsing ACLs - please could we include the new fields in that coverage?

@rajsinghtech rajsinghtech force-pushed the rajsinghtech/attrconfig branch from 258e6aa to 9a05e12 Compare June 4, 2025 18:51
@rajsinghtech
Copy link
Author

Great Idea @tomhjp! Appreciate the notes, how does this look now?

Copy link
Member

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for addressing all the comments. Just left a couple of minor suggestions, but they might need validation as I'm not too familiar with this feature.

@tomhjp
Copy link
Member

tomhjp commented Jun 5, 2025

Oh and make sure your commit references the issue it's addressing before you merge - I only see it in the PR description atm

@rajsinghtech rajsinghtech force-pushed the rajsinghtech/attrconfig branch from 9a05e12 to cfe2215 Compare June 5, 2025 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants