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

feat(operator): Allow setting explicit CredentialMode in LokiStack storage spec #12106

Merged
merged 5 commits into from Mar 5, 2024

Conversation

xperimental
Copy link
Collaborator

@xperimental xperimental commented Mar 1, 2024

What this PR does / why we need it:

Currently it is not possible to run the operator in managed mode and then deploy a LokiStack using a different mode for credentials to the object storage (static credentials or non-managed tokens). This is due to the credential-mode being determined only by the environment of the operator and the provided secret, making it an implicit consequence of the configuration instead of an explicit configuration.

This PR adds an optional attribute to the LokiStack's spec that allows the user to override the default credential mode. The main use-case for this is running a non-managed credential with an operator running in a managed-credentials OpenShift cluster, but it can also be used make the credentials-mode explicit for other deployments instead of having the operator decide based on the provided secret.

Which issue(s) this PR fixes:

Fixes LOG-5105

Special notes for your reviewer:

  • CredentialMode is an enum with just three possible values, but it is optional, so "" is a possible value inside the operator as well, although this can not be set on the LokiStack resource.
  • The CredentialsRequest is only created when running in managed-mode. It is also removed when the mode changes and it had been created before.
  • I still need to properly test this on Azure & GCP, the last tests were only on AWS.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Tests updated
  • CHANGELOG.md updated

@xperimental xperimental self-assigned this Mar 1, 2024
@xperimental xperimental requested review from periklis and a team as code owners March 1, 2024 17:02
Copy link
Collaborator

@periklis periklis left a comment

Choose a reason for hiding this comment

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

Looks overall good to me and works fine on an AWS STS managed cluster with an in-cluster Minio installation. Two cleanup suggestions added below, but approved to unblock you when done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we use Options.CredentialsMode in storage/configure.go to determine which mounts/volumes to consider we can probably remove S3.STS, GCS.WorkloadIdentity and Azure.WorkloadIdentity. WDYT?

operator/controllers/loki/lokistack_controller.go Outdated Show resolved Hide resolved
@xperimental
Copy link
Collaborator Author

Tested the new code on Azure and GCP as well. Seems to work fine 👍

@@ -19,9 +20,9 @@ import (
"github.com/grafana/loki/operator/internal/manifests/openshift"
)

// CreateCredentialsRequest creates a new CredentialsRequest resource for a Lokistack
// CreateUpdateDeleteCredentialsRequest creates a new CredentialsRequest resource for a Lokistack
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit.

Suggested change
// CreateUpdateDeleteCredentialsRequest creates a new CredentialsRequest resource for a Lokistack
// CreateUpdateDeleteCredentialsRequest manages a new CredentialsRequest resource for a Lokistack

Copy link
Contributor

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

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

lgtm

@xperimental xperimental merged commit 5b145ab into grafana:main Mar 5, 2024
18 checks passed
@xperimental xperimental deleted the sts-select-mode branch March 5, 2024 11:34
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
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.

None yet

3 participants