NETOBSERV-788: adding conversion webhook for flowcollector obj #258

msherif1234 · 2023-01-31T18:30:10Z

update to v1beta1 version.
created conversion webhook using v1beta1 as the HUB and v1alpha1 as the spoke, till we have a new release with v1beta1 then we can make v1beta1 as the storage version.
rearrange configs to allow KinD and OpenShift installs since different deployment needs different certifications, to deploy on kind there is new make file option make deploy-kind/undeploy-kind
add Makefile new target for conversion auto-gen tool

Signed-off-by: msherif1234 mmahmoud@redhat.com

jotak · 2023-02-01T07:53:57Z

Hey @msherif1234 ,
Thanks for this PR! Do you have some info about why a dependency on cert-manager is required? I'd like to understand where this is going to lead us in terms of operator dependencies, we should have as few as possible - our (soft) dependencies on loki operator and kafka operator brings already their loads of complexity.
Also, iiuc this dependency is inherent to doing webhooks "the OLM way", I wonder if there are alternatives

msherif1234 · 2023-02-01T11:45:38Z

Hey @msherif1234 , Thanks for this PR! Do you have some info about why a dependency on cert-manager is required? I'd like to understand where this is going to lead us in terms of operator dependencies, we should have as few as possible - our (soft) dependencies on loki operator and kafka operator brings already their loads of complexity. Also, iiuc this dependency is inherent to doing webhooks "the OLM way", I wonder if there are alternatives

cert mgr is needed to extend &authenticate api server to talk to webhook to do admission control this has nothing to do with OLM based operator
https://book.kubebuilder.io/cronjob-tutorial/cert-manager.html#deploying-cert-manager
https://cert-manager.io/docs/concepts/webhook/

I did install for kind env for testing the cert mgr service should be on by default on OCP cluster.

config/crd/bases/flows.netobserv.io_flowcollectors.yaml

openshift-ci-robot · 2023-02-02T17:12:40Z

@msherif1234: This pull request references NETOBSERV-788 which is a valid jira issue.

In response to this:

created v1beta1 version of flowCollector object

created conversion webhook using v1alpha1 as the HUB and v1beta1 as the spoken till we have a new release with v1beta1 then we can make v1beta1 as the new HUB

rearrange configs to allow KinD and Openshift installs since different deployment needs to different certifications tags, to depoly on kind there is new make file option make deploy-kind/undeploy-kind

Signed-off-by: msherif1234 mmahmoud@redhat.com

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2023-02-02T19:01:21Z

@msherif1234: This pull request references NETOBSERV-788 which is a valid jira issue.

In response to this:

update to v1beta1 version.

created conversion webhook using v1alpha1 as the HUB and v1beta1 as the spoken, till we have a new release with v1beta1 then we can make v1beta1 as the new HUB.

rearrange configs to allow KinD and Openshift installs since different deployment needs different certifications, to deploy on kind there is new make file option make deploy-kind/undeploy-kind

Signed-off-by: msherif1234 mmahmoud@redhat.com

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2023-02-02T19:01:34Z

@msherif1234: This pull request references NETOBSERV-788 which is a valid jira issue.

In response to this:

update to v1beta1 version.

created conversion webhook using v1alpha1 as the HUB and v1beta1 as the spoken, till we have a new release with v1beta1 then we can make v1beta1 as the new HUB.

rearrange configs to allow KinD and OpenShift installs since different deployment needs different certifications, to deploy on kind there is new make file option make deploy-kind/undeploy-kind

Signed-off-by: msherif1234 mmahmoud@redhat.com

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

api/v1beta1/flowcollector_webhook.go

bundle/manifests/netobserv-operator.clusterserviceversion.yaml

config/manifests/kustomization.yaml

controllers/consoleplugin/consoleplugin_objects.go

config/manifests/kustomization.yaml

config/openshift/patch.yaml

openshift-ci-robot · 2023-02-08T11:39:05Z

@msherif1234: This pull request references NETOBSERV-788 which is a valid jira issue.

In response to this:

