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

Change scope of test-operator to namespace #264

Conversation

lpiwowar
Copy link
Collaborator

Test-operator is currently designed to be a cluster scoped operator. This means it can watch and modify resources across all OCP cluster.

This patch changes the operator to namespace scoped operator. By default it is going to watch only:

  • openstack-test-operator namespace: This is a namespace where we recommend to install the test-operator. Prior to the installation we recommend to create an OperatorGroup with targetNamespaces value set to openstack-test-operator and openstack.

  • openstack: This is a namespace where the openstack controll plane is deployed. Test-operator requires an access to this namespace in order to read openstack specific CMs and Secrets (e.g., clouds.yaml).

Copy link

openshift-ci bot commented Dec 11, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

openshift-ci bot commented Dec 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lpiwowar

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

@lpiwowar lpiwowar force-pushed the namespace-scoped-operator branch 2 times, most recently from 7fd0f3a to ad48997 Compare December 12, 2024 12:30
lpiwowar added a commit to openstack-k8s-operators/ci-framework that referenced this pull request Dec 12, 2024
The test-operator got changed from cluster scoped operator to namespace
scoped operator in this PR [1]. It is now recommended that the
test-operator gets installed in a separate namespace other than the one
where the test pods are spawned.

This patch ensures that:

 - the test-operator gets installed in the openstack-test-operator
   namespace

 - the test-operator spawns test pods in the openstack namespace

[1] openstack-k8s-operators/test-operator#264
@lpiwowar lpiwowar force-pushed the namespace-scoped-operator branch from ad48997 to 2f211b0 Compare December 12, 2024 17:30
lpiwowar added a commit to openstack-k8s-operators/ci-framework that referenced this pull request Dec 12, 2024
The test-operator got changed from cluster scoped operator to namespace
scoped operator in this PR [1]. It is now recommended that the
test-operator gets installed in a separate namespace other than the one
where the test pods are spawned.

This patch ensures that:

 - the test-operator gets installed in the openstack-test-operator
   namespace

 - the test-operator spawns test pods in the openstack namespace

[1] openstack-k8s-operators/test-operator#264
lpiwowar added a commit to openstack-k8s-operators/ci-framework that referenced this pull request Dec 17, 2024
The test-operator got changed from cluster scoped operator to namespace
scoped operator in this PR [1]. It is now recommended that the
test-operator gets installed in a separate namespace other than the one
where the test pods are spawned.

This patch ensures that:

 - the test-operator gets installed in the openstack-test-operator
   namespace

 - the test-operator spawns test pods in the openstack namespace

[1] openstack-k8s-operators/test-operator#264
@lpiwowar lpiwowar force-pushed the namespace-scoped-operator branch from 2f211b0 to 883001d Compare December 18, 2024 12:22
lpiwowar added a commit to openstack-k8s-operators/ci-framework that referenced this pull request Dec 19, 2024
The test-operator got changed from cluster scoped operator to namespace
scoped operator in this PR [1]. It is now recommended that the
test-operator gets installed in a separate namespace other than the one
where the test pods are spawned.

This patch ensures that:

 - the test-operator gets installed in the openstack-test-operator
   namespace

 - the test-operator spawns test pods in the openstack namespace

[1] openstack-k8s-operators/test-operator#264
@lpiwowar
Copy link
Collaborator Author

/test all

Test-operator is currently designed to be a cluster scoped
operator. This means it can watch and modify resources across all
OCP cluster.

This patch changes the operator to namespace scoped operator. By
default it is going to watch only:

  - openstack-test-operator namespace: This is a namespace where
    we recommend to install the test-operator. Prior to the
    installation we recommend to create an OperatorGroup with
    targetNamespaces value set to openstack-test-operator and
    openstack.

  - openstack: This is a namespace where the openstack controll
    plane is deployed. Test-operator requires an access to this
    namespace in order to read openstack specific CMs and Secrets
    (e.g., clouds.yaml).

Depends-On: openstack-k8s-operators/ci-framework#2600
@lpiwowar lpiwowar force-pushed the namespace-scoped-operator branch from 883001d to 81c09bd Compare December 19, 2024 16:04
@lpiwowar
Copy link
Collaborator Author

/test all

lpiwowar added a commit to openstack-k8s-operators/ci-framework that referenced this pull request Jan 13, 2025
The test-operator got changed from cluster scoped operator to namespace
scoped operator in this PR [1]. It is now recommended that the
test-operator gets installed in a separate namespace other than the one
where the test pods are spawned.

This patch ensures that:

 - the test-operator gets installed in the openstack-test-operator
   namespace

 - the test-operator spawns test pods in the openstack namespace

[1] openstack-k8s-operators/test-operator#264
Copy link
Contributor

@kstrenkova kstrenkova left a comment

Choose a reason for hiding this comment

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

I have tested this patch and it seems to be running well. The only changes I requested are in the documentation for this because some steps have small mistakes.

I also didn't find the installplans to delete according to documentation 🤔 Maybe it's just a mistake in my testing, but I wanted to mention it here at least.

@kstrenkova
Copy link
Contributor

/lgtm

@stuggi
Copy link
Contributor

stuggi commented Jan 14, 2025

why you want to change the scope? we can not do it. this needs further discussion

@lpiwowar
Copy link
Collaborator Author

/hold

At the moment it looks like that we can not switch the test-operator to project scoped because:

  • OLM might be dropping the namespace scoped operators in the near future.
  • There might be an issue when using mutating webhooks with namespace scoped operators.

@stuggi
Copy link
Contributor

stuggi commented Jan 15, 2025

/hold

At the moment it looks like that we can not switch the test-operator to project scoped because:

  • OLM might be dropping the namespace scoped operators in the near future.
  • There might be an issue when using mutating webhooks with namespace scoped operators.

when there will be a new api version, conversion webhooks can do the migration of existing objects to the new version.
Since the CRDs are global objects, the webhooks should be also global to the cluster to be able to perform the migrate.
In theory you could run the webhooks global and limit the operator to be namespaced, but I have not looked into if
thats something easy we could configured to do.
maybe we could revisit our RBAC to set the roles/rolebindings to only the namespaces where there is a deployment.

@lpiwowar
Copy link
Collaborator Author

maybe we could revisit our RBAC to set the roles/rolebindings to only the namespaces where there is a deployment.

What this PR does is that it allows exactly this. It allows the installation of the test-operator in a separate namespace (openstack-operators) and then it will create the roles and rolebindings in the namespaces specified in the targetNamespace value in the OperatorGroup (openstack).

If you install the test-operator in the openstack-operators namespace it should behave like a cluster scoped operator because of the OperatorGroup which is set there [1]:

apiVersion: operators.coreos.com/v1
kind: OperatorGroup
metadata:
  name: openstack
  namespace: openstack-operators
spec:
  upgradeStrategy: Default
status:
  namespaces:
    - ""

But there is the option of installing it in a separate namespace let's say openstack-test-operators which has its own OperatorGroup set like this:

apiVersion: operators.coreos.com/v1
kind: OperatorGroup
metadata:
  name: openstack
  namespace: openstack-test-operator
spec:
  upgradeStrategy: Default
status:
  namespaces:
    - openstack

@stuggi any thoughts, please? I'm 100 % ok with not merging this :) I just want to be sure that there is a justification for it. So do we agree to keep the test-operator ClusterScoped because of?

  • OLM might be dropping the namespace scoped operators in the near future.
  • There might be an issue when using mutating webhooks with namespace scoped operators.

[1] https://github.com/openstack-k8s-operators/test-operator/pull/264/files#diff-2873f79a86c0d8b3335cd7731b0ecf7dd4301eb19a82ef7a1cba7589b5252261R147

@stuggi
Copy link
Contributor

stuggi commented Jan 15, 2025

maybe we could revisit our RBAC to set the roles/rolebindings to only the namespaces where there is a deployment.

What this PR does is that it allows exactly this. It allows the installation of the test-operator in a separate namespace (openstack-operators) and then it will create the roles and rolebindings in the namespaces specified in the targetNamespace value in the OperatorGroup (openstack).

If you install the test-operator in the openstack-operators namespace it should behave like a cluster scoped operator because of the OperatorGroup which is set there [1]:

apiVersion: operators.coreos.com/v1
kind: OperatorGroup
metadata:
  name: openstack
  namespace: openstack-operators
spec:
  upgradeStrategy: Default
status:
  namespaces:
    - ""

how would the clusterroles be managed in this case? you'd have to manually create them?

But there is the option of installing it in a separate namespace let's say openstack-test-operators which has its own OperatorGroup set like this:

apiVersion: operators.coreos.com/v1
kind: OperatorGroup
metadata:
  name: openstack
  namespace: openstack-test-operator
spec:
  upgradeStrategy: Default
status:
  namespaces:
    - openstack

@stuggi any thoughts, please? I'm 100 % ok with not merging this :) I just want to be sure that there is a justification for it. So do we agree to keep the test-operator ClusterScoped because of?

  • OLM might be dropping the namespace scoped operators in the near future.
  • There might be an issue when using mutating webhooks with namespace scoped operators.

the functionality of the conversion webhooks is crucial, especially for the other service operators. for the test-operator it might be ok to say that you'd have to delete old CRs to unblock installation of a new operator version which brings a new api version, and afterwards you can use the new api, but we can not do it for the other services and from my pov we must be consistent across all operators.

[1] https://github.com/openstack-k8s-operators/test-operator/pull/264/files#diff-2873f79a86c0d8b3335cd7731b0ecf7dd4301eb19a82ef7a1cba7589b5252261R147

@lpiwowar
Copy link
Collaborator Author

how would the clusterroles be managed in this case? you'd have to manually create them?

No, you would not have to create them manually. OLM would create them.

@stuggi
Copy link
Contributor

stuggi commented Jan 15, 2025

how would the clusterroles be managed in this case? you'd have to manually create them?

No, you would not have to create them manually. OLM would create them.

but you are changing the type here from ClusterRole -> Role

@lpiwowar
Copy link
Collaborator Author

but you are changing the type here from ClusterRole -> Role

Yes, I know. I find this confusing as well. I have test-operator index build here which contains test-operator with this PR. If you install test-operator like this:

---
apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: test-operator-catalog
  namespace: openstack-operators
spec:
  sourceType: grpc
  image: quay.io/lpiwowar0/test-operator-index:xxx
---
apiVersion: operators.coreos.com/v1alpha1
kind: Subscription
metadata:
  name: test-operator
  namespace: openstack-operators
spec:
  name: test-operator
  source: test-operator-catalog
  sourceNamespace: openstack-operators

Then the OLM will create ClusterRole.

The resolved set of selected namespaces is surfaced in an OperatorGroup’s status.namespaces field. A global OperatorGroup’s status.namespace is of length 1 and contains the empty string, "", which signals a consuming operator that it should watch all namespaces.

https://olm.operatorframework.io/docs/concepts/crds/operatorgroup/

In contrast if you install test-operator (with this change) like this:

---
apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: test-operator-catalog
  namespace: openstack-test-operator
spec:
  sourceType: grpc
  image: quay.io/lpiwowar0/test-operator-index:xxx
---
apiVersion: operators.coreos.com/v1
kind: OperatorGroup
metadata:
  name: test-operator
  namespace: openstack-test-operator
spec:
  targetNamespaces:
    - openstack
---
apiVersion: operators.coreos.com/v1alpha1
kind: Subscription
metadata:
  name: test-operator
  namespace: openstack-test-operator
spec:
  name: test-operator
  source: test-operator-catalog
  sourceNamespace: openstack-test-operator

then only Roles and RolesBindings will get created for the openstack namespace.

@lpiwowar
Copy link
Collaborator Author

As per the previous comments, I'm closing this PR. We won't be switching the test-operator to a namespace-scoped one.

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.

3 participants