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

Missing TLS configuration during self service scrape creation by operator #1033

Closed
naveenbharadwaj-nc opened this issue Jul 10, 2024 · 12 comments
Labels
enhancement New feature or request

Comments

@naveenbharadwaj-nc
Copy link

naveenbharadwaj-nc commented Jul 10, 2024

Hello,

VMServiceScrape object generates part of VMAgent configuration with kubernetes service discovery targets by corresponding Service. It has various options for scraping configuration of target (with basic auth,tls access, by specific port name etc.).

As per this statement from documentation, we expect that VMServiceScrapes created automatically by the operator must honor HTTPS scheme and TLSConfig when TLS is configured.

The endpoints of the automatically created VMServiceScrape for all VM components contains only Port and Path. It is from the logic here. When constructing endpoints, neither scheme nor TLSConfig is populated to VMServiceScrape. This violates the documentation.

Please fix it if its a bug. If my understanding is incorrect and there is some way to tell operator to add TLSConfig and Scheme to generated VMServiceScrapes, please provide such instructions.

@f41gh7 f41gh7 added the enhancement New feature or request label Jul 10, 2024
@f41gh7
Copy link
Collaborator

f41gh7 commented Jul 17, 2024

There is a workaround for it. Manually defined needed options at spec.serviceScrapeSpec setting.

For example:

kind: VMAlert
spec:
  serviceScrapeSpec:
    endpoints:
    - port: http
      scheme: https 

I think we should add best-effort params detection later.

@naveenbharadwaj-nc
Copy link
Author

Sure. It would also mean that we have to disable selfServiceSpecCreation by setting VM_DISABLESELFSERVICESCRAPECREATION to true (just adding this information for reference). Or will it work if we dont disable?
When victoriametrics-operator has an option to create ServiceScrapes automatically, its obvious that most people would rely on such an option because it is more reliable. Using such work around will force us to add a lot of code (write entire ServiceScrapes on our side for all VM components). Another downside of using work around is that if victoriametrics-operator makes any enhancements/fixes to selfServiceSpecCreation creation logic in the future, we would miss it.
Its not high priority, but definitely useful if implemented on the operator side.

@f41gh7
Copy link
Collaborator

f41gh7 commented Jul 18, 2024

Sure. It would also mean that we have to disable selfServiceSpecCreation by setting VM_DISABLESELFSERVICESCRAPECREATION to true (just adding this information for reference). Or will it work if we dont disable?

There is no need to disable servicescrape auto-creation. This setting allows to override default values at operator-generated servicescrape.

It's only workaround for now. Later it should be fixed and operator must respect values from extraArgs to correctly generate servicescrape.

f41gh7 added a commit that referenced this issue Jul 24, 2024
Adds a new type for generic webserver TLS configuration.

* First of all, it allows to properly configure alertmanager and detect tls related params for it.
 It fixes scheme detection for probes, AsURL function.

* Second, it allows later to rollout this type to other application and have a generic way of TLS/mTLS configuration

* Fixes bug with tlsAssets mount with custom config reloader for alertmanager. Previously it was not mounted correctly and may cause issues with inaccessible assets.

related issues:
- #994
- #1033

Signed-off-by: f41gh7 <[email protected]>
f41gh7 added a commit that referenced this issue Jul 26, 2024
* api/vmalertmanager: adds webserver configuration

Adds a new type for generic webserver TLS configuration.

* First of all, it allows to properly configure alertmanager and detect tls related params for it.
 It fixes scheme detection for probes, AsURL function.

* Second, it allows later to rollout this type to other application and have a generic way of TLS/mTLS configuration

* Fixes bug with tlsAssets mount with custom config reloader for alertmanager. Previously it was not mounted correctly and may cause issues with inaccessible assets.

related issues:
- #994
- #1033

Signed-off-by: f41gh7 <[email protected]>
@AndrewChubatiuk AndrewChubatiuk added the waiting for release The change was merged to upstream, but wasn't released yet. label Aug 9, 2024
@naveenbharadwaj-nc
Copy link
Author

naveenbharadwaj-nc commented Aug 13, 2024

