[helm] Add Loki Canary to Helm Chart #7173

trevorwhitney · 2022-09-15T16:51:10Z

What this PR does / why we need it:

This PR adds the Loki canary as well as the GEL provisioner to the Loki helm chart. The Loki canary expands the self-monitoring functionality of the Helm chart, giving operators insight beyond whether Loki is just running, but also if it's functioning properly.

The GEL provisioner was necessary for the canary to work when the enterprise option (enterprise.enabled = true) is enabled. Since enabling enterprise in the helm chart also enables auth/multi-tenancy, the addition of the provisioner allows the helm chart to automatically create a tenant for the canary via the GEL admin API, and to create applicable access policies and tokens. Furthermore, additional tenants can also be specified, which will be provisioned, creating a read and write token for each that will be stored in k8s secrets.

Which issue(s) this PR fixes:
Fixes #7019

Checklist

Documentation added
Tests updated
Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

grafanabot · 2022-09-15T17:00:08Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

DylanGuedes

few nits

production/helm/loki/CHANGELOG.md

production/helm/loki/README.md

production/helm/loki/templates/_helpers.tpl

production/helm/loki/values.yaml

DylanGuedes · 2022-09-15T18:27:31Z

production/helm/loki/templates/monitoring/_helpers-monitoring.tpl

-    cluster: {{ include "loki.fullname" . -}}
+    cluster: {{ include "loki.fullname" . }}
+  {{- if or .Values.loki.auth_enabled .Values.enterprise.enabled }}
+  tenantId: {{ .Values.monitoring.selfMonitoring.tenant }}


is this identation right?

Why is a tenant ID required for enterprise?

probably not required for enterprise since with enterprise we're providing the username below.

changed the logic a bit, and yes double checked indenation, following is with auth_enabled = true but enterprise off:

spec: clients: - url: http://loki-gateway.default.svc.cluster.local/loki/api/v1/push externalLabels: cluster: release-name-loki tenantId: self-monitoring

jeschkies

I need to give it one more pass.

* Add the loki canary as a deamonset * In order to work with enterprise mode, the loki-canary had to be configured with a basic auth user and password. The addition of the provisioner job allows us to provision a tenant for both canary and self-monitoring usage.

Co-authored-by: Dylan Guedes <[email protected]>

grafanabot · 2022-09-21T15:39:25Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
- querier/queryrange	-0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

towolf · 2022-09-23T16:17:50Z

