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

OPRUN-2995: Remove dependency on cluster policy controller in favor of hardcoding #498

Merged

Conversation

perdasilva
Copy link
Contributor

Due to PSA changes, we had to implement another controller in the downstream to label non-payload openshift-* namespaces with CSVs to signal security team's cluster policy controller (CPC) to manage the namespace's PSA security standard label.

Our controller depends on an ignore list exported by the CPC so that we can ignore the same payload namespaces.

There are two problems with this:

  1. OLM's must depend on CPC for the same OpenShift version i.e. OLM@release-4.12 -> CPC@release-4.12
  2. CPC is also a controller and has many of the same dependencies as us (kube api, etc.) and they can interfere with eachother possibly causing vendoring issues. Which is not the type of problem you want when importing a list of strings...

It doesn't seem like there's any good solution here. For our own QoL, this PR removes the dependency and copies the list over to our project. Of course, this can lead to skew...

Having spoken to the security team, they don't think this will change particularly often. In the meantime, we should find some other way to deal with this. Maybe reach out to the security team and ask them to split that list out into its own library and use straight up go types instead of a k8s Set

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2023
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
@perdasilva
Copy link
Contributor Author

/test unit-olm

@perdasilva perdasilva changed the title Remove dependency on cluster policy controller in favor of hardcoding OPRUN-2995: Remove dependency on cluster policy controller in favor of hardcoding Jun 29, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 29, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 29, 2023

@perdasilva: This pull request references OPRUN-2995 which is a valid jira issue.

In response to this:

Due to PSA changes, we had to implement another controller in the downstream to label non-payload openshift-* namespaces with CSVs to signal security team's cluster policy controller (CPC) to manage the namespace's PSA security standard label.

Our controller depends on an ignore list exported by the CPC so that we can ignore the same payload namespaces.

There are two problems with this:

  1. OLM's must depend on CPC for the same OpenShift version i.e. OLM@release-4.12 -> CPC@release-4.12
  2. CPC is also a controller and has many of the same dependencies as us (kube api, etc.) and they can interfere with eachother possibly causing vendoring issues. Which is not the type of problem you want when importing a list of strings...

It doesn't seem like there's any good solution here. For our own QoL, this PR removes the dependency and copies the list over to our project. Of course, this can lead to skew...

Having spoken to the security team, they don't think this will change particularly often. In the meantime, we should find some other way to deal with this. Maybe reach out to the security team and ask them to split that list out into its own library and use straight up go types instead of a k8s Set

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.

@perdasilva
Copy link
Contributor Author

/label qe-approved

@perdasilva
Copy link
Contributor Author

/label px-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jun 29, 2023
@perdasilva
Copy link
Contributor Author

/label docs-approved

@perdasilva
Copy link
Contributor Author

/lgtm

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Jun 29, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2023

@perdasilva: you cannot LGTM your own PR.

In response to this:

/lgtm

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 openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jun 29, 2023
@perdasilva
Copy link
Contributor Author

/override verify

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2023

@perdasilva: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • verify

Only the following failed contexts/checkruns were expected:

  • ci/prow/e2e-gcp-console-olm
  • ci/prow/e2e-gcp-olm
  • ci/prow/e2e-gcp-olm-flaky
  • ci/prow/e2e-gcp-ovn
  • ci/prow/e2e-upgrade
  • ci/prow/images
  • ci/prow/unit-api
  • ci/prow/unit-olm
  • ci/prow/unit-psm
  • ci/prow/unit-registry
  • ci/prow/verify
  • pull-ci-openshift-operator-framework-olm-master-e2e-gcp-console-olm
  • pull-ci-openshift-operator-framework-olm-master-e2e-gcp-olm
  • pull-ci-openshift-operator-framework-olm-master-e2e-gcp-olm-flaky
  • pull-ci-openshift-operator-framework-olm-master-e2e-gcp-ovn
  • pull-ci-openshift-operator-framework-olm-master-e2e-upgrade
  • pull-ci-openshift-operator-framework-olm-master-images
  • pull-ci-openshift-operator-framework-olm-master-unit-api
  • pull-ci-openshift-operator-framework-olm-master-unit-olm
  • pull-ci-openshift-operator-framework-olm-master-unit-psm
  • pull-ci-openshift-operator-framework-olm-master-unit-registry
  • pull-ci-openshift-operator-framework-olm-master-verify
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override verify

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.

@perdasilva
Copy link
Contributor Author

/override ci/prow/verify

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2023

@perdasilva: Overrode contexts on behalf of perdasilva: ci/prow/verify

In response to this:

/override ci/prow/verify

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
Copy link
Contributor

openshift-ci bot commented Jun 29, 2023

@perdasilva: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@perdasilva
Copy link
Contributor Author

This is a bit of maintenance and shouldn't affect product. Therefore I've applied the px/docs/qe-approved labels myself. Also, the verify stage fails because of changes to the staging directory. These changes are downstream only and were necessary. It should be ok to override on these grounds.

Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

Seems reasonable. I'm not seeing the equivalent code upstream, which is expected?

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: perdasilva, tmshort

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

@tmshort
Copy link
Contributor

tmshort commented Jun 29, 2023

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit 8c1c697 into openshift:master Jun 29, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants