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: loki auth for multi-tenancy #1662

Merged
merged 4 commits into from
Aug 1, 2024
Merged

fix: loki auth for multi-tenancy #1662

merged 4 commits into from
Aug 1, 2024

Conversation

srodenhuis
Copy link
Contributor

@srodenhuis srodenhuis commented Jul 29, 2024

This PR fixes the broken Loki multi-tenancy. The problem is the setup of the Loki Distributed Helm Chart. Most of the Loki configuration options are combined in just one value {{ .Values.loki.config }}. The auth-enabled in one of them. In the loki.config other Chart values can be used. But not all Loki configuration options are supported in the values.yaml. Using values within values in the values.yaml makes it very difficult to modify values using a .gotmpl.
If any of you has an idea to go around this issue (other than trying to change this in the upstream Grafana helm-charts project), please let me know.
Another option would be (as previously done) provide the full configuration values in the loki.gotmpl. But if we want users to modify config, this would not be the preferred way.

Copy link
Contributor

@j-zimnowoda j-zimnowoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently apl is leveraging loki-distributed chart (however in apl the chart is called loki)

The original repo (https://github.com/grafana/helm-charts/tree/loki-distributed-0.69.16/charts/loki) has been deprecated and new on is at
https://github.com/grafana/loki/blob/main/production/helm/loki/values.yaml

There, they are calling the chart just "loki" :)

In a new repo we can find a detailed instruction

  ######################################################################################################################
  #
  # Loki Configuration
  #
  # There are several ways to pass configuration to Loki, listing them here in order of our preference for how
  # you should use this chart.
  # 1. Use the templated value of loki.config below and the corresponding override sections which follow.
  #    This allows us to set a lot of important Loki configurations and defaults and also allows us to maintain them
  #    over time as Loki changes and evolves.
  # 2. Use the loki.structuredConfig section.
  #    This will completely override the templated value of loki.config, so you MUST provide the entire Loki config
  #    including any configuration that we set in loki.config unless you explicitly are trying to change one of those
  #    values and are not able to do so with the templated sections.
  #    If you choose this approach the burden is on you to maintain any changes we make to the templated config.
  # 3. Use an existing secret or configmap to provide the configuration.
  #    This option is mostly provided for folks who have external processes which provide or modify the configuration.
  #    When using this option you can specify a different name for loki.generatedConfigObjectName and configObjectName
  #    if you have a process which takes the generated config and modifies it, or you can stop the chart from generating
  #    a config entirely by setting loki.generatedConfigObjectName to
  #
  ######################################################################################################################

I understand that community decided to cover changes in the Loki config file. It makes sense but it does not explain inability to overwrite the individual parameters. Thefore, I create a GitHub issue at grafana/loki#13723

I also verified the templated changes with the following script:

export ENV_DIR=$PWD/tests/fixtures
gco main
otomi template loki-main -l name=loki 
gco loki-sr-fix-loki-auth-1
otomi template loki-sr-fix-loki-auth-1 -l name=loki 
# compare
dyff between helmfile.d/loki-main/helmfile-10.monitoring-65f7013d-loki/loki-distributed/templates/configmap.yaml helmfile.d/loki-sr-fix-loki-auth-1/helmfile-10.monitoring-65f7013d-loki/loki-distributed/templates/configmap.yaml --omit-header --ignore-order-changes

data.config.yaml
  ± value change in multiline text (two inserts, three deletions)
    - auth_enabled: false
    + auth_enabled: true
    -   shared_store: filesystem
    +   shared_store: s3
    - ruler:
    -   alertmanager_url: https://alertmanager.xx
    -   external_url: https://alertmanager.xx
    -   ring:
    -     kvstore:
    -       store: memberlist
    -   rule_path: /tmp/loki/scratch
    -   storage:
    -     local:
    -       directory: /etc/loki/rules
    -     type: local

Action points:

  1. From the above dyff comparison I see that the ruler section has been removed. Was it intentional ?
  2. We should not make any change in the chart itself. I propose to follow community advise and generate the whole Loki config in out loki.gotmpl file. I can help with that. Just let me know.
  3. If the Loki issue is accepted then we can contribute to Loki helm chart. Once it is merged the we will be able to simplify our loki.gotmpl file

@j-zimnowoda j-zimnowoda self-requested a review August 1, 2024 11:31
@j-zimnowoda
Copy link
Contributor

Using the loki.structuredConfig is sufficient to override the individual parameters of Loki config.
I decided to not remove the ruler setting because but default it is disabled.

@j-zimnowoda j-zimnowoda merged commit abb6c81 into main Aug 1, 2024
7 checks passed
@j-zimnowoda j-zimnowoda deleted the sr-fix-loki-auth branch August 1, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants