NETOBSERV-1150 Console plugin columns & filters as config #477

jpinsonneau · 2023-10-25T16:20:22Z

Description

import static config for frontend config containing columns and filters definitions.
move console plugin args related to server and loki to configmap for consistency

It also allow devs/users to remove the ownerReferences metadata of any resource created by FlowCollector to skip update on these.

The controller will log the following every reconcile loop:

2023-10-25T16:12:22.874Z INFO SKIP *v1.ConfigMap update since not owned {"controller": "flowcollector", "controllerGroup": "flows.netobserv.io", "controllerKind": "FlowCollector", "FlowCollector": {"name":"cluster"}, "namespace": "", "name": "cluster", "reconcileID": "41a05182-d7ba-4aeb-8a84-c03e3cdd80c2", "Namespace": "netobserv", "Name": "console-plugin-config"}

To quick reload the configmap, you need to restart the associated pod.

TODO: check console arguments that can be moved to configmap

Dependencies

netobserv/network-observability-console-plugin#417

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

codecov · 2023-10-26T16:06:29Z

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (fc1c80c) 54.93% compared to head (9e37a34) 54.91%.
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #477      +/-   ##
==========================================
- Coverage   54.93%   54.91%   -0.03%     
==========================================
  Files          49       49              
  Lines        6449     6485      +36     
==========================================
+ Hits         3543     3561      +18     
- Misses       2662     2675      +13     
- Partials      244      249       +5

