From ee937cd0d21c430ab2de6a8847c6f7c369ef9f49 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Sun, 6 Mar 2022 14:58:00 -0500 Subject: [PATCH 1/5] Refactor Sync Catalog to use authmethods instead of Kubernetes secrets. --- .../consul/templates/server-acl-init-job.yaml | 2 +- .../templates/sync-catalog-clusterrole.yaml | 9 - .../templates/sync-catalog-deployment.yaml | 87 +++++-- .../consul/test/unit/server-acl-init-job.bats | 4 +- .../test/unit/sync-catalog-clusterrole.bats | 14 -- .../test/unit/sync-catalog-deployment.bats | 218 +++++++++++++++++- .../subcommand/server-acl-init/command.go | 26 ++- .../server-acl-init/command_ent_test.go | 27 ++- .../server-acl-init/command_test.go | 56 ++--- .../server-acl-init/create_or_update.go | 2 +- .../server-acl-init/create_or_update_test.go | 8 +- 11 files changed, 354 insertions(+), 99 deletions(-) diff --git a/charts/consul/templates/server-acl-init-job.yaml b/charts/consul/templates/server-acl-init-job.yaml index 97916c495c..48d38e6ac6 100644 --- a/charts/consul/templates/server-acl-init-job.yaml +++ b/charts/consul/templates/server-acl-init-job.yaml @@ -169,7 +169,7 @@ spec: {{- end }} {{- if .Values.syncCatalog.enabled }} - -create-sync-token=true \ + -create-sync-policy=true \ {{- if .Values.syncCatalog.consulNodeName }} -sync-consul-node-name={{ .Values.syncCatalog.consulNodeName }} \ {{- end }} diff --git a/charts/consul/templates/sync-catalog-clusterrole.yaml b/charts/consul/templates/sync-catalog-clusterrole.yaml index 5ceeb03d47..0b0837c0df 100644 --- a/charts/consul/templates/sync-catalog-clusterrole.yaml +++ b/charts/consul/templates/sync-catalog-clusterrole.yaml @@ -30,15 +30,6 @@ rules: - nodes verbs: - get -{{- if .Values.global.acls.manageSystemACLs }} - - apiGroups: [""] - resources: - - secrets - resourceNames: - - {{ template "consul.fullname" . }}-catalog-sync-acl-token - verbs: - - get -{{- end }} {{- if .Values.global.enablePodSecurityPolicies }} - apiGroups: ["policy"] resources: ["podsecuritypolicies"] diff --git a/charts/consul/templates/sync-catalog-deployment.yaml b/charts/consul/templates/sync-catalog-deployment.yaml index 2aedc54460..4fd94baf3f 100644 --- a/charts/consul/templates/sync-catalog-deployment.yaml +++ b/charts/consul/templates/sync-catalog-deployment.yaml @@ -49,8 +49,11 @@ spec: {{- end }} spec: serviceAccountName: {{ template "consul.fullname" . }}-sync-catalog - {{- if .Values.global.tls.enabled }} volumes: + - name: consul-data + emptyDir: + medium: "Memory" + {{- if .Values.global.tls.enabled }} {{- if not (and .Values.externalServers.enabled .Values.externalServers.useSystemRoots) }} - name: consul-ca-cert secret: @@ -70,9 +73,13 @@ spec: {{- end }} {{- end }} containers: - - name: consul-sync-catalog + - name: sync-catalog image: "{{ default .Values.global.imageK8S .Values.syncCatalog.image }}" env: + {{- if .Values.global.acls.manageSystemACLs }} + - name: CONSUL_HTTP_TOKEN_FILE + value: "/consul/login/acl-token" + {{- end }} - name: HOST_IP valueFrom: fieldRef: @@ -88,13 +95,6 @@ spec: name: {{ .Values.syncCatalog.aclSyncToken.secretName }} key: {{ .Values.syncCatalog.aclSyncToken.secretKey }} {{- end }} - {{- if .Values.global.acls.manageSystemACLs }} - - name: CONSUL_HTTP_TOKEN - valueFrom: - secretKeyRef: - name: "{{ template "consul.fullname" . }}-catalog-sync-acl-token" - key: "token" - {{- end}} {{- if .Values.global.tls.enabled }} {{- if .Values.client.enabled }} - name: CONSUL_HTTP_ADDR @@ -114,16 +114,19 @@ spec: value: http://{{ template "consul.fullname" . }}-server:8500 {{- end }} {{- end }} - {{- if .Values.global.tls.enabled }} volumeMounts: - {{- if (and .Values.global.tls.enableAutoEncrypt $clientEnabled) }} + - mountPath: /consul/login + name: consul-data + readOnly: true + {{- if .Values.global.tls.enabled }} + {{- if and .Values.global.tls.enableAutoEncrypt $clientEnabled }} - name: consul-auto-encrypt-ca-cert {{- else }} - name: consul-ca-cert + {{- end }} {{- end }} mountPath: /consul/tls/ca readOnly: true - {{- end }} command: - "/bin/sh" - "-ec" @@ -188,6 +191,16 @@ spec: -consul-cross-namespace-acl-policy=cross-namespace-policy \ {{- end }} {{- end }} + {{- if .Values.global.acls.manageSystemACLs }} + lifecycle: + preStop: + exec: + command: + - "/bin/sh" + - "-ec" + - | + consul-k8s-control-plane consul-logout + {{- end }} livenessProbe: httpGet: path: /health/ready @@ -214,16 +227,57 @@ spec: {{- end }} {{- if or .Values.global.acls.manageSystemACLs (and .Values.global.tls.enabled .Values.global.tls.enableAutoEncrypt $clientEnabled) }} initContainers: + {{- if (and .Values.global.tls.enabled .Values.global.tls.enableAutoEncrypt $clientEnabled) }} + {{- include "consul.getAutoEncryptClientCA" . | nindent 6 }} + {{- end }} {{- if .Values.global.acls.manageSystemACLs }} - - name: sync-acl-init + - name: sync-catalog-acl-init + env: + - name: HOST_IP + valueFrom: + fieldRef: + fieldPath: status.hostIP + {{- if .Values.global.tls.enabled }} + - name: CONSUL_CACERT + value: /consul/tls/ca/tls.crt + {{- end }} + - name: CONSUL_HTTP_ADDR + {{- if .Values.global.tls.enabled }} + value: https://$(HOST_IP):8501 + {{- else }} + value: http://$(HOST_IP):8500 + {{- end }} image: {{ .Values.global.imageK8S }} + volumeMounts: + - mountPath: /consul/login + name: consul-data + readOnly: false + {{- if .Values.global.tls.enabled }} + {{- if .Values.global.tls.enableAutoEncrypt }} + - name: consul-auto-encrypt-ca-cert + {{- else }} + - name: consul-ca-cert + {{- end }} + mountPath: /consul/tls/ca + readOnly: true + {{- end }} command: - "/bin/sh" - "-ec" - | consul-k8s-control-plane acl-init \ - -secret-name="{{ template "consul.fullname" . }}-catalog-sync-acl-token" \ - -k8s-namespace={{ .Release.Namespace }} + -component-name=sync-catalog \ + {{- if and .Values.global.federation.enabled .Values.global.federation.primaryDatacenter .Values.global.enableConsulNamespaces }} + -acl-auth-method={{ template "consul.fullname" . }}-k8s-component-auth-method-{{ .Values.global.datacenter }} \ + -primary-datacenter={{ .Values.global.federation.primaryDatacenter }} \ + {{- else }} + -acl-auth-method={{ template "consul.fullname" . }}-k8s-component-auth-method \ + {{- end }} + {{- if .Values.global.adminPartitions.enabled }} + -partition={{ .Values.global.adminPartitions.name }} \ + {{- end }} + -log-level={{ default .Values.global.logLevel .Values.controller.logLevel }} \ + -log-json={{ .Values.global.logJSON }} resources: requests: memory: "25Mi" @@ -232,9 +286,6 @@ spec: memory: "25Mi" cpu: "50m" {{- end }} - {{- if (and .Values.global.tls.enabled .Values.global.tls.enableAutoEncrypt $clientEnabled) }} - {{- include "consul.getAutoEncryptClientCA" . | nindent 6 }} - {{- end }} {{- end }} {{- if .Values.syncCatalog.priorityClassName }} priorityClassName: {{ .Values.syncCatalog.priorityClassName | quote }} diff --git a/charts/consul/test/unit/server-acl-init-job.bats b/charts/consul/test/unit/server-acl-init-job.bats index f982c47c28..93b4e8429e 100644 --- a/charts/consul/test/unit/server-acl-init-job.bats +++ b/charts/consul/test/unit/server-acl-init-job.bats @@ -249,7 +249,7 @@ load _helpers -s templates/server-acl-init-job.yaml \ --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | - yq '.spec.template.spec.containers[0].command | any(contains("-create-sync-token"))' | tee /dev/stderr) + yq '.spec.template.spec.containers[0].command | any(contains("-create-sync-policy"))' | tee /dev/stderr) [ "${actual}" = "false" ] } @@ -260,7 +260,7 @@ load _helpers --set 'global.acls.manageSystemACLs=true' \ --set 'syncCatalog.enabled=true' \ . | tee /dev/stderr | - yq '.spec.template.spec.containers[0].command | any(contains("-create-sync-token"))' | tee /dev/stderr) + yq '.spec.template.spec.containers[0].command | any(contains("-create-sync-policy"))' | tee /dev/stderr) [ "${actual}" = "true" ] } diff --git a/charts/consul/test/unit/sync-catalog-clusterrole.bats b/charts/consul/test/unit/sync-catalog-clusterrole.bats index 0688db9b93..17141e434f 100755 --- a/charts/consul/test/unit/sync-catalog-clusterrole.bats +++ b/charts/consul/test/unit/sync-catalog-clusterrole.bats @@ -60,20 +60,6 @@ load _helpers [ "${actual}" = "podsecuritypolicies" ] } -#-------------------------------------------------------------------- -# global.acls.manageSystemACLs - -@test "syncCatalog/ClusterRole: allows secret access with global.acls.manageSystemACLs=true" { - cd `chart_dir` - local actual=$(helm template \ - -s templates/sync-catalog-clusterrole.yaml \ - --set 'syncCatalog.enabled=true' \ - --set 'global.acls.manageSystemACLs=true' \ - . | tee /dev/stderr | - yq -r '.rules[2].resources[0]' | tee /dev/stderr) - [ "${actual}" = "secrets" ] -} - #-------------------------------------------------------------------- # syncCatalog.toK8S={true,false} diff --git a/charts/consul/test/unit/sync-catalog-deployment.bats b/charts/consul/test/unit/sync-catalog-deployment.bats index 8beead1564..18171942af 100755 --- a/charts/consul/test/unit/sync-catalog-deployment.bats +++ b/charts/consul/test/unit/sync-catalog-deployment.bats @@ -421,33 +421,243 @@ load _helpers #-------------------------------------------------------------------- # global.acls.manageSystemACLs -@test "syncCatalog/Deployment: CONSUL_HTTP_TOKEN env variable created when global.acls.manageSystemACLs=true" { +@test "syncCatalog/Deployment: consul-logout preStop hook is added when ACLs are enabled" { cd `chart_dir` local actual=$(helm template \ -s templates/sync-catalog-deployment.yaml \ --set 'syncCatalog.enabled=true' \ --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | - yq '[.spec.template.spec.containers[0].env[].name] | any(contains("CONSUL_HTTP_TOKEN"))' | tee /dev/stderr) + yq '[.spec.template.spec.containers[0].lifecycle.preStop.exec.command[2]] | any(contains("consul-k8s-control-plane consul-logout"))' | tee /dev/stderr) + + [ "${actual}" = "true" ] +} + +@test "syncCatalog/Deployment: CONSUL_HTTP_TOKEN_FILE is not set when acls are disabled" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/sync-catalog-deployment.yaml \ + --set 'syncCatalog.enabled=true' \ + . | tee /dev/stderr | + yq '[.spec.template.spec.containers[0].env[0].name] | any(contains("CONSUL_HTTP_TOKEN_FILE"))' | tee /dev/stderr) + [ "${actual}" = "false" ] +} + +@test "syncCatalog/Deployment: CONSUL_HTTP_TOKEN_FILE is set when acls are enabled" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/sync-catalog-deployment.yaml \ + --set 'syncCatalog.enabled=true' \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq '[.spec.template.spec.containers[0].env[0].name] | any(contains("CONSUL_HTTP_TOKEN_FILE"))' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "syncCatalog/Deployment: init container is created when global.acls.manageSystemACLs=true and has correct command and environment with tls disabled" { + cd `chart_dir` + local object=$(helm template \ + -s templates/sync-catalog-deployment.yaml \ + --set 'syncCatalog.enabled=true' \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq '.spec.template.spec.initContainers[0]' | tee /dev/stderr) + + local actual=$(echo $object | + yq -r '.name' | tee /dev/stderr) + [ "${actual}" = "sync-catalog-acl-init" ] + + local actual=$(echo $object | + yq -r '.command | any(contains("consul-k8s-control-plane acl-init"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[1].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[1].value] | any(contains("http://$(HOST_IP):8500"))' | tee /dev/stderr) + echo $actual + [ "${actual}" = "true" ] +} + +@test "syncCatalog/Deployment: init container is created when global.acls.manageSystemACLs=true and has correct command and environment with tls enabled" { + cd `chart_dir` + local object=$(helm template \ + -s templates/sync-catalog-deployment.yaml \ + --set 'syncCatalog.enabled=true' \ + --set 'global.tls.enabled=true' \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq '.spec.template.spec.initContainers[] | select(.name == "sync-catalog-acl-init")' | tee /dev/stderr) + + local actual=$(echo $object | + yq -r '.command | any(contains("consul-k8s-control-plane acl-init"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[1].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[2].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[2].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr) + echo $actual + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '.volumeMounts[1] | any(contains("consul-ca-cert"))' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "syncCatalog/Deployment: init container is created when global.acls.manageSystemACLs=true and has correct command with Partitions enabled" { + cd `chart_dir` + local object=$(helm template \ + -s templates/sync-catalog-deployment.yaml \ + --set 'syncCatalog.enabled=true' \ + --set 'global.tls.enabled=true' \ + --set 'global.enableConsulNamespaces=true' \ + --set 'global.adminPartitions.enabled=true' \ + --set 'global.adminPartitions.name=default' \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq '.spec.template.spec.initContainers[] | select(.name == "sync-catalog-acl-init")' | tee /dev/stderr) + + local actual=$(echo $object | + yq -r '.command | any(contains("consul-k8s-control-plane acl-init"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq -r '.command | any(contains("-acl-auth-method=RELEASE-NAME-consul-k8s-component-auth-method"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq -r '.command | any(contains("-partition=default"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[1].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[2].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[2].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr) + echo $actual + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '.volumeMounts[1] | any(contains("consul-ca-cert"))' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "syncCatalog/Deployment: init container is created when global.acls.manageSystemACLs=true and has correct command and environment with tls enabled and autoencrypt enabled" { + cd `chart_dir` + local object=$(helm template \ + -s templates/sync-catalog-deployment.yaml \ + --set 'syncCatalog.enabled=true' \ + --set 'global.tls.enabled=true' \ + --set 'global.tls.enableAutoEncrypt=true' \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq '.spec.template.spec.initContainers[] | select(.name == "sync-catalog-acl-init")' | tee /dev/stderr) + + local actual=$(echo $object | + yq -r '.command | any(contains("consul-k8s-control-plane acl-init"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[1].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[2].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '[.env[2].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr) + echo $actual + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq '.volumeMounts[1] | any(contains("consul-auto-encrypt-ca-cert"))' | tee /dev/stderr) [ "${actual}" = "true" ] } -@test "syncCatalog/Deployment: init container is created when global.acls.manageSystemACLs=true" { +@test "syncCatalog/Deployment: auto-encrypt init container is created and is the first init-container when global.acls.manageSystemACLs=true and has correct command and environment with tls enabled and autoencrypt enabled" { cd `chart_dir` local object=$(helm template \ -s templates/sync-catalog-deployment.yaml \ --set 'syncCatalog.enabled=true' \ + --set 'global.tls.enabled=true' \ + --set 'global.tls.enableAutoEncrypt=true' \ --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | yq '.spec.template.spec.initContainers[0]' | tee /dev/stderr) local actual=$(echo $object | yq -r '.name' | tee /dev/stderr) - [ "${actual}" = "sync-acl-init" ] + [ "${actual}" = "get-auto-encrypt-client-ca" ] +} + +@test "syncCatalog/Deployment: init container is created when global.acls.manageSystemACLs=true and has correct command when in non-primary datacenter with Consul Namespaces disabled" { + cd `chart_dir` + local object=$(helm template \ + -s templates/sync-catalog-deployment.yaml \ + --set 'syncCatalog.enabled=true' \ + --set 'global.datacenter=dc2' \ + --set 'global.federation.enabled=true' \ + --set 'global.federation.primaryDatacenter=dc1' \ + --set 'meshGateway.enabled=true' \ + --set 'connectInject.enabled=true' \ + --set 'global.tls.enabled=true' \ + --set 'global.tls.enableAutoEncrypt=true' \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq '.spec.template.spec.initContainers[] | select(.name == "sync-catalog-acl-init")' | tee /dev/stderr) + + local actual=$(echo $object | + yq -r '.command | any(contains("consul-k8s-control-plane acl-init"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq -r '.command | any(contains("-acl-auth-method=RELEASE-NAME-consul-k8s-component-auth-method"))' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "syncCatalog/Deployment: init container is created when global.acls.manageSystemACLs=true and has correct command when in non-primary datacenter with Consul Namespaces enabled" { + cd `chart_dir` + local object=$(helm template \ + -s templates/sync-catalog-deployment.yaml \ + --set 'syncCatalog.enabled=true' \ + --set 'global.datacenter=dc2' \ + --set 'global.enableConsulNamespaces=true' \ + --set 'global.federation.enabled=true' \ + --set 'global.federation.primaryDatacenter=dc1' \ + --set 'meshGateway.enabled=true' \ + --set 'connectInject.enabled=true' \ + --set 'global.tls.enabled=true' \ + --set 'global.tls.enableAutoEncrypt=true' \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq '.spec.template.spec.initContainers[] | select(.name == "sync-catalog-acl-init")' | tee /dev/stderr) local actual=$(echo $object | yq -r '.command | any(contains("consul-k8s-control-plane acl-init"))' | tee /dev/stderr) [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq -r '.command | any(contains("-acl-auth-method=RELEASE-NAME-consul-k8s-component-auth-method-dc2"))' | tee /dev/stderr) + [ "${actual}" = "true" ] + + local actual=$(echo $object | + yq -r '.command | any(contains("-primary-datacenter=dc1"))' | tee /dev/stderr) + [ "${actual}" = "true" ] } #-------------------------------------------------------------------- diff --git a/control-plane/subcommand/server-acl-init/command.go b/control-plane/subcommand/server-acl-init/command.go index bb729b5137..c6abed92cf 100644 --- a/control-plane/subcommand/server-acl-init/command.go +++ b/control-plane/subcommand/server-acl-init/command.go @@ -42,7 +42,7 @@ type Command struct { flagCreateClientToken bool - flagCreateSyncToken bool + flagCreateSyncPolicy bool flagSyncConsulNodeName string flagConnectInject bool @@ -126,8 +126,8 @@ func (c *Command) init() { c.flags.BoolVar(&c.flagCreateClientToken, "create-client-token", true, "Toggle for creating a client agent token. Default is true.") - c.flags.BoolVar(&c.flagCreateSyncToken, "create-sync-token", false, - "Toggle for creating a catalog sync token.") + c.flags.BoolVar(&c.flagCreateSyncPolicy, "create-sync-policy", false, + "Toggle for creating a catalog sync policy.") c.flags.StringVar(&c.flagSyncConsulNodeName, "sync-consul-node-name", "k8s-sync", "The Consul node name to register for catalog sync. Defaults to k8s-sync. To be discoverable "+ "via DNS, the name should only contain alpha-numerics and dashes.") @@ -481,19 +481,27 @@ func (c *Command) Run(args []string) int { } } - if c.flagCreateSyncToken { + if c.flagCreateSyncPolicy { syncRules, err := c.syncRules() if err != nil { c.log.Error("Error templating sync rules", "err", err) return 1 } - // If namespaces are enabled, the policy and token needs to be global - // to be allowed to create namespaces. + serviceAccountName := c.withPrefix("sync-catalog") + componentAuthMethodName := localComponentAuthMethodName + + // If namespaces are enabled, the policy and token need to be global to be allowed to create namespaces. if c.flagEnableNamespaces { - err = c.createGlobalACL("catalog-sync", syncRules, consulDC, primary, consulClient) + // Create the catalog sync ACL Policy, Role and BindingRule but do not issue any ACLTokens or create Kube Secrets. + // SyncCatalog token must be global when namespaces are enabled. This means secondary datacenters need + // a token that is known by the primary datacenters. + if !primary { + componentAuthMethodName = globalComponentAuthMethodName + } + err = c.createACLPolicyRoleAndBindingRule("sync-catalog", syncRules, consulDC, primaryDC, globalToken, primary, componentAuthMethodName, serviceAccountName, consulClient) } else { - err = c.createLocalACL("catalog-sync", syncRules, consulDC, primary, consulClient) + err = c.createACLPolicyRoleAndBindingRule("sync-catalog", syncRules, consulDC, primaryDC, localToken, primary, componentAuthMethodName, serviceAccountName, consulClient) } if err != nil { c.log.Error(err.Error()) @@ -522,7 +530,7 @@ func (c *Command) Run(args []string) int { // If namespaces are enabled, the policy and token need to be global // to be allowed to create namespaces. if c.flagEnableNamespaces { - // Create the controller ACL Policy, Role and BindingRule but do not issue any ACLTokens or create Kube Secrets. + // Create the connect-inject ACL Policy, Role and BindingRule but do not issue any ACLTokens or create Kube Secrets. // ConnectInjector token must be global when namespaces are enabled. This means secondary datacenters need // a token that is known by the primary datacenters. if !primary { diff --git a/control-plane/subcommand/server-acl-init/command_ent_test.go b/control-plane/subcommand/server-acl-init/command_ent_test.go index 5f0c4f8da5..270893e765 100644 --- a/control-plane/subcommand/server-acl-init/command_ent_test.go +++ b/control-plane/subcommand/server-acl-init/command_ent_test.go @@ -287,7 +287,7 @@ func TestRun_ACLPolicyUpdates(t *testing.T) { "-create-client-token", "-allow-dns", "-create-mesh-gateway-token", - "-create-sync-token", + "-create-sync-policy", "-connect-inject", "-create-snapshot-agent-token", "-create-enterprise-license-token", @@ -325,7 +325,7 @@ func TestRun_ACLPolicyUpdates(t *testing.T) { firstRunExpectedPolicies := []string{ "anonymous-token-policy", "client-token", - "catalog-sync-token", + "sync-catalog-policy", "mesh-gateway-token", "client-snapshot-agent-token", "enterprise-license-token", @@ -376,7 +376,7 @@ func TestRun_ACLPolicyUpdates(t *testing.T) { secondRunExpectedPolicies := []string{ "anonymous-token-policy", "client-token", - "catalog-sync-token", + "sync-catalog-policy", "connect-inject-policy", "mesh-gateway-token", "client-snapshot-agent-token", @@ -675,13 +675,6 @@ func TestRun_TokensWithNamespacesEnabled(t *testing.T) { SecretNames: []string{resourcePrefix + "-client-acl-token"}, LocalToken: true, }, - "catalog-sync token": { - TokenFlags: []string{"-create-sync-token"}, - PolicyNames: []string{"catalog-sync-token"}, - PolicyDCs: nil, - SecretNames: []string{resourcePrefix + "-catalog-sync-acl-token"}, - LocalToken: false, - }, "enterprise-license token": { TokenFlags: []string{"-create-enterprise-license-token"}, PolicyNames: []string{"enterprise-license-token"}, @@ -1078,6 +1071,13 @@ func TestRun_NamespaceEnabled_ValidateLoginToken_PrimaryDatacenter(t *testing.T) Namespace: ns, GlobalToken: false, }, + { + ComponentName: "sync-catalog", + TokenFlags: []string{"-create-sync-policy"}, + Roles: []string{resourcePrefix + "-sync-catalog-acl-role"}, + Namespace: ns, + GlobalToken: false, + }, } for _, c := range cases { t.Run(c.ComponentName, func(t *testing.T) { @@ -1162,6 +1162,13 @@ func TestRun_NamespaceEnabled_ValidateLoginToken_SecondaryDatacenter(t *testing. Namespace: ns, GlobalToken: true, }, + { + ComponentName: "sync-catalog", + TokenFlags: []string{"-create-sync-policy"}, + Roles: []string{resourcePrefix + "-sync-catalog-acl-role-dc2"}, + Namespace: ns, + GlobalToken: true, + }, } for _, c := range cases { t.Run(c.ComponentName, func(t *testing.T) { diff --git a/control-plane/subcommand/server-acl-init/command_test.go b/control-plane/subcommand/server-acl-init/command_test.go index bfd2a82d67..4e73f18a99 100644 --- a/control-plane/subcommand/server-acl-init/command_test.go +++ b/control-plane/subcommand/server-acl-init/command_test.go @@ -172,14 +172,6 @@ func TestRun_TokensPrimaryDC(t *testing.T) { SecretNames: []string{resourcePrefix + "-client-acl-token"}, LocalToken: true, }, - { - TestName: "Sync token", - TokenFlags: []string{"-create-sync-token"}, - PolicyNames: []string{"catalog-sync-token"}, - PolicyDCs: []string{"dc1"}, - SecretNames: []string{resourcePrefix + "-catalog-sync-acl-token"}, - LocalToken: true, - }, { TestName: "Enterprise license token", TokenFlags: []string{"-create-enterprise-license-token"}, @@ -398,14 +390,6 @@ func TestRun_TokensReplicatedDC(t *testing.T) { SecretNames: []string{resourcePrefix + "-client-acl-token"}, LocalToken: true, }, - { - TestName: "Sync token", - TokenFlags: []string{"-create-sync-token"}, - PolicyNames: []string{"catalog-sync-token-dc2"}, - PolicyDCs: []string{"dc2"}, - SecretNames: []string{resourcePrefix + "-catalog-sync-acl-token"}, - LocalToken: true, - }, { TestName: "Enterprise license token", TokenFlags: []string{"-create-enterprise-license-token"}, @@ -536,12 +520,6 @@ func TestRun_TokensWithProvidedBootstrapToken(t *testing.T) { PolicyNames: []string{"client-token"}, SecretNames: []string{resourcePrefix + "-client-acl-token"}, }, - { - TestName: "Sync token", - TokenFlags: []string{"-create-sync-token"}, - PolicyNames: []string{"catalog-sync-token"}, - SecretNames: []string{resourcePrefix + "-catalog-sync-acl-token"}, - }, { TestName: "Enterprise license token", TokenFlags: []string{"-create-enterprise-license-token"}, @@ -1035,7 +1013,7 @@ func TestRun_SyncPolicyUpdates(t *testing.T) { "-k8s-namespace=" + ns, "-server-address", strings.Split(testSvr.HTTPAddr, ":")[0], "-server-port", strings.Split(testSvr.HTTPAddr, ":")[1], - "-create-sync-token", + "-create-sync-policy", } firstRunArgs := append(commonArgs, "-sync-consul-node-name=k8s-sync", @@ -1066,7 +1044,7 @@ func TestRun_SyncPolicyUpdates(t *testing.T) { require.NoError(t, err) for _, p := range firstPolicies { - if p.Name == "catalog-sync-token" { + if p.Name == "sync-catalog-policy" { policy, _, err := consul.ACL().PolicyRead(p.ID, nil) require.NoError(t, err) @@ -1089,7 +1067,7 @@ func TestRun_SyncPolicyUpdates(t *testing.T) { require.NoError(t, err) for _, p := range secondPolicies { - if p.Name == "catalog-sync-token" { + if p.Name == "sync-catalog-policy" { policy, _, err := consul.ACL().PolicyRead(p.ID, nil) require.NoError(t, err) @@ -1125,7 +1103,7 @@ func TestRun_ErrorsOnDuplicateACLPolicy(t *testing.T) { // Create the policy manually. description := "not the expected description" policy, _, err := consul.ACL().PolicyCreate(&api.ACLPolicy{ - Name: "catalog-sync-token", + Name: "sync-catalog-policy", Description: description, }, nil) require.NoError(t, err) @@ -1144,7 +1122,7 @@ func TestRun_ErrorsOnDuplicateACLPolicy(t *testing.T) { "-k8s-namespace=" + ns, "-server-address", strings.Split(testAgent.HTTPAddr, ":")[0], "-server-port", strings.Split(testAgent.HTTPAddr, ":")[1], - "-create-sync-token", + "-create-sync-policy", } responseCode := cmd.Run(cmdArgs) @@ -2179,6 +2157,12 @@ func TestRun_PoliciesAndBindingRulesForACLLogin_PrimaryDatacenter(t *testing.T) PolicyNames: []string{"connect-inject-policy"}, Roles: []string{resourcePrefix + "-connect-injector-acl-role"}, }, + { + TestName: "Sync Catalog", + TokenFlags: []string{"-create-sync-policy"}, + PolicyNames: []string{"sync-catalog-policy"}, + Roles: []string{resourcePrefix + "-sync-catalog-acl-role"}, + }, } for _, c := range cases { t.Run(c.TestName, func(t *testing.T) { @@ -2283,6 +2267,13 @@ func TestRun_PoliciesAndBindingRulesACLLogin_SecondaryDatacenter(t *testing.T) { Roles: []string{resourcePrefix + "-connect-injector-acl-role-" + secondaryDatacenter}, GlobalAuthMethod: false, }, + { + TestName: "Sync Catalog", + TokenFlags: []string{"-create-sync-policy"}, + PolicyNames: []string{"sync-catalog-policy-" + secondaryDatacenter}, + Roles: []string{resourcePrefix + "-sync-catalog-acl-role-" + secondaryDatacenter}, + GlobalAuthMethod: false, + }, } for _, c := range cases { t.Run(c.TestName, func(t *testing.T) { @@ -2384,6 +2375,11 @@ func TestRun_ValidateLoginToken_PrimaryDatacenter(t *testing.T) { Roles: []string{resourcePrefix + "-connect-injector-acl-role"}, GlobalToken: false, }, + { + ComponentName: "sync-catalog", + TokenFlags: []string{"-create-sync-policy"}, + Roles: []string{resourcePrefix + "-sync-catalog-acl-role"}, + }, } for _, c := range cases { t.Run(c.ComponentName, func(t *testing.T) { @@ -2472,6 +2468,12 @@ func TestRun_ValidateLoginToken_SecondaryDatacenter(t *testing.T) { GlobalAuthMethod: false, GlobalToken: false, }, + { + ComponentName: "sync-catalog", + TokenFlags: []string{"-create-sync-policy"}, + Roles: []string{resourcePrefix + "-sync-catalog-acl-role-dc2"}, + GlobalAuthMethod: false, + }, } for _, c := range cases { t.Run(c.ComponentName, func(t *testing.T) { diff --git a/control-plane/subcommand/server-acl-init/create_or_update.go b/control-plane/subcommand/server-acl-init/create_or_update.go index 954b1e83c6..1c869b11bf 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update.go +++ b/control-plane/subcommand/server-acl-init/create_or_update.go @@ -313,7 +313,7 @@ func (c *Command) createOrUpdateACLPolicy(policy api.ACLPolicy, consulClient *ap // Allowing the Consul node name to be configurable also requires any sync // policy to be updated in case the node name has changed. if isPolicyExistsErr(err, policy.Name) { - if c.flagEnableNamespaces || c.flagCreateSyncToken { + if c.flagEnableNamespaces || c.flagCreateSyncPolicy { c.log.Info(fmt.Sprintf("Policy %q already exists, updating", policy.Name)) // The policy ID is required in any PolicyUpdate call, so first we need to diff --git a/control-plane/subcommand/server-acl-init/create_or_update_test.go b/control-plane/subcommand/server-acl-init/create_or_update_test.go index 57cdffa2a1..a9e8f0704a 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update_test.go +++ b/control-plane/subcommand/server-acl-init/create_or_update_test.go @@ -20,10 +20,10 @@ func TestCreateOrUpdateACLPolicy_ErrorsIfDescriptionDoesNotMatch(t *testing.T) { ui := cli.NewMockUi() k8s := fake.NewSimpleClientset() cmd := Command{ - UI: ui, - clientset: k8s, - log: hclog.NewNullLogger(), - flagCreateSyncToken: true, + UI: ui, + clientset: k8s, + log: hclog.NewNullLogger(), + flagCreateSyncPolicy: true, } // Start Consul. From b719739967052d2dc92d1ff8cced278a42e91aed Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Sun, 6 Mar 2022 17:42:09 -0500 Subject: [PATCH 2/5] Create component-authmethod service account and rolebinding --- ...mponent-authmethod-clusterrolebinding.yaml | 20 ++++++++ .../component-authmethod-serviceaccount.yaml | 19 +++++++ ...-inject-authmethod-clusterrolebinding.yaml | 6 +-- .../templates/connect-inject-clusterrole.yaml | 18 ++++--- .../connect-inject-clusterrolebinding.yaml | 2 + .../connect-inject-serviceaccount.yaml | 4 +- .../templates/server-acl-init-role.yaml | 49 ++++++++++++------- ...mponent-authmethod-clusterrolebinding.bats | 20 ++++++++ .../component-authmethod-serviceaccount.bats | 41 ++++++++++++++++ .../test/unit/connect-inject-clusterrole.bats | 27 ++++++++++ .../connect-inject-clusterrolebinding.bats | 26 ++++++++++ .../unit/connect-inject-serviceaccount.bats | 27 ++++++++++ .../test/unit/server-acl-init-role.bats | 13 +++++ .../subcommand/server-acl-init/command.go | 4 +- .../server-acl-init/connect_inject.go | 12 ++--- .../server-acl-init/connect_inject_test.go | 2 +- 16 files changed, 250 insertions(+), 40 deletions(-) create mode 100644 charts/consul/templates/component-authmethod-clusterrolebinding.yaml create mode 100644 charts/consul/templates/component-authmethod-serviceaccount.yaml create mode 100644 charts/consul/test/unit/component-authmethod-clusterrolebinding.bats create mode 100644 charts/consul/test/unit/component-authmethod-serviceaccount.bats diff --git a/charts/consul/templates/component-authmethod-clusterrolebinding.yaml b/charts/consul/templates/component-authmethod-clusterrolebinding.yaml new file mode 100644 index 0000000000..3e80bf6800 --- /dev/null +++ b/charts/consul/templates/component-authmethod-clusterrolebinding.yaml @@ -0,0 +1,20 @@ +{{- if .Values.global.acls.manageSystemACLs }} +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ template "consul.fullname" . }}-component-authdelegator + labels: + app: {{ template "consul.name" . }} + chart: {{ template "consul.chart" . }} + heritage: {{ .Release.Service }} + release: {{ .Release.Name }} + component: component-authmethod +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: "system:auth-delegator" +subjects: +- kind: ServiceAccount + name: {{ template "consul.fullname" . }}-component-authmethod + namespace: {{ .Release.Namespace }} +{{- end }} diff --git a/charts/consul/templates/component-authmethod-serviceaccount.yaml b/charts/consul/templates/component-authmethod-serviceaccount.yaml new file mode 100644 index 0000000000..0efd4b26f8 --- /dev/null +++ b/charts/consul/templates/component-authmethod-serviceaccount.yaml @@ -0,0 +1,19 @@ +{{- if .Values.global.acls.manageSystemACLs }} +apiVersion: v1 +kind: ServiceAccount +metadata: + name: {{ template "consul.fullname" . }}-component-authmethod + namespace: {{ .Release.Namespace }} + labels: + app: {{ template "consul.name" . }} + chart: {{ template "consul.chart" . }} + heritage: {{ .Release.Service }} + release: {{ .Release.Name }} + component: component-authmethod +{{- with .Values.global.imagePullSecrets }} +imagePullSecrets: +{{- range . }} +- name: {{ .name }} +{{- end }} +{{- end }} +{{- end }} diff --git a/charts/consul/templates/connect-inject-authmethod-clusterrolebinding.yaml b/charts/consul/templates/connect-inject-authmethod-clusterrolebinding.yaml index 4f9d7c8083..2867c256b7 100644 --- a/charts/consul/templates/connect-inject-authmethod-clusterrolebinding.yaml +++ b/charts/consul/templates/connect-inject-authmethod-clusterrolebinding.yaml @@ -15,8 +15,8 @@ roleRef: kind: ClusterRole name: "system:auth-delegator" subjects: - - kind: ServiceAccount - name: {{ template "consul.fullname" . }}-connect-injector - namespace: {{ .Release.Namespace }} +- kind: ServiceAccount + name: {{ template "consul.fullname" . }}-connect-injector + namespace: {{ .Release.Namespace }} {{- end }} {{- end }} diff --git a/charts/consul/templates/connect-inject-clusterrole.yaml b/charts/consul/templates/connect-inject-clusterrole.yaml index 3a2eb62829..8dd9d80ae7 100644 --- a/charts/consul/templates/connect-inject-clusterrole.yaml +++ b/charts/consul/templates/connect-inject-clusterrole.yaml @@ -1,3 +1,4 @@ +{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }} # The ClusterRole to enable the Connect injector to get, list, watch and patch MutatingWebhookConfiguration. apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole @@ -11,14 +12,14 @@ metadata: component: connect-injector rules: {{- if .Values.global.acls.manageSystemACLs }} -- apiGroups: [""] +- apiGroups: [ "" ] resources: - serviceaccounts verbs: - get {{- end }} -- apiGroups: [""] - resources: ["pods", "endpoints", "services", "namespaces"] +- apiGroups: [ "" ] + resources: [ "pods", "endpoints", "services", "namespaces" ] verbs: - "get" - "list" @@ -33,17 +34,18 @@ rules: - list - update {{- if .Values.global.enablePodSecurityPolicies }} -- apiGroups: ["policy"] - resources: ["podsecuritypolicies"] +- apiGroups: [ "policy" ] + resources: [ "podsecuritypolicies" ] resourceNames: - {{ template "consul.fullname" . }}-connect-injector verbs: - use {{- end }} {{- if .Values.global.acls.manageSystemACLs }} -- apiGroups: [""] +- apiGroups: [ "" ] resources: - - serviceaccounts + - serviceaccounts verbs: - - get + - get +{{- end }} {{- end }} diff --git a/charts/consul/templates/connect-inject-clusterrolebinding.yaml b/charts/consul/templates/connect-inject-clusterrolebinding.yaml index 3a9d041852..c380adb311 100644 --- a/charts/consul/templates/connect-inject-clusterrolebinding.yaml +++ b/charts/consul/templates/connect-inject-clusterrolebinding.yaml @@ -1,3 +1,4 @@ +{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }} apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: @@ -16,3 +17,4 @@ subjects: - kind: ServiceAccount name: {{ template "consul.fullname" . }}-connect-injector namespace: {{ .Release.Namespace }} +{{- end }} \ No newline at end of file diff --git a/charts/consul/templates/connect-inject-serviceaccount.yaml b/charts/consul/templates/connect-inject-serviceaccount.yaml index c95136914a..ea2352c7ac 100644 --- a/charts/consul/templates/connect-inject-serviceaccount.yaml +++ b/charts/consul/templates/connect-inject-serviceaccount.yaml @@ -1,3 +1,4 @@ +{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }} apiVersion: v1 kind: ServiceAccount metadata: @@ -16,6 +17,7 @@ metadata: {{- with .Values.global.imagePullSecrets }} imagePullSecrets: {{- range . }} - - name: {{ .name }} +- name: {{ .name }} +{{- end }} {{- end }} {{- end }} diff --git a/charts/consul/templates/server-acl-init-role.yaml b/charts/consul/templates/server-acl-init-role.yaml index 43d610f7a7..3b5987f01d 100644 --- a/charts/consul/templates/server-acl-init-role.yaml +++ b/charts/consul/templates/server-acl-init-role.yaml @@ -13,26 +13,37 @@ metadata: release: {{ .Release.Name }} component: server-acl-init rules: - - apiGroups: [""] - resources: - - secrets - verbs: - - create - - get - - apiGroups: [""] - resources: - - serviceaccounts - resourceNames: - - {{ template "consul.fullname" . }}-connect-injector - verbs: - - get +- apiGroups: [ "" ] + resources: + - secrets + verbs: + - create + - get +{{- if .Values.connectInject.enabled }} +- apiGroups: [ "" ] + resources: + - serviceaccounts + resourceNames: + - {{ template "consul.fullname" . }}-connect-injector + verbs: + - get +{{- end }} +{{- if .Values.global.acls.manageSystemACLs }} +- apiGroups: [ "" ] + resources: + - serviceaccounts + resourceNames: + - {{ template "consul.fullname" . }}-component-authmethod + verbs: + - get +{{- end }} {{- if .Values.global.enablePodSecurityPolicies }} - - apiGroups: ["policy"] - resources: ["podsecuritypolicies"] - resourceNames: - - {{ template "consul.fullname" . }}-server-acl-init - verbs: - - use +- apiGroups: [ "policy" ] + resources: [ "podsecuritypolicies" ] + resourceNames: + - {{ template "consul.fullname" . }}-server-acl-init + verbs: + - use {{- end }} {{- end }} {{- end }} diff --git a/charts/consul/test/unit/component-authmethod-clusterrolebinding.bats b/charts/consul/test/unit/component-authmethod-clusterrolebinding.bats new file mode 100644 index 0000000000..7d07185573 --- /dev/null +++ b/charts/consul/test/unit/component-authmethod-clusterrolebinding.bats @@ -0,0 +1,20 @@ +#!/usr/bin/env bats + +load _helpers + +@test "componentAuthmethod/ClusterRoleBinding: disabled by default" { + cd `chart_dir` + assert_empty helm template \ + -s templates/component-authmethod-clusterrolebinding.yaml \ + . +} + +@test "componentAuthmethod/ClusterRoleBinding: enabled with global.acls.manageSystemACLs true" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/component-authmethod-clusterrolebinding.yaml \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq -s 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} \ No newline at end of file diff --git a/charts/consul/test/unit/component-authmethod-serviceaccount.bats b/charts/consul/test/unit/component-authmethod-serviceaccount.bats new file mode 100644 index 0000000000..7635e8cc1d --- /dev/null +++ b/charts/consul/test/unit/component-authmethod-serviceaccount.bats @@ -0,0 +1,41 @@ +#!/usr/bin/env bats + +load _helpers + +@test "componentAuthMethod/ServiceAccount: disabled by default" { + cd `chart_dir` + assert_empty helm template \ + -s templates/component-authmethod-serviceaccount.yaml \ + . +} + +@test "componentAuthMethod/ServiceAccount: enabled with global.acls.manageSystemACLs.enabled true" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/component-authmethod-serviceaccount.yaml \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq -s 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +#-------------------------------------------------------------------- +# global.imagePullSecrets + +@test "componentAuthMethod/ServiceAccount: can set image pull secrets" { + cd `chart_dir` + local object=$(helm template \ + -s templates/component-authmethod-serviceaccount.yaml \ + --set 'global.acls.manageSystemACLs=true' \ + --set 'global.imagePullSecrets[0].name=my-secret' \ + --set 'global.imagePullSecrets[1].name=my-secret2' \ + . | tee /dev/stderr) + + local actual=$(echo "$object" | + yq -r '.imagePullSecrets[0].name' | tee /dev/stderr) + [ "${actual}" = "my-secret" ] + + local actual=$(echo "$object" | + yq -r '.imagePullSecrets[1].name' | tee /dev/stderr) + [ "${actual}" = "my-secret2" ] +} diff --git a/charts/consul/test/unit/connect-inject-clusterrole.bats b/charts/consul/test/unit/connect-inject-clusterrole.bats index c7ff654cf2..e954b8908a 100644 --- a/charts/consul/test/unit/connect-inject-clusterrole.bats +++ b/charts/consul/test/unit/connect-inject-clusterrole.bats @@ -2,6 +2,33 @@ load _helpers +@test "connectInject/ClusterRole: disabled by default" { + cd `chart_dir` + assert_empty helm template \ + -s templates/connect-inject-clusterrole.yaml \ + . +} + +@test "connectInject/ClusterRole: enabled with global.enabled false" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/connect-inject-clusterrole.yaml \ + --set 'global.enabled=false' \ + --set 'client.enabled=true' \ + --set 'connectInject.enabled=true' \ + . | tee /dev/stderr | + yq -s 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "connectInject/ClusterRole: disabled with connectInject.enabled" { + cd `chart_dir` + assert_empty helm template \ + -s templates/connect-inject-clusterrole.yaml \ + --set 'connectInject.enabled=false' \ + . +} + #-------------------------------------------------------------------- # global.enablePodSecurityPolicies diff --git a/charts/consul/test/unit/connect-inject-clusterrolebinding.bats b/charts/consul/test/unit/connect-inject-clusterrolebinding.bats index 6900cb2bd5..ccf30083f9 100644 --- a/charts/consul/test/unit/connect-inject-clusterrolebinding.bats +++ b/charts/consul/test/unit/connect-inject-clusterrolebinding.bats @@ -2,3 +2,29 @@ load _helpers +@test "connectInject/ClusterRoleBinding: disabled by default" { + cd `chart_dir` + assert_empty helm template \ + -s templates/connect-inject-clusterrolebinding.yaml \ + . +} + +@test "connectInject/ClusterRoleBinding: enabled with global.enabled false" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/connect-inject-clusterrolebinding.yaml \ + --set 'global.enabled=false' \ + --set 'client.enabled=true' \ + --set 'connectInject.enabled=true' \ + . | tee /dev/stderr | + yq -s 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "connectInject/ClusterRoleBinding: disabled with connectInject.enabled false" { + cd `chart_dir` + assert_empty helm template \ + -s templates/connect-inject-clusterrolebinding.yaml \ + --set 'connectInject.enabled=false' \ + . +} \ No newline at end of file diff --git a/charts/consul/test/unit/connect-inject-serviceaccount.bats b/charts/consul/test/unit/connect-inject-serviceaccount.bats index 4c5fb8e316..07b38c3d49 100644 --- a/charts/consul/test/unit/connect-inject-serviceaccount.bats +++ b/charts/consul/test/unit/connect-inject-serviceaccount.bats @@ -2,6 +2,33 @@ load _helpers +@test "connectInject/ServiceAccount: disabled by default" { + cd `chart_dir` + assert_empty helm template \ + -s templates/connect-inject-serviceaccount.yaml \ + . +} + +@test "connectInject/ServiceAccount: enabled with global.enabled false" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/connect-inject-serviceaccount.yaml \ + --set 'global.enabled=false' \ + --set 'client.enabled=true' \ + --set 'connectInject.enabled=true' \ + . | tee /dev/stderr | + yq -s 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "connectInject/ServiceAccount: disabled with connectInject.enabled false" { + cd `chart_dir` + assert_empty helm template \ + -s templates/connect-inject-serviceaccount.yaml \ + --set 'connectInject.enabled=false' \ + . +} + #-------------------------------------------------------------------- # global.imagePullSecrets diff --git a/charts/consul/test/unit/server-acl-init-role.bats b/charts/consul/test/unit/server-acl-init-role.bats index 9d8d8f4573..99cc610b41 100644 --- a/charts/consul/test/unit/server-acl-init-role.bats +++ b/charts/consul/test/unit/server-acl-init-role.bats @@ -63,6 +63,19 @@ load _helpers --set 'connectInject.enabled=true' \ . | tee /dev/stderr | yq -r '.rules | map(select(.resources[0] == "serviceaccounts")) | length' | tee /dev/stderr) + [ "${actual}" = "2" ] +} + +#-------------------------------------------------------------------- +# manageSystemACLs.enabled + +@test "serverACLInit/Role: allows service accounts when manageSystemACLs.enabled is true" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/server-acl-init-role.yaml \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq -r '.rules | map(select(.resources[0] == "serviceaccounts")) | length' | tee /dev/stderr) [ "${actual}" = "1" ] } diff --git a/control-plane/subcommand/server-acl-init/command.go b/control-plane/subcommand/server-acl-init/command.go index c6abed92cf..f62525975d 100644 --- a/control-plane/subcommand/server-acl-init/command.go +++ b/control-plane/subcommand/server-acl-init/command.go @@ -758,7 +758,7 @@ func (c *Command) Run(args []string) int { // that the Consul components will use to issue global ACL tokens with. func (c *Command) configureGlobalComponentAuthMethod(consulClient *api.Client, authMethodName, primaryDC string) error { // Create the auth method template. This requires calls to the kubernetes environment. - authMethod, err := c.createAuthMethodTmpl(authMethodName, false) + authMethod, err := c.createAuthMethodTmpl(authMethodName, c.withPrefix("component-authmethod"), false) if err != nil { return err } @@ -771,7 +771,7 @@ func (c *Command) configureGlobalComponentAuthMethod(consulClient *api.Client, a // that the Consul components will use to issue local ACL tokens with. func (c *Command) configureLocalComponentAuthMethod(consulClient *api.Client, authMethodName string) error { // Create the auth method template. This requires calls to the kubernetes environment. - authMethod, err := c.createAuthMethodTmpl(authMethodName, false) + authMethod, err := c.createAuthMethodTmpl(authMethodName, c.withPrefix("component-authmethod"), false) if err != nil { return err } diff --git a/control-plane/subcommand/server-acl-init/connect_inject.go b/control-plane/subcommand/server-acl-init/connect_inject.go index 8869fcaef3..8becfd5248 100644 --- a/control-plane/subcommand/server-acl-init/connect_inject.go +++ b/control-plane/subcommand/server-acl-init/connect_inject.go @@ -21,7 +21,7 @@ func (c *Command) configureConnectInjectAuthMethod(consulClient *api.Client, aut // Create the auth method template. This requires calls to the // kubernetes environment. - authMethodTmpl, err := c.createAuthMethodTmpl(authMethodName, true) + authMethodTmpl, err := c.createAuthMethodTmpl(authMethodName, c.withPrefix("connect-injector"), true) if err != nil { return err } @@ -80,14 +80,14 @@ func (c *Command) configureConnectInjectAuthMethod(consulClient *api.Client, aut // createAuthMethodTmpl sets up the auth method template based on the connect-injector's service account // jwt token. It is common for both the connect inject auth method and the component auth method // with the option to add namespace specific configuration to the auth method template via `useNS`. -func (c *Command) createAuthMethodTmpl(authMethodName string, useNS bool) (api.ACLAuthMethod, error) { +func (c *Command) createAuthMethodTmpl(authMethodName, serviceAccountName string, useNS bool) (api.ACLAuthMethod, error) { // Get the Secret name for the auth method ServiceAccount. var authMethodServiceAccount *apiv1.ServiceAccount - saName := c.withPrefix("connect-injector") - err := c.untilSucceeds(fmt.Sprintf("getting %s ServiceAccount", saName), + + err := c.untilSucceeds(fmt.Sprintf("getting %s ServiceAccount", serviceAccountName), func() error { var err error - authMethodServiceAccount, err = c.clientset.CoreV1().ServiceAccounts(c.flagK8sNamespace).Get(c.ctx, saName, metav1.GetOptions{}) + authMethodServiceAccount, err = c.clientset.CoreV1().ServiceAccounts(c.flagK8sNamespace).Get(c.ctx, serviceAccountName, metav1.GetOptions{}) return err }) if err != nil { @@ -119,7 +119,7 @@ func (c *Command) createAuthMethodTmpl(authMethodName string, useNS bool) (api.A // a secret of type ServiceAccountToken. if saSecret == nil { return api.ACLAuthMethod{}, - fmt.Errorf("found no secret of type 'kubernetes.io/service-account-token' associated with the %s service account", saName) + fmt.Errorf("found no secret of type 'kubernetes.io/service-account-token' associated with the %s service account", serviceAccountName) } kubernetesHost := defaultKubernetesHost diff --git a/control-plane/subcommand/server-acl-init/connect_inject_test.go b/control-plane/subcommand/server-acl-init/connect_inject_test.go index e9c38595a0..c82900cf94 100644 --- a/control-plane/subcommand/server-acl-init/connect_inject_test.go +++ b/control-plane/subcommand/server-acl-init/connect_inject_test.go @@ -64,6 +64,6 @@ func TestCommand_createAuthMethodTmpl_SecretNotFound(t *testing.T) { _, err := k8s.CoreV1().Secrets(ns).Create(ctx, secret, metav1.CreateOptions{}) require.NoError(t, err) - _, err = cmd.createAuthMethodTmpl("test", true) + _, err = cmd.createAuthMethodTmpl("test", serviceAccountName, true) require.EqualError(t, err, "found no secret of type 'kubernetes.io/service-account-token' associated with the release-name-consul-connect-injector service account") } From e5b33891ca6f133cc96e47623272d6bf8e486ac4 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 7 Mar 2022 09:47:46 -0500 Subject: [PATCH 3/5] Use a single serviceaccount and rolebinding for all authmethods. --- .../templates/authmethod-clusterrole.yaml | 18 ++++++++ .../authmethod-clusterrolebinding.yaml | 39 +++++++++++++++++ ...nt.yaml => authmethod-serviceaccount.yaml} | 4 +- ...mponent-authmethod-clusterrolebinding.yaml | 20 --------- ...-inject-authmethod-clusterrolebinding.yaml | 22 ---------- .../templates/connect-inject-clusterrole.yaml | 7 ---- .../templates/server-acl-init-role.yaml | 13 +----- .../test/unit/authmethod-clusterrole.bats | 20 +++++++++ .../unit/authmethod-clusterrolebinding.bats | 20 +++++++++ ...nt.bats => authmethod-serviceaccount.bats} | 12 +++--- ...mponent-authmethod-clusterrolebinding.bats | 20 --------- ...-inject-authmethod-clusterrolebinding.bats | 42 ------------------- .../test/unit/server-acl-init-role.bats | 14 ------- .../subcommand/server-acl-init/command.go | 4 +- .../server-acl-init/command_test.go | 6 +-- .../server-acl-init/connect_inject.go | 6 +-- .../server-acl-init/connect_inject_test.go | 6 +-- 17 files changed, 117 insertions(+), 156 deletions(-) create mode 100644 charts/consul/templates/authmethod-clusterrole.yaml create mode 100644 charts/consul/templates/authmethod-clusterrolebinding.yaml rename charts/consul/templates/{component-authmethod-serviceaccount.yaml => authmethod-serviceaccount.yaml} (80%) delete mode 100644 charts/consul/templates/component-authmethod-clusterrolebinding.yaml delete mode 100644 charts/consul/templates/connect-inject-authmethod-clusterrolebinding.yaml create mode 100644 charts/consul/test/unit/authmethod-clusterrole.bats create mode 100644 charts/consul/test/unit/authmethod-clusterrolebinding.bats rename charts/consul/test/unit/{component-authmethod-serviceaccount.bats => authmethod-serviceaccount.bats} (67%) delete mode 100644 charts/consul/test/unit/component-authmethod-clusterrolebinding.bats delete mode 100644 charts/consul/test/unit/connect-inject-authmethod-clusterrolebinding.bats diff --git a/charts/consul/templates/authmethod-clusterrole.yaml b/charts/consul/templates/authmethod-clusterrole.yaml new file mode 100644 index 0000000000..30385aa25f --- /dev/null +++ b/charts/consul/templates/authmethod-clusterrole.yaml @@ -0,0 +1,18 @@ +{{- if .Values.global.acls.manageSystemACLs }} +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: {{ template "consul.fullname" . }}-authmethod + labels: + app: {{ template "consul.name" . }} + chart: {{ template "consul.chart" . }} + heritage: {{ .Release.Service }} + release: {{ .Release.Name }} + component: authmethod +rules: +- apiGroups: [ "" ] + resources: + - serviceaccounts + verbs: + - get +{{- end }} diff --git a/charts/consul/templates/authmethod-clusterrolebinding.yaml b/charts/consul/templates/authmethod-clusterrolebinding.yaml new file mode 100644 index 0000000000..89bc44dea4 --- /dev/null +++ b/charts/consul/templates/authmethod-clusterrolebinding.yaml @@ -0,0 +1,39 @@ +{{- if .Values.global.acls.manageSystemACLs }} +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ template "consul.fullname" . }}-authdelegator + labels: + app: {{ template "consul.name" . }} + chart: {{ template "consul.chart" . }} + heritage: {{ .Release.Service }} + release: {{ .Release.Name }} + component: authmethod +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: "system:auth-delegator" +subjects: +- kind: ServiceAccount + name: {{ template "consul.fullname" . }}-authmethod + namespace: {{ .Release.Namespace }} +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ template "consul.fullname" . }}-authmethod + labels: + app: {{ template "consul.name" . }} + chart: {{ template "consul.chart" . }} + heritage: {{ .Release.Service }} + release: {{ .Release.Name }} + component: authmethod +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: {{ template "consul.fullname" . }}-authmethod +subjects: +- kind: ServiceAccount + name: {{ template "consul.fullname" . }}-authmethod + namespace: {{ .Release.Namespace }} +{{- end }} diff --git a/charts/consul/templates/component-authmethod-serviceaccount.yaml b/charts/consul/templates/authmethod-serviceaccount.yaml similarity index 80% rename from charts/consul/templates/component-authmethod-serviceaccount.yaml rename to charts/consul/templates/authmethod-serviceaccount.yaml index 0efd4b26f8..bfb50dc0dd 100644 --- a/charts/consul/templates/component-authmethod-serviceaccount.yaml +++ b/charts/consul/templates/authmethod-serviceaccount.yaml @@ -2,14 +2,14 @@ apiVersion: v1 kind: ServiceAccount metadata: - name: {{ template "consul.fullname" . }}-component-authmethod + name: {{ template "consul.fullname" . }}-authmethod namespace: {{ .Release.Namespace }} labels: app: {{ template "consul.name" . }} chart: {{ template "consul.chart" . }} heritage: {{ .Release.Service }} release: {{ .Release.Name }} - component: component-authmethod + component: authmethod {{- with .Values.global.imagePullSecrets }} imagePullSecrets: {{- range . }} diff --git a/charts/consul/templates/component-authmethod-clusterrolebinding.yaml b/charts/consul/templates/component-authmethod-clusterrolebinding.yaml deleted file mode 100644 index 3e80bf6800..0000000000 --- a/charts/consul/templates/component-authmethod-clusterrolebinding.yaml +++ /dev/null @@ -1,20 +0,0 @@ -{{- if .Values.global.acls.manageSystemACLs }} -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRoleBinding -metadata: - name: {{ template "consul.fullname" . }}-component-authdelegator - labels: - app: {{ template "consul.name" . }} - chart: {{ template "consul.chart" . }} - heritage: {{ .Release.Service }} - release: {{ .Release.Name }} - component: component-authmethod -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: "system:auth-delegator" -subjects: -- kind: ServiceAccount - name: {{ template "consul.fullname" . }}-component-authmethod - namespace: {{ .Release.Namespace }} -{{- end }} diff --git a/charts/consul/templates/connect-inject-authmethod-clusterrolebinding.yaml b/charts/consul/templates/connect-inject-authmethod-clusterrolebinding.yaml deleted file mode 100644 index 2867c256b7..0000000000 --- a/charts/consul/templates/connect-inject-authmethod-clusterrolebinding.yaml +++ /dev/null @@ -1,22 +0,0 @@ -{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }} -{{- if .Values.global.acls.manageSystemACLs }} -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRoleBinding -metadata: - name: {{ template "consul.fullname" . }}-connect-injector-authdelegator - labels: - app: {{ template "consul.name" . }} - chart: {{ template "consul.chart" . }} - heritage: {{ .Release.Service }} - release: {{ .Release.Name }} - component: connect-injector -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: "system:auth-delegator" -subjects: -- kind: ServiceAccount - name: {{ template "consul.fullname" . }}-connect-injector - namespace: {{ .Release.Namespace }} -{{- end }} -{{- end }} diff --git a/charts/consul/templates/connect-inject-clusterrole.yaml b/charts/consul/templates/connect-inject-clusterrole.yaml index 8dd9d80ae7..683a9c6bf7 100644 --- a/charts/consul/templates/connect-inject-clusterrole.yaml +++ b/charts/consul/templates/connect-inject-clusterrole.yaml @@ -41,11 +41,4 @@ rules: verbs: - use {{- end }} -{{- if .Values.global.acls.manageSystemACLs }} -- apiGroups: [ "" ] - resources: - - serviceaccounts - verbs: - - get -{{- end }} {{- end }} diff --git a/charts/consul/templates/server-acl-init-role.yaml b/charts/consul/templates/server-acl-init-role.yaml index 3b5987f01d..9e38f55310 100644 --- a/charts/consul/templates/server-acl-init-role.yaml +++ b/charts/consul/templates/server-acl-init-role.yaml @@ -19,24 +19,13 @@ rules: verbs: - create - get -{{- if .Values.connectInject.enabled }} - apiGroups: [ "" ] resources: - serviceaccounts resourceNames: - - {{ template "consul.fullname" . }}-connect-injector + - {{ template "consul.fullname" . }}-authmethod verbs: - get -{{- end }} -{{- if .Values.global.acls.manageSystemACLs }} -- apiGroups: [ "" ] - resources: - - serviceaccounts - resourceNames: - - {{ template "consul.fullname" . }}-component-authmethod - verbs: - - get -{{- end }} {{- if .Values.global.enablePodSecurityPolicies }} - apiGroups: [ "policy" ] resources: [ "podsecuritypolicies" ] diff --git a/charts/consul/test/unit/authmethod-clusterrole.bats b/charts/consul/test/unit/authmethod-clusterrole.bats new file mode 100644 index 0000000000..0888ea373b --- /dev/null +++ b/charts/consul/test/unit/authmethod-clusterrole.bats @@ -0,0 +1,20 @@ +#!/usr/bin/env bats + +load _helpers + +@test "authmethod/ClusterRole: disabled by default" { + cd `chart_dir` + assert_empty helm template \ + -s templates/authmethod-clusterrole.yaml \ + . +} + +@test "authmethod/ClusterRole: enabled with global.acls.manageSystemACLs true" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/authmethod-clusterrole.yaml \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq -s 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} \ No newline at end of file diff --git a/charts/consul/test/unit/authmethod-clusterrolebinding.bats b/charts/consul/test/unit/authmethod-clusterrolebinding.bats new file mode 100644 index 0000000000..21b96d0bbb --- /dev/null +++ b/charts/consul/test/unit/authmethod-clusterrolebinding.bats @@ -0,0 +1,20 @@ +#!/usr/bin/env bats + +load _helpers + +@test "authmethod/ClusterRoleBinding: disabled by default" { + cd `chart_dir` + assert_empty helm template \ + -s templates/authmethod-clusterrolebinding.yaml \ + . +} + +@test "authmethod/ClusterRoleBinding: enabled with global.acls.manageSystemACLs true" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/authmethod-clusterrolebinding.yaml \ + --set 'global.acls.manageSystemACLs=true' \ + . | tee /dev/stderr | + yq -s 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} \ No newline at end of file diff --git a/charts/consul/test/unit/component-authmethod-serviceaccount.bats b/charts/consul/test/unit/authmethod-serviceaccount.bats similarity index 67% rename from charts/consul/test/unit/component-authmethod-serviceaccount.bats rename to charts/consul/test/unit/authmethod-serviceaccount.bats index 7635e8cc1d..8cf9633cae 100644 --- a/charts/consul/test/unit/component-authmethod-serviceaccount.bats +++ b/charts/consul/test/unit/authmethod-serviceaccount.bats @@ -2,17 +2,17 @@ load _helpers -@test "componentAuthMethod/ServiceAccount: disabled by default" { +@test "authMethod/ServiceAccount: disabled by default" { cd `chart_dir` assert_empty helm template \ - -s templates/component-authmethod-serviceaccount.yaml \ + -s templates/authmethod-serviceaccount.yaml \ . } -@test "componentAuthMethod/ServiceAccount: enabled with global.acls.manageSystemACLs.enabled true" { +@test "authMethod/ServiceAccount: enabled with global.acls.manageSystemACLs.enabled true" { cd `chart_dir` local actual=$(helm template \ - -s templates/component-authmethod-serviceaccount.yaml \ + -s templates/authmethod-serviceaccount.yaml \ --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | yq -s 'length > 0' | tee /dev/stderr) @@ -22,10 +22,10 @@ load _helpers #-------------------------------------------------------------------- # global.imagePullSecrets -@test "componentAuthMethod/ServiceAccount: can set image pull secrets" { +@test "authMethod/ServiceAccount: can set image pull secrets" { cd `chart_dir` local object=$(helm template \ - -s templates/component-authmethod-serviceaccount.yaml \ + -s templates/authmethod-serviceaccount.yaml \ --set 'global.acls.manageSystemACLs=true' \ --set 'global.imagePullSecrets[0].name=my-secret' \ --set 'global.imagePullSecrets[1].name=my-secret2' \ diff --git a/charts/consul/test/unit/component-authmethod-clusterrolebinding.bats b/charts/consul/test/unit/component-authmethod-clusterrolebinding.bats deleted file mode 100644 index 7d07185573..0000000000 --- a/charts/consul/test/unit/component-authmethod-clusterrolebinding.bats +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env bats - -load _helpers - -@test "componentAuthmethod/ClusterRoleBinding: disabled by default" { - cd `chart_dir` - assert_empty helm template \ - -s templates/component-authmethod-clusterrolebinding.yaml \ - . -} - -@test "componentAuthmethod/ClusterRoleBinding: enabled with global.acls.manageSystemACLs true" { - cd `chart_dir` - local actual=$(helm template \ - -s templates/component-authmethod-clusterrolebinding.yaml \ - --set 'global.acls.manageSystemACLs=true' \ - . | tee /dev/stderr | - yq -s 'length > 0' | tee /dev/stderr) - [ "${actual}" = "true" ] -} \ No newline at end of file diff --git a/charts/consul/test/unit/connect-inject-authmethod-clusterrolebinding.bats b/charts/consul/test/unit/connect-inject-authmethod-clusterrolebinding.bats deleted file mode 100644 index 7a70293f16..0000000000 --- a/charts/consul/test/unit/connect-inject-authmethod-clusterrolebinding.bats +++ /dev/null @@ -1,42 +0,0 @@ -#!/usr/bin/env bats - -load _helpers - -@test "connectInjectAuthMethod/ClusterRoleBinding: disabled by default" { - cd `chart_dir` - assert_empty helm template \ - -s templates/connect-inject-authmethod-clusterrolebinding.yaml \ - . -} - -@test "connectInjectAuthMethod/ClusterRoleBinding: enabled with global.enabled false" { - cd `chart_dir` - local actual=$(helm template \ - -s templates/connect-inject-authmethod-clusterrolebinding.yaml \ - --set 'global.enabled=false' \ - --set 'client.enabled=true' \ - --set 'connectInject.enabled=true' \ - --set 'global.acls.manageSystemACLs=true' \ - . | tee /dev/stderr | - yq -s 'length > 0' | tee /dev/stderr) - [ "${actual}" = "true" ] -} - -@test "connectInjectAuthMethod/ClusterRoleBinding: disabled with connectInject.enabled" { - cd `chart_dir` - assert_empty helm template \ - -s templates/connect-inject-authmethod-clusterrolebinding.yaml \ - --set 'connectInject.enabled=true' \ - . -} - -@test "connectInjectAuthMethod/ClusterRoleBinding: enabled with global.acls.manageSystemACLs.enabled=true" { - cd `chart_dir` - local actual=$(helm template \ - -s templates/connect-inject-authmethod-clusterrolebinding.yaml \ - --set 'connectInject.enabled=true' \ - --set 'global.acls.manageSystemACLs=true' \ - . | tee /dev/stderr | - yq -s 'length > 0' | tee /dev/stderr) - [ "${actual}" = "true" ] -} diff --git a/charts/consul/test/unit/server-acl-init-role.bats b/charts/consul/test/unit/server-acl-init-role.bats index 99cc610b41..92cdd78d16 100644 --- a/charts/consul/test/unit/server-acl-init-role.bats +++ b/charts/consul/test/unit/server-acl-init-role.bats @@ -52,20 +52,6 @@ load _helpers [ "${actual}" = "true" ] } -#-------------------------------------------------------------------- -# connectInject.enabled - -@test "serverACLInit/Role: allows service accounts when connectInject.enabled is true" { - cd `chart_dir` - local actual=$(helm template \ - -s templates/server-acl-init-role.yaml \ - --set 'global.acls.manageSystemACLs=true' \ - --set 'connectInject.enabled=true' \ - . | tee /dev/stderr | - yq -r '.rules | map(select(.resources[0] == "serviceaccounts")) | length' | tee /dev/stderr) - [ "${actual}" = "2" ] -} - #-------------------------------------------------------------------- # manageSystemACLs.enabled diff --git a/control-plane/subcommand/server-acl-init/command.go b/control-plane/subcommand/server-acl-init/command.go index f62525975d..c6abed92cf 100644 --- a/control-plane/subcommand/server-acl-init/command.go +++ b/control-plane/subcommand/server-acl-init/command.go @@ -758,7 +758,7 @@ func (c *Command) Run(args []string) int { // that the Consul components will use to issue global ACL tokens with. func (c *Command) configureGlobalComponentAuthMethod(consulClient *api.Client, authMethodName, primaryDC string) error { // Create the auth method template. This requires calls to the kubernetes environment. - authMethod, err := c.createAuthMethodTmpl(authMethodName, c.withPrefix("component-authmethod"), false) + authMethod, err := c.createAuthMethodTmpl(authMethodName, false) if err != nil { return err } @@ -771,7 +771,7 @@ func (c *Command) configureGlobalComponentAuthMethod(consulClient *api.Client, a // that the Consul components will use to issue local ACL tokens with. func (c *Command) configureLocalComponentAuthMethod(consulClient *api.Client, authMethodName string) error { // Create the auth method template. This requires calls to the kubernetes environment. - authMethod, err := c.createAuthMethodTmpl(authMethodName, c.withPrefix("component-authmethod"), false) + authMethod, err := c.createAuthMethodTmpl(authMethodName, false) if err != nil { return err } diff --git a/control-plane/subcommand/server-acl-init/command_test.go b/control-plane/subcommand/server-acl-init/command_test.go index 4e73f18a99..3f1408581e 100644 --- a/control-plane/subcommand/server-acl-init/command_test.go +++ b/control-plane/subcommand/server-acl-init/command_test.go @@ -2803,7 +2803,7 @@ func getBootToken(t *testing.T, k8s *fake.Clientset, prefix string, k8sNamespace func setUpK8sServiceAccount(t *testing.T, k8s *fake.Clientset, namespace string) (string, string) { // Create ServiceAccount for the kubernetes auth method if it doesn't exist, // otherwise, do nothing. - serviceAccountName := resourcePrefix + "-connect-injector" + serviceAccountName := resourcePrefix + "-authmethod" sa, _ := k8s.CoreV1().ServiceAccounts(namespace).Get(context.Background(), serviceAccountName, metav1.GetOptions{}) if sa == nil { // Create a service account that references two secrets. @@ -2820,7 +2820,7 @@ func setUpK8sServiceAccount(t *testing.T, k8s *fake.Clientset, namespace string) Name: resourcePrefix + "-some-other-secret", }, { - Name: resourcePrefix + "-connect-injector", + Name: resourcePrefix + "-authmethod", }, }, }, @@ -2835,7 +2835,7 @@ func setUpK8sServiceAccount(t *testing.T, k8s *fake.Clientset, namespace string) require.NoError(t, err) // Create a Kubernetes secret if it doesn't exist, otherwise update it - secretName := resourcePrefix + "-connect-injector" + secretName := resourcePrefix + "-authmethod" secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, diff --git a/control-plane/subcommand/server-acl-init/connect_inject.go b/control-plane/subcommand/server-acl-init/connect_inject.go index 8becfd5248..5f7d9a4c47 100644 --- a/control-plane/subcommand/server-acl-init/connect_inject.go +++ b/control-plane/subcommand/server-acl-init/connect_inject.go @@ -21,7 +21,7 @@ func (c *Command) configureConnectInjectAuthMethod(consulClient *api.Client, aut // Create the auth method template. This requires calls to the // kubernetes environment. - authMethodTmpl, err := c.createAuthMethodTmpl(authMethodName, c.withPrefix("connect-injector"), true) + authMethodTmpl, err := c.createAuthMethodTmpl(authMethodName, true) if err != nil { return err } @@ -80,10 +80,10 @@ func (c *Command) configureConnectInjectAuthMethod(consulClient *api.Client, aut // createAuthMethodTmpl sets up the auth method template based on the connect-injector's service account // jwt token. It is common for both the connect inject auth method and the component auth method // with the option to add namespace specific configuration to the auth method template via `useNS`. -func (c *Command) createAuthMethodTmpl(authMethodName, serviceAccountName string, useNS bool) (api.ACLAuthMethod, error) { +func (c *Command) createAuthMethodTmpl(authMethodName string, useNS bool) (api.ACLAuthMethod, error) { // Get the Secret name for the auth method ServiceAccount. var authMethodServiceAccount *apiv1.ServiceAccount - + serviceAccountName := c.withPrefix("authmethod") err := c.untilSucceeds(fmt.Sprintf("getting %s ServiceAccount", serviceAccountName), func() error { var err error diff --git a/control-plane/subcommand/server-acl-init/connect_inject_test.go b/control-plane/subcommand/server-acl-init/connect_inject_test.go index c82900cf94..8a9793610b 100644 --- a/control-plane/subcommand/server-acl-init/connect_inject_test.go +++ b/control-plane/subcommand/server-acl-init/connect_inject_test.go @@ -30,7 +30,7 @@ func TestCommand_createAuthMethodTmpl_SecretNotFound(t *testing.T) { ctx: ctx, } - serviceAccountName := resourcePrefix + "-connect-injector" + serviceAccountName := resourcePrefix + "-authmethod" secretName := resourcePrefix + "-connect-injector" // Create a service account referencing secretName @@ -64,6 +64,6 @@ func TestCommand_createAuthMethodTmpl_SecretNotFound(t *testing.T) { _, err := k8s.CoreV1().Secrets(ns).Create(ctx, secret, metav1.CreateOptions{}) require.NoError(t, err) - _, err = cmd.createAuthMethodTmpl("test", serviceAccountName, true) - require.EqualError(t, err, "found no secret of type 'kubernetes.io/service-account-token' associated with the release-name-consul-connect-injector service account") + _, err = cmd.createAuthMethodTmpl("test", true) + require.EqualError(t, err, "found no secret of type 'kubernetes.io/service-account-token' associated with the release-name-consul-authmethod service account") } From 5c0db173880664ae26663dbca1c873981ed9932b Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 7 Mar 2022 16:16:13 -0500 Subject: [PATCH 4/5] wip --- ...rrole.yaml => auth-method-clusterrole.yaml} | 4 ++-- ...aml => auth-method-clusterrolebinding.yaml} | 12 ++++++------ ...nt.yaml => auth-method-serviceaccount.yaml} | 4 ++-- .../consul/templates/server-acl-init-job.yaml | 2 +- .../consul/templates/server-acl-init-role.yaml | 2 +- ...rrole.bats => auth-method-clusterrole.bats} | 8 ++++---- ...ats => auth-method-clusterrolebinding.bats} | 8 ++++---- ...nt.bats => auth-method-serviceaccount.bats} | 12 ++++++------ .../consul/test/unit/server-acl-init-job.bats | 4 ++-- .../subcommand/server-acl-init/command.go | 10 +++++----- .../server-acl-init/command_ent_test.go | 6 +++--- .../subcommand/server-acl-init/command_test.go | 18 +++++++++--------- .../server-acl-init/connect_inject.go | 2 +- .../server-acl-init/connect_inject_test.go | 4 ++-- .../server-acl-init/create_or_update.go | 2 +- .../server-acl-init/create_or_update_test.go | 8 ++++---- 16 files changed, 53 insertions(+), 53 deletions(-) rename charts/consul/templates/{authmethod-clusterrole.yaml => auth-method-clusterrole.yaml} (81%) rename charts/consul/templates/{authmethod-clusterrolebinding.yaml => auth-method-clusterrolebinding.yaml} (76%) rename charts/consul/templates/{authmethod-serviceaccount.yaml => auth-method-serviceaccount.yaml} (83%) rename charts/consul/test/unit/{authmethod-clusterrole.bats => auth-method-clusterrole.bats} (55%) rename charts/consul/test/unit/{authmethod-clusterrolebinding.bats => auth-method-clusterrolebinding.bats} (52%) rename charts/consul/test/unit/{authmethod-serviceaccount.bats => auth-method-serviceaccount.bats} (70%) diff --git a/charts/consul/templates/authmethod-clusterrole.yaml b/charts/consul/templates/auth-method-clusterrole.yaml similarity index 81% rename from charts/consul/templates/authmethod-clusterrole.yaml rename to charts/consul/templates/auth-method-clusterrole.yaml index 30385aa25f..6b8f2c5451 100644 --- a/charts/consul/templates/authmethod-clusterrole.yaml +++ b/charts/consul/templates/auth-method-clusterrole.yaml @@ -2,13 +2,13 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - name: {{ template "consul.fullname" . }}-authmethod + name: {{ template "consul.fullname" . }}-auth-method labels: app: {{ template "consul.name" . }} chart: {{ template "consul.chart" . }} heritage: {{ .Release.Service }} release: {{ .Release.Name }} - component: authmethod + component: auth-method rules: - apiGroups: [ "" ] resources: diff --git a/charts/consul/templates/authmethod-clusterrolebinding.yaml b/charts/consul/templates/auth-method-clusterrolebinding.yaml similarity index 76% rename from charts/consul/templates/authmethod-clusterrolebinding.yaml rename to charts/consul/templates/auth-method-clusterrolebinding.yaml index 89bc44dea4..9bd6c64113 100644 --- a/charts/consul/templates/authmethod-clusterrolebinding.yaml +++ b/charts/consul/templates/auth-method-clusterrolebinding.yaml @@ -8,32 +8,32 @@ metadata: chart: {{ template "consul.chart" . }} heritage: {{ .Release.Service }} release: {{ .Release.Name }} - component: authmethod + component: auth-method roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole name: "system:auth-delegator" subjects: - kind: ServiceAccount - name: {{ template "consul.fullname" . }}-authmethod + name: {{ template "consul.fullname" . }}-auth-method namespace: {{ .Release.Namespace }} --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: - name: {{ template "consul.fullname" . }}-authmethod + name: {{ template "consul.fullname" . }}-auth-method labels: app: {{ template "consul.name" . }} chart: {{ template "consul.chart" . }} heritage: {{ .Release.Service }} release: {{ .Release.Name }} - component: authmethod + component: auth-method roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: {{ template "consul.fullname" . }}-authmethod + name: {{ template "consul.fullname" . }}-auth-method subjects: - kind: ServiceAccount - name: {{ template "consul.fullname" . }}-authmethod + name: {{ template "consul.fullname" . }}-auth-method namespace: {{ .Release.Namespace }} {{- end }} diff --git a/charts/consul/templates/authmethod-serviceaccount.yaml b/charts/consul/templates/auth-method-serviceaccount.yaml similarity index 83% rename from charts/consul/templates/authmethod-serviceaccount.yaml rename to charts/consul/templates/auth-method-serviceaccount.yaml index bfb50dc0dd..098339b8c8 100644 --- a/charts/consul/templates/authmethod-serviceaccount.yaml +++ b/charts/consul/templates/auth-method-serviceaccount.yaml @@ -2,14 +2,14 @@ apiVersion: v1 kind: ServiceAccount metadata: - name: {{ template "consul.fullname" . }}-authmethod + name: {{ template "consul.fullname" . }}-auth-method namespace: {{ .Release.Namespace }} labels: app: {{ template "consul.name" . }} chart: {{ template "consul.chart" . }} heritage: {{ .Release.Service }} release: {{ .Release.Name }} - component: authmethod + component: auth-method {{- with .Values.global.imagePullSecrets }} imagePullSecrets: {{- range . }} diff --git a/charts/consul/templates/server-acl-init-job.yaml b/charts/consul/templates/server-acl-init-job.yaml index 48d38e6ac6..04f4e2de3e 100644 --- a/charts/consul/templates/server-acl-init-job.yaml +++ b/charts/consul/templates/server-acl-init-job.yaml @@ -169,7 +169,7 @@ spec: {{- end }} {{- if .Values.syncCatalog.enabled }} - -create-sync-policy=true \ + -sync-catalog=true \ {{- if .Values.syncCatalog.consulNodeName }} -sync-consul-node-name={{ .Values.syncCatalog.consulNodeName }} \ {{- end }} diff --git a/charts/consul/templates/server-acl-init-role.yaml b/charts/consul/templates/server-acl-init-role.yaml index 9e38f55310..eb7b6a928e 100644 --- a/charts/consul/templates/server-acl-init-role.yaml +++ b/charts/consul/templates/server-acl-init-role.yaml @@ -23,7 +23,7 @@ rules: resources: - serviceaccounts resourceNames: - - {{ template "consul.fullname" . }}-authmethod + - {{ template "consul.fullname" . }}-auth-method verbs: - get {{- if .Values.global.enablePodSecurityPolicies }} diff --git a/charts/consul/test/unit/authmethod-clusterrole.bats b/charts/consul/test/unit/auth-method-clusterrole.bats similarity index 55% rename from charts/consul/test/unit/authmethod-clusterrole.bats rename to charts/consul/test/unit/auth-method-clusterrole.bats index 0888ea373b..935a448161 100644 --- a/charts/consul/test/unit/authmethod-clusterrole.bats +++ b/charts/consul/test/unit/auth-method-clusterrole.bats @@ -2,17 +2,17 @@ load _helpers -@test "authmethod/ClusterRole: disabled by default" { +@test "auth-method/ClusterRole: disabled by default" { cd `chart_dir` assert_empty helm template \ - -s templates/authmethod-clusterrole.yaml \ + -s templates/auth-method-clusterrole.yaml \ . } -@test "authmethod/ClusterRole: enabled with global.acls.manageSystemACLs true" { +@test "auth-method/ClusterRole: enabled with global.acls.manageSystemACLs true" { cd `chart_dir` local actual=$(helm template \ - -s templates/authmethod-clusterrole.yaml \ + -s templates/auth-method-clusterrole.yaml \ --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | yq -s 'length > 0' | tee /dev/stderr) diff --git a/charts/consul/test/unit/authmethod-clusterrolebinding.bats b/charts/consul/test/unit/auth-method-clusterrolebinding.bats similarity index 52% rename from charts/consul/test/unit/authmethod-clusterrolebinding.bats rename to charts/consul/test/unit/auth-method-clusterrolebinding.bats index 21b96d0bbb..dcb293ba14 100644 --- a/charts/consul/test/unit/authmethod-clusterrolebinding.bats +++ b/charts/consul/test/unit/auth-method-clusterrolebinding.bats @@ -2,17 +2,17 @@ load _helpers -@test "authmethod/ClusterRoleBinding: disabled by default" { +@test "auth-method/ClusterRoleBinding: disabled by default" { cd `chart_dir` assert_empty helm template \ - -s templates/authmethod-clusterrolebinding.yaml \ + -s templates/auth-method-clusterrolebinding.yaml \ . } -@test "authmethod/ClusterRoleBinding: enabled with global.acls.manageSystemACLs true" { +@test "auth-method/ClusterRoleBinding: enabled with global.acls.manageSystemACLs true" { cd `chart_dir` local actual=$(helm template \ - -s templates/authmethod-clusterrolebinding.yaml \ + -s templates/auth-method-clusterrolebinding.yaml \ --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | yq -s 'length > 0' | tee /dev/stderr) diff --git a/charts/consul/test/unit/authmethod-serviceaccount.bats b/charts/consul/test/unit/auth-method-serviceaccount.bats similarity index 70% rename from charts/consul/test/unit/authmethod-serviceaccount.bats rename to charts/consul/test/unit/auth-method-serviceaccount.bats index 8cf9633cae..9413a03291 100644 --- a/charts/consul/test/unit/authmethod-serviceaccount.bats +++ b/charts/consul/test/unit/auth-method-serviceaccount.bats @@ -2,17 +2,17 @@ load _helpers -@test "authMethod/ServiceAccount: disabled by default" { +@test "auth-method/ServiceAccount: disabled by default" { cd `chart_dir` assert_empty helm template \ - -s templates/authmethod-serviceaccount.yaml \ + -s templates/auth-method-serviceaccount.yaml \ . } -@test "authMethod/ServiceAccount: enabled with global.acls.manageSystemACLs.enabled true" { +@test "auth-method/ServiceAccount: enabled with global.acls.manageSystemACLs.enabled true" { cd `chart_dir` local actual=$(helm template \ - -s templates/authmethod-serviceaccount.yaml \ + -s templates/auth-method-serviceaccount.yaml \ --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | yq -s 'length > 0' | tee /dev/stderr) @@ -22,10 +22,10 @@ load _helpers #-------------------------------------------------------------------- # global.imagePullSecrets -@test "authMethod/ServiceAccount: can set image pull secrets" { +@test "auth-method/ServiceAccount: can set image pull secrets" { cd `chart_dir` local object=$(helm template \ - -s templates/authmethod-serviceaccount.yaml \ + -s templates/auth-method-serviceaccount.yaml \ --set 'global.acls.manageSystemACLs=true' \ --set 'global.imagePullSecrets[0].name=my-secret' \ --set 'global.imagePullSecrets[1].name=my-secret2' \ diff --git a/charts/consul/test/unit/server-acl-init-job.bats b/charts/consul/test/unit/server-acl-init-job.bats index 93b4e8429e..5cbd5a7b8f 100644 --- a/charts/consul/test/unit/server-acl-init-job.bats +++ b/charts/consul/test/unit/server-acl-init-job.bats @@ -249,7 +249,7 @@ load _helpers -s templates/server-acl-init-job.yaml \ --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | - yq '.spec.template.spec.containers[0].command | any(contains("-create-sync-policy"))' | tee /dev/stderr) + yq '.spec.template.spec.containers[0].command | any(contains("-sync-catalog"))' | tee /dev/stderr) [ "${actual}" = "false" ] } @@ -260,7 +260,7 @@ load _helpers --set 'global.acls.manageSystemACLs=true' \ --set 'syncCatalog.enabled=true' \ . | tee /dev/stderr | - yq '.spec.template.spec.containers[0].command | any(contains("-create-sync-policy"))' | tee /dev/stderr) + yq '.spec.template.spec.containers[0].command | any(contains("-sync-catalog"))' | tee /dev/stderr) [ "${actual}" = "true" ] } diff --git a/control-plane/subcommand/server-acl-init/command.go b/control-plane/subcommand/server-acl-init/command.go index c6abed92cf..82d35193d3 100644 --- a/control-plane/subcommand/server-acl-init/command.go +++ b/control-plane/subcommand/server-acl-init/command.go @@ -42,7 +42,7 @@ type Command struct { flagCreateClientToken bool - flagCreateSyncPolicy bool + flagSyncCatalog bool flagSyncConsulNodeName string flagConnectInject bool @@ -126,7 +126,7 @@ func (c *Command) init() { c.flags.BoolVar(&c.flagCreateClientToken, "create-client-token", true, "Toggle for creating a client agent token. Default is true.") - c.flags.BoolVar(&c.flagCreateSyncPolicy, "create-sync-policy", false, + c.flags.BoolVar(&c.flagSyncCatalog, "sync-catalog", false, "Toggle for creating a catalog sync policy.") c.flags.StringVar(&c.flagSyncConsulNodeName, "sync-consul-node-name", "k8s-sync", "The Consul node name to register for catalog sync. Defaults to k8s-sync. To be discoverable "+ @@ -481,7 +481,7 @@ func (c *Command) Run(args []string) int { } } - if c.flagCreateSyncPolicy { + if c.flagSyncCatalog { syncRules, err := c.syncRules() if err != nil { c.log.Error("Error templating sync rules", "err", err) @@ -499,9 +499,9 @@ func (c *Command) Run(args []string) int { if !primary { componentAuthMethodName = globalComponentAuthMethodName } - err = c.createACLPolicyRoleAndBindingRule("sync-catalog", syncRules, consulDC, primaryDC, globalToken, primary, componentAuthMethodName, serviceAccountName, consulClient) + err = c.createACLPolicyRoleAndBindingRule("sync-catalog", syncRules, consulDC, primaryDC, globalPolicy, primary, componentAuthMethodName, serviceAccountName, consulClient) } else { - err = c.createACLPolicyRoleAndBindingRule("sync-catalog", syncRules, consulDC, primaryDC, localToken, primary, componentAuthMethodName, serviceAccountName, consulClient) + err = c.createACLPolicyRoleAndBindingRule("sync-catalog", syncRules, consulDC, primaryDC, localPolicy, primary, componentAuthMethodName, serviceAccountName, consulClient) } if err != nil { c.log.Error(err.Error()) diff --git a/control-plane/subcommand/server-acl-init/command_ent_test.go b/control-plane/subcommand/server-acl-init/command_ent_test.go index 270893e765..a7e3f71e51 100644 --- a/control-plane/subcommand/server-acl-init/command_ent_test.go +++ b/control-plane/subcommand/server-acl-init/command_ent_test.go @@ -287,7 +287,7 @@ func TestRun_ACLPolicyUpdates(t *testing.T) { "-create-client-token", "-allow-dns", "-create-mesh-gateway-token", - "-create-sync-policy", + "-sync-catalog", "-connect-inject", "-create-snapshot-agent-token", "-create-enterprise-license-token", @@ -1073,7 +1073,7 @@ func TestRun_NamespaceEnabled_ValidateLoginToken_PrimaryDatacenter(t *testing.T) }, { ComponentName: "sync-catalog", - TokenFlags: []string{"-create-sync-policy"}, + TokenFlags: []string{"-sync-catalog"}, Roles: []string{resourcePrefix + "-sync-catalog-acl-role"}, Namespace: ns, GlobalToken: false, @@ -1164,7 +1164,7 @@ func TestRun_NamespaceEnabled_ValidateLoginToken_SecondaryDatacenter(t *testing. }, { ComponentName: "sync-catalog", - TokenFlags: []string{"-create-sync-policy"}, + TokenFlags: []string{"-sync-catalog"}, Roles: []string{resourcePrefix + "-sync-catalog-acl-role-dc2"}, Namespace: ns, GlobalToken: true, diff --git a/control-plane/subcommand/server-acl-init/command_test.go b/control-plane/subcommand/server-acl-init/command_test.go index 3f1408581e..50dd929fe9 100644 --- a/control-plane/subcommand/server-acl-init/command_test.go +++ b/control-plane/subcommand/server-acl-init/command_test.go @@ -1013,7 +1013,7 @@ func TestRun_SyncPolicyUpdates(t *testing.T) { "-k8s-namespace=" + ns, "-server-address", strings.Split(testSvr.HTTPAddr, ":")[0], "-server-port", strings.Split(testSvr.HTTPAddr, ":")[1], - "-create-sync-policy", + "-sync-catalog", } firstRunArgs := append(commonArgs, "-sync-consul-node-name=k8s-sync", @@ -1122,7 +1122,7 @@ func TestRun_ErrorsOnDuplicateACLPolicy(t *testing.T) { "-k8s-namespace=" + ns, "-server-address", strings.Split(testAgent.HTTPAddr, ":")[0], "-server-port", strings.Split(testAgent.HTTPAddr, ":")[1], - "-create-sync-policy", + "-sync-catalog", } responseCode := cmd.Run(cmdArgs) @@ -2159,7 +2159,7 @@ func TestRun_PoliciesAndBindingRulesForACLLogin_PrimaryDatacenter(t *testing.T) }, { TestName: "Sync Catalog", - TokenFlags: []string{"-create-sync-policy"}, + TokenFlags: []string{"-sync-catalog"}, PolicyNames: []string{"sync-catalog-policy"}, Roles: []string{resourcePrefix + "-sync-catalog-acl-role"}, }, @@ -2269,7 +2269,7 @@ func TestRun_PoliciesAndBindingRulesACLLogin_SecondaryDatacenter(t *testing.T) { }, { TestName: "Sync Catalog", - TokenFlags: []string{"-create-sync-policy"}, + TokenFlags: []string{"-sync-catalog"}, PolicyNames: []string{"sync-catalog-policy-" + secondaryDatacenter}, Roles: []string{resourcePrefix + "-sync-catalog-acl-role-" + secondaryDatacenter}, GlobalAuthMethod: false, @@ -2377,7 +2377,7 @@ func TestRun_ValidateLoginToken_PrimaryDatacenter(t *testing.T) { }, { ComponentName: "sync-catalog", - TokenFlags: []string{"-create-sync-policy"}, + TokenFlags: []string{"-sync-catalog"}, Roles: []string{resourcePrefix + "-sync-catalog-acl-role"}, }, } @@ -2470,7 +2470,7 @@ func TestRun_ValidateLoginToken_SecondaryDatacenter(t *testing.T) { }, { ComponentName: "sync-catalog", - TokenFlags: []string{"-create-sync-policy"}, + TokenFlags: []string{"-sync-catalog"}, Roles: []string{resourcePrefix + "-sync-catalog-acl-role-dc2"}, GlobalAuthMethod: false, }, @@ -2803,7 +2803,7 @@ func getBootToken(t *testing.T, k8s *fake.Clientset, prefix string, k8sNamespace func setUpK8sServiceAccount(t *testing.T, k8s *fake.Clientset, namespace string) (string, string) { // Create ServiceAccount for the kubernetes auth method if it doesn't exist, // otherwise, do nothing. - serviceAccountName := resourcePrefix + "-authmethod" + serviceAccountName := resourcePrefix + "-auth-method" sa, _ := k8s.CoreV1().ServiceAccounts(namespace).Get(context.Background(), serviceAccountName, metav1.GetOptions{}) if sa == nil { // Create a service account that references two secrets. @@ -2820,7 +2820,7 @@ func setUpK8sServiceAccount(t *testing.T, k8s *fake.Clientset, namespace string) Name: resourcePrefix + "-some-other-secret", }, { - Name: resourcePrefix + "-authmethod", + Name: resourcePrefix + "-auth-method", }, }, }, @@ -2835,7 +2835,7 @@ func setUpK8sServiceAccount(t *testing.T, k8s *fake.Clientset, namespace string) require.NoError(t, err) // Create a Kubernetes secret if it doesn't exist, otherwise update it - secretName := resourcePrefix + "-authmethod" + secretName := resourcePrefix + "-auth-method" secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, diff --git a/control-plane/subcommand/server-acl-init/connect_inject.go b/control-plane/subcommand/server-acl-init/connect_inject.go index 5f7d9a4c47..64666b01d2 100644 --- a/control-plane/subcommand/server-acl-init/connect_inject.go +++ b/control-plane/subcommand/server-acl-init/connect_inject.go @@ -83,7 +83,7 @@ func (c *Command) configureConnectInjectAuthMethod(consulClient *api.Client, aut func (c *Command) createAuthMethodTmpl(authMethodName string, useNS bool) (api.ACLAuthMethod, error) { // Get the Secret name for the auth method ServiceAccount. var authMethodServiceAccount *apiv1.ServiceAccount - serviceAccountName := c.withPrefix("authmethod") + serviceAccountName := c.withPrefix("auth-method") err := c.untilSucceeds(fmt.Sprintf("getting %s ServiceAccount", serviceAccountName), func() error { var err error diff --git a/control-plane/subcommand/server-acl-init/connect_inject_test.go b/control-plane/subcommand/server-acl-init/connect_inject_test.go index 8a9793610b..959f02e178 100644 --- a/control-plane/subcommand/server-acl-init/connect_inject_test.go +++ b/control-plane/subcommand/server-acl-init/connect_inject_test.go @@ -30,7 +30,7 @@ func TestCommand_createAuthMethodTmpl_SecretNotFound(t *testing.T) { ctx: ctx, } - serviceAccountName := resourcePrefix + "-authmethod" + serviceAccountName := resourcePrefix + "-auth-method" secretName := resourcePrefix + "-connect-injector" // Create a service account referencing secretName @@ -65,5 +65,5 @@ func TestCommand_createAuthMethodTmpl_SecretNotFound(t *testing.T) { require.NoError(t, err) _, err = cmd.createAuthMethodTmpl("test", true) - require.EqualError(t, err, "found no secret of type 'kubernetes.io/service-account-token' associated with the release-name-consul-authmethod service account") + require.EqualError(t, err, "found no secret of type 'kubernetes.io/service-account-token' associated with the release-name-consul-auth-method service account") } diff --git a/control-plane/subcommand/server-acl-init/create_or_update.go b/control-plane/subcommand/server-acl-init/create_or_update.go index 1c869b11bf..291eead327 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update.go +++ b/control-plane/subcommand/server-acl-init/create_or_update.go @@ -313,7 +313,7 @@ func (c *Command) createOrUpdateACLPolicy(policy api.ACLPolicy, consulClient *ap // Allowing the Consul node name to be configurable also requires any sync // policy to be updated in case the node name has changed. if isPolicyExistsErr(err, policy.Name) { - if c.flagEnableNamespaces || c.flagCreateSyncPolicy { + if c.flagEnableNamespaces || c.flagSyncCatalog { c.log.Info(fmt.Sprintf("Policy %q already exists, updating", policy.Name)) // The policy ID is required in any PolicyUpdate call, so first we need to diff --git a/control-plane/subcommand/server-acl-init/create_or_update_test.go b/control-plane/subcommand/server-acl-init/create_or_update_test.go index a9e8f0704a..5cd01fac25 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update_test.go +++ b/control-plane/subcommand/server-acl-init/create_or_update_test.go @@ -20,10 +20,10 @@ func TestCreateOrUpdateACLPolicy_ErrorsIfDescriptionDoesNotMatch(t *testing.T) { ui := cli.NewMockUi() k8s := fake.NewSimpleClientset() cmd := Command{ - UI: ui, - clientset: k8s, - log: hclog.NewNullLogger(), - flagCreateSyncPolicy: true, + UI: ui, + clientset: k8s, + log: hclog.NewNullLogger(), + flagSyncCatalog: true, } // Start Consul. From f12995454986777bac5f9e2829cd6e7348b91ea3 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Tue, 8 Mar 2022 16:46:47 -0500 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Iryna Shustava --- control-plane/subcommand/server-acl-init/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/subcommand/server-acl-init/command.go b/control-plane/subcommand/server-acl-init/command.go index 82d35193d3..b98a84e859 100644 --- a/control-plane/subcommand/server-acl-init/command.go +++ b/control-plane/subcommand/server-acl-init/command.go @@ -493,7 +493,7 @@ func (c *Command) Run(args []string) int { // If namespaces are enabled, the policy and token need to be global to be allowed to create namespaces. if c.flagEnableNamespaces { - // Create the catalog sync ACL Policy, Role and BindingRule but do not issue any ACLTokens or create Kube Secrets. + // Create the catalog sync ACL Policy, Role and BindingRule. // SyncCatalog token must be global when namespaces are enabled. This means secondary datacenters need // a token that is known by the primary datacenters. if !primary {