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

feat : Operator Installation outside of targetNamespace #2589

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bnshr
Copy link
Contributor

@bnshr bnshr commented Nov 25, 2024

Refers the issue

@bnshr bnshr marked this pull request as draft November 25, 2024 14:08
@bnshr bnshr force-pushed the CNFCERT-1088 branch 3 times, most recently from 98cb58b to 56d4773 Compare November 25, 2024 14:39
@dcibot
Copy link
Collaborator

dcibot commented Nov 25, 2024

@bnshr bnshr force-pushed the CNFCERT-1088 branch 2 times, most recently from 1f928d0 to e3fd824 Compare November 25, 2024 15:09
@dcibot
Copy link
Collaborator

dcibot commented Nov 25, 2024

@bnshr bnshr force-pushed the CNFCERT-1088 branch 2 times, most recently from 33278d6 to 26570f7 Compare November 25, 2024 16:20
@dcibot
Copy link
Collaborator

dcibot commented Nov 25, 2024

@bnshr bnshr marked this pull request as ready for review November 25, 2024 19:17
Copy link
Member

@sebrandon1 sebrandon1 left a comment

Choose a reason for hiding this comment

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

Initial set of comments

tests/operator/suite.go Outdated Show resolved Hide resolved
tests/operator/helper.go Outdated Show resolved Hide resolved
tests/operator/helper.go Outdated Show resolved Hide resolved
tests/operator/helper.go Show resolved Hide resolved

csvNamespace := csv.Namespace
operatorNamespace := csv.Annotations["olm.operatorNamespace"]
targetNamespaces := operator.TargetNamespaces
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm wrong but, to me, "tenant app namespaces" are the ones declared in certsuite_config.yaml (field targetNamespaces), not the operator.TargetNamespaces (which comes from operatorGroup.spec.targetNamespaces).

So isn't it good enough just checking whether the operator installation namespace (operator.Csv.Annotations["olm.operatorNamespace"]) is in the certsuite_config.yaml's "targetNamespaces" field?

Copy link
Contributor Author

@bnshr bnshr Nov 29, 2024

Choose a reason for hiding this comment

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

As per the requirement, the tenant dedicated operator namespace is mentioned by the namespace where CR is allowed to be created. In any case, we are not considering any other namespace other than the config file specified namespaces for our testing. But the check on targetNamespace is better to be done in the operator specific way (generic), instead of going back to our configuration.

Copy link
Member

@edcdavid edcdavid Dec 10, 2024

Choose a reason for hiding this comment

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

The requirement for this test is:

Operators are not permitted to be installed into a tenant app namespace. Operators that are installed with the "Single Namespace" olm install mode must be installed into the tenants dedicated operator namespace. Upstream 3rd party operators will be installed Globally with "All Namespaces" olm install mode into the openshift-operators namespace.

In my understanding, the idea of this test is to make sure that tenants dedicate a special namespace for installing operator(s) and that it is not shared with other CNF application namespaces. So I would expect the dedicated operator namespace to be part of the certsuite_config.yaml's "targetNamespaces" filed as @greyerof mentioned, since this hold the namespaces under test. Also, this reserved namespace should only be holding resources deployed/owned by the operator. So if non-operator pods are found in a namespace containing operator pods/resources, the test should fail.

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.

5 participants