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

Use global token for controller #369

Merged
merged 1 commit into from
Nov 5, 2020
Merged

Use global token for controller #369

merged 1 commit into from
Nov 5, 2020

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Oct 27, 2020

Controller token must be global because config entries are global and therefore all writes/reads
go to the primary datacenter. This means secondary datacenters need
a token that is known by the primary datacenters, i.e. a global token.

How I've tested this PR:

  • federated two dcs using consul k8s image ghcr.io/lkysow/consul-k8s-dev:oct29
  • created servicedefaults in secondary dc
    cat <<EOF | kubectl apply -f -
    apiVersion: consul.hashicorp.com/v1alpha1
    kind: ServiceDefaults
    metadata:
      name: api
    spec:
      protocol: http
    EOF
    servicedefaults.consul.hashicorp.com/api created
    
    k get servicedefaults
    NAME   SYNCED
    api    True
    

How I expect reviewers to test this PR:

  • code review

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@lkysow lkysow force-pushed the crd-global-token branch 2 times, most recently from 4525355 to 7b3e80c Compare October 29, 2020 22:05
@lkysow lkysow marked this pull request as ready for review October 29, 2020 22:18
@lkysow lkysow requested review from a team, kschoche and ishustava and removed request for a team October 29, 2020 22:18
PolicyDCs: nil,
SecretNames: []string{resourcePrefix + "-controller-acl-token"},
LocalToken: false,
},
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We were missing a bunch of tests for these tokens.

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!

CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
## UNRELEASED

BUG FIXES:
* CRDs: Ensure ACL token is global so that secondary DC's can manage custom resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* CRDs: Ensure ACL token is global so that secondary DC's can manage custom resources.
* CRDs: Ensure ACL token is global so that secondary DCs can manage custom resources.

Copy link
Contributor

@kschoche kschoche 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!

@lkysow
Copy link
Member Author

lkysow commented Nov 2, 2020

Waiting on #368 to be merged to avoid conflicts in that PR.

@thisisnotashwin
Copy link
Contributor

This can be merged now @lkysow

Controller token must be global because config entry writes all
go to the primary datacenter. This means secondary datacenters need
a token that is known by the primary datacenters.
@lkysow lkysow merged commit 380bb1e into master Nov 5, 2020
@lkysow lkysow deleted the crd-global-token branch November 5, 2020 18:29
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
* helm3 support for NOTES.txt

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
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.

4 participants