update to v1beta1 version.

created conversion webhook using v1alpha1 as the HUB and v1beta1 as the spoken, till we have a new release with v1beta1 then we can make v1beta1 as the new HUB.

rearrange configs to allow KinD and OpenShift installs since different deployment needs different certifications, to deploy on kind there is new make file option make deploy-kind/undeploy-kind

add Makefile new target for conversion auto-gen tool

Signed-off-by: msherif1234 mmahmoud@redhat.com

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

api/v1beta1/flowcollector_webhook.go

Signed-off-by: msherif1234 <mmahmoud@redhat.com>

JoelSpeed · 2023-02-08T17:21:19Z

api/v1alpha1/flowcollector_webhook.go

+	// Manually restore data.
+	restored := &FlowCollector{}
+	if ok, err := utilconversion.UnmarshalData(r, restored); err != nil || !ok {
+		return err
+	}


Ok this looks good now, so, what fields are present in v1beta1 that aren't present in v1alpha1? Those will need to be manually restored here

I'll add two fields in this PR: #252

Should I add these in both versions of the api or only in v1beta1 ?

This really depends on what you're trying to achieve

Looking at 252, if both of those fields are optional (which they are because they have defaults assigned) then you can add them to v1alpha1 without that being a breaking change. Which brings me back to the question, what are we trying to achieve with a new version of the API, is it to say yes, this API is more mature, or is there some change you want to make that isn't compatible with the current API? (eg renaming or restructuring a field)

For now I would say it's just a matter of API maturity. We restructured a lot before GA and I don't see breaking changes coming for now.
But it's definetly good to have this conversion code in place for the time we will bring breaking changes !

@OlivierCazade @jotak

@JoelSpeed holding means not introducing betav1 at all or just not mark it the new storage API till we have the loki changes in or both ? I remember u said beta version need to get released then after that it will can be the new storage so is the plan to get it out in 4.13 and then make it the new storage with the breaking change in 4.14 ? or get it out in 4.14 with the breaking change then make it the new storage in 4.15 ?

My suggestion would be to hold of introducing v1beta1 as a supported version until you've made those breaking changes. So the first time it gets released would be with the breaking changes you're planning for loki. So, you can merge this all now, but I'd probably remove the CRD schema generation part for v1beta1 until you're ready to start installing that on clusters (ie you have those extra changes in you've talked about)

can we keep all but just let controller and packages refer back to v1alpha1 so that makes the v1beta version noop and once we ready change them to point to v1beta1 ?

You can, but if you expose the v1beta1 API into a running cluster (ie a customer cluster via a release), then you have to support the shape it is at that point, you can't then make breaking changes to it from there

ok then better to hold the entire PR so I at least I can keep the whole context together instead of piece meal and regress it
/hold

openshift-ci-robot · 2023-02-08T21:20:16Z

@msherif1234: This pull request references NETOBSERV-788 which is a valid jira issue.

In response to this:

update to v1beta1 version.

created conversion webhook using v1beta1 as the HUB and v1alpha1 as the spoke, till we have a new release with v1beta1 then we can make v1beta1 as the storage version.

rearrange configs to allow KinD and OpenShift installs since different deployment needs different certifications, to deploy on kind there is new make file option make deploy-kind/undeploy-kind

add Makefile new target for conversion auto-gen tool

Signed-off-by: msherif1234 mmahmoud@redhat.com

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jpinsonneau

LGTM. Deployed on cluster bot and flows.netobserv.io/v1alpha1 is correctly translated to flows.netobserv.io/v1beta1

memodi · 2023-02-09T21:32:22Z

/ok-to-test

github-actions · 2023-02-09T21:35:07Z

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

memodi · 2023-02-09T21:35:19Z

/rm-ok-to-test

msherif1234 · 2023-02-10T15:08:30Z

/hold cancel
since the breaking changes won't happen in the upcoming release(s) then we should proceed as planned and when time comes we will have new api version and by then we should all infra in place to make it just straight fwd

Fix e2e tests with podman

memodi · 2023-02-15T00:27:34Z

/qe-approved
completed basic preliminary testing with NOO image: quay.io/netobserv/network-observability-operator:815b810, comprehensive testing to be followed post-merge.

jotak · 2023-02-15T10:32:18Z

thanks!
/approve

openshift-ci · 2023-02-15T10:32:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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~~ [jotak]

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

…serv#258) * Implementing conversion webhook and add make target for auto conversion Signed-off-by: msherif1234 <mmahmoud@redhat.com> * refactor config to allow deployment on OCP or KinD platforms Signed-off-by: msherif1234 <mmahmoud@redhat.com> * update bundle Signed-off-by: msherif1234 <mmahmoud@redhat.com> * update controllers and pkg to use new api version Signed-off-by: msherif1234 <mmahmoud@redhat.com> --------- Signed-off-by: msherif1234 <mmahmoud@redhat.com>

msherif1234 force-pushed the webhook branch from 19d4ddb to 81d92fd Compare January 31, 2023 23:45

msherif1234 force-pushed the webhook branch 5 times, most recently from 5b80565 to 1b22132 Compare February 2, 2023 14:12

JoelSpeed reviewed Feb 2, 2023

View reviewed changes

config/crd/bases/flows.netobserv.io_flowcollectors.yaml Outdated Show resolved Hide resolved

config/crd/bases/flows.netobserv.io_flowcollectors.yaml Outdated Show resolved Hide resolved

msherif1234 force-pushed the webhook branch 2 times, most recently from cc5d2d6 to df3f2d9 Compare February 2, 2023 16:34

msherif1234 changed the title ~~NETOBSERV-788: WIP adding conversion webhook for flowcollector obj~~ NETOBSERV-788: adding conversion webhook for flowcollector obj Feb 2, 2023

openshift-ci-robot added the jira/valid-reference label Feb 2, 2023

msherif1234 force-pushed the webhook branch 3 times, most recently from 5a6edc8 to 24907ed Compare February 3, 2023 07:00

jotak reviewed Feb 3, 2023

View reviewed changes

api/v1beta1/flowcollector_webhook.go Outdated Show resolved Hide resolved

jotak reviewed Feb 3, 2023

View reviewed changes

api/v1beta1/flowcollector_webhook.go Outdated Show resolved Hide resolved

jotak reviewed Feb 3, 2023

View reviewed changes

bundle/manifests/netobserv-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

jotak reviewed Feb 3, 2023

View reviewed changes

config/manifests/kustomization.yaml Show resolved Hide resolved

msherif1234 force-pushed the webhook branch 2 times, most recently from 60655e6 to e4ebe3a Compare February 4, 2023 12:13

jotak reviewed Feb 6, 2023

View reviewed changes

controllers/consoleplugin/consoleplugin_objects.go Outdated Show resolved Hide resolved

config/manifests/kustomization.yaml Show resolved Hide resolved

jotak reviewed Feb 6, 2023

View reviewed changes

config/openshift/patch.yaml Outdated Show resolved Hide resolved

msherif1234 force-pushed the webhook branch 3 times, most recently from 31f3922 to 635b6c5 Compare February 6, 2023 20:21

msherif1234 force-pushed the webhook branch from f8c7efe to 9311efe Compare February 7, 2023 21:17

JoelSpeed reviewed Feb 8, 2023

View reviewed changes

api/v1beta1/flowcollector_webhook.go Outdated Show resolved Hide resolved

msherif1234 added 2 commits February 8, 2023 11:48

Implementing conversion webhook and add make target for auto conversion

9cc4a00

Signed-off-by: msherif1234 <mmahmoud@redhat.com>

refactor config to allow deployment on OCP or KinD platforms

8fd7521

Signed-off-by: msherif1234 <mmahmoud@redhat.com>

msherif1234 force-pushed the webhook branch from 9311efe to ad7a834 Compare February 8, 2023 16:51

msherif1234 added 2 commits February 8, 2023 11:57

update bundle

6bdcbff

Signed-off-by: msherif1234 <mmahmoud@redhat.com>

update controllers and pkg to use new api version

db2b2ac

Signed-off-by: msherif1234 <mmahmoud@redhat.com>

msherif1234 force-pushed the webhook branch from ad7a834 to db2b2ac Compare February 8, 2023 16:57

JoelSpeed reviewed Feb 8, 2023

View reviewed changes

jpinsonneau mentioned this pull request Feb 9, 2023

NETOBSERV-567 Enable connection tracking in NOO #252

Merged

jpinsonneau approved these changes Feb 9, 2023

View reviewed changes

openshift-ci bot assigned jpinsonneau Feb 9, 2023

openshift-ci bot added the lgtm label Feb 9, 2023

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Feb 9, 2023

openshift-ci bot added the do-not-merge/hold label Feb 10, 2023

openshift-ci bot removed the do-not-merge/hold label Feb 10, 2023

KalmanMeth pushed a commit to KalmanMeth/network-observability-operator that referenced this pull request Feb 13, 2023

Merge pull request netobserv#258 from jotak/e2e-podman

8469fe0

Fix e2e tests with podman

jpinsonneau mentioned this pull request Feb 13, 2023

NETOBSERV-788: WIP, not working: v1 of FlowCollector + conversion webhook #251

Closed

openshift-ci bot added the approved label Feb 15, 2023

openshift-merge-robot merged commit 90d83f5 into netobserv:main Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-788: adding conversion webhook for flowcollector obj #258

NETOBSERV-788: adding conversion webhook for flowcollector obj #258

msherif1234 commented Jan 31, 2023 •

edited

Loading

jotak commented Feb 1, 2023

msherif1234 commented Feb 1, 2023

openshift-ci-robot commented Feb 2, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 2, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 2, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 8, 2023 •

edited by openshift-ci bot

Loading

JoelSpeed Feb 8, 2023

msherif1234 Feb 8, 2023

jpinsonneau Feb 9, 2023

JoelSpeed Feb 9, 2023

jpinsonneau Feb 9, 2023

msherif1234 Feb 10, 2023 •

edited

Loading

JoelSpeed Feb 10, 2023

msherif1234 Feb 10, 2023

JoelSpeed Feb 10, 2023

msherif1234 Feb 10, 2023

openshift-ci-robot commented Feb 8, 2023 •

edited by openshift-ci bot

Loading

jpinsonneau left a comment

memodi commented Feb 9, 2023

github-actions bot commented Feb 9, 2023

memodi commented Feb 9, 2023

msherif1234 commented Feb 10, 2023 •

edited

Loading

memodi commented Feb 15, 2023

jotak commented Feb 15, 2023

openshift-ci bot commented Feb 15, 2023

NETOBSERV-788: adding conversion webhook for flowcollector obj #258

NETOBSERV-788: adding conversion webhook for flowcollector obj #258

Conversation

msherif1234 commented Jan 31, 2023 • edited Loading

jotak commented Feb 1, 2023

msherif1234 commented Feb 1, 2023

openshift-ci-robot commented Feb 2, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented Feb 2, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented Feb 2, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented Feb 8, 2023 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msherif1234 Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 8, 2023 • edited by openshift-ci bot Loading

jpinsonneau left a comment

Choose a reason for hiding this comment

memodi commented Feb 9, 2023

github-actions bot commented Feb 9, 2023

memodi commented Feb 9, 2023

msherif1234 commented Feb 10, 2023 • edited Loading

memodi commented Feb 15, 2023

jotak commented Feb 15, 2023

openshift-ci bot commented Feb 15, 2023

msherif1234 commented Jan 31, 2023 •

edited

Loading

openshift-ci-robot commented Feb 2, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 2, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 2, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 8, 2023 •

edited by openshift-ci bot

Loading

msherif1234 Feb 10, 2023 •

edited

Loading

openshift-ci-robot commented Feb 8, 2023 •

edited by openshift-ci bot

Loading

msherif1234 commented Feb 10, 2023 •

edited

Loading