-
Notifications
You must be signed in to change notification settings - Fork 364
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
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
73734fb
to
5cd04bc
Compare
/test e2e-agnostic |
1 similar comment
/test e2e-agnostic |
5cd04bc
to
8345d86
Compare
8345d86
to
7bb7ebb
Compare
/skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good apart from a small comment, also do we have a Jira issue or a BZ with more content for the change?
@@ -1657,13 +1677,12 @@ func (f *Factory) PrometheusK8s(grpcTLS *v1.Secret, trustedCABundleCM *v1.Config | |||
Replacement: "alerts", | |||
}, | |||
}, | |||
MetadataConfig: &monv1.MetadataConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems to not be related, was it intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you still want to add a comment for this @simonpasquier ?
For traceability I'm also linking this here since AFAIK these two PRs are related openshift/telemeter#430 |
@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? |
7bb7ebb
to
8b68d6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the questions looks good 👌
|
||
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thank you 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we have no way to turn on native remote write without setting CMO into unmanaged state and hacking the deployment args.
/skip |
/retest |
/assign @jan--f |
clusterID := f.config.ClusterMonitoringConfiguration.TelemeterClientConfig.ClusterID | ||
if telemetryEnabled && f.config.RemoteWrite { | ||
|
||
if telemetrySecret != nil { | ||
selectorRelabelConfig, err := promqlgen.LabelSelectorsToRelabelConfig(f.config.ClusterMonitoringConfiguration.PrometheusK8sConfig.TelemetryMatches) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/retitle: MON-2807: Use bearer token file for remote write authentication with telemeter |
Once change request discussed offline added here as a comment, otherwise this looks good. |
/retitle MON-2807: Use bearer token file for remote write authentication with telemeter |
43ab1e6
to
c6d21a3
Compare
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
c6d21a3
to
e9b7689
Compare
/test e2e-agnostic-operator |
/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.
e9b7689
to
dfa372d
Compare
/test e2e-agnostic |
@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. |
/lgtm On a side note, I was wondering about the |
[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:
Approvers can indicate their approval by writing |
/label docs-approved |
cc @saswatamcode