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

upgrade: controller-runtime and code change accordingly #1189

Merged

Conversation

zdtsw
Copy link
Member

@zdtsw zdtsw commented Aug 19, 2024

Description

split origin PR #1048 into 2 parts.
this PR is part I, focusing on upgrade controller-runtime and clenaup go packages.
details:

  • the current controller-runtime 0.14.6 does not support new features:
  • uplift controller-runtime to v0.17.5 which is the last support go 1.21
  • update Watches() to match 0.17.5
  • webhook in the new 0.16.3 does not use method InjectDecoder InjectClient, updates
  • remove MWC since we are not using it for now
  • cleanup old go.mod, remove unnecessary replacement
  • some formatting for SetupWithManager()
  • add missing "get" on console

How Has This Been Tested?

local build: quay.io/wenzhou/opendatahub-operator-catalog:v2.17.2-60

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@zdtsw zdtsw requested review from bartoszmajsak, lburgazzoli and ykaliuta and removed request for VaishnaviHire and gzaronikas August 19, 2024 10:09
annotations:
service.beta.openshift.io/serving-cert-secret-name: opendatahub-operator-controller-webhook-cert
service.beta.openshift.io/serving-cert-secret-name: opendatahub-operator-webhook-cert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these changes related to the update?

And more general: can I take webhook changes as a separate PR? They do not require the update, can go now already and it will be easier then to take the patch downstream.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can leave the changes in bundle/ + config/webhook/ out from this PR
but the one in the controllers/webhook/ will be needed for 0.17.5 to work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but webhook initialization changes do not require updated runtime and can be submitted right now, don't they?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i understand you correctly:
you want to have another PR only for the webhook decoder part in ODH first, and you want to get it merge into downstream without introduce the upgrade runtime in downstream.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i understand you correctly: you want to have another PR only for the webhook decoder part in ODH first, and you want to get it merge into downstream without introduce the upgrade runtime in downstream.

I want webhook changes in a separate PR, it may be more general with both cleanup in names and manifests (if you want it) and decoder part. To track it for backporting separately from the upgrade.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i have no objection on this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have narrow the changes in this PR, explicitly removed some part of webhook.
but due to the fact with a newer version or controller-runtime the old implementation on webhook has been changed, in this case: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.6/pkg/webhook/admission#InjectDecoderInto is gone.

zdtsw added a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Aug 19, 2024
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw added a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Aug 19, 2024
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw zdtsw force-pushed the chore_60_part1 branch 3 times, most recently from 390a74a to 25e0a86 Compare August 19, 2024 15:59
@ykaliuta
Copy link
Contributor

There are some tiny unrelated changes (some formatting and error texts), but I can put my ACK. Just will wait if @bartoszmajsak has comments.

- the current controller-runtime 0.14.6 does not support new featurs:
- uplift controller-runtime to v0.17.5 which is the last support go 1.21
- update Watches() to match 0.17.5
- webhook in the new 0.16.3 does not use method InjectDecoder InjectClient, updates
- remove MWC since we are not using it for now
- cleanup old go.mod, remove unnecessary replacement
- some formatting for SetupWithManager()
- add missing "get" on console

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
Comment on lines +100 to +101
k8s.io/api => k8s.io/api v0.28.4
k8s.io/client-go => k8s.io/client-go v0.28.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we replace can we say through a comment why is it needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will need to check these two, with a different ticket, just a quick look it will break scheme and flowcontrol

func (r *DSCInitializationReconciler) watchDSCResource(ctx context.Context) []reconcile.Request {
instanceList := &dscv1.DataScienceClusterList{}
if err := r.Client.List(ctx, instanceList); err != nil {
// do not handle if cannot get list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about logging the error to even see that something weird happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

Copy link
Contributor

@bartoszmajsak bartoszmajsak Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an error message too, otherwise it's not really actionable - we are losing information WHY it is failing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change from info to error

main.go Show resolved Hide resolved
- change are needed for working with runtime 0.17.5 still requires here
- InjectDecoder is removed from 0.17.5
- formatting changes in DSCI and DSC controller
- clean up some replace in go.mod

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Copy link
Contributor

@bartoszmajsak bartoszmajsak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a long-awaited upgrade :) Thanks for working on it. I will let someone from the team to formally approve it.

Copy link

openshift-ci bot commented Aug 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ykaliuta

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

@openshift-merge-bot openshift-merge-bot bot merged commit 03c1abc into opendatahub-io:incubation Aug 22, 2024
8 checks passed
zdtsw added a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Aug 22, 2024
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw added a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Aug 22, 2024
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw referenced this pull request in zdtsw-forking/rhods-operator Aug 29, 2024
…-services#1189)

* upgrade: controller-runtime and code change accordingly
- the current controller-runtime 0.14.6 does not support new featurs:
- uplift controller-runtime to v0.17.5 which is the last support go 1.21
- update Watches() to match 0.17.5
- cleanup old go.mod, remove unnecessary replacement
- some formatting for SetupWithManager()
- add missing "get" on console

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
(cherry picked from commit 03c1abc)
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw referenced this pull request in zdtsw-forking/rhods-operator Aug 30, 2024
…-services#1189)

* upgrade: controller-runtime and code change accordingly
- the current controller-runtime 0.14.6 does not support new featurs:
- uplift controller-runtime to v0.17.5 which is the last support go 1.21
- update Watches() to match 0.17.5
- cleanup old go.mod, remove unnecessary replacement
- some formatting for SetupWithManager()
- add missing "get" on console

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
(cherry picked from commit 03c1abc)
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw added a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Aug 30, 2024
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw referenced this pull request in red-hat-data-services/rhods-operator Aug 30, 2024
* upgrade: controller-runtime and code change accordingly
- the current controller-runtime 0.14.6 does not support new featurs:
- uplift controller-runtime to v0.17.5 which is the last support go 1.21
- update Watches() to match 0.17.5
- cleanup old go.mod, remove unnecessary replacement
- some formatting for SetupWithManager()
- add missing "get" on console
---------

(cherry picked from commit 03c1abc)

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Sep 2, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268

This patch reverts

5288015 ("Revert "DSC, DSCI: add validating webhook (opendatahub-io#711)"")

* webhook: add initial skeleton

Originally it was generated with

```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster  --programmatic-validation```

but webhook.Validator interface (like described in the kubebuilder
book[1]) does not work well for the purpose of the webhook due to
needs to access openshift cluster (client.Client) to check existing
instances of DSC.

So, direct implementation of Handler was done inspired by [2] and
odh-notebooks implementation [3].

Move it from api package closer to controllers as in [3] as well
since it's not DataScienceCluster or DSCInitialization extention
anymore. Amend webhook_suite_test.go's path to configs accordingly.

Fix linter issues in webhook_suite_test.go:
- disable ssl check;
- move to package webhook_test

certmanager files removed too due to usage of OpenShift service
serving certificates[4] (see also
service.beta.openshift.io/inject-cabundle annotation in
config/webhook/kustomization.yaml).

Add webhook generation to `make manifests` target so
webhook/manifests.yaml is generated with it.

Since DSCI creation now requires webhook it should be delayed after
manager started. Move it to a closure and add it to the manager for
run with Add() API. It requires explicit declaration of the
interface variable otherwise complains about type mismatch for the
function literal.

[1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation
[2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html
[3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go
[4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* webhook: implement one instance enforcing

The webhook is written with the idea to handle both Create and
Update requests (configured in config/webhook/manifests.yaml), but
at the moment only duplication check on Create is implemented.

Implements the logic which is done now on reconcile time [1] (same
for DSCI).

It checks for 0 instances, not 1, since when the webhook is running
the object has not been created yet. Means if it's 1 then it handles
request to create a second one.

It could be probably possible to use generics but does not make a
lot of sense for such a simple case.

Closes: opendatahub-io#693

[1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: add tests to check duplication blocking

Add both envtest and e2e tests of a second DataScienceCluster
instance creation blocking.

envtest's one is a part of webhook test suite.

e2e:

Add `name` parameter to setupDSCInstance() function to reuse it.

Use require.Error() as the assertion, shorter and more straight
logic than implementing it in the test itself.

Add e2e test to check DSCInitialization similar way.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: e2e: refactor duplication tests in more abstract way

Factor out common code using Unstructured/List objects.

Change structure to remind more prepare/action/assert.

Use "require" features when appropriate.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

---------

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

chore(webhook): (opendatahub-io#870)

- add testcase on DSCI
- remove kubebuilder marker not needed
- remove checks on instance number in existing controllers
- re-generate bundle
- we do not act on update but we keep it on webhook for now

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

fix uncommented tests/e2e/dsc_creation_test.go with a line from
9be146f ("chore(lint): updates to latest version (opendatahub-io#1074)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

webhook: partial: upgrade: controller-runtime and code change accordingly

Partial application of already applied

03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)")

Webhook related changes.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Sep 2, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268

This patch reverts

5288015 ("Revert "DSC, DSCI: add validating webhook (opendatahub-io#711)"")

* webhook: add initial skeleton

Originally it was generated with

```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster  --programmatic-validation```

but webhook.Validator interface (like described in the kubebuilder
book[1]) does not work well for the purpose of the webhook due to
needs to access openshift cluster (client.Client) to check existing
instances of DSC.

So, direct implementation of Handler was done inspired by [2] and
odh-notebooks implementation [3].

Move it from api package closer to controllers as in [3] as well
since it's not DataScienceCluster or DSCInitialization extention
anymore. Amend webhook_suite_test.go's path to configs accordingly.

Fix linter issues in webhook_suite_test.go:
- disable ssl check;
- move to package webhook_test

certmanager files removed too due to usage of OpenShift service
serving certificates[4] (see also
service.beta.openshift.io/inject-cabundle annotation in
config/webhook/kustomization.yaml).

Add webhook generation to `make manifests` target so
webhook/manifests.yaml is generated with it.

Since DSCI creation now requires webhook it should be delayed after
manager started. Move it to a closure and add it to the manager for
run with Add() API. It requires explicit declaration of the
interface variable otherwise complains about type mismatch for the
function literal.

[1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation
[2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html
[3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go
[4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* webhook: implement one instance enforcing

The webhook is written with the idea to handle both Create and
Update requests (configured in config/webhook/manifests.yaml), but
at the moment only duplication check on Create is implemented.

Implements the logic which is done now on reconcile time [1] (same
for DSCI).

It checks for 0 instances, not 1, since when the webhook is running
the object has not been created yet. Means if it's 1 then it handles
request to create a second one.

It could be probably possible to use generics but does not make a
lot of sense for such a simple case.

Closes: opendatahub-io#693

[1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: add tests to check duplication blocking

Add both envtest and e2e tests of a second DataScienceCluster
instance creation blocking.

envtest's one is a part of webhook test suite.

e2e:

Add `name` parameter to setupDSCInstance() function to reuse it.

Use require.Error() as the assertion, shorter and more straight
logic than implementing it in the test itself.

Add e2e test to check DSCInitialization similar way.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: e2e: refactor duplication tests in more abstract way

Factor out common code using Unstructured/List objects.

Change structure to remind more prepare/action/assert.

Use "require" features when appropriate.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

---------

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

chore(webhook): (opendatahub-io#870)

- add testcase on DSCI
- remove kubebuilder marker not needed
- remove checks on instance number in existing controllers
- re-generate bundle
- we do not act on update but we keep it on webhook for now

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

fix uncommented tests/e2e/dsc_creation_test.go with a line from
9be146f ("chore(lint): updates to latest version (opendatahub-io#1074)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

webhook: partial: upgrade: controller-runtime and code change accordingly

Partial application of already applied

03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)")

Webhook related changes.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

tests: e2e: fix requireInstalled to check actuall list emptiness

Partial backport of

6acf1db ("chore: update golangci-lint to v1.60.2, fix misleading test (opendatahub-io#1195)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
zdtsw added a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Sep 2, 2024
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw added a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Sep 2, 2024
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
openshift-merge-bot bot pushed a commit that referenced this pull request Sep 3, 2024
…l and network impact (#1190)

* feat: improve Operator performance by using caching to reduce api call
 and network impact
(work based on #1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* revert: back out changes for webhook

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: move namespace for cache into functions and only add the ones for platform needs

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: rename functions

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw added a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Sep 4, 2024
…l and network impact (opendatahub-io#1190)

* feat: improve Operator performance by using caching to reduce api call
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* revert: back out changes for webhook

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: move namespace for cache into functions and only add the ones for platform needs

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: rename functions

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Sep 12, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268

This patch reverts

5288015 ("Revert "DSC, DSCI: add validating webhook (opendatahub-io#711)"")

* webhook: add initial skeleton

Originally it was generated with

```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster  --programmatic-validation```

but webhook.Validator interface (like described in the kubebuilder
book[1]) does not work well for the purpose of the webhook due to
needs to access openshift cluster (client.Client) to check existing
instances of DSC.

So, direct implementation of Handler was done inspired by [2] and
odh-notebooks implementation [3].

Move it from api package closer to controllers as in [3] as well
since it's not DataScienceCluster or DSCInitialization extention
anymore. Amend webhook_suite_test.go's path to configs accordingly.

Fix linter issues in webhook_suite_test.go:
- disable ssl check;
- move to package webhook_test

certmanager files removed too due to usage of OpenShift service
serving certificates[4] (see also
service.beta.openshift.io/inject-cabundle annotation in
config/webhook/kustomization.yaml).

Add webhook generation to `make manifests` target so
webhook/manifests.yaml is generated with it.

Since DSCI creation now requires webhook it should be delayed after
manager started. Move it to a closure and add it to the manager for
run with Add() API. It requires explicit declaration of the
interface variable otherwise complains about type mismatch for the
function literal.

[1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation
[2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html
[3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go
[4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* webhook: implement one instance enforcing

The webhook is written with the idea to handle both Create and
Update requests (configured in config/webhook/manifests.yaml), but
at the moment only duplication check on Create is implemented.

Implements the logic which is done now on reconcile time [1] (same
for DSCI).

It checks for 0 instances, not 1, since when the webhook is running
the object has not been created yet. Means if it's 1 then it handles
request to create a second one.

It could be probably possible to use generics but does not make a
lot of sense for such a simple case.

Closes: opendatahub-io#693

[1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: add tests to check duplication blocking

Add both envtest and e2e tests of a second DataScienceCluster
instance creation blocking.

envtest's one is a part of webhook test suite.

e2e:

Add `name` parameter to setupDSCInstance() function to reuse it.

Use require.Error() as the assertion, shorter and more straight
logic than implementing it in the test itself.

Add e2e test to check DSCInitialization similar way.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: e2e: refactor duplication tests in more abstract way

Factor out common code using Unstructured/List objects.

Change structure to remind more prepare/action/assert.

Use "require" features when appropriate.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

---------

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

chore(webhook): (opendatahub-io#870)

- add testcase on DSCI
- remove kubebuilder marker not needed
- remove checks on instance number in existing controllers
- re-generate bundle
- we do not act on update but we keep it on webhook for now

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

fix uncommented tests/e2e/dsc_creation_test.go with a line from
9be146f ("chore(lint): updates to latest version (opendatahub-io#1074)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

webhook: partial: upgrade: controller-runtime and code change accordingly

Partial application of already applied

03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)")

Webhook related changes.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

tests: e2e: fix requireInstalled to check actuall list emptiness

Partial backport of

6acf1db ("chore: update golangci-lint to v1.60.2, fix misleading test (opendatahub-io#1195)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

networkpolicy: allow connections from webhook

Allow connection to the operator pod from host network, marked with
label
`policy-group.network.openshift.io/host-network: ""`

https://access.redhat.com/solutions/7008681

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Sep 13, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268

This patch reverts

5288015 ("Revert "DSC, DSCI: add validating webhook (opendatahub-io#711)"")

* webhook: add initial skeleton

Originally it was generated with

```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster  --programmatic-validation```

but webhook.Validator interface (like described in the kubebuilder
book[1]) does not work well for the purpose of the webhook due to
needs to access openshift cluster (client.Client) to check existing
instances of DSC.

So, direct implementation of Handler was done inspired by [2] and
odh-notebooks implementation [3].

Move it from api package closer to controllers as in [3] as well
since it's not DataScienceCluster or DSCInitialization extention
anymore. Amend webhook_suite_test.go's path to configs accordingly.

Fix linter issues in webhook_suite_test.go:
- disable ssl check;
- move to package webhook_test

certmanager files removed too due to usage of OpenShift service
serving certificates[4] (see also
service.beta.openshift.io/inject-cabundle annotation in
config/webhook/kustomization.yaml).

Add webhook generation to `make manifests` target so
webhook/manifests.yaml is generated with it.

Since DSCI creation now requires webhook it should be delayed after
manager started. Move it to a closure and add it to the manager for
run with Add() API. It requires explicit declaration of the
interface variable otherwise complains about type mismatch for the
function literal.

[1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation
[2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html
[3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go
[4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* webhook: implement one instance enforcing

The webhook is written with the idea to handle both Create and
Update requests (configured in config/webhook/manifests.yaml), but
at the moment only duplication check on Create is implemented.

Implements the logic which is done now on reconcile time [1] (same
for DSCI).

It checks for 0 instances, not 1, since when the webhook is running
the object has not been created yet. Means if it's 1 then it handles
request to create a second one.

It could be probably possible to use generics but does not make a
lot of sense for such a simple case.

Closes: opendatahub-io#693

[1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: add tests to check duplication blocking

Add both envtest and e2e tests of a second DataScienceCluster
instance creation blocking.

envtest's one is a part of webhook test suite.

e2e:

Add `name` parameter to setupDSCInstance() function to reuse it.

Use require.Error() as the assertion, shorter and more straight
logic than implementing it in the test itself.

Add e2e test to check DSCInitialization similar way.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: e2e: refactor duplication tests in more abstract way

Factor out common code using Unstructured/List objects.

Change structure to remind more prepare/action/assert.

Use "require" features when appropriate.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

---------

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

chore(webhook): (opendatahub-io#870)

- add testcase on DSCI
- remove kubebuilder marker not needed
- remove checks on instance number in existing controllers
- re-generate bundle
- we do not act on update but we keep it on webhook for now

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

fix uncommented tests/e2e/dsc_creation_test.go with a line from
9be146f ("chore(lint): updates to latest version (opendatahub-io#1074)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

webhook: partial: upgrade: controller-runtime and code change accordingly

Partial application of already applied

03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)")

Webhook related changes.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

tests: e2e: fix requireInstalled to check actuall list emptiness

Partial backport of

6acf1db ("chore: update golangci-lint to v1.60.2, fix misleading test (opendatahub-io#1195)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

networkpolicy: allow connections from webhook

Allow connection to the operator pod from host network, marked with
label
`policy-group.network.openshift.io/host-network: ""`

https://access.redhat.com/solutions/7008681

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
openshift-merge-bot bot referenced this pull request in red-hat-data-services/rhods-operator Sep 13, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268

This patch reverts

5288015 ("Revert "DSC, DSCI: add validating webhook (#711)"")

* webhook: add initial skeleton

Originally it was generated with

```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster  --programmatic-validation```

but webhook.Validator interface (like described in the kubebuilder
book[1]) does not work well for the purpose of the webhook due to
needs to access openshift cluster (client.Client) to check existing
instances of DSC.

So, direct implementation of Handler was done inspired by [2] and
odh-notebooks implementation [3].

Move it from api package closer to controllers as in [3] as well
since it's not DataScienceCluster or DSCInitialization extention
anymore. Amend webhook_suite_test.go's path to configs accordingly.

Fix linter issues in webhook_suite_test.go:
- disable ssl check;
- move to package webhook_test

certmanager files removed too due to usage of OpenShift service
serving certificates[4] (see also
service.beta.openshift.io/inject-cabundle annotation in
config/webhook/kustomization.yaml).

Add webhook generation to `make manifests` target so
webhook/manifests.yaml is generated with it.

Since DSCI creation now requires webhook it should be delayed after
manager started. Move it to a closure and add it to the manager for
run with Add() API. It requires explicit declaration of the
interface variable otherwise complains about type mismatch for the
function literal.

[1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation
[2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html
[3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go
[4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* webhook: implement one instance enforcing

The webhook is written with the idea to handle both Create and
Update requests (configured in config/webhook/manifests.yaml), but
at the moment only duplication check on Create is implemented.

Implements the logic which is done now on reconcile time [1] (same
for DSCI).

It checks for 0 instances, not 1, since when the webhook is running
the object has not been created yet. Means if it's 1 then it handles
request to create a second one.

It could be probably possible to use generics but does not make a
lot of sense for such a simple case.

Closes: #693

[1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: add tests to check duplication blocking

Add both envtest and e2e tests of a second DataScienceCluster
instance creation blocking.

envtest's one is a part of webhook test suite.

e2e:

Add `name` parameter to setupDSCInstance() function to reuse it.

Use require.Error() as the assertion, shorter and more straight
logic than implementing it in the test itself.

Add e2e test to check DSCInitialization similar way.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: e2e: refactor duplication tests in more abstract way

Factor out common code using Unstructured/List objects.

Change structure to remind more prepare/action/assert.

Use "require" features when appropriate.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

---------

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

chore(webhook): (#870)

- add testcase on DSCI
- remove kubebuilder marker not needed
- remove checks on instance number in existing controllers
- re-generate bundle
- we do not act on update but we keep it on webhook for now

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

fix uncommented tests/e2e/dsc_creation_test.go with a line from
9be146f ("chore(lint): updates to latest version (#1074)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

webhook: partial: upgrade: controller-runtime and code change accordingly

Partial application of already applied

03c1abc ("upgrade: controller-runtime and code change accordingly (#1189)")

Webhook related changes.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

tests: e2e: fix requireInstalled to check actuall list emptiness

Partial backport of

6acf1db ("chore: update golangci-lint to v1.60.2, fix misleading test (#1195)")

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

networkpolicy: allow connections from webhook

Allow connection to the operator pod from host network, marked with
label
`policy-group.network.openshift.io/host-network: ""`

https://access.redhat.com/solutions/7008681

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
VaishnaviHire pushed a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Sep 16, 2024
…l and network impact (opendatahub-io#1190)

* feat: improve Operator performance by using caching to reduce api call
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* revert: back out changes for webhook

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: move namespace for cache into functions and only add the ones for platform needs

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: rename functions

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
(cherry picked from commit 5759f5e)
VaishnaviHire pushed a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Sep 16, 2024
…l and network impact (opendatahub-io#1190)

* feat: improve Operator performance by using caching to reduce api call
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* revert: back out changes for webhook

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: move namespace for cache into functions and only add the ones for platform needs

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: rename functions

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
(cherry picked from commit 5759f5e)
ykaliuta pushed a commit to ykaliuta/opendatahub-operator that referenced this pull request Sep 17, 2024
* RHOAIENG-2974: Restrict DSCI deletion before DSC

* Added tests and minor enhancements

* Admission allow DSC

* lint fix

(cherry picked from commit 2439104)

Add DELETE operation for webhook in CSV, upstream

60a44c2 ("update: cleanup remove kfdef during uninstallation (opendatahub-io#1100)")

Amend client -> Client due to already applied

03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)")
openshift-merge-bot bot pushed a commit to red-hat-data-services/rhods-operator that referenced this pull request Sep 18, 2024
…l and network impact (#1190)

* feat: improve Operator performance by using caching to reduce api call
 and network impact
(work based on opendatahub-io#1189)
- secret: default application namespace + other default ones + istio cert
- configmap: all
- namespace: all
- ingressctrler: "default" one
- deployment: default application namespaces + default namespaces

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* revert: back out changes for webhook

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: move namespace for cache into functions and only add the ones for platform needs

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: rename functions

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
(cherry picked from commit 5759f5e)
openshift-merge-bot bot referenced this pull request in red-hat-data-services/rhods-operator Sep 18, 2024
* RHOAIENG-2974: Restrict DSCI deletion before DSC

* Added tests and minor enhancements

* Admission allow DSC

* lint fix

(cherry picked from commit 2439104)

Add DELETE operation for webhook in CSV, upstream

60a44c2 ("update: cleanup remove kfdef during uninstallation (#1100)")

Amend client -> Client due to already applied

03c1abc ("upgrade: controller-runtime and code change accordingly (#1189)")
MarianMacik pushed a commit to MarianMacik/opendatahub-operator that referenced this pull request Jan 22, 2025
…flux/component-updates/odh-modelmesh-serving-controller-v2-17

chore(deps): update odh-modelmesh-serving-controller-v2-17 to 36190bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants