Skip to content
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

Fix duplicate key in connect-inject ACL policy #4434

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

nathancoleman
Copy link
Member

@nathancoleman nathancoleman commented Nov 27, 2024

Changes proposed in this PR

Stop including a duplicate acl = "write" rule at the root level of the connect-inject ACL policy when namespaces are not enabled. The security patch in hashicorp/consul#21908 led to the connect-injector being unable to start up as its ACL policy is rejected by Consul.

How I've tested this PR

  1. Expanded the test matrix to cover all possible flag combinations w/ current policy output
  2. Fixed policy generation
  3. Updated the test assertions to account for the bug fix, verifying that acl = "write" never appears twice at the root level of the policy with any combination of flags

How I expect reviewers to test this PR

Review commit-by-commit observing the above

Checklist

@nathancoleman nathancoleman requested a review from a team as a code owner November 27, 2024 22:21
@nathancoleman nathancoleman added pr/no-changelog PR does not need a corresponding .changelog entry and removed pr/no-changelog PR does not need a corresponding .changelog entry labels Nov 27, 2024
Comment on lines 925 to 940
partition "part-1" {
mesh = "write"
acl = "write"
node_prefix "" {
policy = "write"
}
policy = "write"
acl = "write"
service_prefix "" {
policy = "write"
intentions = "write"
}
identity_prefix "" {
policy = "write"
intentions = "write"
}
Copy link
Member Author

Choose a reason for hiding this comment

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

You can see the bug here where acl = "write" winds up being included twice in the root of the policy whenever namespaces are disabled.

The later commit in this PR that adjusts the test assertions after the fix should indicate no scenario in which acl or any other type of rule is included twice at the same level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants