MON-2807: Use bearer token file for remote write authentication with telemeter #1733

simonpasquier · 2022-08-05T07:58:29Z

cc @saswatamcode

simonpasquier · 2022-08-11T08:06:28Z

/test e2e-agnostic

simonpasquier · 2022-08-11T11:25:04Z

/test e2e-agnostic

JoaoBraveCoding · 2022-09-05T14:20:31Z

/skip

JoaoBraveCoding

Looks good apart from a small comment, also do we have a Jira issue or a BZ with more content for the change?

JoaoBraveCoding · 2022-09-05T14:29:18Z

pkg/manifests/manifests.go

@@ -1657,13 +1677,12 @@ func (f *Factory) PrometheusK8s(grpcTLS *v1.Secret, trustedCABundleCM *v1.Config
 					Replacement:  "alerts",
 				},
 			},
+			MetadataConfig: &monv1.MetadataConfig{


This change seems to not be related, was it intentional?

Yes but it should be commented. Basically we don't want/need to forward metadata information to the telemeter server.

It comes from #1416 which was the previous PR that exercised the receive path of telemeter.

Yes, Telemeter forwards rw requests to Thanos, and Thanos doesn't have metadata ingestion yet. We can still send it, but it would just be dropped. 🙂

Did you still want to add a comment for this @simonpasquier ?

JoaoBraveCoding · 2022-09-05T14:31:48Z

For traceability I'm also linking this here since AFAIK these two PRs are related openshift/telemeter#430

simonpasquier · 2022-09-06T13:29:37Z

@jan--f I'm wondering if we shouldn't add an end-to-end test to validate the /receive path. Even though we don't support it yet in OCP, it would help detecting regressions on the server side. WDYT?

JoaoBraveCoding

Apart from the questions looks good 👌

JoaoBraveCoding · 2022-09-08T15:39:20Z

test/e2e/telemeter_test.go

+
+// TestTelemeterRemoteWrite verifies that the monitoring stack can send data to
+// the telemeter server using the native Prometheus remote write endpoint.
+func TestTelemeterRemoteWrite(t *testing.T) {


Small curiosity questions:

Where does CMO get the config/info that Prom is supposed to remote-write to https://infogw.api.openshift.com.+? I see we are enabling remote-write but I don't get how it knows it's supposed to send them there.

Why are we modifying the deployment instead of using the ConfigMap

I think it's hardcoded in the SetRemoteWrite function https://github.com/openshift/cluster-monitoring-operator/blob/master/pkg/manifests/config.go#L449. This is called in Operator.sync https://github.com/openshift/cluster-monitoring-operator/blob/master/pkg/operator/operator.go#L608.

Indeed, thank you 👍

yes, we have no way to turn on native remote write without setting CMO into unmanaged state and hacking the deployment args.

simonpasquier · 2022-09-09T09:59:32Z

/skip

simonpasquier · 2022-09-09T09:59:37Z

/retest

simonpasquier · 2022-09-12T15:39:32Z

/assign @jan--f

jan--f · 2022-09-19T11:12:02Z

pkg/manifests/manifests.go

 	clusterID := f.config.ClusterMonitoringConfiguration.TelemeterClientConfig.ClusterID
-	if telemetryEnabled && f.config.RemoteWrite {
-
+	if telemetrySecret != nil {
 		selectorRelabelConfig, err := promqlgen.LabelSelectorsToRelabelConfig(f.config.ClusterMonitoringConfiguration.PrometheusK8sConfig.TelemetryMatches)


Discussed separately: Future readers of this code would benefit from checking the actual config condition here (if telemetryEnabled && f.config.RemoteWrite) and assuming the secret was created instead of coupling this to the secret creating.

jan--f · 2022-09-19T11:13:57Z

/retitle: MON-2807: Use bearer token file for remote write authentication with telemeter

jan--f · 2022-09-19T11:14:58Z

Once change request discussed offline added here as a comment, otherwise this looks good.

jan--f · 2022-09-19T11:15:15Z

/retitle MON-2807: Use bearer token file for remote write authentication with telemeter

openshift-bot · 2022-12-21T09:00:50Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

simonpasquier · 2023-01-04T10:54:47Z

/test e2e-agnostic-operator

simonpasquier · 2023-01-04T10:55:14Z

/remove-lifecycle stale

For security concerns, it's better to pass the bearer token via a Secret rather than sticking it in the Prometheus custom resource. Added TestTelemeterRemoteWrite to the end-to-end tests to ensure that the Telemetry remote-write path is tested though it isn't (yet) enabled.

simonpasquier · 2023-01-05T09:38:51Z

/test e2e-agnostic

openshift-ci · 2023-01-05T11:17:54Z

@simonpasquier: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jan--f · 2023-01-05T14:27:01Z

/lgtm
I think the commend about MetadataConfig would still be useful, but I can live without it.

On a side note, I was wondering about the cluster_id field in the secret...seems like that has been there since day 1 of this feature. Does anyone have an idea what the purpose is?

openshift-ci · 2023-01-05T14:28:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, simonpasquier

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jan--f,simonpasquier]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jan--f · 2023-01-05T14:52:14Z

/label docs-approved
/label px-approved
/label qe-approved
Adding labels for an addition to our e2e test suite.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2022

openshift-ci bot requested review from JoaoBraveCoding and raptorsun August 5, 2022 08:00

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2022

simonpasquier force-pushed the test-telemeter-receive branch from 73734fb to 5cd04bc Compare August 10, 2022 14:00

simonpasquier force-pushed the test-telemeter-receive branch from 5cd04bc to 8345d86 Compare August 11, 2022 13:11

simonpasquier force-pushed the test-telemeter-receive branch from 8345d86 to 7bb7ebb Compare September 2, 2022 13:03

simonpasquier changed the title ~~[WIP] Test /metrics/v1/receive write path~~ Use bearer token file for remote write authentication with telemeter Sep 2, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 2, 2022

JoaoBraveCoding reviewed Sep 5, 2022

View reviewed changes

simonpasquier force-pushed the test-telemeter-receive branch from 7bb7ebb to 8b68d6c Compare September 8, 2022 14:08

JoaoBraveCoding reviewed Sep 8, 2022

View reviewed changes

openshift-ci bot assigned jan--f Sep 12, 2022

jan--f reviewed Sep 19, 2022

View reviewed changes

openshift-ci bot changed the title ~~Use bearer token file for remote write authentication with telemeter~~ : MON-2807: Use bearer token file for remote write authentication with telemeter Sep 19, 2022

openshift-ci bot changed the title ~~: MON-2807: Use bearer token file for remote write authentication with telemeter~~ MON-2807: Use bearer token file for remote write authentication with telemeter Sep 19, 2022

simonpasquier force-pushed the test-telemeter-receive branch 2 times, most recently from 43ab1e6 to c6d21a3 Compare September 21, 2022 15:17

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 21, 2022

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2022

simonpasquier force-pushed the test-telemeter-receive branch from c6d21a3 to e9b7689 Compare December 22, 2022 10:22

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2022

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 4, 2023

simonpasquier force-pushed the test-telemeter-receive branch from e9b7689 to dfa372d Compare January 4, 2023 12:59

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2023

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR labels Jan 5, 2023

openshift-merge-robot merged commit 2f1182a into openshift:master Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MON-2807: Use bearer token file for remote write authentication with telemeter #1733

MON-2807: Use bearer token file for remote write authentication with telemeter #1733

simonpasquier commented Aug 5, 2022

simonpasquier commented Aug 11, 2022

simonpasquier commented Aug 11, 2022

JoaoBraveCoding commented Sep 5, 2022

JoaoBraveCoding left a comment

JoaoBraveCoding Sep 5, 2022

simonpasquier Sep 6, 2022

saswatamcode Sep 7, 2022

jan--f Jan 5, 2023

JoaoBraveCoding commented Sep 5, 2022

simonpasquier commented Sep 6, 2022

JoaoBraveCoding left a comment

JoaoBraveCoding Sep 8, 2022

saswatamcode Sep 8, 2022

JoaoBraveCoding Sep 8, 2022

simonpasquier Sep 9, 2022

simonpasquier commented Sep 9, 2022

simonpasquier commented Sep 9, 2022

simonpasquier commented Sep 12, 2022

jan--f Sep 19, 2022

jan--f commented Sep 19, 2022 •

edited by openshift-ci bot

Loading

jan--f commented Sep 19, 2022

jan--f commented Sep 19, 2022 •

edited by openshift-ci bot

Loading

openshift-bot commented Dec 21, 2022

simonpasquier commented Jan 4, 2023

simonpasquier commented Jan 4, 2023

simonpasquier commented Jan 5, 2023

openshift-ci bot commented Jan 5, 2023

jan--f commented Jan 5, 2023

openshift-ci bot commented Jan 5, 2023

jan--f commented Jan 5, 2023

MON-2807: Use bearer token file for remote write authentication with telemeter #1733

MON-2807: Use bearer token file for remote write authentication with telemeter #1733

Conversation

simonpasquier commented Aug 5, 2022

simonpasquier commented Aug 11, 2022

simonpasquier commented Aug 11, 2022

JoaoBraveCoding commented Sep 5, 2022

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoBraveCoding commented Sep 5, 2022

simonpasquier commented Sep 6, 2022

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Sep 9, 2022

simonpasquier commented Sep 9, 2022

simonpasquier commented Sep 12, 2022

Choose a reason for hiding this comment

jan--f commented Sep 19, 2022 • edited by openshift-ci bot Loading

jan--f commented Sep 19, 2022

jan--f commented Sep 19, 2022 • edited by openshift-ci bot Loading

openshift-bot commented Dec 21, 2022

simonpasquier commented Jan 4, 2023

simonpasquier commented Jan 4, 2023

simonpasquier commented Jan 5, 2023

openshift-ci bot commented Jan 5, 2023

jan--f commented Jan 5, 2023

openshift-ci bot commented Jan 5, 2023

jan--f commented Jan 5, 2023

jan--f commented Sep 19, 2022 •

edited by openshift-ci bot

Loading

jan--f commented Sep 19, 2022 •

edited by openshift-ci bot

Loading