Add ACL support #766

thisisnotashwin · 2021-10-06T22:29:09Z

Changes proposed in this PR:

Update server-acl-init job to create tokens that are partition aware when Admin Partitions are enabled.
server-acl-init creates a partition-token that is used by partition-init and server-acl-init in non-default-partitions.
Update partition-init to use provided partition-token when ACLs are enabled.

How I've tested this PR:

deployed manifests with ACLs enabled on server and non-server cluster and validated things worked as expected.
sample server YAML

global:
  enableConsulNamespaces: true
  tls:
    enabled: true
  image: ashwinvenkatesh/consul@sha256:d799bdc9f6c42fd42438b22cb17cce1e3f49a188df2b55a85b8bc0906b78b081
  imageK8S: ashwinvenkatesh/consul-k8s@sha256:2c328df1337dddeebe85b886098e70ed8ab8e9ee44f027cd9502b5f14f7c28fa
  adminPartitions:
    enabled: true
  acls:
    manageSystemACLs: true
server:
  exposeGossipAndRPCPorts: true
  enterpriseLicense:
    secretName: license
    secretKey: key
connectInject:
  enabled: true
  transparentProxy:
    defaultEnabled: false
  consulNamespaces:
    mirroringK8S: true
controller:
  enabled: true

sample non-server YAML

global:
  enabled: false
  enableConsulNamespaces: true
  image: ashwinvenkatesh/consul@sha256:d799bdc9f6c42fd42438b22cb17cce1e3f49a188df2b55a85b8bc0906b78b081
  imageK8S: ashwinvenkatesh/consul-k8s@sha256:2c328df1337dddeebe85b886098e70ed8ab8e9ee44f027cd9502b5f14f7c28fa
  adminPartitions:
    enabled: true
    name: "red"
  tls:
    enabled: true
    caCert:
      secretName: consul-consul-ca-cert
      secretKey: tls.crt
    caKey:
      secretName: consul-consul-ca-key
      secretKey: tls.key
  acls:
    manageSystemACLs: true
    bootstrapToken:
      secretName: consul-consul-partitions-acl-token
      secretKey: token
server:
  enterpriseLicense:
    secretName: license
    secretKey: key
externalServers:
  enabled: true
  hosts: [ "35.192.119.38" ]
  tlsServerName: server.dc1.consul
  k8sAuthMethodHost: https://34.121.209.52
client:
  enabled: true
  exposeGossipPorts: true
  join: [ "35.192.119.38" ]
connectInject:
  enabled: true
  consulNamespaces:
    mirroringK8S: true
controller:
  enabled: true

k8s auth method host and join addresses will vary depending on your clusters. copy over the tls certs for ca-cert and ca-key and also the consul-partitions-acl-token

How I expect reviewers to test this PR:

code review
try creating partitions. reach out if you'd like to pair on it

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 · 2021-10-07T12:48:31Z

Looks like connect appears to not work right now. Moving this PR back to draft until I fix this.

thisisnotashwin · 2021-10-07T16:58:20Z

UPDATE: The connect issue has been resolved, but there is a bug in Consul where there isn't a partitioned policy that allows namespace creation. That is being worked on but this PR should have the changes required to work once those changes have been made to Consul.

control-plane/subcommand/server-acl-init/command.go

charts/consul/templates/partition-init-job.yaml

charts/consul/test/acceptance/tests/partitions/partitions_test.go

charts/consul/test/unit/partition-init-job.bats

control-plane/subcommand/server-acl-init/command_ent_test.go

control-plane/subcommand/server-acl-init/rules.go

- Update server-acl-init job to create tokens that are partition aware when Admin Partitions are enabled. - server-acl-init creates a partition-token that is used by partition-init and server-acl-init in non-default-partitions. - Update partition-init to use provided partition-token when ACLs are enabled. - Update license-policy to be acl:write when created in a partition.

partitions

lkysow · 2021-10-09T17:33:25Z

control-plane/subcommand/server-acl-init/command.go

@@ -169,6 +173,11 @@ func (c *Command) init() {
 	c.flags.BoolVar(&c.flagUseHTTPS, "use-https", false,
 		"Toggle for using HTTPS for all API calls to Consul.")

+	c.flags.BoolVar(&c.flagEnablePartitions, "enable-partitions", false,
+		"[Enterprise Only] Enables Admin Partitions [Enterprise only feature]")


probably don't need both Enterprise warnings, I think just the [Enterprise Only] will do

lkysow · 2021-10-09T17:34:22Z

control-plane/subcommand/server-acl-init/command.go

@@ -382,19 +396,33 @@ func (c *Command) Run(args []string) int {
 		}
 	}

+	if c.flagEnablePartitions && c.flagPartitionName == "default" && isPrimary {


const defaultPartitionName?

lkysow · 2021-10-09T17:35:33Z

control-plane/subcommand/server-acl-init/command.go

@@ -382,19 +396,33 @@ func (c *Command) Run(args []string) int {
 		}
 	}

+	if c.flagEnablePartitions && c.flagPartitionName == "default" && isPrimary {
+		// Partition token must be local because only the Primary datacenter can have Admin Partitions.


must be local or can be local? It would still work if it was global, no?

That makes sense. It can be local. It would still work, but given Admin Partitions are not supported in non-primary DCs, it could be confusing for it to be global. I'll reword this.

control-plane/subcommand/server-acl-init/command.go

control-plane/subcommand/server-acl-init/rules.go

lkysow · 2021-10-09T17:42:04Z

control-plane/subcommand/server-acl-init/rules.go

+  mesh = "write"
+}`
+
+func (c *Command) crossNamespaceRule() (string, error) {


why singular when the rest are plural?

* Use new HashiCorp Docker mirror

thisisnotashwin requested review from ishustava, a team and lkysow and removed request for a team October 6, 2021 22:29

thisisnotashwin force-pushed the partition-acls branch from 537b135 to cee07e4 Compare October 6, 2021 22:33

thisisnotashwin marked this pull request as draft October 7, 2021 12:48

thisisnotashwin force-pushed the partition-acls branch 3 times, most recently from a8008ac to 084fa55 Compare October 7, 2021 16:19

thisisnotashwin marked this pull request as ready for review October 7, 2021 19:02

thisisnotashwin force-pushed the partition-acls branch 5 times, most recently from 9f4e8bb to c91a5f7 Compare October 9, 2021 02:14

ishustava reviewed Oct 12, 2021

View reviewed changes

thisisnotashwin force-pushed the partition-acls branch 2 times, most recently from f94899a to dcb2d87 Compare October 15, 2021 16:05

Ashwin Venkatesh added 2 commits October 15, 2021 09:15

Add connect style acceptance tests to test namespace and connect within

f8b74c7

partitions

thisisnotashwin force-pushed the partition-acls branch 2 times, most recently from 8382bc9 to a1931c4 Compare October 15, 2021 16:20

lkysow approved these changes Oct 15, 2021

View reviewed changes

Update with Iryna's review

cce858e

thisisnotashwin force-pushed the partition-acls branch from a1931c4 to cce858e Compare October 15, 2021 18:30

ishustava approved these changes Oct 15, 2021

View reviewed changes

Add changelog and update consul image

de511ba

thisisnotashwin force-pushed the partition-acls branch from 91943c9 to de511ba Compare October 15, 2021 20:17

thisisnotashwin merged commit 14ce739 into main Oct 15, 2021

thisisnotashwin deleted the partition-acls branch October 15, 2021 20:18

ottaviohartman pushed a commit to ottaviohartman/consul-k8s that referenced this pull request Nov 3, 2021

Re-enabled openshift tests (hashicorp#766)

e38981f

* Use new HashiCorp Docker mirror

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ACL support #766

Add ACL support #766

thisisnotashwin commented Oct 6, 2021 •

edited

Loading

thisisnotashwin commented Oct 7, 2021

thisisnotashwin commented Oct 7, 2021

lkysow Oct 9, 2021

lkysow Oct 9, 2021

lkysow Oct 9, 2021

thisisnotashwin Oct 15, 2021

lkysow Oct 9, 2021

Add ACL support #766

Add ACL support #766

Conversation

thisisnotashwin commented Oct 6, 2021 • edited Loading

thisisnotashwin commented Oct 7, 2021

thisisnotashwin commented Oct 7, 2021

lkysow Oct 9, 2021

Choose a reason for hiding this comment

lkysow Oct 9, 2021

Choose a reason for hiding this comment

lkysow Oct 9, 2021

Choose a reason for hiding this comment

thisisnotashwin Oct 15, 2021

Choose a reason for hiding this comment

lkysow Oct 9, 2021

Choose a reason for hiding this comment

thisisnotashwin commented Oct 6, 2021 •

edited

Loading