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

Add support for prometheus TLS #533

Merged
merged 4 commits into from
Dec 28, 2020
Merged

Add support for prometheus TLS #533

merged 4 commits into from
Dec 28, 2020

Conversation

Gsantomaggio
Copy link
Member

This closes #479

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

Add TLS support for prometheus

Additional Context

Local Testing

Please ensure you run the unit, integration and system tests before approving the PR.

To run the unit and integration tests:

$ make unit-tests integration-tests

You will need to target a k8s cluster and have the operator deployed for running the system tests.

For example, for a Kubernetes context named dev-bunny:

$ kubectx dev-bunny
$ make destroy deploy-dev
# wait for operator to be deployed
$ make system-tests

@Gsantomaggio
Copy link
Member Author

Tested with:

apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
metadata:
  name:  tls
spec:
  replicas: 2
  tls:
    secretName: rabbitmq-tls-secret
    caSecretName: ca-secret
    disableNonTLSListeners: true

Prometheus:

 additionalPodMonitors:
   - name: rabbitmq
     podMetricsEndpoints:
     - port: prometheus-tls
       scheme: https
       tlsConfig:
         insecureSkipVerify: true
     selector:
       matchLabels:
         app.kubernetes.io/component: rabbitmq
     namespaceSelector:
       any: true

Prometheus UI:

Screenshot 2020-12-24 at 16 38 51

Grafana UI:

Screenshot 2020-12-24 at 17 32 20

@Gsantomaggio Gsantomaggio marked this pull request as ready for review December 24, 2020 16:34
@Gsantomaggio Gsantomaggio changed the title WIP DONT MERGE Add support for prometheus TLS Add support for prometheus TLS Dec 24, 2020
@gerhard gerhard merged commit f5d195b into main Dec 28, 2020
@gerhard gerhard deleted the cluster-operator_479 branch December 28, 2020 11:37
Copy link
Member

@ansd ansd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested this change and observed the following 2 issues.

Issue 1:
Update: Fixed by #541
When I deploy a RabbitMQ cluster with TLS but without disabling non-TLS listeners like

apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
metadata:
  name: r1
  namespace: rabbitmq-system
spec: 
  tls:
    secretName: r1-tls

my expectation is that both Prometheus ports are open, i.e. 15691 and 15692.
However, only 15691 is open:

k exec r1-server-0 -- rabbitmq-diagnostics listeners
Asking node [email protected] to report its protocol listeners ...
Interface: [::], port: 15672, protocol: http, purpose: HTTP API
Interface: [::], port: 15671, protocol: https, purpose: HTTP API over TLS (HTTPS)
Interface: [::], port: 15691, protocol: https/prometheus, purpose: Prometheus exporter API over TLS (HTTPS)
Interface: [::], port: 25672, protocol: clustering, purpose: inter-node and CLI tool communication
Interface: [::], port: 5672, protocol: amqp, purpose: AMQP 0-9-1 and AMQP 1.0
Interface: [::], port: 5671, protocol: amqp/ssl, purpose: AMQP 0-9-1 and AMQP 1.0 over TLS

As we can see with the management and AMQP ports, both the TLS and non-TLS ports are open. Can we do the same with the Prometheus port?

Issue 2:
When I first deploy a RabbitMQ cluster without TLS like

apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
metadata:
  name: r1
  namespace: rabbitmq-system
spec: {}

and thereafter update the cluster to use TLS and to disable non-TLS listeners like

apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
metadata:
  name: r1
  namespace: rabbitmq-system
spec:
  tls:
    secretName: r1-tls
    disableNonTLSListeners: true

my expectation is that the pod annotation prometheus.io/port gets updated to 15691. However the annotation stays 15692:

k get pod r1-server-0 -o yaml | grep 1569 -C 4
apiVersion: v1
kind: Pod
metadata:
  annotations:
    prometheus.io/port: "15692"
    prometheus.io/scrape: "true"
    rabbitmq.com/lastRestartAt: "2021-01-04T16:15:37Z"
  creationTimestamp: "2021-01-04T16:15:38Z"
  generateName: r1-server-
--
--
                .: {}
                f:containerPort: {}
                f:name: {}
                f:protocol: {}
              k:{"containerPort":15691,"protocol":"TCP"}:
                .: {}
                f:containerPort: {}
                f:name: {}
                f:protocol: {}
--
--
      protocol: TCP
    - containerPort: 15671
      name: management-tls
      protocol: TCP
    - containerPort: 15691
      name: prometheus-tls
      protocol: TCP
    readinessProbe:
      failureThreshold: 3

@Gsantomaggio
Copy link
Member Author

thank you for the review.

The first one is solved here: #541

the second one seems to be a problem with the function ReconcileAnnotations https://github.com/rabbitmq/cluster-operator/blob/main/internal/metadata/annotation.go#L14-L16

it doesn't merge the maps correctly

ansd added a commit that referenced this pull request Jan 7, 2021
Before this commit, existing pod annotations took precedence over
updated annotations (e.g. prometheus port).

This commit fixes issue 2 in
#533 (review)
ansd added a commit that referenced this pull request Jan 8, 2021
Before this commit, existing pod annotations took precedence over
updated annotations (e.g. prometheus port).

This commit fixes issue 2 in
#533 (review)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Share TLS config with the Prometheus plugin
3 participants