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

add k8s events logging to alloy #263

Merged
merged 46 commits into from
Nov 25, 2024
Merged

add k8s events logging to alloy #263

merged 46 commits into from
Nov 25, 2024

Conversation

QuantumEnigmaa
Copy link
Contributor

What this PR does / why we need it

Towards giantswarm/roadmap#3750

Checklist

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

@QuantumEnigmaa QuantumEnigmaa self-assigned this Nov 12, 2024
@QuantumEnigmaa
Copy link
Contributor Author

I still need to find a way to enable the operator to switch between alloy or grafana-agent for k8s events logging based on which logging agent is deployed.

@QuantumEnigmaa
Copy link
Contributor Author

Still need to handle the usage of a new events-logger-secret secret that will be used by either of alloy or grafana-agent to authenticate against Loki

@QuantumEnigmaa QuantumEnigmaa marked this pull request as ready for review November 14, 2024 13:57
@QuantumEnigmaa QuantumEnigmaa requested a review from a team as a code owner November 14, 2024 13:57
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@QuentinBisson
Copy link
Contributor

You're missing the toggle as well

@QuantumEnigmaa
Copy link
Contributor Author

You're missing the toggle as well

Oh indeed ! My bad

@QuantumEnigmaa
Copy link
Contributor Author

Created a separate PR for events-logger toggle : #270

@QuantumEnigmaa
Copy link
Contributor Author

I think I can't go further in terms of splitting this PR into smaller ones as now all files are intertwined between each other.

main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
@@ -44,6 +44,11 @@ func (r *Reconciler) ReconcileCreate(ctx context.Context, lc loggedcluster.Inter
return ctrl.Result{}, errors.WithStack(err)
}

// If the observability-bundle version is too old, we don't need to do anything.
Copy link
Contributor

Choose a reason for hiding this comment

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

What we do here is too disabled both grafana agent and alloy-event apps, not not do anything because we still need to toggle promtail right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok my bad. Since 0.9.0 is such an old version of the observability-bundle I wasn't even sure that promtail was already managed by the logging-operator.

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 updated the handling of this specific case. WDYT about it ?

ScrapedNamespaces string
}{
ClusterID: lc.GetClusterName(),
Installation: lc.GetInstallationName(),
InsecureSkipVerify: fmt.Sprintf("%t", lc.IsInsecureCA()),
SecretName: fmt.Sprintf("%s-%s", lc.GetClusterName(), common.GrafanaAgentExtraSecretName()),
ScrapedNamespaces: common.FormatScrapedNamespaces(lc, defaultWorkloadClusterNamespaces),
SecretName: eventsloggersecret.GetEventsLoggerSecretName(lc),
Copy link
Contributor

@QuentinBisson QuentinBisson Nov 21, 2024

Choose a reason for hiding this comment

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

Secret name should bé différent for grafana agent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have kube-system hardcoded in the grafana-agent template so I can indeed remove the SecretNamespace field.

As for the SecretName one we still need to keep it defined as it is right now as the name will depend on the WC's right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but i'm reviewing on m'y phone so I might have lost context 😅

@QuentinBisson
Copy link
Contributor

@QuantumEnigmaa do not forget to test the grafana agent case

Copy link
Contributor

@QuentinBisson QuentinBisson left a comment

Choose a reason for hiding this comment

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

LGTM, can you create another PR for hte alloy-events templating tests?

@QuantumEnigmaa
Copy link
Contributor Author

Follow-up PR : #277

QuantumEnigmaa and others added 2 commits November 25, 2024 14:47
* add tests for alloy-events in events-logger-config

* changelog

* create alloy-events tests golden files
@QuantumEnigmaa QuantumEnigmaa enabled auto-merge (squash) November 25, 2024 16:57
@QuantumEnigmaa QuantumEnigmaa merged commit 4b2f9e7 into main Nov 25, 2024
7 checks passed
@QuantumEnigmaa QuantumEnigmaa deleted the alloy-log-events branch November 25, 2024 17:03
QuentinBisson added a commit that referenced this pull request Dec 17, 2024
* add k8s events logging to alloy

* changelog

* create the events-logger resource : either alloy or grafana-agent

* handle secret for authentication

* fix alloy config property name

* fix build errors

* rename k8s-events-config package to events-logger-config

* Apply suggestions from code review

Co-authored-by: Quentin Bisson <quentin@giantswarm.io>

* fix error log

* return correct configmap name depending on the events logger

* return correct secret name depending on the events logger

* add scrapedNamespaces to alloy config create related function in common

* add tls config for alloy

* fix formating in grafana-agent template

* hardcode controller.type and controller.replicas in alloyEvents template

* remove unused observabilityBundleVersion parameters and variable in events-logger-config

* remove grafanaAgentSecretName and eventsLoggerSecretName from security checking

* rename variables in events-logger-config's reconciler to a neutral name

* make several functions private

* hardcode several fields in grafana-agent config template

* rename comments and logs in agents-toggle's reconciler

* enable events logger in main.go

* fix formating in events-logger templates

* remove unused field in alloy generateAlloyConfig

* fix alloy-events secret name

* Apply suggestions from code review

Co-authored-by: Quentin Bisson <quentin@giantswarm.io>

* update test files

* handle specific case of observability-bundle version < 0.9.0

* remove secret namespace field

* remove extra line in alloy and grafana-agent config

* fix variable name in reconcileDelete method from event-logger-config reconciler

* fix UTs for grafana-agent template

* disable crds creation in alloy-events template

* fixed grafana-agent tests in events-logger-config

* fix enabling of events logger depending on observability-bundle version

* minor fixes

* add tests for alloy-events in events-logger-config (#277)

* add tests for alloy-events in events-logger-config

* changelog

* create alloy-events tests golden files

---------

Co-authored-by: Quentin Bisson <quentin@giantswarm.io>
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