Update server-acl-init to always check for the deployed serviceAccountToken secret #1770

kschoche · 2022-12-05T19:19:05Z

Changes proposed in this PR:

OpenShift injects SecretRefs into ServiceAccounts (one for service account and the other for docker registry credentials), even in Kubernetes 1.24+. In the current logic of server-acl-init we expect to use the Secret deployed by the helm chart, but only in the case where the ServiceAccount does not contain SecretRefs. Due to OpenShift injecting these we never look for the deployed Secret for the consul-auth-method.
Since the helm chart and consul-k8s versions are sync'd and we do not have backward compatibility issues we should be able to rely on the consul auth method Secret always existing and we should use that.
I believe this should resolve Consul-server-acl-init found no secret of type 'kubernetes.io/service-account-token' associated with the consul-auth-method service account #1768.

How I've tested this PR:
Unit + acceptance tests should pass.

How I expect reviewers to test this PR:
👀
Looking for some review comments about the approach as well :)

Checklist:

Tests added
CHANGELOG entry added

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

kschoche · 2022-12-15T20:28:09Z

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

+	// In Kube 1.24+ there is no automatically generated long term JWT token for a ServiceAccount.
+	// Furthermore, there is no reference to a Secret in the ServiceAccount. Instead we have deployed
+	// a Secret in Helm which references the ServiceAccount and contains a permanent JWT token.
+	secretNames = append(secretNames, c.withPrefix("auth-method"))


I'm wondering about the order here for the Openshift case where it injects secrets into the ServiceAccount.
Are those supposed to hold the JWT token that we're supposed to use? if so I think we just need to re-order these four lines so that we attempt to read the Helm deployed Secret last?

thisisnotashwin · 2022-12-21T16:26:57Z

Have we been able to reproduce the associated issue and resolve it with this fix?

rgish · 2023-01-17T12:43:40Z

Any movement on 1 approving review remaining?

david-yu · 2023-01-17T16:55:13Z

@rgish Would you be able to test this PR on your OpenShift issue to tell us whether it resolves the issue you filed? #1768

rgish · 2023-01-17T17:27:50Z

@rgish Would you be able to test this PR on your OpenShift issue to tell us whether it resolves the issue you filed? #1768

If you can direct me to explicit instructions on how to do so (ideally via Helm Charts or oc apply commands), I would be very willing to try it out!

david-yu · 2023-01-17T22:58:56Z

@rgish You should be able to make and run your own consul k8s images using the guide described here: https://github.com/hashicorp/consul-k8s/blob/main/CONTRIBUTING.md#contributing-101

rgish · 2023-02-14T00:28:07Z

@rgish You should be able to make and run your own consul k8s images using the guide described here: https://github.com/hashicorp/consul-k8s/blob/main/CONTRIBUTING.md#contributing-101

Receive the following error in api-gateway-controller-acl-init:

2023-02-14T00:13:21.897Z [ERROR] unable to login: error="Post "https://10.121.11.147:8501/v1/acl/login?dc=dc1\": x509: certificate signed by unknown authority"

But, server-acl-init-job does not display any errors in our OpenShift environment, so the commit should seem to be considered an improvement:

2023-02-14T00:04:03.508Z [INFO] Success: getting consul-auth-method ServiceAccount
2023-02-14T00:04:03.511Z [INFO] Success: getting consul-auth-method Secret
2023-02-14T00:04:03.514Z [INFO] Success: creating auth method consul-k8s-auth-method
2023-02-14T00:04:03.514Z [INFO] creating inject binding rule
2023-02-14T00:04:03.515Z [INFO] Success: listing binding rules for auth method consul-k8s-auth-method
2023-02-14T00:04:03.516Z [INFO] Success: creating acl binding rule for consul-k8s-auth-method
2023-02-14T00:04:03.518Z [INFO] Success: creating connect-inject-policy policy
2023-02-14T00:04:03.518Z [INFO] Success: update or create acl role for consul-connect-inject-acl-role
2023-02-14T00:04:03.519Z [INFO] Success: listing binding rules for auth method consul-k8s-component-auth-method
2023-02-14T00:04:03.519Z [INFO] unable to find a matching ACL binding rule to update. creating ACL binding rule.
2023-02-14T00:04:03.521Z [INFO] Success: creating acl binding rule for consul-k8s-component-auth-method
2023-02-14T00:04:03.521Z [INFO] Success: creating api-gateway-controller-policy policy
2023-02-14T00:04:03.522Z [INFO] Success: update or create acl role for consul-api-gateway-controller-acl-role
2023-02-14T00:04:03.523Z [INFO] Success: listing binding rules for auth method consul-k8s-component-auth-method
2023-02-14T00:04:03.523Z [INFO] unable to find a matching ACL binding rule to update. creating ACL binding rule.
2023-02-14T00:04:03.523Z [INFO] Success: creating acl binding rule for consul-k8s-component-auth-method
2023-02-14T00:04:03.523Z [INFO] server-acl-init completed successfully
2023-02-14T00:04:03.523Z [INFO] consul-server-connection-manager: stopping

kschoche · 2023-02-14T00:33:35Z

@david-yu - might be worth rebasing this PR off main since some related PRs have closed #1881 to see if it works?

* Exclude openebs namespace from injection. OpenEBS is a Kubernetes storage solution. When you spin up a PVC, under the hood OpenEBS creates a pod to handle the necessary storage operations. If the openebs namespace is not excluded from injection, that pod can't start because our mutatingwebhook config requires all pod scheduling requests make it to our webhook and our webhook isn't running yet because the consul servers aren't running. This is a breaking change but I think it's worth it because it's very unlikely anyone is using the openebs namespace for anything other than openebs. * Changelog

The generate_lease=true configuration is unnecessary and generates a note about performance implications in Vault logs. Remove this configuration so that the default value of generate_lease=false is used instead.

* Remove gnupg * Update CHANGELOG.md

* Dockerfile: remove `gnupg` from dev image * Update CHANGELOG.md

- Add troubleshooting commands for 'upstream' and 'proxy' to allow troubleshooting of envoy config.

Move format parsing into envoy package Move enovy to common package, move param parsing to calling package use a LoggerParams struct for handling a format for log changes to envoy refactor to use logger params and methods to set and validate logger and log levels before calling envoy linting changes clean up from rebase Improve comment on envoy logging endpoint function, switched to using '-update-level' for updating envoy log level flag for better usability

* pinned consul test image to latest dev nightly - created a new variable for setting the consul test image used for the majority of tests - fixed some incorrect spacing * updated builtin lambda envoy extensions in tests

david-yu · 2023-02-14T02:38:46Z

Looks like all test passed. @thisisnotashwin could you have another look?

thisisnotashwin

Excellent!! im glad these worked!

…tToken secret (#1770)

…tToken secret (#1770) (#1907) Co-authored-by: Kyle Schochenmaier <[email protected]>

…tToken secret (#1770)

test because I cant get tests to pass locally

fed11b7

kschoche changed the title ~~[ignore] test because I cant get tests to pass locally~~ [wip] Update server-acl-init to always check for the deployed serviceAccountToken secret Dec 5, 2022

kschoche self-assigned this Dec 12, 2022

kschoche added the theme/openshift label Dec 12, 2022

kschoche commented Dec 15, 2022

View reviewed changes

kschoche changed the title ~~[wip] Update server-acl-init to always check for the deployed serviceAccountToken secret~~ Update server-acl-init to always check for the deployed serviceAccountToken secret Dec 15, 2022

kschoche marked this pull request as ready for review December 15, 2022 20:30

kschoche requested review from a team, analogue and ishustava and removed request for a team December 15, 2022 21:27

david-yu requested review from a team, malizz and erichaberkorn and removed request for analogue, ishustava, a team and malizz December 16, 2022 18:25

kschoche requested a review from thisisnotashwin December 20, 2022 21:41

curtbushko and others added 13 commits February 13, 2023 16:48

missing a line in the changelog (#1868)

95e4b97

feat: add peer to service-defaults overrides

99c68b3

chore: fix generation for peering CRDs

146e3f9

Refine server TLS Vault PKI role config (#1877)

2d2a763

The generate_lease=true configuration is unnecessary and generates a note about performance implications in Vault logs. Remove this configuration so that the default value of generate_lease=false is used instead.

Dockerfile: Remove gnupg from Consul k8s container (#1882)

b01ebd4

* Remove gnupg * Update CHANGELOG.md

Dockerfile: remove gnupg from dev image (#1885)

4b7e240

* Dockerfile: remove `gnupg` from dev image * Update CHANGELOG.md

NET-1750 Fixes ACL init command Consul Login Datacenter mixup (#1881)

74849a0

Minor fixes to docs for acceptance tests (#1895)

ff5b17e

Service to service troubleshooting (#1851)

a595d01

- Add troubleshooting commands for 'upstream' and 'proxy' to allow troubleshooting of envoy config.

pinned consul test image to latest dev nightly (#1901)

a8f6337

* pinned consul test image to latest dev nightly - created a new variable for setting the consul test image used for the majority of tests - fixed some incorrect spacing * updated builtin lambda envoy extensions in tests

reset to original commit

38190ce

david-yu added the backport/1.0.x label Feb 14, 2023

thisisnotashwin approved these changes Feb 14, 2023

View reviewed changes

Add Changelog

1940e96

thisisnotashwin force-pushed the test_fix_server_acl_init_openshift branch from 59af407 to 1940e96 Compare February 14, 2023 16:20

thisisnotashwin merged commit 22bb4e2 into main Feb 14, 2023

thisisnotashwin deleted the test_fix_server_acl_init_openshift branch February 14, 2023 16:21

thisisnotashwin mentioned this pull request Feb 14, 2023

Backport of Update server-acl-init to always check for the deployed serviceAccountToken secret into release/1.0.x #1905

Closed

2 tasks

thisisnotashwin pushed a commit that referenced this pull request Feb 14, 2023

Update server-acl-init to always check for the deployed serviceAccoun…

c12b3e7

…tToken secret (#1770)

thisisnotashwin mentioned this pull request Feb 14, 2023

server-acl-init to always check for the deployed serviceAccountToken secret (#1770) #1907

Merged

thisisnotashwin added a commit that referenced this pull request Feb 14, 2023

Update server-acl-init to always check for the deployed serviceAccoun…

832eb4c

…tToken secret (#1770) (#1907) Co-authored-by: Kyle Schochenmaier <[email protected]>

david-yu pushed a commit that referenced this pull request Feb 14, 2023

move GH-1770 to unreleased

fdb7631

david-yu mentioned this pull request Feb 14, 2023

CHANGELOG: move GH-1770 to unreleased #1909

Merged

2 tasks

david-yu pushed a commit that referenced this pull request Feb 15, 2023

move GH-1770 to unreleased (#1909)

6655508

ishustava pushed a commit that referenced this pull request Feb 18, 2023

Update server-acl-init to always check for the deployed serviceAccoun…

66ed075

…tToken secret (#1770)

ishustava pushed a commit that referenced this pull request Feb 18, 2023

move GH-1770 to unreleased (#1909)

bf4ec3f

natitomattis mentioned this pull request Jun 20, 2023

INFRA 11282 team-telnyx/consul-k8s#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update server-acl-init to always check for the deployed serviceAccountToken secret #1770

Update server-acl-init to always check for the deployed serviceAccountToken secret #1770

kschoche commented Dec 5, 2022 •

edited

Loading

kschoche Dec 15, 2022

thisisnotashwin commented Dec 21, 2022

rgish commented Jan 17, 2023

david-yu commented Jan 17, 2023

rgish commented Jan 17, 2023

david-yu commented Jan 17, 2023

rgish commented Feb 14, 2023

kschoche commented Feb 14, 2023

david-yu commented Feb 14, 2023

thisisnotashwin left a comment

Update server-acl-init to always check for the deployed serviceAccountToken secret #1770

Update server-acl-init to always check for the deployed serviceAccountToken secret #1770

Conversation

kschoche commented Dec 5, 2022 • edited Loading

kschoche Dec 15, 2022

Choose a reason for hiding this comment

thisisnotashwin commented Dec 21, 2022

rgish commented Jan 17, 2023

david-yu commented Jan 17, 2023

rgish commented Jan 17, 2023

david-yu commented Jan 17, 2023

rgish commented Feb 14, 2023

kschoche commented Feb 14, 2023

david-yu commented Feb 14, 2023

thisisnotashwin left a comment

Choose a reason for hiding this comment

kschoche commented Dec 5, 2022 •

edited

Loading