NETOBSERV-396: Flp prom tls #158

OlivierCazade · 2022-08-26T14:52:56Z

Adding TLS to the FLP prometheus configuration.
This provide two options:

using an automatic mode using openshift annotations
letting the user provide its own certificates

Breaking change: spec.flowlogsPipeline.prometheusPort is now spec.flowlogsPipeline.prometheus.port

jpinsonneau · 2022-08-29T09:02:05Z

config/crd/bases/flows.netobserv.io_flowcollectors.yaml

+                        maximum: 65535
+                        minimum: 1
+                        type: integer
+                      tlsType:


What's the difference between tlsType: DISABLED and enable: false ?

Thanks, I changed it and manualtls only contain cert configuration now.

jotak · 2022-08-31T09:18:06Z

api/v1alpha1/flowcollector_types.go

+
+	// TLS configuration.
+	// +optional
+	ManualTLS ClientTLS `json:"manualTls"`


Maybe we should not reuse ClientTLS here, and create a new struct, I believe it is not 100% relevant when used to configure tls on server side (or at least it should be renamed).
As @jpinsonneau pointed, ClientTLS.enable setting can be confusing as we have TLSType here. Also I believe InsecureSkipVerify is not relevant on server side?

Yes, after Julien comment, I changed the code to use CertificateReference instead of ClientTLS

jotak · 2022-08-31T09:18:43Z

api/v1alpha1/flowcollector_types.go

@@ -201,6 +201,37 @@ type FlowCollectorKafka struct {
 	TLS ClientTLS `json:"tls"`
 }

+const (
+	PrometheusTLSDiasbled = "DISABLED"


nit: typo "Disabled"

Fixed thanks

jotak · 2022-08-31T09:33:06Z

controllers/flowlogspipeline/flp_objects.go

@@ -508,6 +539,48 @@ func (b *builder) service(old *corev1.Service) *corev1.Service {
 	return newService
 }

+func (b *builder) promService(old *corev1.Service) *corev1.Service {


I already didn't like much this pattern of "if service doesn't exist do ... else do ..." (I blame myself for that), now it spreads in the code, it's ok for this PR but at some point I'd like to see if we can have something more concise | readable | elegant ...

I agree, but as your comment state this come from the immutable cluster ip field in the service.

I am curious to know how the kubectl cli works when updating a service. Does it first check existing service and enrich the new one? This would be strange since it add a lot of service specific code to kubectl.

i'm not sure either

jotak · 2022-08-31T13:12:03Z

api/v1alpha1/flowcollector_types.go

+	// Select the type of TLS configuration
+	// "DISABLED" (default) to not configure TLS for the endpoint, "MANUAL" to manually provide cert file and a key file,
+	// and "AUTO" to use Openshift auto generated certificate using annotations
+	// +unionDiscriminator


cc @mariomac is that how unionDiscriminator works? Looking at your PR, I would think that the enum values need to match the discriminated field names; in other words, here, the enum for MANUAL would have to be ManualTLS ?
or this is pure coincidence in your code (with IPFIX/EBPF) and it is not required?

PS I could find any doc about this unionDiscriminator annotation, do you have a link?

According to the documentation here

Discriminant values should be PascalCase and should be equivalent to the camelCase field name (json tag) of one member of the union

So having the enum and the field name matching is a recommendation, I will change the name here.

yeah, in the PR @jotak mention, it is done different, following the suggestion of an API review comment, but double-checking the documentation examples, it seems the correct way is the way Olivier did.

…ions

mariomac

Mostly LGTM. Just few comments open for discussion

mariomac · 2022-09-01T07:59:14Z

api/v1alpha1/flowcollector_types.go

+
+	// TLS configuration.
+	// +optional
+	Manual *CertificateReference `json:"manual"`


I'd change manual by certificate

According to the recommendation this field name should match with the name in the enum discriminator.
I will also set it to provided to match your other comment.

could it be CERTIFICATE is a valid option for the tls type field instead of MANUAL or PROVIDED?

I think having only CERTIFICATE would be a bit ambiguous since AUTO mode also use certificate, it is just that they are generated and not provided by the user.

mariomac · 2022-09-01T07:59:24Z

api/v1alpha1/flowcollector_types.go

+	// "DISABLED" (default) to not configure TLS for the endpoint, "MANUAL" to manually provide cert file and a key file,
+	// and "AUTO" to use Openshift auto generated certificate using annotations
+	// +unionDiscriminator
+	// +kubebuilder:validation:Enum:="DISABLED";"MANUAL";"AUTO"


I'd change MANUAL by PROVIDED

Changed, thanks!

mariomac · 2022-09-01T08:02:52Z

controllers/flowlogspipeline/flp_objects.go

@@ -510,6 +540,48 @@ func (b *builder) service(old *corev1.Service) *corev1.Service {
 	return newService
 }

+func (b *builder) promService(old *corev1.Service) *corev1.Service {
+	if old == nil {


I see that later, promService method is invoked with a non-nil object and also explicitly nil.

Could be this method explicitly divided into two? e.g.:

func (b *builder) newPromService() *corev1.Service func (b *builder) updatedPromService(old *corev1.Service) *corev1.Service

mariomac · 2022-09-01T08:05:16Z

controllers/flowlogspipeline/flp_reconciler.go

@@ -123,8 +126,8 @@ func (r *FLPReconciler) GetServiceName(kafka *flowsv1alpha1.FlowCollectorKafka)
 }

 func (r *FLPReconciler) Reconcile(ctx context.Context, desired *flowsv1alpha1.FlowCollector) error {
-	for _, singleFlp := range r.singleReconcilers {
-		err := singleFlp.Reconcile(ctx, desired)
+	for i := 0; i < len(r.singleReconcilers); i++ {


Maybe it's more "idiomatic":

for i := range r.singleReconcilers

but do it as you prefer.

Yes, thanks

jotak · 2022-09-01T15:47:46Z

/lgtm

Tested with this ServiceMonitor (in "AUTO" mode) :

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  name: flowlogs-pipeline
  namespace: network-observability
spec:
  endpoints:
  - interval: 15s
    port: prometheus
    scheme: https
    tlsConfig:
      insecureSkipVerify: true
  namespaceSelector:
    matchNames:
    - network-observability
  selector:
    matchLabels:
      app: flowlogs-pipeline

Prom config in CR:

    prometheus:
      port: 9102
      tls:
        type: AUTO

Two remarks:

We must figure out what's required to get rid of the insecureSkipVerify: true in ServiceMonitor and document it. I haven't dug much, but had a host name mismatch issue without it.
I just noticed we use the old "alpha" version of the cert annotation, whereas a "beta" now exists and is recommended. We should update everywhere we use it.

jotak · 2022-09-01T16:31:05Z

Quick update: I'm trying with this ServiceMonitor and it doesn't work, I don't know why:

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  name: flowlogs-pipeline
  namespace: network-observability
spec:
  endpoints:
  - interval: 15s
    port: prometheus
    scheme: https
    tlsConfig:
#      insecureSkipVerify: true
      caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt
      serverName: flowlogs-pipeline-prom.network-observability.svc
  namespaceSelector:
    matchNames:
    - network-observability
  selector:
    matchLabels:
      app: flowlogs-pipeline

Whereas this works:

oc exec -n openshift-monitoring prometheus-k8s-0 -- curl https://flowlogs-pipeline-prom.network-observability.svc:9102/metrics --cacert /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt

Which I believe should be an equivalent call...

OlivierCazade · 2022-09-02T07:17:19Z

The CA cert file was already present in the prometheus pod?

jotak · 2022-09-02T07:36:13Z

Yeah I expected the curl command would fail, but it worked :)
See also how other servicemonitors are configured: e.g. from monitor-ovn-node

    - bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token
      interval: 30s
      port: metrics
      scheme: https
      tlsConfig:
        caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt
        serverName: ovn-kubernetes-node.openshift-ovn-kubernetes.svc

They all use the same service-ca.crt, and adapt serverName.

Amoghrd · 2022-09-02T16:33:27Z

/ok-to-test

github-actions · 2022-09-02T16:35:48Z

New image: ["quay.io/netobserv/network-observability-operator:0785789"]. It will expire after two weeks.

OlivierCazade · 2022-09-06T07:21:12Z

I got the service monitor working with this config:

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  name: flowlogs-pipeline
  namespace: network-observability
spec:
  endpoints:
  - interval: 15s
    port: prometheus
    scheme: https
    tlsConfig:
      # caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt
      ca:
        configMap:
          name: "openshift-service-ca.crt"
          key: "service-ca.crt"
      serverName: flowlogs-pipeline-prom.network-observability.svc
  namespaceSelector:
    matchNames:
    - network-observability
  selector:
    matchLabels:
      app: flowlogs-pipeline

My understanding is that user workload metrics are handled by different pods in a different namespace. The contrary to the openshift monitoring pod the ca cert may not be already there.

jotak · 2022-09-06T11:17:33Z

/lgtm

OlivierCazade · 2022-09-06T12:55:55Z

/approve

openshift-ci · 2022-09-06T12:56:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: OlivierCazade

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [OlivierCazade]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

OlivierCazade requested review from mariomac and jpinsonneau August 26, 2022 14:52

openshift-merge-robot added the needs-rebase label Aug 26, 2022

jpinsonneau reviewed Aug 29, 2022

View reviewed changes

OlivierCazade added 3 commits August 30, 2022 16:00

Added promtetheus tls configuration for flp

fbc46b8

Updating flp dependency to get prom tls configuration

03d2c92

Fixed build

b3dbb35

jotak reviewed Aug 31, 2022

View reviewed changes

Changed prometheus tls configuration for better understanding

12ea196

OlivierCazade force-pushed the flp-prom-tls branch from 197dd66 to 12ea196 Compare August 31, 2022 10:04

openshift-merge-robot removed the needs-rebase label Aug 31, 2022

OlivierCazade added 2 commits August 31, 2022 12:27

Fixed typo

47d5ecf

Fixed linting error

4501bbb

jotak reviewed Aug 31, 2022

View reviewed changes

Updated prometheus tls configuration to comply with union recommendat…

3a4eb7b

…ions

mariomac reviewed Sep 1, 2022

View reviewed changes

OlivierCazade added 2 commits September 1, 2022 11:19

Changed code for better comprehension

742ea74

Changed manual name in prom tls discriminator value to provided

c8e9c43

openshift-ci bot assigned jotak Sep 1, 2022

openshift-ci bot added the lgtm label Sep 1, 2022

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 2, 2022

openshift-ci bot added the approved label Sep 6, 2022

openshift-merge-robot merged commit 1a04395 into netobserv:main Sep 6, 2022

jotak added the breaking-change This pull request has breaking changes. They should be described in PR description. label Sep 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-396: Flp prom tls #158

NETOBSERV-396: Flp prom tls #158

OlivierCazade commented Aug 26, 2022 •

edited by jotak

Loading

jpinsonneau Aug 29, 2022

OlivierCazade Aug 31, 2022

jotak Aug 31, 2022

OlivierCazade Aug 31, 2022

jotak Aug 31, 2022

OlivierCazade Aug 31, 2022

jotak Aug 31, 2022

OlivierCazade Aug 31, 2022

jotak Aug 31, 2022

jotak Aug 31, 2022

jotak Aug 31, 2022

OlivierCazade Aug 31, 2022

mariomac Sep 1, 2022

mariomac left a comment

mariomac Sep 1, 2022

OlivierCazade Sep 1, 2022

mariomac Sep 1, 2022

OlivierCazade Sep 1, 2022

mariomac Sep 1, 2022

OlivierCazade Sep 1, 2022

mariomac Sep 1, 2022

mariomac Sep 1, 2022

OlivierCazade Sep 1, 2022

jotak commented Sep 1, 2022 •

edited

Loading

jotak commented Sep 1, 2022

OlivierCazade commented Sep 2, 2022

jotak commented Sep 2, 2022

Amoghrd commented Sep 2, 2022

github-actions bot commented Sep 2, 2022

OlivierCazade commented Sep 6, 2022

jotak commented Sep 6, 2022

OlivierCazade commented Sep 6, 2022

openshift-ci bot commented Sep 6, 2022

NETOBSERV-396: Flp prom tls #158

NETOBSERV-396: Flp prom tls #158

Conversation

OlivierCazade commented Aug 26, 2022 • edited by jotak Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariomac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak commented Sep 1, 2022 • edited Loading

jotak commented Sep 1, 2022

OlivierCazade commented Sep 2, 2022

jotak commented Sep 2, 2022

Amoghrd commented Sep 2, 2022

github-actions bot commented Sep 2, 2022

OlivierCazade commented Sep 6, 2022

jotak commented Sep 6, 2022

OlivierCazade commented Sep 6, 2022

openshift-ci bot commented Sep 6, 2022

OlivierCazade commented Aug 26, 2022 •

edited by jotak

Loading

jotak commented Sep 1, 2022 •

edited

Loading