Flag	Coverage Δ
unittests	`54.91% <72.53%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
controllers/ebpf/agent_controller.go	`73.15% <100.00%> (ø)`
pkg/helper/client_helper.go	`59.70% <100.00%> (+6.25%)`	⬆️
...ntrollers/ebpf/internal/permissions/permissions.go	`46.06% <50.00%> (ø)`
...ollers/flowlogspipeline/flp_monolith_reconciler.go	`62.50% <50.00%> (ø)`
...trollers/flowlogspipeline/flp_ingest_reconciler.go	`62.60% <0.00%> (ø)`
...rollers/flowlogspipeline/flp_transfo_reconciler.go	`57.63% <0.00%> (ø)`
controllers/reconcilers/common.go	`56.30% <57.14%> (ø)`
...trollers/consoleplugin/consoleplugin_reconciler.go	`61.74% <12.50%> (-1.03%)`	⬇️
controllers/consoleplugin/consoleplugin_objects.go	`91.86% <79.62%> (-2.69%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jpinsonneau · 2023-10-27T13:33:29Z

It's a bit hard to maintain this PR and continue working in parallel on columns. I've included the following PRs in the config:
netobserv/network-observability-console-plugin#423
netobserv/network-observability-console-plugin#422
netobserv/network-observability-console-plugin#425

It would be great to avoid keeping this PR open too long

jotak · 2023-11-07T08:51:07Z

controllers/consoleplugin/consoleplugin_objects.go

+	// ensure volumes are up to date
+	if b.loki.TLS.Enable && !b.loki.TLS.InsecureSkipVerify {
+		b.volumes.AddCACertificate(&b.loki.TLS, "loki-certs")
+	}
+	if b.loki.StatusTLS.Enable && !b.loki.StatusTLS.InsecureSkipVerify {
+		b.volumes.AddMutualTLSCertificates(&b.loki.StatusTLS, "loki-status-certs")
+	}
+	if b.loki.UseHostToken() {
+		b.volumes.AddToken(constants.PluginName)
+	}


it seems like the volumes are added twice: here and from setLokiConfig. Is there a reason? Ideally if we could keep them from setLokiConfig it would be better, so we only have in a single place code related to checking TLS config and doing from the same place both volume mount and passing down TLS config

All of these call the insertOrReplace function behind the scene

network-observability-operator/pkg/volumes/builder.go

Lines 133 to 144 in 5fa0717

func (b *Builder) insertOrReplace(vi *VolumeInfo) {

// find any existing volume info and replace it

for i := range b.info {

if b.info[i].Volume.Name == vi.Volume.Name || b.info[i].Mount.Name == vi.Mount.Name {

b.info[i] = *vi

return

}

}

// else just append new volume info

b.info = append(b.info, *vi)

}

From what I seen, without that piece, the volumes are not up to date

hmm, this is weird ... I'd prefer to keep the builder process as straightforward as possible, we shouldn't need this logic IMO .. I can take a look at that outside of your PR

jotak · 2023-11-07T08:59:10Z

controllers/consoleplugin/consoleplugin_objects.go

-	configStr := "{}"
-	if bs, err := yaml.Marshal(config); err == nil {
+	var configStr string
+	bs, err := yaml.Marshal(config)


good thing to add error handling here ... since you did that you could also do the same line 407 below :-)
(error currently ignored on hasher.Write )

jotak

/lgtm
I'll take a closer look at the volume creation in parallel

github-actions · 2023-11-07T10:02:06Z

New images:

quay.io/netobserv/network-observability-operator:2f78264
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-2f78264
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-2f78264

They will expire after two weeks.

To deploy this build:

# Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:2f78264 make deploy

# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-2f78264

Or as a 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-2f78264
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

openshift-ci · 2023-11-07T16:39:29Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

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

jpinsonneau mentioned this pull request Oct 25, 2023

NETOBSERV-1150 Console plugin columns & filters as config netobserv/network-observability-console-plugin#417

Merged

12 tasks

jpinsonneau marked this pull request as draft October 25, 2023 16:27

openshift-ci bot added the do-not-merge/work-in-progress label Oct 25, 2023

jpinsonneau force-pushed the 1150 branch 2 times, most recently from c70ba5a to 17ae3e9 Compare October 26, 2023 15:59

jpinsonneau force-pushed the 1150 branch from 17ae3e9 to bfcc4e5 Compare October 26, 2023 16:08

jpinsonneau marked this pull request as ready for review October 26, 2023 16:17

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 26, 2023

jpinsonneau requested review from msherif1234 and OlivierCazade October 26, 2023 16:17

openshift-merge-robot added the needs-rebase label Oct 31, 2023

jpinsonneau force-pushed the 1150 branch from 0194ac5 to eb43223 Compare October 31, 2023 12:03

openshift-merge-robot added needs-rebase and removed needs-rebase labels Oct 31, 2023

jpinsonneau added 4 commits November 2, 2023 15:18

fontend config + UpdateIfOwned

de1e422

dns latency & response code as default columns

7055f86

separate DSCP & ICMP columns

90e5442

added DNSErrNo

b74301f

jpinsonneau force-pushed the 1150 branch from eb43223 to b74301f Compare November 2, 2023 14:18

openshift-merge-robot removed the needs-rebase label Nov 2, 2023

jotak reviewed Nov 7, 2023

View reviewed changes

manage hasher error

9e37a34

jotak approved these changes Nov 7, 2023

View reviewed changes

openshift-ci bot assigned jotak Nov 7, 2023

openshift-ci bot added the lgtm label Nov 7, 2023

jpinsonneau added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Nov 7, 2023

jpinsonneau added the approved label Nov 7, 2023

jpinsonneau added the no-qe This PR doesn't necessitate QE approval label Nov 7, 2023

openshift-merge-bot bot merged commit 6c1b0b7 into netobserv:main Nov 7, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-1150 Console plugin columns & filters as config #477

NETOBSERV-1150 Console plugin columns & filters as config #477

jpinsonneau commented Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

jpinsonneau commented Oct 27, 2023 •

edited

Loading

jotak Nov 7, 2023 •

edited

Loading

jpinsonneau Nov 7, 2023

jotak Nov 7, 2023 •

edited

Loading

jotak Nov 7, 2023

jpinsonneau Nov 7, 2023

jotak left a comment

github-actions bot commented Nov 7, 2023

openshift-ci bot commented Nov 7, 2023

	func (b Builder) insertOrReplace(vi VolumeInfo) {
	// find any existing volume info and replace it
	for i := range b.info {
	if b.info[i].Volume.Name == vi.Volume.Name \|\| b.info[i].Mount.Name == vi.Mount.Name {
	b.info[i] = *vi
	return
	}
	}

	// else just append new volume info
	b.info = append(b.info, *vi)
	}

NETOBSERV-1150 Console plugin columns & filters as config #477

NETOBSERV-1150 Console plugin columns & filters as config #477

Conversation

jpinsonneau commented Oct 25, 2023 • edited Loading

Description

Dependencies

Checklist

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

jpinsonneau commented Oct 27, 2023 • edited Loading

jotak Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

jpinsonneau Nov 7, 2023

Choose a reason for hiding this comment

jotak Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

jotak Nov 7, 2023

Choose a reason for hiding this comment

jpinsonneau Nov 7, 2023

Choose a reason for hiding this comment

jotak left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2023

openshift-ci bot commented Nov 7, 2023

jpinsonneau commented Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

jpinsonneau commented Oct 27, 2023 •

edited

Loading

jotak Nov 7, 2023 •

edited

Loading

jotak Nov 7, 2023 •

edited

Loading