NETOBSERV-764 - ADD v1beta2 api version with simplified Loki configuration options

[acmenezes]

On this PR:

• Add v1beta2 api version
• Move the hub for conversion webhook to this new version
• Add the options for Loki configuration

Example usage (also available in config/samples/flows_v1beta2_flowcollector_lokistack.yaml):

apiVersion: flows.netobserv.io/v1beta2
kind: FlowCollector
spec:
  loki:
    lokiStack:
      name: loki
      namespace: netobserv

DOC: openshift/openshift-docs#63276

[acmenezes]

@jpinsonneau with my latest update to this code I have a working v1beta2 that behaves exactly as the v1beta1. Now it is the new hub. I compiled and tested on a cluster. Would be nice to have a second pair of eyes testing it as it is. I'll proceed to add changes to the loki types.

[jpinsonneau]

@jpinsonneau with my latest update to this code I have a working v1beta2 that behaves exactly as the v1beta1. Now it is the new hub. I compiled and tested on a cluster. Would be nice to have a second pair of eyes testing it as it is. I'll proceed to add changes to the loki types.

Tested and it works as expected 👍 Code looks good also. I wonder if there is a way to simplify these changes in the future instead of changing all the imports 🤔

[codecov]

Codecov Report

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

Comparison is base (530ba8c) 55.69% compared to head (0397cf9) 55.01%.
Report is 6 commits behind head on main.

❗ Current head 0397cf9 differs from pull request most recent head 3de9f1a. Consider uploading reports for the commit 3de9f1a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
- Coverage   55.69%   55.01%   -0.69%     
==========================================
  Files          47       47              
  Lines        6063     6358     +295     
==========================================
+ Hits         3377     3498     +121     
- Misses       2447     2625     +178     
+ Partials      239      235       -4     

Flag	Coverage Δ
unittests	55.01% <24.28%> (-0.69%)	⬇️

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

Files	Coverage Δ
api/v1beta1/flowcollector_types.go	100.00% <ø> (ø)
api/v1beta1/zz_generated.deepcopy.go	0.00% <ø> (-53.11%)	⬇️
api/v1beta2/flowcollector_types.go	100.00% <100.00%> (ø)
controllers/ebpf/agent_controller.go	73.15% <ø> (ø)
...ntrollers/ebpf/internal/permissions/permissions.go	46.06% <ø> (ø)
controllers/flowcollector_controller.go	55.42% <ø> (+4.65%)	⬆️
controllers/flowlogspipeline/flp_ingest_objects.go	73.84% <ø> (ø)
...trollers/flowlogspipeline/flp_ingest_reconciler.go	64.28% <ø> (ø)
...ntrollers/flowlogspipeline/flp_monolith_objects.go	88.46% <ø> (ø)
controllers/flowlogspipeline/flp_reconciler.go	60.56% <ø> (ø)
... and 20 more

... and 4 files with indirect coverage changes

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

[jotak]

It's only the second time we introduce a new CRD version so I must say I'm not very familiar with the process, but in my understanding when doing so, we don't change immediately the "stored" version, it still has to be the previous one that is stored, for at least one release.
Which means, if I'm correct, that the conversions still needs to be v1alpha1 <-> v1beta1, plus the new one v1beta1 <-> v1beta2; but not (yet) v1alpha1 <-> v1beta2 (conversions are from/to the stored version)

@msherif1234 do you confirm if this is correct?

Side question: should we stop serving and/or remove v1alpha1?

[msherif1234]

It's only the second time we introduce a new CRD version so I must say I'm not very familiar with the process, but in my understanding when doing so, we don't change immediately the "stored" version, it still has to be the previous one that is stored, for at least one release. Which means, if I'm correct, that the conversions still needs to be v1alpha1 <-> v1beta1, plus the new one v1beta1 <-> v1beta2; but not (yet) v1alpha1 <-> v1beta2 (conversions are from/to the stored version)

@msherif1234 do you confirm if this is correct?

Side question: should we stop serving and/or remove v1alpha1?
so that is correct we can't make v1beta2 as teh storgae in the same release we introduce it it needs to be done in the following release.
but however v1beta2 can be the new hub and both v1alpha1 and v1beta1 implements the API for CovertFrom() and ConvertTo() similar to what is done here and what was done originally when we introduced the conversion webhook once this PR out of WIP state I will take a closer look pls tag me when its ready

[jotak]

Thanks @acmenezes for starting this!
As you can see it needs a rebase and there's a bunch of conflicts 😨
don't hesitate to ask if you need some help, I can take some time this week to work with you on it

[jotak]

this looks a bit weird to me to allow variable args, while only the 0 index is used, for switching between modes . Is there a reason to do that instead of an explicit switch / single arg?
(anyway this can be addressed in a follow-up)

[jpinsonneau]

the goal was to have the ability to call getConfig with some optionnal param(s) like getConfig("LOKISTACK") and keep the default empty getConfig()

Can you please add any thoughts on testing in NETOBSERV-1302 so we can improve all at once ?

[jpinsonneau]

I fixed the tests according to @msherif1234 solution in 282b28f Thanks for the help 🥳

It's not perfect but it forces v1beta2 to be storage in the unit tests.
Warning: we need to keep the hack CRD up to date to keep it work. That can be automated if we need it long term (but it's not supposed to)

[jpinsonneau]

FYI I'm working on a followup introducing missing modes and automating the hack CRD

[github-actions]

New images:

• quay.io/netobserv/network-observability-operator:148d4a5
• quay.io/netobserv/network-observability-operator-bundle:v0.0.0-148d4a5
• quay.io/netobserv/network-observability-operator-catalog:v0.0.0-148d4a5

They will expire after two weeks.

To deploy this build:

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

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

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-148d4a5
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

[jotak]

thanks, LGTM, I'll report my unresolved comments on the follow-up PR

[jpinsonneau]

@nathan-weinberg since the API changed, I guess this will impact your test bench, at least if you want to create the latest API version and use LOKISTACK mode. Else you can keep creating v1beta1 and it will fallback to MANUAL mode.

Please let us know if you have any issue, and feel free to merge when you are ready.
Thanks !

[nathan-weinberg]

@nathan-weinberg since the API changed, I guess this will impact your test bench, at least if you want to create the latest API version and use LOKISTACK mode. Else you can keep creating v1beta1 and it will fallback to MANUAL mode.

Please let us know if you have any issue, and feel free to merge when you are ready. Thanks !

Thanks for the heads up @jpinsonneau - two quick questions, first does this PR require pre-merge QE testing, second will old flowcollector configs still be compatable? If the latter and not the former I can create a QE automation task to update it sometime this release and we can go ahead and merge this, if the former I can do both at once

[nathan-weinberg]

/cc @Amoghrd

[jpinsonneau]

does this PR require pre-merge QE testing ?

No, it can be done after to unlock other PRs (any touching API is impacted)
@jotak @msherif1234 is it good for you guys to merge as is ?

will old flowcollector configs still be compatable?

Yes it is ! you can create v1beta1 and it will be automatically converted to v1beta2

[openshift-ci]

[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

[jotak]

@acmenezes big thanks for the initial work and for your patience!