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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create anonymous-policy and token from non-default partitions. #966

Merged
merged 6 commits into from
Jan 24, 2022
Merged

Create anonymous-policy and token from non-default partitions. #966

merged 6 commits into from
Jan 24, 2022

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Jan 14, 2022 •
edited

Changes proposed in this PR:

  • Ensure the anonymous token in the default partition is updated to support cross-partition DNS resolution even if the server-acl-init is running in a non-default partition. This ensures DNS lookups work even if the default partition is on VMs and hence does not have the requisite policy required to do the same.
  • Update SDK to the latest version that update which updates ACL.Tokens.Master to ACL.Tokens.InitialManagement

How I've tested this PR:

  • Acceptance tests don't break
  • Tested on HCP with help from the cloud team.

How I expect reviewers to test this PR:

  • 馃憖

Checklist:

  • Tests added
  • CHANGELOG entry added

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

@thisisnotashwin thisisnotashwin marked this pull request as ready for review January 14, 2022 22:31
@thisisnotashwin thisisnotashwin requested review from ndhanushkodi, a team and ishustava and removed request for a team January 14, 2022 22:32
@thisisnotashwin thisisnotashwin marked this pull request as draft January 18, 2022 21:38
ndhanushkodi
ndhanushkodi approved these changes Jan 18, 2022
View reviewed changes
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Looks great Ashwin! To make sure I understand correctly-- from any new partition where server-acl-init is run, it will idempotently configure the anonymous policy in the default partition?

@ndhanushkodi
Copy link
Contributor

Oh I see it got updated to draft, just re-request me if you need a review again later

@thisisnotashwin
Copy link
Contributor Author

Looks great Ashwin! To make sure I understand correctly-- from any new partition where server-acl-init is run, it will idempotently configure the anonymous policy in the default partition?

Exactly!!

Oh I see it got updated to draft, just re-request me if you need a review again later

Yeah. Iryna and I had a conversation and I think I know how I want to test this. Will un-draft it once I have a unit test and re-request a review. Sorry bout that.

@thisisnotashwin thisisnotashwin marked this pull request as ready for review January 21, 2022 00:23
thisisnotashwin
thisisnotashwin commented Jan 21, 2022
View reviewed changes
control-plane/go.mod
@@ -129,4 +129,6 @@ require (
sigs.k8s.io/yaml v1.2.0 // indirect
)

replace github.com/hashicorp/consul/sdk v0.9.0 => github.com/hashicorp/consul/sdk v0.4.1-0.20220120214936-7568f3a102a8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to update the version of the consul/sdk without impacting the version of the consul/api.

thisisnotashwin
thisisnotashwin commented Jan 21, 2022
View reviewed changes
.circleci/config.yml Outdated
@@ -70,7 +70,7 @@ commands:
type: string
consul-k8s-image:
type: string
default: "docker.mirror.hashicorp.services/hashicorpdev/consul-k8s-control-plane:latest"
default: "ashwinvenkatesh/consul-k8s@sha256:9b46fae446fcf99c715d9063231857eef5525830f2cd3cdcb46266c84db11deb"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be deleted before merge.

ishustava
ishustava approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple of minor edits

CHANGELOG.md Outdated Show resolved Hide resolved
control-plane/subcommand/server-acl-init/command_ent_test.go Outdated Show resolved Hide resolved
ndhanushkodi
ndhanushkodi approved these changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Looks great!! The test is :chefs_kiss!!

thisisnotashwin and others added 3 commits January 24, 2022 13:11
@thisisnotashwin @ishustava
Update CHANGELOG.md
804c7bc 
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
@thisisnotashwin @ishustava
Update control-plane/subcommand/server-acl-init/command_ent_test.go
e679322 
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
@thisisnotashwin
revert consul-k8s image
03f2f17
@thisisnotashwin thisisnotashwin merged commit 37c409d into main Jan 24, 2022
@thisisnotashwin thisisnotashwin deleted the always-create-anon-token branch January 24, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndhanushkodi ndhanushkodi ndhanushkodi approved these changes

@ishustava ishustava ishustava approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@thisisnotashwin @ndhanushkodi @ishustava