-
Notifications
You must be signed in to change notification settings - Fork 26
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-309 skip TLS checks & add TenantID #120
Conversation
/ok-to-test |
New image: ["quay.io/netobserv/network-observability-operator:6536b7c"]. It will expire after two weeks. |
@@ -103,6 +103,9 @@ func buildArgs(desired *flowsv1alpha1.FlowCollectorConsolePlugin, desiredLoki *f | |||
"-key", "/var/serving-cert/tls.key", | |||
"-loki", querierURL(desiredLoki), | |||
"-loki-labels", strings.Join(constants.LokiIndexFields, ","), | |||
"-loki-tenant-id", desiredLoki.TenantID, | |||
//TODO: add loki tls config https://issues.redhat.com/browse/NETOBSERV-309 | |||
"-loki-skip-tls", "true", |
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.
this could also be promoted right now as a CR config, like you did for TenantID.
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 can do that if you prefer but the goal is to remove this after TLS implementation
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 don't think it has to be removed, it happens commonly that people want to just quick try software without securing everything, so this is still a good option to have IMO, especially as people can "bring their own Loki". What matters more is that it defaults to false
.
@jpinsonneau - after deploying NOO off this PR and above image, I am no longer seeing cert error on console side, however for FLP post request I am seeing below error: when I do push manually it succeeds: |
Following up on my comment above: #120 (comment): I am getting same error when I try to work with zero-click loki server: not sure if there needs to be some updates to be made to FLP to be able to work with NOO created off this PR |
Hm sorry about that ! It seems We didn't had this in the PoC because we used a I'm still looking for a solution |
@jpinsonneau Could it be related to @OlivierCazade 's netobserv/flowlogs-pipeline#229 ? (missing omitempty in FLP) |
No it actually comes from |
/retest |
/ok-to-test |
New image: ["quay.io/netobserv/network-observability-operator:ba70519"]. It will expire after two weeks. |
@jpinsonneau - I don't know if this PR needs rebase after this PR was merged, since I am running into below error on FLP side: time=2022-06-16T21:41:14Z level=debug msg=findStageType: stage = decode |
Yeah seems to ! Let's rebase |
/ok-to-test |
@memodi I have tested today with latest |
New image: ["quay.io/netobserv/network-observability-operator:474f67e"]. It will expire after two weeks. |
Yes, it works for me too. Thank you. Although I couldn't see any flows on Netflow Table and always ends up in 503 error after trying to load :D I had extra small LokiStack, not sure if Loki from Loki-operator has poor performance than what we install with zero-click Loki config, |
Performances are better on my side with |
/ok-to-test |
/retest |
/ok-to-test |
/lgtm |
vendor/github.com/prometheus/common/config/http_config.go
Outdated
Show resolved
Hide resolved
/retest |
/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 |
Integrate components from Goflow-Kube
Following conversation with @memodi, this PR force skip TLS checks on both FLP & console plugin until implementation and allow configuration of Loki tenant ID