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

operator: Fix storing authentication credentials in the Loki ConfigMap #11357

Merged
merged 22 commits into from
Dec 11, 2023

Conversation

periklis
Copy link
Collaborator

@periklis periklis commented Dec 1, 2023

What this PR does / why we need it:
The following PR replaces all sensitive authentication information from the Loki configuration file by reading values from environment variables. Latter are projected from the object storage secret via EnvVar.ValueFrom source declaration.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

  • The environment variables are projected into the ConfigMap cause of Loki's inability to leave the underlying SDKs reading their values instead of passing them via it's config.
  • For testing on Azure you can consider using the scripts from this PR:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@periklis periklis requested review from xperimental and a team as code owners December 1, 2023 10:55
@periklis periklis self-assigned this Dec 1, 2023
Copy link
Contributor

github-actions bot commented Dec 1, 2023

Trivy scan found the following vulnerabilities:

@periklis periklis changed the title operator: Fix storing authentication info in Loki config operator: Fix storing authentication credentials in the Loki ConfigMap Dec 1, 2023
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

Looks good overall. Only tested it with S3 so far. Found a few typos, see comments.

operator/internal/manifests/storage/var.go Outdated Show resolved Hide resolved
operator/internal/handlers/internal/storage/secrets.go Outdated Show resolved Hide resolved
operator/internal/handlers/internal/storage/secrets.go Outdated Show resolved Hide resolved
operator/internal/manifests/storage/configure.go Outdated Show resolved Hide resolved
operator/internal/manifests/storage/configure.go Outdated Show resolved Hide resolved
operator/internal/manifests/storage/configure.go Outdated Show resolved Hide resolved
operator/internal/manifests/storage/configure.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

While testing today something occurred to me, that I did not think of yesterday:

This change makes the configuration file static with regards to some of the fields of the secret (the ones that are passed using environment variables now).
We currently pass an annotation containing the hash of the configuration file to all the deployments / statefulsets that we create, so that they automatically re-create their pods when the configuration changes. Because some fields of the secret now do not correspond to a change in the configuration file it can happen that a change to the secret does not trigger an update of the deployments/statefulsets anymore causing old configuration to persist in the Loki instances.

I guess a simple fix for this would be to take the contents of the secret into account when creating the "config-hash" as well. Adding a new annotation is also an alternative.

operator/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

Pod updates seem to work fine now 👍

One refactoring idea in the comments, looks fine for me otherwise.

operator/internal/handlers/internal/storage/secrets.go Outdated Show resolved Hide resolved
operator/internal/manifests/compactor_test.go Show resolved Hide resolved
@periklis periklis enabled auto-merge (squash) December 11, 2023 19:03
@periklis periklis merged commit c573def into grafana:main Dec 11, 2023
14 checks passed
periklis added a commit to periklis/loki that referenced this pull request Dec 11, 2023
grafana#11357)

Co-authored-by: Robert Jacob <xperimental@solidproject.de>
Co-authored-by: Robert Jacob <rojacob@redhat.com>
periklis added a commit to periklis/loki that referenced this pull request Dec 11, 2023
grafana#11357)

Co-authored-by: Robert Jacob <xperimental@solidproject.de>
Co-authored-by: Robert Jacob <rojacob@redhat.com>
periklis added a commit to periklis/loki that referenced this pull request Dec 11, 2023
grafana#11357)

Co-authored-by: Robert Jacob <xperimental@solidproject.de>
Co-authored-by: Robert Jacob <rojacob@redhat.com>
periklis added a commit to openshift/loki that referenced this pull request Dec 13, 2023
periklis added a commit to openshift/loki that referenced this pull request Dec 13, 2023
periklis added a commit to openshift/loki that referenced this pull request Dec 13, 2023
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
grafana#11357)

Co-authored-by: Robert Jacob <xperimental@solidproject.de>
Co-authored-by: Robert Jacob <rojacob@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants