Added support for configuring CA, cert, and key via secret or configmap. #3249
Conversation
|
Looks like an awesome start! Let us know if you need any input, otherwise looking forward to reviewing the full PR! :) |
ybettan
force-pushed
the
devel
branch
4 times, most recently
from
4b7a627
to
d1ec23e
Compare
test/e2e/main_test.go
Outdated
| //"PromGetAuthSecret": testPromGetAuthSecret, | ||
| //"PromArbitraryFSAcc": testPromArbitraryFSAcc, | ||
| //"PromTLSConfigViaSecret": testPromTLSConfigViaSecret, | ||
| "PromRemoteWriteWithTLS": testPromRemoteWriteWithTLS, |
There was a problem hiding this comment.
fyi these are all subtests so you can run them without uncommenting the rest with:
make test-e2e TEST_RUN_ARGS="-run TestAllNS/y/PromRemoteWriteWithTLS"
There was a problem hiding this comment.
Thanks, I didn't know that.
|
As discussed on slack: I would start by fixing the unit tests they might lead to you to finding what is wrong with e2e tests. https://travis-ci.org/github/coreos/prometheus-operator/jobs/695974726 As for running locally make sure to run the same version as we use in travis, seems like our docs are a bit of out date https://github.com/coreos/prometheus-operator#running-end-to-end-tests-on-local-minikube-cluster (feel free to fix that :) )We use 1.18.2 https://github.com/coreos/prometheus-operator/blob/master/scripts/create-minikube.sh#L12 |
|
Thanks for you replay @lilic. I am not talking about the full e2e tests, I am talking about the only e2e test that run in this PR. Prometheus pods run correctly, prometheus scraping is working as well (checked with grafana) but I no longer get the logs describing a successful send or a "bad request error" according to the test variants. The only thing that looks bugged and make me start with it in order to understand the problem is the output of the command In order to reproduce the error on this PR:
BTW, unit tests on this PR are working correctly (at lease the same way as |
|
The unit tests are failing it says in travis 🤔 or maybe you have not pushed something? Can you just push what you have locally and remove the uncommented tests like Frederic suggested. Some tests maybe rely on others or need to be run in order, so easier to see if you push your work on the PR. thanks! :) |
|
Done. |
ybettan
force-pushed
the
devel
branch
5 times, most recently
from
b97112b
to
f17d5f1
Compare
ybettan
force-pushed
the
devel
branch
8 times, most recently
from
f5584f5
to
eb88372
Compare
ybettan
changed the title
pkg/prometheus/promcfg.go
Outdated
| tlsConfig := yaml.MapSlice{ | ||
| {Key: "insecure_skip_verify", Value: tls.InsecureSkipVerify}, | ||
| } | ||
| if tls.CAFile != "" { | ||
| tlsConfig = append(tlsConfig, yaml.MapItem{Key: "ca_file", Value: tls.CAFile}) | ||
| } | ||
| if tls.CA.Secret != nil { | ||
| tlsConfig = append(tlsConfig, yaml.MapItem{Key: "ca_file", Value: pathPrefix + "_" + tls.CA.Secret.Name + "_" + tls.CA.Secret.Key}) | ||
| tlsConfig = append(tlsConfig, yaml.MapItem{Key: "ca_file", Value: path.Join(secretsDir, tls.CA.Secret.Name, tls.CA.Secret.Key)}) |
There was a problem hiding this comment.
pathPrefix previously had the namespace in there, which is important, this secretsDir is flat, so secrets with the same namespace would collide and cause errors
There was a problem hiding this comment.
I don't mind adding namespace as prefix for secrets names.
In this implementation secretsDir isn't flat, my-secret-a and my-secret-b in the same namespace will be mounted in etc/prometheus/my-secret-a/<secret-a key-values> and etc/prometheus/my-secret-b/<secret-b key-values>
In addition 2 different secrets with the same name but different namespace will fail anyway since only the secret within the same namespace as prometheus pod can be mounted as implemented here https://github.com/coreos/prometheus-operator/blob/f811728eefec5504dd189cbc9534647a858ac0cd/pkg/prometheus/statefulset.go#L534-L542
Am I missing something?
There was a problem hiding this comment.
The same function is used for ServiceMonitors, which can be in different namespaces than the Prometheus object. Meaning this will now collide if there are two ServiceMonitors in different namespaces, each having a secret with the same name in their namespace and referencing it.
pkg/prometheus/promcfg.go
Outdated
| caResourceName = tls.CA.Secret.LocalObjectReference.Name | ||
| caPrefixedResourceName = "secret-" + caResourceName | ||
| if caResourceName != keySecretName && caPrefixedResourceName != certPrefixedResourceName { | ||
| promSpec.Secrets = append(promSpec.Secrets, caResourceName) |
There was a problem hiding this comment.
if this is the strategy we want to go with, then this function should rather return volumes and volumemounts, modifying the prometheus object with extra information inferred from some other settings is not the right thing to do here .. we kind of have precedence of this type of thing, which is the alertmanager endpoint TLS configuration being in the same namespace as the prometheus, I think that strategy would be what we should do for remote write as well
There was a problem hiding this comment.
I am not sure I completely understand what you mean here, I tough that the point of prometheus.Spec.Secrets/prometheus.Spec.ConfigMaps is to describe the names of the secrets/configmaps to be mounted automatically at the StatefulSet stage.
This function isn't mounting actually but just mention the names of the resources to be mounter later in pkg/prometheus/statefulset.go. Which volumes/volumemountes would you expect to be returned?
Can you please elaborate a bit more about you preferred solution - "alertmanager endpoint TLS configuration"?
There was a problem hiding this comment.
Sorry it appears the alertmanager functionality doesn't actually exist. What I was trying to say is prometheus.Spec.ConfigMaps is only ever supposed to be used by users, the operator should not set them in order to force some behavior, that would be a cyclic dependency in some way. We've violated this in the early days of the Prometheus Operator a bit, but we should aim for never modifying the objects coming in to be processed, this only sets up for confusion of what modifies the object where.
Now we can configure the operator to use mTLS RemoteWrite by referencing the CA, cert and key directly from k8s Secrets/ConfigMaps. If the key and the cert are both Secrets, they can exist as a single Secret which contain both 'cert.pem' and 'key.pem' otherwise they can exist as 2 different Secrets (or a Secret for the key and ConfigMap for the cert). Signed-off-by: Yoni Bettan <ybettan@redhat.com>
|
lgtm 👍 🎉 |
Added support for configuring RemoteWrite TLS via Secret or Configmap.
Now we can configure the operator to use mTLS RemoteWrite by referencing
the CA, cert and key directly from k8s Secrets/ConfigMaps.
If the key and the cert are both Secrets, they can exist as a single
Secret which contain both 'cert.pem' and 'key.pem' otherwise they can
exist as 2 different Secrets (or a Secret for the key and ConfigMap for
the cert).
Signed-off-by: Yoni Bettan ybettan@redhat.com
Issue: #3118