Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix consul-telemetry-collector deployment templates #3184

Merged
merged 6 commits into from
Nov 8, 2023

Conversation

jjti
Copy link
Contributor

@jjti jjti commented Nov 8, 2023

Changes proposed in this PR:

  • consul-telemetry-collector deployment: remove foo arg
  • consul-telemetry-collector deployment: remove the quotes around "default" auth login namespace that break auth

How I've tested this PR:

I reproduced the behavior described by @david-yu

I did a Helm install using a template that includes global.acls.manageSystemACLs=true: https://github.com/jjti/consul-experiments/blob/main/telemetry-collector/helm/consul.yaml

I kubectl apply'ed a manually created deployment (based on a fixed version of helm's output): https://github.com/jjti/consul-experiments/blame/e2da8fe2d768472b549780f8eea2cc10e7da04e4/telemetry-collector/resources/consul-telemetry-collector.yaml#L231

I saw the telemetry collector start and push metrics:

# josh @ josh-XWGWHJ0JV2 in ~/GitHub/jjti/consul-exp/telemetry-collector on git:main x [21:21:02]
$ k get pods
NAME                                           READY   STATUS    RESTARTS   AGE
consul-connect-injector-74d6fb76b8-87pck       1/1     Running   0          15m
consul-server-0                                1/1     Running   0          15m
consul-telemetry-collector-9d858d459-5c6gq     2/2     Running   0          7m47s
consul-terminating-gateway-7f5877fdb8-rgtcj    1/1     Running   0          15m
consul-webhook-cert-manager-7c5bc5fb59-ln2m4   1/1     Running   0          15m
prometheus-server-695744d587-k9k6l             2/2     Running   0          15m

I also ran the bats tests locally:

$ bats --jobs 4 charts/consul/test/unit/telemetry-collector-deployment.bats 
telemetry-collector-deployment.bats
70 tests, 0 failures

$ bats --jobs 4 charts/consul/test/unit/telemetry-collector-v2-deployment.bats
telemetry-collector-v2-deployment.bats
69 tests, 0 failures

How I expect reviewers to test this PR:

Checklist:

@jjti jjti requested a review from david-yu November 8, 2023 02:38
@jjti jjti added backport/1.1.x Backport to release/1.1.x branch backport/1.2.x This release branch is no longer active. backport/1.3.x type/bug Something isn't working theme/consul-namespaces labels Nov 8, 2023
@jjti jjti marked this pull request as ready for review November 8, 2023 02:42
.changelog/3184.txt Outdated Show resolved Hide resolved
Co-authored-by: Dan Stough <[email protected]>
@jjti jjti enabled auto-merge (squash) November 8, 2023 21:02
@jjti jjti merged commit a3d1715 into main Nov 8, 2023
24 of 48 checks passed
@jjti jjti deleted the jjtimmons/fix-ctc-with-acls branch November 8, 2023 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.1.x Backport to release/1.1.x branch backport/1.2.x This release branch is no longer active. backport/1.3.x theme/consul-namespaces type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants