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

Update ACLs, add namespace.write permission #2029

Merged
merged 7 commits into from
Mar 28, 2023

Conversation

sarahalsmiller
Copy link
Member

@sarahalsmiller sarahalsmiller commented Mar 24, 2023

Changes proposed in this PR:

How I've tested this PR:

How I expect reviewers to test this PR:

  • CI passes

Checklist:

  • [ X ] Tests added
  • [ X ] CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@sarahalsmiller sarahalsmiller deleted the bug/gateway-controller-incomplete-acl branch March 25, 2023 04:47
@sarahalsmiller sarahalsmiller restored the bug/gateway-controller-incomplete-acl branch March 25, 2023 14:27
@sarahalsmiller sarahalsmiller changed the title WIP- update ACLs, add operator.write permission Update ACLs, add operator.write permission Mar 25, 2023
@sarahalsmiller sarahalsmiller added the backport/1.1.x Backport to release/1.1.x branch label Mar 25, 2023
@sarahalsmiller sarahalsmiller marked this pull request as ready for review March 25, 2023 18:06
@@ -147,12 +147,15 @@ func (c *Command) apiGatewayControllerRules() (string, error) {
partition "{{ .PartitionName }}" {
mesh = "write"
acl = "write"
operator = "write"
Copy link
Contributor

@mikemorris mikemorris Mar 27, 2023

Choose a reason for hiding this comment

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

Is operator = "write" within the partition needed (this feels like more permission than we may want to grant our controller), or is

namespace_prefix "" {
  policy = "write"
}

sufficient?

https://developer.hashicorp.com/consul/commands/namespace/create does say operator:write is required, but that may be inaccurate, as https://developer.hashicorp.com/consul/docs/security/acl/acl-rules#namespace-rules seems to indicate the latter policy would be sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to Mike, it's worth investigating if we can avoid operator:write. And I saw this comment: https://github.com/hashicorp/consul-k8s/blob/main/control-plane/subcommand/server-acl-init/rules.go#L40-L41 higher up in the file as well, which indicates operator=write cannot be done within a partition anyways. Not sure if it'll cause issues when deployed (possibly not since you've tested this), but it sounds like if that's the case it should be possible to not include operator:write.

Copy link
Member Author

@sarahalsmiller sarahalsmiller Mar 27, 2023

Choose a reason for hiding this comment

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

After additional testing, I have removed the operator:write permission. I think in the initial tests it was simply ignoring the operator:write permission.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also worth nothing, I'm not sure what was wrong with my env on friday, but on this round of testing, the fix worked with both api gateway 1.5.1 and 1.5.2

Copy link
Contributor

@david-yu david-yu Mar 28, 2023

Choose a reason for hiding this comment

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

I assume you mean API Gateway 0.5.1 and 0.5.2?

@mikemorris mikemorris changed the title Update ACLs, add operator.write permission Update ACLs, add namespace.write permission Mar 28, 2023
@david-yu david-yu linked an issue Mar 28, 2023 that may be closed by this pull request
@david-yu
Copy link
Contributor

@sarahalsmiller Just an FYI, just linked #1911 to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.1.x Backport to release/1.1.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

server-acl-init generates incomplete API Gateway Controller policy
4 participants