🌱 Migrate E2e NetworkPolicy tests to static analysis with kube-score and conftest
#2393
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
pedjak
changed the title
kube-score and conftest
There was a problem hiding this comment.
Pull request overview
This PR replaces runtime E2E NetworkPolicy tests with static analysis tools that validate NetworkPolicy configurations at build time, providing faster feedback during development.
Key changes:
- Removes test/e2e/network_policy_test.go (413 lines of runtime test code)
- Adds OPA policies for conftest to validate NetworkPolicy requirements
- Integrates kube-score and conftest into Makefile lint targets and manifest generation
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/network_policy_test.go | Removed E2E runtime tests (now covered by static analysis) |
| hack/conftest/policy/olm-networkpolicies.rego | OPA policy validating deny-all, catalogd, and operator-controller NetworkPolicies |
| hack/conftest/policy/prometheus-networkpolicies.rego | OPA policy validating Prometheus NetworkPolicy |
| hack/conftest/policy/README.md | Documentation for OPA policies and usage |
| Makefile | Integrated conftest into lint-helm and manifest generation; added lint-deployed-resources target |
| .github/workflows/files-diff.yaml | CI workflow to detect and alert on NetworkPolicy changes |
| .bingo/* | Added conftest, kube-score, and kube-linter tools via bingo |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Fail if NetworkPolicy files changed | ||
| if: steps.filter.outputs.networkpolicy == 'true' | ||
| run: | | ||
| echo "::error::NetworkPolicy files have been modified. See PR comment for details." | ||
| exit 1 |
There was a problem hiding this comment.
This workflow step will always fail when NetworkPolicy files change, blocking PRs that legitimately need to modify NetworkPolicies. Consider removing the hard failure (exit 1) and instead rely on the PR comment to alert reviewers. Alternatively, add a way to bypass this check (e.g., with a specific label) for intentional NetworkPolicy changes.
.bingo/Variables.mk
Outdated
| KUBE_LINTER := $(GOBIN)/kube-linter-v0.7.1 | ||
| $(KUBE_LINTER): $(BINGO_DIR)/kube-linter.mod | ||
| @# Install binary/ries using Go 1.14+ build command. This is using bwplotka/bingo-controlled, separate go module with pinned dependencies. | ||
| @echo "(re)installing $(GOBIN)/kube-linter-v0.7.1" | ||
| @cd $(BINGO_DIR) && GOWORK=off $(GO) build -mod=mod -modfile=kube-linter.mod -o=$(GOBIN)/kube-linter-v0.7.1 "golang.stackrox.io/kube-linter/cmd/kube-linter" |
There was a problem hiding this comment.
kube-linter is added to the bingo dependencies but is not mentioned in the PR description and doesn't appear to be used in any Makefile targets or documentation. If it's not needed for this PR, it should be removed. If it is needed, the PR description and documentation should be updated to mention it.
| deny[msg] { | ||
| count(catalogd_policies) == 1 | ||
| not catalogd_has_egress | ||
| msg := "Missing egress rules in catalogd-controller-manager NetworkPolicy. General egress is required to enables operator-controller to pull bundle images from arbitrary image registries, connect to catalogd's HTTPS server for metadata, and interact with the Kubernetes API server." |
There was a problem hiding this comment.
The error message incorrectly states "enables operator-controller to pull bundle images" but this is for the catalogd-controller-manager policy. The message should describe catalogd's need for egress, not operator-controller's needs. Consider: "General egress is required to permit catalogd to fetch catalog images from arbitrary container registries and communicate with the Kubernetes API server for its operational needs."
| msg := "Missing egress rules in catalogd-controller-manager NetworkPolicy. General egress is required to enables operator-controller to pull bundle images from arbitrary image registries, connect to catalogd's HTTPS server for metadata, and interact with the Kubernetes API server." | |
| msg := "Missing egress rules in catalogd-controller-manager NetworkPolicy. General egress is required to permit catalogd to fetch catalog images from arbitrary container registries and communicate with the Kubernetes API server for its operational needs." |
| deny[msg] { | ||
| count(operator_controller_policies) == 1 | ||
| not operator_controller_ingress_port_numbers[8443] | ||
| msg := "Allow traffic to port 8443 to allow Prometheus to scrape metrics from catalogd, which is essential for monitoring its performance and health." |
There was a problem hiding this comment.
The error message incorrectly states "to allow Prometheus to scrape metrics from catalogd" but this is for operator-controller-controller-manager on port 8443. The message should be: "Allow traffic to port 8443 to allow Prometheus to scrape metrics from operator-controller, which is crucial for monitoring its activity, reconciliations, and overall health."
| msg := "Allow traffic to port 8443 to allow Prometheus to scrape metrics from catalogd, which is essential for monitoring its performance and health." | |
| msg := "Allow traffic to port 8443 to allow Prometheus to scrape metrics from operator-controller, which is crucial for monitoring its activity, reconciliations, and overall health." |
pedjak
force-pushed
the
migrate-network-policy-tests
branch
from
26f3f91 to
0d17f25
Compare
pedjak
force-pushed
the
migrate-network-policy-tests
branch
from
0d17f25 to
3f2af72
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| deny[msg] { | ||
| count(catalogd_policies) == 1 | ||
| not catalogd_has_egress | ||
| msg := "Missing egress rules in catalogd-controller-manager NetworkPolicy. General egress is required to enables operator-controller to pull bundle images from arbitrary image registries, connect to catalogd's HTTPS server for metadata, and interact with the Kubernetes API server." |
There was a problem hiding this comment.
Grammar error: "to enables" should be "to enable" (singular form).
| msg := "Missing egress rules in catalogd-controller-manager NetworkPolicy. General egress is required to enables operator-controller to pull bundle images from arbitrary image registries, connect to catalogd's HTTPS server for metadata, and interact with the Kubernetes API server." | |
| msg := "Missing egress rules in catalogd-controller-manager NetworkPolicy. General egress is required to enable operator-controller to pull bundle images from arbitrary image registries, connect to catalogd's HTTPS server for metadata, and interact with the Kubernetes API server." |
| deny[msg] { | ||
| count(operator_controller_policies) == 1 | ||
| not operator_controller_has_egress | ||
| msg := "Missing egress rules in operator-controller-controller-manager NetworkPolicy. General egress is required to enables operator-controller to pull bundle images from arbitrary image registries, connect to catalogd's HTTPS server for metadata, and interact with the Kubernetes API server." |
There was a problem hiding this comment.
Grammar error: "to enables" should be "to enable" (singular form).
| msg := "Missing egress rules in operator-controller-controller-manager NetworkPolicy. General egress is required to enables operator-controller to pull bundle images from arbitrary image registries, connect to catalogd's HTTPS server for metadata, and interact with the Kubernetes API server." | |
| msg := "Missing egress rules in operator-controller-controller-manager NetworkPolicy. General egress is required to enable operator-controller to pull bundle images from arbitrary image registries, connect to catalogd's HTTPS server for metadata, and interact with the Kubernetes API server." |
| - name: Fail if NetworkPolicy files changed | ||
| if: steps.filter.outputs.networkpolicy == 'true' | ||
| run: | | ||
| echo "::error::NetworkPolicy files have been modified. See PR comment for details." | ||
| exit 1 |
There was a problem hiding this comment.
This workflow step will fail any PR that modifies NetworkPolicy files, which could block legitimate changes. The workflow appears to be designed as a safety check, but the hard failure (exit 1) makes it impossible to merge NetworkPolicy changes without removing or modifying this workflow first. Consider either: (1) making this a non-blocking check (remove the exit 1), (2) requiring manual approval for NetworkPolicy changes via a label or review requirement, or (3) documenting that this workflow needs to be temporarily disabled for intentional NetworkPolicy changes.
.bingo/variables.env
Outdated
| KUBE_LINTER="${GOBIN}/kube-linter-v0.7.1" | ||
|
|
There was a problem hiding this comment.
KUBE_LINTER is declared in variables.env but is not used anywhere in the project (not found in Makefile or Variables.mk). This appears to be an unused dependency that should either be integrated into the build or removed to avoid confusion.
| KUBE_LINTER="${GOBIN}/kube-linter-v0.7.1" |
| .PHONY: lint | ||
| lint: lint-custom $(GOLANGCI_LINT) #HELP Run golangci linter. | ||
| $(GOLANGCI_LINT) run --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS) | ||
|
|
There was a problem hiding this comment.
The lint-helm target is missing the .PHONY directive, while other similar targets (like lint at line 120 and lint-deployed-resources at line 129) have it. This should be marked as .PHONY since it doesn't produce a file output.
| .PHONY: lint-helm |
| **Changed files:** | ||
| ``` | ||
| ${{ steps.filter.outputs.networkpolicy_files }} |
There was a problem hiding this comment.
The output variable "networkpolicy_files" does not exist for the dorny/paths-filter action. The action provides outputs like "networkpolicy" (boolean), "networkpolicy_count", and "networkpolicy_files" only when list-files is set to a specific format (e.g., shell, json, csv). To display the changed files, you need to either: (1) add "list-files: shell" (or another format) to the paths-filter configuration, or (2) use a different approach to list the files, such as using git diff commands.
pedjak
force-pushed
the
migrate-network-policy-tests
branch
from
3f2af72 to
12252d5
Compare
pedjak
force-pushed
the
migrate-network-policy-tests
branch
from
12252d5 to
0f0027d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `make lint-helm` - Validates both helm/olmv1 and helm/prometheus charts | ||
| - `make manifests` - Validates generated manifests (main namespace only) |
There was a problem hiding this comment.
The documentation states that "make manifests" validates generated manifests with the main namespace only, but this is inconsistent with the actual behavior where some manifests (e.g., e2e manifests) may include prometheus resources. This should clarify that prometheus policies are only validated during "make lint-helm" or explain why they're skipped during manifest generation.
| - `make lint-helm` - Validates both helm/olmv1 and helm/prometheus charts | |
| - `make manifests` - Validates generated manifests (main namespace only) | |
| - `make lint-helm` - Validates both helm/olmv1 and helm/prometheus charts (runs `main` and `prometheus` packages) | |
| - `make manifests` - Generates and validates core OLM manifests using only `main` package policies (Prometheus policies are intentionally skipped here, even if manifests include Prometheus resources; they are validated via `make lint-helm`) |
|
|
||
| .PHONY: run-internal | ||
| run-internal: docker-build kind-cluster kind-load kind-deploy wait | ||
| run-internal: docker-build kind-cluster kind-load kind-deploy lint-deployed-resources wait |
There was a problem hiding this comment.
Adding lint-deployed-resources to the run-internal target means that every local development run will now attempt to lint all resources in the cluster. This could slow down the development workflow and may fail if the cluster is not fully ready or if resources don't exist yet. Consider moving this to a separate verification step or making it optional for development workflows.
| image: busybox:1.36 | ||
| name: tar | ||
| securityContext: | ||
| readOnlyRootFilesystem: true |
There was a problem hiding this comment.
Setting readOnlyRootFilesystem to true will likely cause the busybox container to fail. The container runs "sleep infinity" which may need to write to temporary directories like /tmp. Consider adding an emptyDir volume mounted at /tmp if readOnlyRootFilesystem is required, or verify that this specific busybox command works with a read-only root filesystem.
.bingo/variables.env
Outdated
| KUBE_LINTER="${GOBIN}/kube-linter-v0.7.1" | ||
|
|
There was a problem hiding this comment.
The KUBE_LINTER variable is declared in variables.env but there's no corresponding kube-linter.mod, kube-linter.sum, or Variables.mk entry to actually install this tool. Either this tool should be removed from variables.env if it's not needed, or the full bingo configuration should be added (including .mod, .sum files and Variables.mk target).
| KUBE_LINTER="${GOBIN}/kube-linter-v0.7.1" |
| deny contains msg if { | ||
| count(catalogd_policies) == 1 | ||
| not catalogd_ingress_port_numbers[9443] | ||
| msg := "Allow traffic to port 9443 to enable Kubernetes API server to reach catalogd's mutating admission webhook, ensuring integrity of catalog resources." |
There was a problem hiding this comment.
For consistency with the original test justifications, this should use "Permits" instead of "to enable" to match the phrasing: "Permits Kubernetes API server to reach catalogd's mutating admission webhook".
| msg := "Allow traffic to port 9443 to enable Kubernetes API server to reach catalogd's mutating admission webhook, ensuring integrity of catalog resources." | |
| msg := "Allow traffic to port 9443. Permits Kubernetes API server to reach catalogd's mutating admission webhook, ensuring integrity of catalog resources." |
…d conftest Replace the e2e NetworkPolicy tests with static analysis tools that validate NetworkPolicy configurations at build time rather than runtime. Tools: - kube-score: https://github.com/zegl/kube-score - conftest: https://www.conftest.dev/ - OPA (Open Policy Agent): https://www.openpolicyagent.org/docs/latest/policy-language/ Changes: - Add kube-score via bingo for validating deployed NetworkPolicy resources - Add conftest via bingo for OPA-based policy validation of Helm charts - Add OPA policies to enforce NetworkPolicy requirements: - Deny-all default policy must exist - catalogd-controller-manager must allow ingress on ports 7443, 8443, 9443 - operator-controller-controller-manager must allow ingress on port 8443 - Both controllers must have general egress enabled - Prometheus NetworkPolicy must allow ingress/egress (when deployed) - Add lint-helm target integration with conftest policy checks - Add lint-deployed-resources target for runtime validation with kube-score - Add conftest validation to manifest generation - Add CI workflow to detect NetworkPolicy changes in PRs and post a comment - Remove network_policy_test.go as tests are now covered by static analysis This approach provides faster feedback by catching NetworkPolicy issues during helm linting and manifest generation rather than requiring a full e2e test run. Co-Authored-By: Claude <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2393 +/- ##
==========================================
+ Coverage 68.79% 73.00% +4.20%
==========================================
Files 100 100
Lines 7641 7641
==========================================
+ Hits 5257 5578 +321
+ Misses 1948 1625 -323
- Partials 436 438 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pedjak
force-pushed
the
migrate-network-policy-tests
branch
from
0f0027d to
7fa3d04
Compare
| image: busybox:1.36 | ||
| name: tar | ||
| securityContext: | ||
| readOnlyRootFilesystem: true |
There was a problem hiding this comment.
note to reviewers: kube-score detect this while working on this PR and was needed to add it.
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| .PHONY: lint-deployed-resources | ||
| lint-deployed-resources: $(KUBE_SCORE) #HELP Lint NetworkPolicy resources in olmv1-system namespace using kube-score. | ||
| (for ns in $$(echo -e "olmv1-system\n$(CATD_NAMESPACE)" | uniq); do \ |
There was a problem hiding this comment.
The command uses echo -e which may not work reliably across all shells. The -e flag is not POSIX-compliant and may not be available in /bin/sh on some systems. Consider using printf instead for better portability, or explicitly using bash with a here-string.
| (for ns in $$(echo -e "olmv1-system\n$(CATD_NAMESPACE)" | uniq); do \ | |
| (for ns in $$(printf 'olmv1-system\n%s\n' "$(CATD_NAMESPACE)" | uniq); do \ |
1 participant
Replace the e2e NetworkPolicy tests with static analysis tools that validate
NetworkPolicy configurations at build time rather than runtime.
Tools:
Changes:
This approach provides faster feedback by catching NetworkPolicy issues during
helm linting and manifest generation rather than requiring a full e2e test run.
Co-Authored-By: Claude [email protected]
Reviewer Checklist