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

added check if anonymous token policy exists #2790

Merged
merged 9 commits into from Aug 30, 2023

Conversation

aahel
Copy link
Contributor

@aahel aahel commented Aug 18, 2023

Changes proposed in this PR:

Prevent updating anonymous token if it already exists and is attached to the anonymous token policy

How I've tested this PR:

  • setup the deafault partiton and create a non-default partition called my-partition.
  • modified the anonymous token policy rule.
  • created helm deployment in my-partiton using acl bootstrap token with following policies.
acl = "read"
operator = "read"
agent_prefix "" {
  policy = "read"
}
partition "my-partition" {
  acl = "write"
  mesh = "write"
  peering = "write"
  namespace_prefix "" {
    policy = "write"
  }
  service_prefix "" {
    policy = "write"
  }
}
  • server-acl-acl init was successfull and anonymous token policy rule didn't get updated

How I expect reviewers to test this PR:

Checklist:

@aahel aahel requested a review from Ganeshrockz August 18, 2023 05:20
@aahel aahel added the consul-india PRs/Issues assigned to Consul India team label Aug 18, 2023
@aahel aahel requested a review from Ganeshrockz August 18, 2023 05:54
Copy link
Contributor

@Ganeshrockz Ganeshrockz left a comment

Choose a reason for hiding this comment

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

It would be good to add test for the changes in configureAnonymousPolicy function

@aahel aahel requested a review from Ganeshrockz August 23, 2023 04:11
Copy link
Contributor

@Ganeshrockz Ganeshrockz left a comment

Choose a reason for hiding this comment

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

Looks good to me. Deferring the approval to someone with more domain context

Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This is looking good. i think we need to think about the unit-test a little more to ensure we are testing this behavior correctly. Otherwise, i like this.

Copy link
Contributor

@thisisnotashwin thisisnotashwin 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 good. I dont think i need to review this again but I think we should update the names of the policy in the test.

@aahel
Copy link
Contributor Author

aahel commented Aug 29, 2023

@david-yu which versions should we backport this to?

@david-yu david-yu added backport/1.0.x backport/1.1.x Backport to release/1.1.x branch backport/1.2.x 1.2.x release branch labels Aug 29, 2023
@david-yu
Copy link
Contributor

Added backports, thank you.

@aahel aahel merged commit 3056323 into main Aug 30, 2023
21 of 31 checks passed
@aahel aahel deleted the NET-5174/anonymous-token-policy branch August 30, 2023 05:24
aahel added a commit that referenced this pull request Aug 30, 2023
* added check if anonymous token policy exists

* changed checkIfAnonymousTokenPolicyExists impl

* made consts private

* added test for configureAnonymousPolicy

* fixed unit test

* fixed test and minor refactoring

* fix typo

* changed some var names

* added changelog
aahel added a commit that referenced this pull request Aug 30, 2023
* added check if anonymous token policy exists

* changed checkIfAnonymousTokenPolicyExists impl

* made consts private

* added test for configureAnonymousPolicy

* fixed unit test

* fixed test and minor refactoring

* fix typo

* changed some var names

* added changelog
aahel added a commit that referenced this pull request Aug 30, 2023
* added check if anonymous token policy exists

* changed checkIfAnonymousTokenPolicyExists impl

* made consts private

* added test for configureAnonymousPolicy

* fixed unit test

* fixed test and minor refactoring

* fix typo

* changed some var names

* added changelog
aahel added a commit that referenced this pull request Aug 30, 2023
…/1.2.x (#2864)

* backport of commit deecdb9

* added check if anonymous token policy exists (#2790)

* added check if anonymous token policy exists

* changed checkIfAnonymousTokenPolicyExists impl

* made consts private

* added test for configureAnonymousPolicy

* fixed unit test

* fixed test and minor refactoring

* fix typo

* changed some var names

* added changelog

---------

Co-authored-by: aahel <aahel.guha@hashicorp.com>
aahel added a commit that referenced this pull request Aug 30, 2023
…/1.1.x (#2863)

* backport of commit deecdb9

* added check if anonymous token policy exists (#2790)

* added check if anonymous token policy exists

* changed checkIfAnonymousTokenPolicyExists impl

* made consts private

* added test for configureAnonymousPolicy

* fixed unit test

* fixed test and minor refactoring

* fix typo

* changed some var names

* added changelog

---------

Co-authored-by: aahel <aahel.guha@hashicorp.com>
aahel added a commit that referenced this pull request Aug 30, 2023
added check if anonymous token policy exists (#2790)

* added check if anonymous token policy exists

* changed checkIfAnonymousTokenPolicyExists impl

* made consts private

* added test for configureAnonymousPolicy

* fixed unit test

* fixed test and minor refactoring

* fix typo

* changed some var names

* added changelog
@aahel aahel self-assigned this Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.0.x backport/1.1.x Backport to release/1.1.x branch backport/1.2.x 1.2.x release branch consul-india PRs/Issues assigned to Consul India team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve server-acl-init: Avoid forcibly creating the anonymous token policy when it already exists
4 participants