Always configure anonymous token in primary DC #106
Conversation
pglass
requested review from
a team,
sanon-dev and
cthain
and removed request for
a team and
sanon-dev
| Namespace: controller.DefaultPartition, | ||
| Partition: controller.DefaultNamespace, | ||
| } | ||
| } |
There was a problem hiding this comment.
Forcing the default partition here seems to match the k8s behavior:
| policyNames := []string{} | ||
| for _, policy := range policies { | ||
| policyNames = append(policyNames, policy.Name) | ||
| } |
There was a problem hiding this comment.
I changed this bit to collect the policy names. Then we do a contains check and the order of policies doesn't matter.
| partitionsEnabled: true, | ||
| expPolicy: expEntAnonTokenPolicy, | ||
| }, | ||
| } |
There was a problem hiding this comment.
This I moved to the command_ent_test.go so that it doesn't run for the OSS binary, since the controller now passes in an explicit partition=default query option.
| Config: Config{Datacenter: "dc1"}, | ||
| DebugConfig: Config{ | ||
| PrimaryDatacenter: "dc1", | ||
| MeshGatewayWANFederationEnabled: true, |
There was a problem hiding this comment.
The MeshGatewayWANFederationEnabled flag isn't needed for the test anymore.. maybe we should remove it, what do you think?
There was a problem hiding this comment.
Updated! I renamed some of the tests. See what you think!
There was a problem hiding this comment.
👍 for the renaming.
Sorry for any confusion, I wasn't very clear about my comment for the MeshGatewayWANFederationEnabled field. I was trying to say that we don't actually use this field in the code anymore. And since we don't need it, maybe we should remove it from both the ACL controller and the tests. As you mentioned before it's in the DebugConfig part of the agent/self response, so it's not "stable". So it may just be better to get rid of it.
There was a problem hiding this comment.
Oh, I meant to remove the field, too (forgot to git add again before committing)
| require.Len(t, policies, 2) | ||
| require.Equal(t, policies[1].Name, "global-management") | ||
| // Otherwise, we expect the global-management policy and anonymous-token-policy | ||
| // in the default partition, or if partitions are not enabled. |
There was a problem hiding this comment.
This comment reads a bit awkwardly. It seems like the ", or if partitions are not enabled" at the end of the comment is not needed.
There was a problem hiding this comment.
Yeah, I can fix the wording. Basically, we hit this in the OSS case (no partitions) or in ENT but in the default partition (partitions disabled in the controller).
Changes proposed in this PR:
This changes the acl-controller to always configure the anonymous token in the primary datacenter, and in the default partition.
How I've tested this PR:
Unit tests
How I expect reviewers to test this PR:
👀
Checklist: