-
Notifications
You must be signed in to change notification settings - Fork 24
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
NETOBSERV-844 Unable to have a working statusUrl in FlowCollector with Loki Operator 5.6 #307
Conversation
Codecov Report
@@ Coverage Diff @@
## main #307 +/- ##
==========================================
+ Coverage 49.67% 50.41% +0.73%
==========================================
Files 43 43
Lines 5075 5096 +21
==========================================
+ Hits 2521 2569 +48
+ Misses 2343 2318 -25
+ Partials 211 209 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
args = append(args, "--loki-status-user-key-path", helper.GetUserKeyPath(&statusTLS, lokiStatusCerts)) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statusTLS := helper.LokiStatusTLS(&desired.Loki) | ||
if statusTLS.Enable { | ||
if statusTLS.InsecureSkipVerify { | ||
args = append(args, "-loki-status-skip-tls") |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 !
pkg/helper/flowcollector.go
Outdated
@@ -42,6 +42,13 @@ func LokiForwardUserToken(spec *flowslatest.FlowCollectorLoki) bool { | |||
return spec.AuthToken == flowslatest.LokiAuthForwardUserToken | |||
} | |||
|
|||
func LokiStatusTLS(spec *flowslatest.FlowCollectorLoki) flowslatest.ClientTLS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: GetLokiStatusTLS
?
/ok-to-test |
New images:
They will expire after two weeks. Catalog source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-1b8fdb7
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
pkg/helper/flowcollector.go
Outdated
if spec.StatusTLS != nil { | ||
return *spec.StatusTLS | ||
} | ||
return spec.TLS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having a pointer and allowing nil to switch to using global TLS , wouldn't it be better to rely on whether the status URL is set, to look at StatusTLS or just TLS ?
- If we use main URL as the status URL, then we use also main TLS as the TLS config - StatusTLS is unused in that case
- If statusURL is set, look at StatusTLS (which can still be disabled if not wanted)
IMO it makes things more explicit so less surprising (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm |
/ok-to-test |
New images:
They will expire after two weeks. Catalog source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-f80106b
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
@Amoghrd I've restored the default values as we discussed together 467289f
|
/ok-to-test |
/rm ok-to-test |
/ok-to-test |
New images:
They will expire after two weeks. Catalog source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-56e777e
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
/ok-to-test |
New images:
They will expire after two weeks. Catalog source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-9f45e7d
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
/label qe-approved |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpinsonneau 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:
Approvers can indicate their approval by writing |
Added
statusTls
option in CRD for status URLCheck example yaml: