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

AsURL always uses HTTP URLs #994

Open
naveenbharadwaj-nc opened this issue Jun 26, 2024 · 19 comments
Open

AsURL always uses HTTP URLs #994

naveenbharadwaj-nc opened this issue Jun 26, 2024 · 19 comments
Labels
bug Something isn't working

Comments

@naveenbharadwaj-nc
Copy link

naveenbharadwaj-nc commented Jun 26, 2024

Hi,

In every *_types.go, the implementation of AsURL() function returns HTTP URL.
return fmt.Sprintf("http://%s.%s.svc:%s", cr.PrefixedName(), cr.Namespace, port)

I'm trying to configure TLS between victoriametrics components which means all components must be accessed via HTTPS. For example, VMAuth must invoke https://vmsingle-.... However, because of hard-coding of http, VMAuth cannot proxy requests to other components. I get following error.

remoteAddr: "10.130.252.154:59080, X-Forwarded-For: 10.230.17.71"; requestURI: /; cannot proxy the request to http://vmsingle-k8s.svc:8429: read tcp 10.130.252.164:33032->172.30.133.124:8429: read: connection reset by peer

Please fix this if its a bug or advise any work arounds.

@f41gh7 f41gh7 added the bug Something isn't working label Jun 26, 2024
f41gh7 added a commit that referenced this issue Jul 2, 2024
its possible only at best effort. https certificates must be valid.

#994
Signed-off-by: f41gh7 <[email protected]>
f41gh7 added a commit that referenced this issue Jul 2, 2024
its possible only at best effort. https certificates must be valid.

#994

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

Haleygo commented Jul 4, 2024

Protocol check is included in v0.46.0, close as completed.
Feel free to reopen if there are further questions.

@Haleygo Haleygo closed this as completed Jul 4, 2024
@naveenbharadwaj-nc
Copy link
Author

It seems like there is one point that is missed. To configure TLS for VMAlertManager, we don't specify tls: true in flags. As per alertmanager documentation, we need to specify tls_config in web.config.file (see documentation).
I can see that no change has been made for vmalertmanager_types.go. It means vmauth will try to proxy the request to HTTP URL of VMAlertmanager. But VMAlertmanager is running on HTTPS. This needs to be addressed.

@naveenbharadwaj-nc
Copy link
Author

naveenbharadwaj-nc commented Jul 16, 2024

Screenshot of vmauth's config.yaml:
image
Logs from vmalertmanager pod that runs on TLS:

ts=2024-07-16T08:24:46.085Z caller=main.go:181 level=info msg="Starting Alertmanager" version="(version=0.27.0, branch=HEAD, revision=0aa3c2aad14cff039931923ab16b26b7481783b5)"
ts=2024-07-16T08:24:46.085Z caller=main.go:182 level=info build_context="(go=go1.21.7, platform=linux/amd64, user=root@22cd11f671e9, date=20240228-11:51:20, tags=netgo)"
ts=2024-07-16T08:24:46.584Z caller=coordinator.go:113 level=info component=configuration msg="Loading configuration file" file=/etc/alertmanager/config/alertmanager.yaml
ts=2024-07-16T08:24:46.584Z caller=coordinator.go:126 level=info component=configuration msg="Completed loading of configuration file" file=/etc/alertmanager/config/alertmanager.yaml
ts=2024-07-16T08:24:46.588Z caller=tls_config.go:313 level=info msg="Listening on" address=[::]:9093
ts=2024-07-16T08:24:46.588Z caller=tls_config.go:349 level=info msg="TLS is enabled." http2=true address=[::]:9093

@naveenbharadwaj-nc
Copy link
Author

naveenbharadwaj-nc commented Jul 16, 2024

I dont seem to have an option to reopen the issue. Please reopen this issue. Let me know if I should create a new issue.

@f41gh7 f41gh7 reopened this Jul 16, 2024
@f41gh7
Copy link
Collaborator

f41gh7 commented Jul 17, 2024

Yes, currently it's tricky to check should it use https scheme or not. Since it only allows to configure it via configuration file.

Maybe it's safe to assume, that if web.config.file extraArg is defined, https should be used.

@naveenbharadwaj-nc
Copy link
Author

I'm not sure if checking the web.config.file extraArg is sufficient. Because the same file can be used to configure HTTP Basic authentication as well. See alertmanager doc. So it may be possible that this file may not be used for TLS, but only to setup Basic Auth? I can suggest to check if tls_server_config is configured in the file identified by web.config.file.

@f41gh7
Copy link
Collaborator

f41gh7 commented Jul 18, 2024

Yes, I share the same concern. Proper solution would be add webConfig to VMAlertmanager.spec and generated configuration file for alertmanager with it. It allows to properly detect scheme for web access and validate web configuration.

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
@AndrewChubatiuk
Copy link
Contributor

released in v0.47.0

@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

I have a secret containing certificates generated using cert-manager that I add to vmAlertManager.Spec.Secrets. This gets mounted to /etc/vm/secrets by the operator. Next I use following logic to construct webConfig:

vmalertmgr.Spec.WebConfig = &vmetricsv1b1.AlertmanagerWebConfig{
					TLSServerConfig: &vmetricsv1b1.TLSServerConfig{
						Certs: vmetricsv1b1.Certs{
							CertFile: "/etc/vm/secrets/vmalertmanager-tls-secret/tls.crt",
							KeyFile:  "/etc/vm/secrets/vmalertmanager-tls-secret/tls.key",
						},
						ClientCAFile:   "/etc/vm/secrets/vmalertmanager-tls-secret/ca.crt",
						ClientAuthType: "RequireAndVerifyClientCert",
					},
				}

which is equivalent to:

webConfig:
    tls_server_config:
      cert_file: /etc/vm/secrets/vmalertmanager-tls-secret/tls.crt
      client_auth_type: RequireAndVerifyClientCert
      client_ca_file: /etc/vm/secrets/vmalertmanager-tls-secret/ca.crt
      key_file: /etc/vm/secrets/vmalertmanager-tls-secret/tls.key

Even though this is a valid configuration, operator looks for web.config.file from /etc/alertmanager/tls_assets.

I think this is a bug. There is no documentation specifying that the secret should be mounted on /etc/alertmanager/tls_assets only. And operator simply accepts all paths as is.

@naveenbharadwaj-nc
Copy link
Author

naveenbharadwaj-nc commented Aug 19, 2024

I have used examples from repository. But I have issues in alertmanager pod for below configuration.

tls_server_config:
  client_ca_file: /etc/alertmanager/tls_assets/vmalertmanager-tls-secret_ca.crt
  cert_file: /etc/alertmanager/tls_assets/vmalertmanager-tls-secret_tls.crt
  key_file: /etc/alertmanager/tls_assets/vmalertmanager-tls-secret_tls.key
  client_auth_type: NoClientCert

ts=2024-08-19T06:39:31.922Z caller=main.go:517 level=error msg="Listen error" err="Client CA's have been configured without a Client Auth Policy"

With client_auth_type: RequireAndVerifyClientCert, I have following error:

2024/08/19 06:54:11 http: TLS handshake error from 10.105.32.121:48140: tls: client didn't provide a certificate

@naveenbharadwaj-nc
Copy link
Author

Liveness probe failed: Get "https://10.131.60.138:9093/-/healthy": remote error: tls: certificate required
Probes fails too.

@f41gh7
Copy link
Collaborator

f41gh7 commented Aug 19, 2024

Liveness probe failed: Get "https://10.131.60.138:9093/-/healthy": remote error: tls: certificate required Probes fails too.

Provided config at examples does not have client ca definition:

   tls_server_config:
    cert_secret_ref:
     key: certValue
     name: alertmanager-tls
    key_secret_ref:
      key: certKey
      name: alertmanager-tls

Remove client_ca_file and it must work.

For mTLS part - configuring mTLS is non-trivial task for kubernetes. It doesn't supported directly by probes.

There are following workarounds:

  1. change probe to TCP. Alertmanager will print error log for each probe. Config-reloader container will not work without client certificate mount and configuration.
  2. build own alertmanager with curl binary and use custom script for probes https://stackoverflow.com/questions/64029212/kubernetes-liveness-probe-on-a-secure-mtls-health-check-endpoint . Configure reloader container with client certificates ( needed client auth options were added at v0.47.0 release of operator config-reloader).

@f41gh7
Copy link
Collaborator

f41gh7 commented Aug 19, 2024

It'd be great to have a guide - "How to configure mTLS with operator".

I think, liveness and readiness probes are useless for mTLS. It's possible to replace it with "no-op" command and configure minReadySeconds for actual start-up probe check. If any runtime issue happened, application will crash and restart.

@f41gh7 f41gh7 reopened this Aug 19, 2024
@naveenbharadwaj-nc
Copy link
Author

naveenbharadwaj-nc commented Aug 19, 2024

livenessProbe:
      timeoutSeconds: 5
      periodSeconds: 5
      successThreshold: 1
      failureThreshold: 10
      httpGet:
        path: "/-/healthy"
        port: web
        scheme: https
 readinessProbe:
      timeoutSeconds: 5
      periodSeconds: 5
      successThreshold: 1
      failureThreshold: 10
      httpGet:
        path: "/-/healthy"
        port: web
        scheme: https

I managed to add custom probes to VMAlertManager and it works. Additionally, I had to remove client_auth_type parameter and client_ca_secret_ref.
May be this can be added in operator logic too? If TLS is enabled, then change scheme of livenessProbe and readinessProbe to https.

f41gh7 added a commit that referenced this issue Aug 26, 2024
@f41gh7
Copy link
Collaborator

f41gh7 commented Aug 26, 2024

See linked example with mTLS configuration for alertmanager. It requires external dependency - cert-manager.

https://github.com/VictoriaMetrics/operator/blob/master/config/examples/vmalertmanager_with_mtls.yaml

@naveenbharadwaj-nc
Copy link
Author

Thank you. As I said earlier, the path at which these certificates are mounted via secret can be any path. I had already used cert-manager to generate certificates successfully. The generated secret by cert-manager was mounted to /etc/vm/secrets. But operator checks /etc/alertmanager/tls_assets.

@f41gh7
Copy link
Collaborator

f41gh7 commented Sep 12, 2024

Could you please give some reproducible example?

You shouldn't care about secret mount point. If you put secret key references into web configuration file, operator will fill mount points on its own.

kind: VMAlertmanager
spec:
  webConfig:
   tls_server_config:
    client_auth_type: RequireAndVerifyClientCert
    client_ca_secret_ref:
     key: ca.crt
     name: alertmanager-tls
    cert_secret_ref:
     key: tls.crt
     name: alertmanager-tls
    key_secret_ref:
      key: tls.key
      name: alertmanager-tls

@naveenbharadwaj-nc
Copy link
Author

naveenbharadwaj-nc commented Sep 13, 2024

I already provided example in above comment. Using cert-manager, I generate a secret containing TLS certificates. Let's call it vmalertmanager-tls-secret.
Next, I specify,
vmalertmanager.Spec.Secrets = append(vmalertmgr.Spec.Secrets, "vmalertmanager-tls-secret")
So, as per documentation, the provided secret will be mounted by the operator into /etc/vm/secrets directory. So, now my TLS certificates are available and successfully mounted on VMAlertManager pod into /etc/vm/secrets folder. But as I pointed out earlier, VMAlertManager looks for certificates at /etc/alertmanager/tls_assets folder.

There are no problems when secretRef is provided as shown in your example. In this case, there are no file paths involved. Certificates are taken from secrets directly. Currently I'm using this approach and this works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants