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

NETOBSERV-309 skip TLS checks & add TenantID #120

Merged
merged 4 commits into from
Jul 22, 2022

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Jun 14, 2022

Following conversation with @memodi, this PR force skip TLS checks on both FLP & console plugin until implementation and allow configuration of Loki tenant ID

@memodi
Copy link
Contributor

memodi commented Jun 14, 2022

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 14, 2022
@github-actions
Copy link

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",
Copy link
Member

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.

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 can do that if you prefer but the goal is to remove this after TLS implementation

Copy link
Member

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.

@memodi
Copy link
Contributor

memodi commented Jun 14, 2022

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

@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:
time=2022-06-14T20:12:01Z level=info component=client error=Post "http://lokistack-sample-distributor-http.network-observability:3100/loki/api/v1/push": proxyconnect tcp: dial tcp :0: connect: connection refused fields.level=warn fields.msg=error sending batch, will retry host=lokistack-sample-distributor-http.network-observability:3100 module=export/loki status=-1

when I do push manually it succeeds:
$ oc exec -it flowlogs-pipeline-55flk -- curl -w "%{http_code}\\n" -k -XPOST https://lokistack-sample-distributor-http.network-observability:3100/loki/api/v1/push --data-raw \ '{"streams": [{ "stream": { "foo": "bar2" }, "values": [ [ "1655238630742828000", "fizzbuzz" ] ] }]}' -H "X-Scope-OrgID: netobserv" -H "Content-Type: application/json" 204

@memodi
Copy link
Contributor

memodi commented Jun 14, 2022

Following up on my comment above: #120 (comment):

I am getting same error when I try to work with zero-click loki server:
time=2022-06-14T20:51:16Z level=info component=client error=Post "http://loki:3100/loki/api/v1/push": proxyconnect tcp: dial tcp :0: connect: connection refused fields.level=warn fields.msg=error sending batch, will retry host=loki:3100 module=export/loki status=-1

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

@jpinsonneau
Copy link
Contributor Author

@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: time=2022-06-14T20:12:01Z level=info component=client error=Post "http://lokistack-sample-distributor-http.network-observability:3100/loki/api/v1/push": proxyconnect tcp: dial tcp :0: connect: connection refused fields.level=warn fields.msg=error sending batch, will retry host=lokistack-sample-distributor-http.network-observability:3100 module=export/loki status=-1

Hm sorry about that ! It seems HTTPClientConfig ProxyURL is used even if it's not set on our side. Since the proxy is not resolved, the query fails.
I have tried to set nil or "" but it doesn't work as is.

We didn't had this in the PoC because we used a map[string]interface{}

I'm still looking for a solution

@jotak
Copy link
Member

jotak commented Jun 15, 2022

@jpinsonneau Could it be related to @OlivierCazade 's netobserv/flowlogs-pipeline#229 ? (missing omitempty in FLP)

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 15, 2022
@jpinsonneau
Copy link
Contributor Author

@jpinsonneau Could it be related to @OlivierCazade 's netobserv/flowlogs-pipeline#229 ? (missing omitempty in FLP)

No it actually comes from prometheus http_config file

@jpinsonneau
Copy link
Contributor Author

/retest

@memodi
Copy link
Contributor

memodi commented Jun 16, 2022

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 16, 2022
@github-actions
Copy link

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

@memodi
Copy link
Contributor

memodi commented Jun 16, 2022

@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
time=2022-06-16T21:41:14Z level=fatal msg=failed to initialize pipeline invalid stage type: unknown

@jpinsonneau
Copy link
Contributor Author

@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

Yeah seems to ! Let's rebase

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 17, 2022
@jpinsonneau
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 17, 2022
@jpinsonneau
Copy link
Contributor Author

@memodi I have tested today with latest flowlogs-pipeline version and it works fine 👍

@github-actions
Copy link

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

@memodi
Copy link
Contributor

memodi commented Jun 17, 2022

@memodi I have tested today with latest flowlogs-pipeline version and it works fine 👍

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

image

I had extra small LokiStack, not sure if Loki from Loki-operator has poor performance than what we install with zero-click Loki config,

@jpinsonneau
Copy link
Contributor Author

Performances are better on my side with LokiStack @memodi
I use gp2 AWS s3 bucket witth extra small config. Do you have a running cluster so I can check what happens ?

@memodi
Copy link
Contributor

memodi commented Jul 8, 2022

/ok-to-test

@memodi
Copy link
Contributor

memodi commented Jul 8, 2022

/retest

@memodi
Copy link
Contributor

memodi commented Jul 8, 2022

/ok-to-test

@jotak
Copy link
Member

jotak commented Jul 13, 2022

/lgtm
cc @OlivierCazade once this is merged we'll need to rebase and fix conflicts on the TLS branch work

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 20, 2022
@jpinsonneau
Copy link
Contributor Author

/retest

@jotak
Copy link
Member

jotak commented Jul 22, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 22, 2022
@jpinsonneau
Copy link
Contributor Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 22, 2022

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jpinsonneau jpinsonneau merged commit e33031a into netobserv:main Jul 22, 2022
KalmanMeth pushed a commit to KalmanMeth/network-observability-operator that referenced this pull request Feb 13, 2023
Integrate components from Goflow-Kube
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants