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

Backport of [CC-5719] Add support for builtin global-read-only policy into release/1.14.x #18343

Merged

Conversation

hc-github-team-consul-core
Copy link
Contributor

Backport

This PR is auto-generated from #18319 to be assessed for backporting due to the inclusion of the label backport/1.14.

🚨

Warning automatic cherry-pick of commits failed. If the first commit failed,
you will see a blank no-op commit below. If at least one commit succeeded, you
will see the cherry-picked commits up to, not including, the commit where
the merge conflict occurred.

The person who merged in the original PR is:
@jjacobson93
This person should manually cherry-pick the original PR into a new backport PR,
and close this one when the manual backport PR is merged in.

merge conflict error: POST https://api.github.com/repos/hashicorp/consul/merges: 409 Merge conflict []

The below text is copied from the body of the original PR.


Description

This adds a new builtin policy that provides global read-only access, in contrast to the global read-write access that the builtin global-management policy provides. Other changes were made to process builtin policies more generically, since there are several places where checks or validations are done before processing or altering a policy.

Links

Ticket
RFC

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

Overview of commits

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 1, 2023

CLA assistant check
All committers have signed the CLA.

@hc-github-team-consul-core hc-github-team-consul-core force-pushed the backport/jer/read-only-policy/evenly-happy-doe branch 2 times, most recently from 954839f to e71f8a5 Compare August 1, 2023 17:12
Copy link

Choose a reason for hiding this comment

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

Auto approved Consul Bot automated PR

@vercel vercel bot temporarily deployed to Preview – consul-ui-staging August 1, 2023 17:16 Inactive
@vercel vercel bot temporarily deployed to Preview – consul August 1, 2023 17:22 Inactive
@lornasong lornasong force-pushed the backport/jer/read-only-policy/evenly-happy-doe branch from ef6783f to a4c1acb Compare August 1, 2023 21:56
@lornasong
Copy link
Member

lornasong commented Aug 1, 2023

Resolved this by:

git remote update
git checkout -B backport/jer/read-only-policy/evenly-happy-doe origin/release/1.14.x
git cherry-pick 6424ef6a56435191aae5da1392ae2f5a89bc5591
git push -f origin backport/jer/read-only-policy/evenly-happy-doe

Commit cherry-picked: 6424ef6

Files that required resolving:
Screen Shot 2023-08-01 at 5 28 28 PM

The conflicts seemed to be centered around that in 1.14:

  • ACLTokenAnonymousID was in a different file
  • ACLPolicy{} used to set Syntax to acl.SyntaxCurrent
  • Some different variable names aclvs. aclEp in agent/consul/acl_endpoint_test.go

Name: ACLPolicyGlobalManagementName,
Description: ACLPolicyGlobalManagementDesc,
Rules: ACLPolicyGlobalManagementRules,
Syntax: acl.SyntaxCurrent,
Copy link
Member

Choose a reason for hiding this comment

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

A notable change is that I added Syntax: acl.SyntaxCurrent here. It is not in the original PR

Name: "global-management",
Description: "Builtin Policy that grants unlimited access",
Rules: structs.ACLPolicyGlobalManagement,
Syntax: acl.SyntaxCurrent,
Copy link
Member

Choose a reason for hiding this comment

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

This Syntax is extra in 1.14. I didn't add it to writeBuiltinACLPolicy() because it's set here instead

* [CC-5719] Add support for builtin global-read-only policy

* Add changelog

* Add read-only to docs

* Fix some minor issues.

* Change from ReplaceAll to Sprintf

* Change IsValidPolicy name to return an error instead of bool

* Fix PolicyList test

* Fix other tests

* Apply suggestions from code review

Co-authored-by: Paul Glass <pglass@hashicorp.com>

* Fix state store test for policy list.

* Fix naming issues

* Update acl/validation.go

Co-authored-by: Chris Thain <32781396+cthain@users.noreply.github.com>

* Update agent/consul/acl_endpoint.go

---------

Co-authored-by: Paul Glass <pglass@hashicorp.com>
Co-authored-by: Chris Thain <32781396+cthain@users.noreply.github.com>
@lornasong lornasong force-pushed the backport/jer/read-only-policy/evenly-happy-doe branch from a4c1acb to fe84ccf Compare August 1, 2023 22:06
@lornasong lornasong marked this pull request as ready for review August 1, 2023 22:20
@lornasong lornasong requested a review from a team as a code owner August 1, 2023 22:20
@lornasong lornasong merged commit 6adfc1d into release/1.14.x Aug 1, 2023
84 checks passed
@lornasong lornasong deleted the backport/jer/read-only-policy/evenly-happy-doe branch August 1, 2023 22:38
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.

5 participants