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

Monitoring Endpoint causing HTTPS to be default scheme if only one endpoint present while it says default is HTTP. #620

Open
dhanush251201 opened this issue Mar 22, 2024 · 1 comment
Assignees

Comments

@dhanush251201
Copy link

Bug Report

What did you do?

with regards to theMonitoring feature, the endpoint was defined with scheme http, but the serviceMonitor created was of scheme HTTPS causing it to not function as intended. Even if the scheme was left empty, the default as per the documentation is http but the serviceMonitor was of scheme https

What did you expect to see?

Expected to see http as the default scheme for ServiceMonitor created by the operator.

What did you see instead?

HTTPS was the scheme in the service monitor created by the operator

Environment

OpenShift

Possible solution

The issue is identified to be in the utils/utils.go file in line 1203-1207

if ba.GetManageTLS() == nil || *ba.GetManageTLS() {
		if len(ba.GetMonitoring().GetEndpoints()) == 0 || ba.GetMonitoring().GetEndpoints()[0].TLSConfig == nil {
			sm.Spec.Endpoints[0].Scheme = "https"
			if sm.Spec.Endpoints[0].TLSConfig == nil {
				sm.Spec.Endpoints[0].TLSConfig = &prometheusv1.TLSConfig{}
			}
			sm.Spec.Endpoints[0].TLSConfig.CA = prometheusv1.SecretOrConfigMap{}
			sm.Spec.Endpoints[0].TLSConfig.CA.Secret = &corev1.SecretKeySelector{}
			sm.Spec.Endpoints[0].TLSConfig.CA.Secret.Name = ba.GetStatus().GetReferences()[common.StatusReferenceCertSecretName]
			sm.Spec.Endpoints[0].TLSConfig.CA.Secret.Key = "tls.crt"
			sm.Spec.Endpoints[0].TLSConfig.ServerName = obj.GetName() + "." + obj.GetNamespace() + ".svc"
		}

	}

The problem is solved if the tlsConfig of the first endpoint is not nil. So giving an empty value in any of the fields results in http scheme being used.

if ba.GetManageTLS() == nil || *ba.GetManageTLS() {
		if len(ba.GetMonitoring().GetEndpoints()) == 0 || ba.GetMonitoring().GetEndpoints()[0].TLSConfig == nil {
			sm.Spec.Endpoints[0].Scheme = "https"
                ... }
... }

in the second if statement if there are no endpoints or if the first endpoint has no TLSConfig, it is setting the scheme to https. This is the issue that needs to be fixed.

Additional context

If there is only one intended end point and that has to be http then this logic does not allow the same.

@kabicin
Copy link
Collaborator

kabicin commented Apr 3, 2024

@dhanush251201 TLS management (at both app level and the ServiceMonitor level) is enabled by default on the RuntimeComponent through the .spec.manageTLS flag.
So we override the ServiceMonitor endpoint to be https to correspond to the RuntimeComponent which is normal behavior from the operator. When setting .spec.manageTLS to false, it falls back to http, as per Prometheus docs.

However, I see that your issue is with the lack of customizability with the scheme forced to be https for the single ServiceMonitor endpoint. We can make it easier to allow opting-out of the https override for the endpoint but still have TLS managed on the RuntimeComponent app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants