Skip to content

OCPBUGS-60380: Backporting NetworkPolicy support for OLM bundles #429

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 13, 2025
Merged

OCPBUGS-60380: Backporting NetworkPolicy support for OLM bundles #429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
406020c
UPSTREAM: 1981: Backporting NetworkPolicy support for OLM bundles
anik120 Aug 11, 2025
f140c53
UPSTREAM: 2078: Add Network Policy to e2e test bundles
trgeiger Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions hack/test/pre-upgrade-setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@ rules:
- "watch"
- "bind"
- "escalate"
- apiGroups:
- networking.k8s.io
resources:
- networkpolicies
verbs:
- get
- list
- create
- update
- delete
- apiGroups:
- "olm.operatorframework.io"
resources:
Expand Down
11 changes: 11 additions & 0 deletions internal/operator-controller/rukpak/convert/registryv1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,17 @@ func (c Converter) Convert(rv1 RegistryV1, installNamespace string, targetNamesp
for _, obj := range rv1.Others {
obj := obj
supported, namespaced := registrybundle.IsSupported(obj.GetKind())
// 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.
Comment on lines +405 to +412
Copy link
Contributor

@camilamacedo86 camilamacedo86 Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
// 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if obj.GetKind() == "NetworkPolicy" {
supported, namespaced = true, true
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hold on what commit are you looking at @camilamacedo86 coz I don't see the rukpak/render folder in the release-4.19 branch

https://github.com/openshift/operator-framework-operator-controller/tree/release-4.19/internal/operator-controller/rukpak

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you're looking at upstream @camilamacedo86.

Please see the downstream repo in its 4.19 release, there's only this one occurrence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, I didn't catch this before. @camilamacedo86 linked the upstream code. This change is downstream only.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
if !supported {
return nil, fmt.Errorf("bundle contains unsupported resource: Name: %v, Kind: %v", obj.GetName(), obj.GetKind())
}
Expand Down
23 changes: 23 additions & 0 deletions test/e2e/cluster_extension_install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
rbacv1 "k8s.io/api/rbac/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -154,6 +155,23 @@ func createClusterRoleAndBindingForSA(ctx context.Context, name string, sa *core
"escalate",
},
},
{
APIGroups: []string{
"networking.k8s.io",
},
Resources: []string{
"networkpolicies",
},
Verbs: []string{
"get",
"list",
"watch",
"create",
"update",
"patch",
"delete",
},
},
},
}
err := c.Create(ctx, cr)
Expand Down Expand Up @@ -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'")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice thank you !!!

require.EventuallyWithT(t, func(ct *assert.CollectT) {
var np networkingv1.NetworkPolicy
assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: "test-operator-network-policy", Namespace: ns.Name}, &np))
}, pollDuration, pollInterval)
})
}
}
Expand Down
10 changes: 10 additions & 0 deletions ...ata/images/bundles/test-operator/v1.0.0/manifests/testoperator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,16 @@ spec:
- update
- patch
- delete
- apiGroups:
- networking.k8s.io
resources:
- networkpolicies
verbs:
- get
- list
- create
- update
- delete
- apiGroups:
- coordination.k8s.io
resources:
Expand Down
8 changes: 8 additions & 0 deletions testdata/images/bundles/test-operator/v1.0.0/manifests/testoperator.networkpolicy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: test-operator-network-policy
spec:
podSelector: {}
policyTypes:
- Ingress