Skip to content

✨ Include network policy for all configmap and grpc catalogsources #3568

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

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

Conversation

joelanford
Copy link
Member

@joelanford joelanford commented Apr 30, 2025

Description of the change:

This change updates OLMv0 to generate and reconcile two NetworkPolicy objects for each CatalogSource where OLMv0 also manages a catalog pod (i.e. configmap and grpc-based catalog sources).

  1. GRPC server pods need to listen and receive requests on port 50051
  2. Bundle unpack pods need to make connections to the kubernetes API server.

Related implementation details:

  • Informer events from network policies are handled by the catalog operator, so any deletions or changes are reverted.
  • Each NetworkPolicy gets an owner reference so that it is automatically cleaned up when the catalog source is deleted.
  • Each NetworkPolicy uses a pod selector that specifically targets the appropriate pods, so there should be no accidental blocking of traffic of unrelated pods.

Unit tests are updated to ensure that NetworkPolicy objects are handled correctly.

Motivation for the change:

By adding NetworkPolicy, we can provide more security to mitigate vulnerabilities and avoid accidental data leaks.

Architectural changes:

None (unless you count managing NetworkPolicy for CatalogSources as an architectural change)

Testing remarks:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

@openshift-ci openshift-ci bot requested review from dinhxuanvu and oceanc80 April 30, 2025 20:22
@joelanford joelanford changed the title include network policy for all configmap and grpc catalogsources Include network policy for all configmap and grpc catalogsources Apr 30, 2025
@joelanford joelanford force-pushed the catalog-source-network-policy branch 2 times, most recently from 7e73286 to b8dbde0 Compare May 1, 2025 01:27
@joelanford joelanford changed the title Include network policy for all configmap and grpc catalogsources ✨ Include network policy for all configmap and grpc catalogsources May 1, 2025
@joelanford joelanford force-pushed the catalog-source-network-policy branch from b8dbde0 to 437b8cb Compare May 1, 2025 12:35
@joelanford joelanford force-pushed the catalog-source-network-policy branch from 437b8cb to b9ecf89 Compare May 1, 2025 14:24
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

Looking good. A few comments.

dinhxuanvu
dinhxuanvu previously approved these changes May 1, 2025
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/approve

@joelanford joelanford marked this pull request as draft May 5, 2025 13:31
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 5, 2025
return np
}

func sanitizedDeepEqual(a client.Object, b client.Object) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible that some future additional field breaks this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is possible. I looked at using equality.Semantic.DeepDerivative() instead, but I didn't want to allow someone to add an extra network policy rule or change the selector in a way that would say "all good" from a deep derivative perspective.

We could potentially write a diff detector in a different way that would be less susceptible to future API changes, but also still secure in terms of reverting undesirable changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to be specific to network policy and exactly what we care about:

  1. Our labels are there (but others can be added)
  2. The specs are identical

@@ -750,6 +753,119 @@ var _ = Describe("Starting CatalogSource e2e tests", Label("CatalogSource"), fun
Expect(registryPods.Items).To(HaveLen(1), "unexpected number of replacement registry pods found")
})

It("delete registry network policy triggers recreation", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need an e2e? could a unit test cover this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? I didn't look too closely. The main bit of coverage I wanted here was "is there an NP event that the catalog-operator notices and causes there to be a sync of the relevant CatSrc"

It seemed like there were other e2e's for similar catsrc reconcile behaviors, so I followed the pattern.

@perdasilva
Copy link
Collaborator

@joelanford is this ready for review? it looks pretty good to me =D

@joelanford
Copy link
Member Author

is this ready for review? it looks pretty good to me =D

Yes-ish. I think the catalog pod network policy is reviewable. But I realized I also need to add dynamic network policy handling for bundle unpack pods.

I could do that here, or I could do it in a separate PR. I don't really care which.

I also have this listed as a draft because the RFC is still in review: https://docs.google.com/document/d/1TCb2YsSHaXBUnUUFp44Bt2hdsb2jwLnL77imP25QGVs/edit?tab=t.0#heading=h.x3tfh25grvnv

@joelanford joelanford force-pushed the catalog-source-network-policy branch 2 times, most recently from c8fd1f1 to f2a4870 Compare May 10, 2025 11:44
@joelanford joelanford force-pushed the catalog-source-network-policy branch from f2a4870 to 026ceff Compare May 10, 2025 11:46
@joelanford joelanford marked this pull request as ready for review May 10, 2025 16:36
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2025
@openshift-ci openshift-ci bot requested review from benluddy and kevinrizza May 10, 2025 16:36
@joelanford
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants