Skip to content

added check if anonymous token policy exists#2790

Merged
aahel merged 9 commits intomainfrom
NET-5174/anonymous-token-policy
Aug 30, 2023
Merged

added check if anonymous token policy exists#2790
aahel merged 9 commits intomainfrom
NET-5174/anonymous-token-policy

Conversation

@aahel
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@aahel aahel requested a review from thisisnotashwin August 23, 2023 15:49
Copy link
Copy Markdown
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
Copy Markdown
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 This release branch is no longer active. labels Aug 29, 2023
@david-yu
Copy link
Copy Markdown
Contributor

Added backports, thank you.

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.1.x Backport to release/1.1.x branch backport/1.2.x This release branch is no longer active. 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