What about ServiceMonitor/VMServiceScrape for victoriametrics-operator itself? Operator sends metrics over http by default. How to tell the operator to send metrics to https endpoint?
The operator always uses insecure way of exposing metrics.
{"level":"info","ts":"2024-08-13T11:21:42Z","logger":"controller-runtime.metrics","msg":"Serving metrics server","bindAddress":":8080","secure":false}

f41gh7 added a commit that referenced this issue Aug 14, 2024
* automagically add `tls_config.insecure_skip_verify: true` and scheme: https
  to VMServiceScrape's generated by operator, if extraArgs have tls=true
* automagically sets `authKey` scrape param to generated VMServiceScrape, if extraArgs have `metricsAuthKey` set

#1033
Signed-off-by: f41gh7 <[email protected]>
f41gh7 added a commit that referenced this issue Aug 14, 2024
It's useful for mTLS configuration at reload-url target.

#1033
Signed-off-by: f41gh7 <[email protected]>
f41gh7 added a commit that referenced this issue Aug 14, 2024
It allows to protect metrics endpoint with tls and mTLS protection.
 It may be required for security hardened environment.

 `tls.enable=true` - switches metrics webserver into secure mode. Corresponding certificate must be accessible by manager.

related issue #1033

Signed-off-by: f41gh7 <[email protected]>
@AndrewChubatiuk
Copy link
Contributor

released in v0.47.0

@f41gh7
Copy link
Collaborator

f41gh7 commented Aug 15, 2024

What about ServiceMonitor/VMServiceScrape for victoriametrics-operator itself? Operator sends metrics over http by default. How to tell the operator to send metrics to https endpoint? The operator always uses insecure way of exposing metrics. {"level":"info","ts":"2024-08-13T11:21:42Z","logger":"controller-runtime.metrics","msg":"Serving metrics server","bindAddress":":8080","secure":false}

It's possible to protect operator endpoints with tls configuration starting with v0.47.0 release for endpoints:

  1. webhook server with flags webhook.enable,webhook.certDir,webhook.certName,webhook.keyName
  2. metrics server with flags tls.enable,tls.certDir,tls.certName,tls.keyName. Optionally, it's possible to configure mTLS with mtls.enable and mtls.CAName

In addition, operator will automatically configure VMServiceScrape objects for managed CRDs.

@AndrewChubatiuk AndrewChubatiuk removed the waiting for release The change was merged to upstream, but wasn't released yet. label Aug 17, 2024
@naveenbharadwaj-nc
Copy link
Author

naveenbharadwaj-nc commented Sep 12, 2024

Why TLSConfig is added with insecureSkipVerify: true for VMServiceScrapes?
It checks argument if tls: true to identify whether TLS is enabled or not. Similarly, why can't other arguments related to certificates be checked and configure TLSConfig accordingly?

@f41gh7
Copy link
Collaborator

f41gh7 commented Sep 12, 2024

The main reason behind that - operator is not aware ( at least for now), how to access needed tls certificate parts used at vmagent side.

It's a good place for the feature improvements.

@naveenbharadwaj-nc
Copy link
Author

TLS certificates may be passed using tlsCertFile, tlsKeyFile and tlsCaFile arguments. Combination of tls: true and insecureSkipVerify: true is not secure as there is no TLS verification. Operator already provides above arguments to pass certificates. So, operator can check these parameters and specify them in VMServiceScrapes.

@f41gh7
Copy link
Collaborator

f41gh7 commented Sep 13, 2024

TLS certificates may be passed using tlsCertFile, tlsKeyFile and tlsCaFile arguments. Combination of tls: true and insecureSkipVerify: true is not secure as there is no TLS verification. Operator already provides above arguments to pass certificates. So, operator can check these parameters and specify them in VMServiceScrapes.

The problem with tlsCaFile. Operator must know secret reference for this CaFile. Since it must be properly mounted at vmagent for scrape configuration.

When we introduce webTLSConfig for CRD objects, it would be possible.

@naveenbharadwaj-nc
Copy link
Author

Sorry CAFile is not necessary. tlsCertFile, tlsKeyFile can be checked.
VMServiceScrapes is generated for all VM components. If I pass tls, tlsCertFile and tlsKeyFile, it is sufficient to load them.

2024-09-12T11:57:30.082Z info VictoriaMetrics/lib/logger/flag.go:12 build version: vmagent-20240425-145801-tags-v1.101.0-0-g5334f0c2c
2024-09-12T11:57:30.087Z info VictoriaMetrics/lib/logger/flag.go:13 command-line flags
2024-09-12T11:57:30.087Z info VictoriaMetrics/lib/logger/flag.go:20   -httpListenAddr=":8429"
2024-09-12T11:57:30.087Z info VictoriaMetrics/lib/logger/flag.go:20   -promscrape.config="/etc/vmagent/config_out/vmagent.env.yaml"
2024-09-12T11:57:30.087Z info VictoriaMetrics/lib/logger/flag.go:20   -promscrape.maxScrapeSize="128MiB"
2024-09-12T11:57:30.087Z info VictoriaMetrics/lib/logger/flag.go:20   -remoteWrite.maxDiskUsagePerURL="1073741824"
2024-09-12T11:57:30.087Z info VictoriaMetrics/lib/logger/flag.go:20   -remoteWrite.tlsCAFile="/etc/vm/secrets/vmagent-tls-secret/ca.crt"
2024-09-12T11:57:30.087Z info VictoriaMetrics/lib/logger/flag.go:20   -remoteWrite.tlsCertFile="/etc/vm/secrets/vmagent-tls-secret/tls.crt"
2024-09-12T11:57:30.089Z info VictoriaMetrics/lib/logger/flag.go:20   -remoteWrite.tlsKeyFile="secret"
2024-09-12T11:57:30.089Z info VictoriaMetrics/lib/logger/flag.go:20   -remoteWrite.tmpDataPath="/tmp/vmagent-remotewrite-data"
2024-09-12T11:57:30.089Z info VictoriaMetrics/lib/logger/flag.go:20   -remoteWrite.url="secret"
2024-09-12T11:57:30.089Z info VictoriaMetrics/lib/logger/flag.go:20   -tls="true"
2024-09-12T11:57:30.089Z info VictoriaMetrics/lib/logger/flag.go:20   -tlsCertFile="/etc/vm/secrets/vmagent-tls-secret/tls.crt"
2024-09-12T11:57:30.089Z info VictoriaMetrics/lib/logger/flag.go:20   -tlsKeyFile="secret"
2024-09-12T11:57:30.089Z info VictoriaMetrics/app/vmagent/main.go:127 starting vmagent at "[:8429]"...
2024-09-12T11:57:30.089Z info VictoriaMetrics/lib/memory/memory.go:42 limiting caches to 503316480 bytes, leaving 335544320 bytes to the OS according to -memory.allowedPercent=60
2024-09-12T11:57:30.338Z info VictoriaMetrics/lib/persistentqueue/fastqueue.go:66 opened fast persistent queue at "/tmp/vmagent-remotewrite-data/persistent-queue/1_A3E1D505482736A9" with maxInmemoryBlocks=200, it contains 0 pending bytes
2024-09-12T11:57:30.385Z info VictoriaMetrics/app/vmagent/remotewrite/client.go:202 initialized client for -remoteWrite.url="1:secret-url"
2024-09-12T11:57:30.388Z info VictoriaMetrics/app/vmagent/main.go:152 started vmagent in 0.299 seconds
2024-09-12T11:57:30.389Z info VictoriaMetrics/lib/promscrape/scraper.go:113 reading scrape configs from "/etc/vmagent/config_out/vmagent.env.yaml"
2024-09-12T11:57:30.389Z info VictoriaMetrics/lib/httpserver/httpserver.go:119 starting server at https://127.0.0.1:8429/
2024-09-12T11:57:30.389Z info VictoriaMetrics/lib/httpserver/httpserver.go:120 pprof handlers are exposed at https://127.0.0.1:8429/debug/pprof/

See above logs from VMAgent. It loads arguments tls, tlsCertFile and tlsKeyFile and runs VMAgent on https. When I have already provided the certificates to VMAgent, why can't they be used to generate VMServiceScrape using them?

if tls {
if tlsCertFile != "" && tlsKeyFile != "" {
//create VMServiceScrape with certificates
} else {
//create VMServiceScrape with insecureSkipVerify as true
}
}

@naveenbharadwaj-nc
Copy link
Author

Please let me know if new issue is required to continue work on above mentioned problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants