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.

Sign up for GitHub

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

Jump to bottom

NETOBSERV-844 Unable to have a working statusUrl in FlowCollector with Loki Operator 5.6 #307

Merged
merged 4 commits into from
Mar 30, 2023
Merged

NETOBSERV-844 Unable to have a working statusUrl in FlowCollector with Loki Operator 5.6 #307

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1c47ff
loki statusTls CRD option
jpinsonneau Mar 27, 2023
7d64e7c
addressed feedback
jpinsonneau Mar 28, 2023
5af9b70
rely on status URL for status TLS
jpinsonneau Mar 28, 2023
9fb3657
status tls default values
jpinsonneau Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions api/v1alpha1/flowcollector_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ func (r *FlowCollector) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.Processor.Metrics.DisableAlerts = restored.Spec.Processor.Metrics.DisableAlerts
}

if restored.Spec.Loki.StatusTLS != nil {
dst.Spec.Loki.StatusTLS = restored.Spec.Loki.StatusTLS
}

return nil
}

Expand Down Expand Up @@ -100,3 +104,10 @@ func Convert_v1beta1_FlowCollectorFLP_To_v1alpha1_FlowCollectorFLP(in *v1beta1.F
func Convert_v1beta1_FLPMetrics_To_v1alpha1_FLPMetrics(in *v1beta1.FLPMetrics, out *FLPMetrics, s apiconversion.Scope) error {
return autoConvert_v1beta1_FLPMetrics_To_v1alpha1_FLPMetrics(in, out, s)
}

// This function need to be manually created because conversion-gen not able to create it intentionally because
// we have new defined fields in v1beta1 not in v1alpha1
// nolint:golint,stylecheck,revive
func Convert_v1beta1_FlowCollectorLoki_To_v1alpha1_FlowCollectorLoki(in *v1beta1.FlowCollectorLoki, out *FlowCollectorLoki, s apiconversion.Scope) error {
return autoConvert_v1beta1_FlowCollectorLoki_To_v1alpha1_FlowCollectorLoki(in, out, s)
}
16 changes: 6 additions & 10 deletions api/v1alpha1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions api/v1beta1/flowcollector_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,11 @@ type FlowCollectorLoki struct {
// tls client configuration.
// +optional
TLS ClientTLS `json:"tls"`

// tls client configuration for status URL.
// fallback on tls if not set
// +optional
StatusTLS *ClientTLS `json:"statusTls"`
}

// FlowCollectorConsolePlugin defines the desired ConsolePlugin state of FlowCollector
Expand Down
5 changes: 5 additions & 0 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

66 changes: 66 additions & 0 deletions bundle/manifests/flows.netobserv.io_flowcollectors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3363,6 +3363,72 @@ spec:
description: staticLabels is a map of common labels to set on
each flow.
type: object
statusTls:
description: tls client configuration for status URL. fallback
on tls if not set
properties:
caCert:
description: caCert defines the reference of the certificate
for the Certificate Authority
properties:
certFile:
description: certFile defines the path to the certificate
file name within the config map or secret
type: string
certKey:
description: certKey defines the path to the certificate
private key file name within the config map or secret.
Omit when the key is not necessary.
type: string
name:
description: name of the config map or secret containing
certificates
type: string
type:
description: 'type for the certificate reference: "configmap"
or "secret"'
enum:
- configmap
- secret
type: string
type: object
enable:
default: false
description: enable TLS
type: boolean
insecureSkipVerify:
default: false
description: insecureSkipVerify allows skipping client-side
verification of the server certificate If set to true, CACert
field will be ignored
type: boolean
userCert:
description: userCert defines the user certificate reference,
used for mTLS (you can ignore it when using regular, one-way
TLS)
properties:
certFile:
description: certFile defines the path to the certificate
file name within the config map or secret
type: string
certKey:
description: certKey defines the path to the certificate
private key file name within the config map or secret.
Omit when the key is not necessary.
type: string
name:
description: name of the config map or secret containing
certificates
type: string
type:
description: 'type for the certificate reference: "configmap"
or "secret"'
enum:
- configmap
- secret
type: string
type: object
type: object
statusUrl:
description: statusURL specifies the address of the Loki /ready
/metrics /config endpoints, in case it is different from the
Expand Down
66 changes: 66 additions & 0 deletions config/crd/bases/flows.netobserv.io_flowcollectors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3350,6 +3350,72 @@ spec:
description: staticLabels is a map of common labels to set on
each flow.
type: object
statusTls:
description: tls client configuration for status URL. fallback
on tls if not set
properties:
caCert:
description: caCert defines the reference of the certificate
for the Certificate Authority
properties:
certFile:
description: certFile defines the path to the certificate
file name within the config map or secret
type: string
certKey:
description: certKey defines the path to the certificate
private key file name within the config map or secret.
Omit when the key is not necessary.
type: string
name:
description: name of the config map or secret containing
certificates
type: string
type:
description: 'type for the certificate reference: "configmap"
or "secret"'
enum:
- configmap
- secret
type: string
type: object
enable:
default: false
description: enable TLS
type: boolean
insecureSkipVerify:
default: false
description: insecureSkipVerify allows skipping client-side
verification of the server certificate If set to true, CACert
field will be ignored
type: boolean
userCert:
description: userCert defines the user certificate reference,
used for mTLS (you can ignore it when using regular, one-way
TLS)
properties:
certFile:
description: certFile defines the path to the certificate
file name within the config map or secret
type: string
certKey:
description: certKey defines the path to the certificate
private key file name within the config map or secret.
Omit when the key is not necessary.
type: string
name:
description: name of the config map or secret containing
certificates
type: string
type:
description: 'type for the certificate reference: "configmap"
or "secret"'
enum:
- configmap
- secret
type: string
type: object
type: object
statusUrl:
description: statusURL specifies the address of the Loki /ready
/metrics /config endpoints, in case it is different from the
Expand Down
13 changes: 12 additions & 1 deletion config/samples/flows_v1beta1_flowcollector.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,18 @@ spec:
# Uncomment lines below for typical installation with loki-operator (5.6+ needed)
# url: 'https://loki-gateway-http.netobserv.svc:8080/api/logs/v1/network/'
# statusUrl: 'https://loki-query-frontend-http.netobserv.svc:3100/'
# authToken: HOST
# authToken: FORWARD
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FORWARD is recommended in the doc; we should also recommend it in the sample

# statusTls:
# enable: true
# caCert:
# certFile: service-ca.crt
# name: loki-ca-bundle
# type: configmap
# userCert:
# certFile: tls.crt
# certKey: tls.key
# name: loki-query-frontend-http
# type: secret
Copy link
Contributor Author

Choose a reason for hiding this comment

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

statusTls needs both loki-ca-bundle and loki-query-frontend-http crt and key files for mTLS

tls:
enable: false
caCert:
Expand Down
18 changes: 18 additions & 0 deletions controllers/consoleplugin/consoleplugin_objects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const configFile = "config.yaml"
const configVolume = "config-volume"
const configPath = "/opt/app-root/"
const lokiCerts = "loki-certs"
const lokiStatusCerts = "loki-status-certs"
const tokensPath = "/var/run/secrets/tokens/"

type builder struct {
Expand Down Expand Up @@ -184,6 +185,18 @@ func buildArgs(desired *flowslatest.FlowCollectorSpec) []string {
args = append(args, "--loki-ca-path", helper.GetCACertPath(&desired.Loki.TLS, lokiCerts))
}
}

statusTLS := helper.LokiStatusTLS(&desired.Loki)
if statusTLS.Enable {
if statusTLS.InsecureSkipVerify {
args = append(args, "-loki-status-skip-tls")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a typo it should be --loki-status-skip-tls ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should harmonize everything with a single minus character as it's already the case for most of these args. I will also fix the existing --loki-ca-path

Thanks for pointing that !

} else {
args = append(args, "--loki-status-ca-path", helper.GetCACertPath(&statusTLS, lokiStatusCerts))
args = append(args, "--loki-status-user-cert-path", helper.GetUserCertPath(&statusTLS, lokiStatusCerts))
args = append(args, "--loki-status-user-key-path", helper.GetUserKeyPath(&statusTLS, lokiStatusCerts))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

can we extend unit-test to cover this newly added field ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/netobserv/network-observability-operator/pull/307/files#diff-22b7d1b615362ec58a371291942fa2bfe7290375729e4fb6b0be03f3af4ca993

if helper.LokiUseHostToken(&desired.Loki) {
args = append(args, "-loki-token-path", tokenPath(&desired.Loki))
}
Expand Down Expand Up @@ -226,6 +239,11 @@ func (b *builder) podTemplate(cmDigest string) *corev1.PodTemplateSpec {
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &b.desired.Loki.TLS, lokiCerts, b.cWatcher)
}

statusTLS := helper.LokiStatusTLS(&b.desired.Loki)
if b.desired != nil && statusTLS.Enable && !statusTLS.InsecureSkipVerify {
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &statusTLS, lokiStatusCerts, b.cWatcher)
}

if helper.LokiUseHostToken(&b.desired.Loki) {
volumes, volumeMounts = helper.AppendTokenVolume(volumes, volumeMounts, constants.PluginName, constants.PluginName)
}
Expand Down
2 changes: 1 addition & 1 deletion controllers/consoleplugin/consoleplugin_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func (r *CPReconciler) reconcileDeployment(ctx context.Context, builder builder,

newDepl := builder.deployment(cmDigest)
// Annotate pod with certificate reference so that it is reloaded if modified
if err := r.CertWatcher.AnnotatePod(ctx, r.Client, &newDepl.Spec.Template, lokiCerts); err != nil {
if err := r.CertWatcher.AnnotatePod(ctx, r.Client, &newDepl.Spec.Template, lokiCerts, lokiStatusCerts); err != nil {
return err
}
if !r.nobjMngr.Exists(r.owned.deployment) {
Expand Down