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

Remove mtp #282

Open
wants to merge 25 commits into
base: remove-loki-datasource-on-capi
Choose a base branch
from
Open

Remove mtp #282

wants to merge 25 commits into from

Conversation

QuentinBisson
Copy link
Contributor

@QuentinBisson QuentinBisson commented Nov 27, 2024

What this PR does / why we need it

Towards giantswarm/roadmap#3726

This PR switches from mtp to loki ingress for authentication

It also changes the way we configure the alloy secrets to be able to change the values when we update them on the clusters as the config reloader does not handle evn variable changes

Checklist

  • Update changelog in CHANGELOG.md.
  • Follow deployment test procedure in the tests/manual_e2e directory and have a working branch.

@QuentinBisson QuentinBisson self-assigned this Nov 27, 2024
@QuentinBisson QuentinBisson requested a review from a team as a code owner November 27, 2024 10:10
@QuentinBisson QuentinBisson changed the base branch from main to remove-loki-datasource-on-capi November 27, 2024 11:02
@QuentinBisson QuentinBisson force-pushed the remove-mtp branch 2 times, most recently from 21e2a47 to f3b778f Compare November 27, 2024 12:40
@QuentinBisson QuentinBisson force-pushed the remove-loki-datasource-on-capi branch 2 times, most recently from 7e0ffc0 to ec5c003 Compare November 27, 2024 12:49
@QuentinBisson QuentinBisson force-pushed the remove-mtp branch 3 times, most recently from e0b2692 to 64ab01f Compare November 27, 2024 14:45
@QuentinBisson QuentinBisson force-pushed the remove-loki-datasource-on-capi branch from ec5c003 to df4a0a5 Compare November 27, 2024 14:48
pkg/common/common.go Show resolved Hide resolved
Comment on lines 19 to 25
url = nonsensitive(remote.kubernetes.secret.credentials.data["logging-url"])
max_backoff_period = "{{ .MaxBackoffPeriod }}"
tenant_id = env("{{ .TenantIDEnvVarName }}")
tenant_id = nonsensitive(remote.kubernetes.secret.credentials.data["logging-tenant-id"])

basic_auth {
username = env("{{ .BasicAuthUsernameEnvVarName }}")
password = env("{{ .BasicAuthPasswordEnvVarName }}")
username = nonsensitive(remote.kubernetes.secret.credentials.data["logging-username"])
password = remote.kubernetes.secret.credentials.data["logging-password"]
Copy link
Member

Choose a reason for hiding this comment

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

Can we please define the keys in a single place and then pass them around, just that we avoid any mistake or don't forget to update a set of keys in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to ideas on this because I've no clue how to do this

Copy link
Member

Choose a reason for hiding this comment

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

That's a template so we can just inject those names like this

password = remote.kubernetes.secret.credentials.data["{{ .LoggingPasswordKey }}"]

And then passing this new LoggingPasswordKey as field to the template data.

pkg/resource/logging-secret/alloy-logging-secret.go Outdated Show resolved Hide resolved
Comment on lines +40 to +51
// listWriteUsers returns a map of users found in a credentialsSecret
func listWriteUsers(credentialsSecret *v1.Secret) []string {
var usersList []string
for myUser := range credentialsSecret.Data {
// bypass read user
if myUser != common.ReadUser {
usersList = append(usersList, myUser)
}
}

return usersList
}
Copy link
Member

Choose a reason for hiding this comment

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

If we are storing all read and write users in the same place how do we know later on what's a reader and what's a writer ?

Copy link
Contributor Author

@QuentinBisson QuentinBisson Dec 2, 2024

Choose a reason for hiding this comment

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

The read user is actually just called read as in, it's his username hence this if myUser != common.ReadUser part.

Other than that, the rest are write users. The goal of this new code is to only have write users as the read user will only be grafana anyway with the new multi org

QuentinBisson and others added 2 commits December 2, 2024 16:46
Co-authored-by: Théo Brigitte <theo.brigitte@gmail.com>
@QuentinBisson
Copy link
Contributor Author

@TheoBrigitte it should be fine now

Copy link
Member

@TheoBrigitte TheoBrigitte left a comment

Choose a reason for hiding this comment

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

LGTM

@QuentinBisson QuentinBisson force-pushed the remove-loki-datasource-on-capi branch from 27b0f51 to 6dae2e4 Compare December 17, 2024 16:17
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