Hi @trevorwhitney, could you explain, why you added the loki-canary ServiceAccount as a Helm post-install hook? This fails to work with Helm via Terraform (hashicorp/terraform-provider-helm#683), and I see no reason for this.

https://github.com/grafana/loki/blob/main/production/helm/loki/templates/loki-canary/serviceaccount.yaml#L10

towolf · 2022-09-23T16:34:36Z

And another question @trevorwhitney, why do you default to latest and not to .Chart.Appversion here?

https://github.com/grafana/loki/blob/main/production/helm/loki/templates/loki-canary/_helpers.tpl#L28

trevorwhitney · 2022-09-23T20:41:15Z

@towolf the reason for the post-install hook was probably because I copied the service account template from either the tokengen or provisioner jobs, and my guess for those being post-install hooks are to delay the running of the job. But I agree, we probably don't need to delay anything for the canary so we can remove that.

Also, we should definitely use .Char.Appversion here, agreed!

trevorwhitney · 2022-09-23T22:53:15Z

@towolf here's a PR to fix, thanks for pointing these out!
#7236

@towolf

**What this PR does / why we need it**: Bring the Loki Canary service account in line with the Loki service account, removing the post install hook, and sharing the top level properties for `automountServiceAccountToken` and `imagePullSecrets`. I'm open to creating canary specific values for these setting if needed, but want to start by sharing the top level values to reduce config if possible. Also default canary to Appversion to be in sync with default Loki version **Which issue(s) this PR fixes**: Fixes two issues raised in comments from @towolf (thanks!) on this [PR](#7173 (comment)). Fixes #7237

**What this PR does / why we need it**: This PR adds the Loki canary as well as the GEL provisioner to the Loki helm chart. The Loki canary expands the self-monitoring functionality of the Helm chart, giving operators insight beyond whether Loki is just running, but also if it's functioning properly. The GEL provisioner was necessary for the canary to work when the enterprise option (`enterprise.enabled = true`) is enabled. Since enabling enterprise in the helm chart also enables auth/multi-tenancy, the addition of the provisioner allows the helm chart to automatically create a tenant for the canary via the GEL admin API, and to create applicable access policies and tokens. Furthermore, additional tenants can also be specified, which will be provisioned, creating a read and write token for each that will be stored in k8s secrets. **Which issue(s) this PR fixes**: Fixes grafana#7019 Co-authored-by: Karsten Jeschkies <[email protected]> Co-authored-by: Dylan Guedes <[email protected]>

@towolf

**What this PR does / why we need it**: Bring the Loki Canary service account in line with the Loki service account, removing the post install hook, and sharing the top level properties for `automountServiceAccountToken` and `imagePullSecrets`. I'm open to creating canary specific values for these setting if needed, but want to start by sharing the top level values to reduce config if possible. Also default canary to Appversion to be in sync with default Loki version **Which issue(s) this PR fixes**: Fixes two issues raised in comments from @towolf (thanks!) on this [PR](grafana#7173 (comment)). Fixes grafana#7237

@towolf

**What this PR does / why we need it**: Bring the Loki Canary service account in line with the Loki service account, removing the post install hook, and sharing the top level properties for `automountServiceAccountToken` and `imagePullSecrets`. I'm open to creating canary specific values for these setting if needed, but want to start by sharing the top level values to reduce config if possible. Also default canary to Appversion to be in sync with default Loki version **Which issue(s) this PR fixes**: Fixes two issues raised in comments from @towolf (thanks!) on this [PR](grafana#7173 (comment)). Fixes grafana#7237

@towolf

**What this PR does / why we need it**: Bring the Loki Canary service account in line with the Loki service account, removing the post install hook, and sharing the top level properties for `automountServiceAccountToken` and `imagePullSecrets`. I'm open to creating canary specific values for these setting if needed, but want to start by sharing the top level values to reduce config if possible. Also default canary to Appversion to be in sync with default Loki version **Which issue(s) this PR fixes**: Fixes two issues raised in comments from @towolf (thanks!) on this [PR](grafana#7173 (comment)). Fixes grafana#7237

**What this PR does / why we need it**: This PR adds the Loki canary as well as the GEL provisioner to the Loki helm chart. The Loki canary expands the self-monitoring functionality of the Helm chart, giving operators insight beyond whether Loki is just running, but also if it's functioning properly. The GEL provisioner was necessary for the canary to work when the enterprise option (`enterprise.enabled = true`) is enabled. Since enabling enterprise in the helm chart also enables auth/multi-tenancy, the addition of the provisioner allows the helm chart to automatically create a tenant for the canary via the GEL admin API, and to create applicable access policies and tokens. Furthermore, additional tenants can also be specified, which will be provisioned, creating a read and write token for each that will be stored in k8s secrets. **Which issue(s) this PR fixes**: Fixes grafana#7019 Co-authored-by: Karsten Jeschkies <[email protected]> Co-authored-by: Dylan Guedes <[email protected]>

@towolf

**What this PR does / why we need it**: Bring the Loki Canary service account in line with the Loki service account, removing the post install hook, and sharing the top level properties for `automountServiceAccountToken` and `imagePullSecrets`. I'm open to creating canary specific values for these setting if needed, but want to start by sharing the top level values to reduce config if possible. Also default canary to Appversion to be in sync with default Loki version **Which issue(s) this PR fixes**: Fixes two issues raised in comments from @towolf (thanks!) on this [PR](grafana#7173 (comment)). Fixes grafana#7237

trevorwhitney requested a review from a team as a code owner September 15, 2022 16:51

pull-request-size bot added the size/XL label Sep 15, 2022

github-actions bot added the area/helm label Sep 15, 2022

DylanGuedes approved these changes Sep 15, 2022

View reviewed changes

jeschkies reviewed Sep 16, 2022

View reviewed changes

trevorwhitney force-pushed the add-canary-2 branch 3 times, most recently from 576c0cb to 2fc7aa2 Compare September 21, 2022 15:08

trevorwhitney and others added 11 commits September 21, 2022 09:33

Update CHANGELOG and README, bump version

382105b

Revert build-vars.nix

f582a63

Bump minor version to 3.1.0

dacc4a7

Update production/helm/loki/CHANGELOG.md

f994ef5

Co-authored-by: Dylan Guedes <[email protected]>

Update production/helm/loki/values.yaml

1135f83

Co-authored-by: Dylan Guedes <[email protected]>

Update production/helm/loki/values.yaml

ff048ff

Co-authored-by: Dylan Guedes <[email protected]>

Update production/helm/loki/values.yaml

2d68ffe

Co-authored-by: Dylan Guedes <[email protected]>

Update production/helm/loki/README.md

6e81c8c

Co-authored-by: Dylan Guedes <[email protected]>

Fixed up logic around including tenantId and non-gateway

516b19e

Update README

b356620

trevorwhitney force-pushed the add-canary-2 branch from 2fc7aa2 to b356620 Compare September 21, 2022 15:34

trevorwhitney merged commit afd63c5 into grafana:main Sep 21, 2022

trevorwhitney deleted the add-canary-2 branch September 21, 2022 16:02

trevorwhitney mentioned this pull request Sep 23, 2022

[helm] Mimic loki service account with canary account #7236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[helm] Add Loki Canary to Helm Chart #7173

[helm] Add Loki Canary to Helm Chart #7173

trevorwhitney commented Sep 15, 2022

grafanabot commented Sep 15, 2022

DylanGuedes left a comment

DylanGuedes Sep 15, 2022

jeschkies Sep 16, 2022

trevorwhitney Sep 19, 2022

trevorwhitney Sep 19, 2022 •

edited

Loading

jeschkies left a comment

grafanabot commented Sep 21, 2022

towolf commented Sep 23, 2022 •

edited

Loading

towolf commented Sep 23, 2022

trevorwhitney commented Sep 23, 2022

trevorwhitney commented Sep 23, 2022

[helm] Add Loki Canary to Helm Chart #7173

[helm] Add Loki Canary to Helm Chart #7173

Conversation

trevorwhitney commented Sep 15, 2022

grafanabot commented Sep 15, 2022

DylanGuedes left a comment

Choose a reason for hiding this comment

DylanGuedes Sep 15, 2022

Choose a reason for hiding this comment

jeschkies Sep 16, 2022

Choose a reason for hiding this comment

trevorwhitney Sep 19, 2022

Choose a reason for hiding this comment

trevorwhitney Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

jeschkies left a comment

Choose a reason for hiding this comment

grafanabot commented Sep 21, 2022

towolf commented Sep 23, 2022 • edited Loading

towolf commented Sep 23, 2022

trevorwhitney commented Sep 23, 2022

trevorwhitney commented Sep 23, 2022

trevorwhitney Sep 19, 2022 •

edited

Loading

towolf commented Sep 23, 2022 •

edited

Loading