OCPBUGS-60380: Backporting NetworkPolicy support for OLM bundles #429
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@anik120: This pull request references Jira Issue OCPBUGS-60260, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
anik120
changed the title
|
@anik120: This pull request references Jira Issue OCPBUGS-60380, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@anik120: This pull request references Jira Issue OCPBUGS-60380, which is invalid:
Comment In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@anik120: This pull request references Jira Issue OCPBUGS-60380, which is invalid:
Comment In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@anik120: This pull request references Jira Issue OCPBUGS-60380, which is valid. The bug has been moved to the POST state. 7 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
anik120
force-pushed
the
backport-networkpolicy-support
branch
from
21de341 to
b8409b0
Compare
This is a hack to backport "NetworkPolicy object kind in bundles" support from operator-framework/operator-registry#1675 The feature was introduced in OCP 4.20 with a operator-registry bump to v1.55.0. Ref: 1. operator-registry v1.55.0 release: https://github.com/operator-framework/operator-registry/releases/tag/v1.55.0 2. operator-registry bump in OCP 4.20: operator-framework/operator-controller#1981 Because the upstream PR is not being backported in operator-registry to older tags (with new z stream releases), this achieves a downstream-only backport for this feature.
anik120
force-pushed
the
backport-networkpolicy-support
branch
from
b8409b0 to
406020c
Compare
|
@anik120: This pull request references Jira Issue OCPBUGS-60380, which is valid. 7 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/approve I'd want someone else to also review this method. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anik120, 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 |
openshift-ci
bot
added
the
approved
|
/label backport-risk-assessed |
openshift-ci
bot
added
the
backport-risk-assessed
| // Because the upstream PR is not being backported in operator-registry to older tags (with new z stream releases), | ||
| // this achieves a downstream-only backport for this feature. | ||
| if obj.GetKind() == "NetworkPolicy" { | ||
| supported, namespaced = true, true |
There was a problem hiding this comment.
I do not think it will not work well to add directly here.
It should be added in: https://github.com/operator-framework/operator-controller/blob/3d6a33b60dab6aedec2b676eba3a7631d3961340/internal/operator-controller/rukpak/render/registryv1/generators/generators.go#L282-L298
@tmshort ^ WDYT?
There was a problem hiding this comment.
I don't think adding it to generators.go replaces this. But I agree that it looks like there are multiple places where operator-registry's bundle.IsSupported is used and we need to patch all of those places or make one function where we add the new validation and then use that everywhere that operator-registry's bundle.IsSupported is used.
There was a problem hiding this comment.
Hold on what commit are you looking at @camilamacedo86 coz I don't see the rukpak/render folder in the release-4.19 branch
I did a search in the 4.19 release branch and only one place popped up that's using isSupported(), and that's patched in this PR
There was a problem hiding this comment.
Oh you're looking at upstream @camilamacedo86.
Please see the downstream repo in its 4.19 release, there's only this one occurrence.
There was a problem hiding this comment.
Ah, sorry, I didn't catch this before. @camilamacedo86 linked the upstream code. This change is downstream only.
There was a problem hiding this comment.
But what is the goal of the change?
Should not that be in 4.20 as well?
Why just 4.19 ? Why do we need to support NP in 4.19?
How do we test this one?
Did we have a pre-merge test?
There was a problem hiding this comment.
Here's the business context that @oceanc80 sent me:
basically the pipelines inconsistently test/stop NPs so to mitigate on multiple fronts, they're adding checks to stop NPs and we're backporting the feature so we won't break anyone if there ends up being an operator with NPs in the catalog
I went with the PR number based on this conversation with Todd: https://redhat-internal.slack.com/archives/C03MFMLT8TY/p1744830832493459?thread_ts=1744830637.432549&cid=C03MFMLT8TY |
|
@tmshort @camilamacedo86 @camilamacedo86 fyi pushed a new commit, and pulled in the e2e tests for network policy: bc0f609#diff-3529e12a1beab8415b618ce80377d3618461a794a769398decde5e4e79cd5a85 That gives us more confidence on this backport. |
Adds a Netwok Policy to the end-to-end test bundles and adds a check to the tests that the Network Policy resources are created. Signed-off-by: Tayler Geiger <[email protected]>
anik120
force-pushed
the
backport-networkpolicy-support
branch
from
4f310d1 to
f140c53
Compare
|
Hey folks, We need to ensure a commit message with the prefix
Otherwise, it will fail in the very commit check ;-) Could we squash add both people as authors and fix the message? |
@camilamacedo86 please see this comment #429 (comment) Had this discussion already with Catherine |
|
verify-commits is passing though |
| // NOTE: This is a hack to backport "NetworkPolicy object kind in bundles" support from | ||
| // https://github.com/operator-framework/operator-registry/pull/1675 | ||
| // The feature was introduced in OCP 4.20 with a operator-registry bump to v1.55.0. | ||
| // Ref: | ||
| // 1. operator-registry v1.55.0 release: https://github.com/operator-framework/operator-registry/releases/tag/v1.55.0 | ||
| // 2. operator-registry bump in OCP 4.20: https://github.com/operator-framework/operator-controller/pull/1981 | ||
| // Because the upstream PR is not being backported in operator-registry to older tags (with new z stream releases), | ||
| // this achieves a downstream-only backport for this feature. |
There was a problem hiding this comment.
| // NOTE: This is a hack to backport "NetworkPolicy object kind in bundles" support from | |
| // https://github.com/operator-framework/operator-registry/pull/1675 | |
| // The feature was introduced in OCP 4.20 with a operator-registry bump to v1.55.0. | |
| // Ref: | |
| // 1. operator-registry v1.55.0 release: https://github.com/operator-framework/operator-registry/releases/tag/v1.55.0 | |
| // 2. operator-registry bump in OCP 4.20: https://github.com/operator-framework/operator-controller/pull/1981 | |
| // Because the upstream PR is not being backported in operator-registry to older tags (with new z stream releases), | |
| // this achieves a downstream-only backport for this feature. | |
| // Backport operator-registry support for the "NetworkPolicy" kind so that | |
| // catalogs do not break when a bundle includes NPs. CI/pipeline checks to | |
| // block NPs are being added but are currently inconsistent; this change keeps | |
| // older streams— including 4.19—resilient. Some 4.19-distributed bundles may | |
| // already include NPs, and we are officially supporting them from 4.20+. | |
| // | |
| // References: | |
| // - Upstream PR: https://github.com/operator-framework/operator-registry/pull/1675 | |
| // - v1.55.0 release: https://github.com/operator-framework/operator-registry/releases/tag/v1.55.0 | |
| // - OCP 4.20 bump: https://github.com/operator-framework/operator-controller/pull/1981 |
What do you think about clarifying why we are doing this, so that in the future, if we face any issues, it’s easier to understand why it was added here?
There was a problem hiding this comment.
@camilamacedo86 I just edited the description to add the following line at the end:
We're backporting this support to prevent any breakage in case a bundle is included in a 4.19 catalog.
That should be enough for someone looking at that comment, and following the PR link to this description (also trying to prevent pushing again to re-run CI)
There was a problem hiding this comment.
I think the context is very helpful here
How can we know that we are adding it in 4.19, since we should only support it from 4.20?
Where is this info clear?
There was a problem hiding this comment.
We could make the info more straightforward here and in the description.
Just as I need to ask to understand what it is about someone, someone will also need to do the same when looking at the code in this PR. However, I will not block this one due to it.
/lgtm
| @@ -383,6 +401,11 @@ func TestClusterExtensionInstallRegistry(t *testing.T) { | |||
| require.NoError(t, c.Get(context.Background(), types.NamespacedName{Namespace: ns.Name, Name: "test-configmap"}, &cm)) | |||
| require.Contains(t, cm.Annotations, "shouldNotTemplate") | |||
| require.Contains(t, cm.Annotations["shouldNotTemplate"], "{{ $labels.namespace }}") | |||
| t.Log("By eventually creating the NetworkPolicy named 'test-operator-network-policy'") | |||
There was a problem hiding this comment.
👍 Nice thank you !!!
|
Thanks for looking into that. It’s now passing in the latest commit. Once that’s addressed, I’m happy to LGTM. |
|
/test openshift-e2e-aws |
|
@anik120: 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-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit abf9503
into
openshift:release-4.19
|
@anik120: Jira Issue OCPBUGS-60380: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-60380 has been moved to the MODIFIED state. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-olm-operator-controller |
|
[ART PR BUILD NOTIFIER] Distgit: ose-olm-catalogd |
This is a hack to backport "NetworkPolicy object kind in bundles" support from operator-framework/operator-registry#1675 The feature was introduced in OCP 4.20 with a operator-registry bump to v1.55.0. Ref:
Because the upstream PR is not being backported in operator-registry to older tags (with new z stream releases), this achieves a downstream-only backport for this feature.
We're backporting this support to prevent any breakage in case a bundle is included in a 4.19 catalog.