WIP: SPIRE-220: Implementation for the Network Policy for the ZTWIM operands#55
WIP: SPIRE-220: Implementation for the Network Policy for the ZTWIM operands#55PillaiManish wants to merge 6 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@PillaiManish: This pull request references SPIRE-220 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn 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. |
WalkthroughAdds 11 Kubernetes NetworkPolicy manifests as embedded assets; introduces decoding and labeling utilities; adds reconciler logic to create/apply these NetworkPolicies during reconcile; registers networking.k8s.io/v1 scheme and RBAC/watch support for NetworkPolicy resources. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: PillaiManish 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
pkg/operator/assets/bindata.go (1)
148-164: Consider restricting egress destination to SPIRE server pods.The spire-agent egress to port 8081 has no destination selector, allowing connections to any pod on that port, not just the SPIRE server.
Add a
podSelectorto thetofield:egress: # Allow connection to SPIRE server - - ports: + - to: + - podSelector: + matchLabels: + app.kubernetes.io/name: spire-server + ports: - protocol: TCP port: 8081
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
bindata/network-policy/default-deny-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-agent/spire-agent-allow-egress-to-api-server-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-agent/spire-agent-allow-egress-to-spire-server-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-agent/spire-agent-allow-ingress-to-metrics-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-oidc-discovery-provider/spire-oidc-allow-ingress-to-8443-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-server/spire-server-allow-egress-ingress-to-federation-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-server/spire-server-allow-egress-to-api-server-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-server/spire-server-allow-egress-to-dns-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-server/spire-server-allow-ingress-to-8081-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-server/spire-server-allow-ingress-to-metrics-networkpolicy.yaml(1 hunks)bindata/network-policy/spire-server/spire-server-allow-ingress-to-webhook-networkpolicy.yaml(1 hunks)pkg/operator/assets/bindata.go(4 hunks)
🔇 Additional comments (9)
bindata/network-policy/spire-server/spire-server-allow-egress-to-dns-networkpolicy.yaml (1)
12-24: Well-scoped DNS egress policy with proper peer selectors.This policy correctly restricts egress traffic to the openshift-dns namespace and specific DNS pods, following zero-trust principles. The configuration properly allows both TCP and UDP on port 5353.
bindata/network-policy/spire-agent/spire-agent-allow-ingress-to-metrics-networkpolicy.yaml (1)
12-19: Well-scoped metrics ingress policy with proper source selector.This policy correctly restricts ingress traffic to the openshift-monitoring namespace. Note: the namespace selector uses the label key
namerather thankubernetes.io/metadata.name(used in the DNS policy). Verify this label key is correct for the openshift-monitoring namespace, or consider standardizing tokubernetes.io/metadata.namefor consistency across policies.bindata/network-policy/spire-server/spire-server-allow-ingress-to-metrics-networkpolicy.yaml (1)
1-21: LGTM! Well-scoped metrics ingress policy.This NetworkPolicy correctly restricts metrics ingress to the OpenShift monitoring namespace, with appropriate port specifications for both SPIRE server (9402) and controller manager (8082) metrics endpoints.
bindata/network-policy/default-deny-networkpolicy.yaml (2)
6-7: Verify the cert-manager ownership label.The
cert-manager.operator.openshift.io/owned-by: cert-managerlabel appears inconsistent with a default-deny NetworkPolicy for the ZTWIM namespace. This policy governs all SPIRE and related components, not just cert-manager resources.Confirm whether this label is intentional or should reference the ZTWIM operator instead.
8-12: LGTM! Proper default-deny structure.The empty podSelector with Ingress and Egress policy types (and no rules) correctly implements a default deny-all policy for the namespace.
pkg/operator/assets/bindata.go (4)
1-13: Note: This is a generated file.As indicated by the header comment, this file is generated by go-bindata. Manual edits will be overwritten. Security issues identified in the embedded YAML policies should be addressed in the source YAML files, then regenerated.
116-131: Verify unrestricted egress to API server is acceptable.The spire-agent egress policy allows TCP egress to port 6443 without destination restrictions. While this is typically the Kubernetes API server port, the policy doesn't restrict which endpoints can be reached.
For defense in depth, consider whether this should be restricted to specific API server endpoints (though this may be challenging in dynamic cluster environments).
288-303: Verify unrestricted egress to API server is acceptable.Similar to the spire-agent policy, this allows egress to port 6443 without destination restrictions.
Consider whether API server egress should be more tightly scoped, though this may be operationally challenging.
217-233: ****OIDC discovery endpoints are designed to be publicly accessible for federation purposes. The unrestricted ingress is required for external cloud providers and identity consumers (e.g., AWS, Vault, other OIDC clients) to discover and consume the SPIFFE issuance. Restricting access to specific namespaces or pods would prevent OIDC federation from functioning. Security is achieved at the application layer (serving only discovery and JWKS endpoints) and infrastructure layer (TLS, rate limiting, monitoring), not through NetworkPolicy ingress restrictions.
Likely an incorrect or invalid review comment.
bindata/network-policy/spire-agent/spire-agent-allow-egress-to-api-server-networkpolicy.yaml
Show resolved
Hide resolved
| egress: | ||
| # Allow connection to SPIRE server | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8081 |
There was a problem hiding this comment.
Missing destination selector in egress rule—overly permissive for zero-trust environment.
The egress rule allows traffic to ANY destination on port 8081, not just SPIRE server pods. In a zero-trust namespace, this should be restricted to only spire-server pods.
Consider updating the egress rule to target spire-server pods explicitly:
egress:
# Allow connection to SPIRE server
- - ports:
+ - to:
+ - podSelector:
+ matchLabels:
+ app.kubernetes.io/name: spire-server
+ ports:
- protocol: TCP
port: 8081📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| egress: | |
| # Allow connection to SPIRE server | |
| - ports: | |
| - protocol: TCP | |
| port: 8081 | |
| egress: | |
| # Allow connection to SPIRE server | |
| - to: | |
| - podSelector: | |
| matchLabels: | |
| app.kubernetes.io/name: spire-server | |
| ports: | |
| - protocol: TCP | |
| port: 8081 |
🤖 Prompt for AI Agents
In
bindata/network-policy/spire-agent/spire-agent-allow-egress-to-spire-server-networkpolicy.yaml
around lines 12 to 16, the egress rule is currently allowing port 8081 to any
destination; tighten it by adding a destination selector that limits egress to
the SPIRE server pods only—add a "to" block with a podSelector matching the
spire-server pod labels (and/or a namespaceSelector if the server runs in a
different namespace) so the rule only permits TCP:8081 traffic to those pods.
...ork-policy/spire-oidc-discovery-provider/spire-oidc-allow-ingress-to-8443-networkpolicy.yaml
Show resolved
Hide resolved
| ingress: | ||
| # Allow federation traffic | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8443 | ||
| egress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8443 |
There was a problem hiding this comment.
Federation policy lacks both source and destination selectors—critically permissive for inter-cluster communication.
Federation traffic should be tightly controlled. Currently, this policy allows ingress from any source and egress to any destination on port 8443. For a zero-trust model, both rules must restrict to known federation peers (likely via namespace selectors for remote SPIRE servers).
Add source and destination selectors to restrict federation traffic to known peers:
ingress:
# Allow federation traffic
- - ports:
+ - from:
+ - namespaceSelector: {} # Restrict to specific federation peer namespace(s)
+ ports:
- protocol: TCP
port: 8443
egress:
- - ports:
+ - to:
+ - namespaceSelector: {} # Restrict to specific federation peer namespace(s)
+ ports:
- protocol: TCP
port: 8443📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ingress: | |
| # Allow federation traffic | |
| - ports: | |
| - protocol: TCP | |
| port: 8443 | |
| egress: | |
| - ports: | |
| - protocol: TCP | |
| port: 8443 | |
| ingress: | |
| # Allow federation traffic | |
| - from: | |
| - namespaceSelector: {} # Restrict to specific federation peer namespace(s) | |
| ports: | |
| - protocol: TCP | |
| port: 8443 | |
| egress: | |
| - to: | |
| - namespaceSelector: {} # Restrict to specific federation peer namespace(s) | |
| ports: | |
| - protocol: TCP | |
| port: 8443 |
🤖 Prompt for AI Agents
In
bindata/network-policy/spire-server/spire-server-allow-egress-ingress-to-federation-networkpolicy.yaml
around lines 13–21, the ingress and egress rules currently allow traffic on port
8443 from/to any endpoint; tighten them by adding explicit selectors: for
ingress add from: with namespaceSelector and/or podSelector that match your
known federation SPIRE server peers (e.g., namespaceSelector.matchLabels:
{federation: "true"} or podSelector.matchLabels for the peer pods) and for
egress add to: with the same selectors so traffic is only allowed to those
peers; keep the TCP port 8443 and ensure the selectors reflect your cluster’s
federation labels (or use ipBlock if you must restrict by CIDR).
bindata/network-policy/spire-server/spire-server-allow-egress-to-api-server-networkpolicy.yaml
Show resolved
Hide resolved
| ingress: | ||
| # Allow SPIRE agents to connect | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8081 |
There was a problem hiding this comment.
Critical: Missing source restriction allows unrestricted access to SPIRE server API.
The ingress rule lacks a from selector, permitting ANY pod from ANY namespace to connect to the SPIRE server gRPC API on port 8081. This undermines the default-deny policy and violates zero-trust principles.
Restrict ingress to SPIRE agent pods only:
ingress:
# Allow SPIRE agents to connect
- - ports:
+ - from:
+ - podSelector:
+ matchLabels:
+ app.kubernetes.io/name: spire-agent
+ ports:
- protocol: TCP
port: 8081📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ingress: | |
| # Allow SPIRE agents to connect | |
| - ports: | |
| - protocol: TCP | |
| port: 8081 | |
| ingress: | |
| # Allow SPIRE agents to connect | |
| - from: | |
| - podSelector: | |
| matchLabels: | |
| app.kubernetes.io/name: spire-agent | |
| ports: | |
| - protocol: TCP | |
| port: 8081 |
🤖 Prompt for AI Agents
In
bindata/network-policy/spire-server/spire-server-allow-ingress-to-8081-networkpolicy.yaml
around lines 12–16, the ingress rule currently has no `from` clause so it allows
any source to reach port 8081; restrict it to SPIRE agents by adding a `from`
selector that matches the SPIRE agent pods (or the agents' namespace). Update
the ingress entry to include a `from` with the correct podSelector labels used
by your spire-agent deployment (or a namespaceSelector + podSelector combination
if agents are isolated by namespace) so only agent pods can connect to TCP port
8081.
bindata/network-policy/spire-server/spire-server-allow-ingress-to-webhook-networkpolicy.yaml
Show resolved
Hide resolved
| var _networkPolicySpireServerSpireServerAllowEgressIngressToFederationNetworkpolicyYaml = []byte(`apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| name: spire-server-allow-egress-ingress-to-federation | ||
| namespace: zero-trust-workload-identity-manager | ||
| spec: | ||
| podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: spire-server | ||
| policyTypes: | ||
| - Ingress | ||
| - Egress | ||
| ingress: | ||
| # Allow federation traffic | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8443 | ||
| egress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 8443 | ||
| `) |
There was a problem hiding this comment.
Critical: Federation policy allows unrestricted bidirectional traffic.
The federation NetworkPolicy allows both ingress and egress on port 8443 without source or destination restrictions. This permits any pod to connect to/from SPIRE server federation endpoints.
If federation is with specific external SPIRE servers, consider restricting based on known federation partners. At minimum, add comments documenting why unrestricted access is required.
🤖 Prompt for AI Agents
In pkg/operator/assets/bindata.go around lines 250 to 271 the embedded
NetworkPolicy for spire-server-allow-egress-ingress-to-federation currently
permits ingress and egress on TCP/8443 with no peer restrictions; tighten the
policy by adding appropriate from/to peer selectors or CIDR blocks (e.g.,
specific podSelector/namespaceSelector blocks for known federation partners or
external IPBlocks for external SPIRE servers) to restrict allowed sources and
destinations, and if truly required to remain open, add a clear comment above
the YAML explaining why unrestricted access is necessary and any compensating
controls.
| var _networkPolicySpireServerSpireServerAllowIngressToWebhookNetworkpolicyYaml = []byte(`apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| name: spire-server-allow-ingress-to-webhook | ||
| namespace: zero-trust-workload-identity-manager | ||
| spec: | ||
| podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: spire-server | ||
| policyTypes: | ||
| - Ingress | ||
| ingress: | ||
| # Allow webhook traffic | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 9443 | ||
| `) |
There was a problem hiding this comment.
Critical: Webhook endpoint has unrestricted ingress.
The webhook NetworkPolicy allows ingress from ANY source to port 9443. The Kubernetes API server needs to reach this webhook, but the policy doesn't restrict ingress to just the API server.
In OpenShift/Kubernetes, webhooks are called by the API server. However, restricting by source IP/podSelector can be complex. At minimum:
- Add a comment documenting why unrestricted access is necessary
- Consider if there are OpenShift-specific labels or network constructs to restrict this further
Alternatively, if the API server runs in a specific namespace with identifiable labels, add appropriate selectors.
🤖 Prompt for AI Agents
In pkg/operator/assets/bindata.go around lines 432 to 448, the NetworkPolicy for
the webhook currently allows unrestricted ingress to port 9443; update the
manifest to (1) add an explicit comment above the policy explaining why open
access is present if you determine it is required for API server reachability,
and (2) if possible restrict ingress instead of leaving it open by adding
appropriate selectors—e.g. a namespaceSelector or podSelector that targets the
API server namespace/pods (or OpenShift-specific labels) or any cluster network
construct you use—so only the API server (or known control-plane pods) can
connect; if you cannot restrict it, document that reason and the risk in the
comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/controller/utils/utils.go (1)
113-119: Function follows established pattern but inherits panic behavior.The implementation is consistent with other decode functions in this file. However, the function panics on decode errors or type assertion failures, which could cause runtime crashes if assets are malformed.
This is a pre-existing pattern across all decode functions in this file, so addressing it here would be inconsistent. Consider adding error handling to all decode functions in a future refactor.
pkg/controller/static-resource-controller/network-policy.go (1)
54-122: Consider refactoring to reduce code duplication.All 11 helper methods follow an identical pattern: decode from asset, apply labels, and return. This could be consolidated into a single parameterized helper method.
For example:
func (r *StaticResourceReconciler) getNetworkPolicy(assetName string, labelFunc func(map[string]string) map[string]string) *v1.NetworkPolicy { policy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(assetName)) policy.Labels = labelFunc(policy.Labels) return policy }Then each getter becomes:
func (r *StaticResourceReconciler) getSpireAgentAllowEgressToApiServerNetworkPolicy() *v1.NetworkPolicy { return r.getNetworkPolicy(utils.SpireAgentAllowEgressToApiServerNetworkPolicyAssetName, utils.SpireAgentLabels) }This would reduce maintenance burden and improve consistency across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
pkg/controller/static-resource-controller/network-policy.go(1 hunks)pkg/controller/utils/constants.go(1 hunks)pkg/controller/utils/utils.go(3 hunks)
🔇 Additional comments (5)
pkg/controller/utils/utils.go (2)
16-16: LGTM!The import is correctly placed and follows the established ordering pattern.
35-35: LGTM!The scheme registration follows the established pattern and enables NetworkPolicy decoding.
pkg/controller/static-resource-controller/network-policy.go (2)
12-20: LGTM!The method cleanly orchestrates NetworkPolicy creation/updates with appropriate error handling.
22-51: LGTM!The method is well-organized with clear grouping and helpful comments. The default-deny policy is correctly placed first.
pkg/controller/utils/constants.go (1)
47-58: LGTM!The constants are well-named, consistently structured, and follow the established naming convention. The logical grouping (default-deny, then by component) makes the code easy to navigate.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/static-resource-controller/controller.go (1)
59-107: Missing RBAC for networking.k8s.io/NetworkPolicy will cause Forbidden errorsThe new CreateOrApplyNetworkPolicyResources step needs permissions to manage NetworkPolicies. Add the kubebuilder RBAC marker so the operator ClusterRole includes them.
Apply this diff near the existing RBAC markers:
// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=apps,resources=statefulsets,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=security.openshift.io,resources=securitycontextconstraints,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=networking.k8s.io,resources=networkpolicies,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;list;watch;create;update;patch;delete
🧹 Nitpick comments (3)
pkg/controller/static-resource-controller/controller.go (3)
213-230: Pre-RBAC NetworkPolicy reconcile step added — confirm labeling and drift strategyGood to run NetworkPolicy creation early. Please verify:
- NetworkPolicies are labeled with utils.AppManagedByLabelKey/Value so predicates match.
- Decide if you want reconcile-on-drift for NetworkPolicies; without a watch, external edits won’t enqueue. See suggested watch addition below.
334-345: Add watch for NetworkPolicies to reconcile on driftRegister a watch for networkingv1.NetworkPolicy so label-matched changes enqueue the “cluster” request, consistent with other managed resources.
Import and watch changes:
import ( "context" @@ storagev1 "k8s.io/api/storage/v1" + networkingv1 "k8s.io/api/networking/v1" @@ err := ctrl.NewControllerManagedBy(mgr). @@ Watches(&corev1.Service{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerManagedResourcePredicates). + Watches(&networkingv1.NetworkPolicy{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerManagedResourcePredicates). Watches(&rbacv1.RoleBinding{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerManagedResourcePredicates).Also ensure networkingv1 has been added to the scheme during manager setup (usually via client-go scheme registration).
216-229: Normalize event reason and success message (optional)Use CamelCase reasons and “reconciled” wording for clarity/consistency.
- r.eventRecorder.Event(&config, corev1.EventTypeWarning, "failed to create network-policy resources", - err.Error()) + r.eventRecorder.Event(&config, corev1.EventTypeWarning, "NetworkPolicyResourceCreateFailed", err.Error()) - Reason: "NetworkPolicyResourceCreated", - Message: "All NetworkPolicy resources for operands created", + Reason: "NetworkPolicyResourcesReconciled", + Message: "All NetworkPolicy resources for operands are reconciled",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/controller/static-resource-controller/controller.go(2 hunks)
🔇 Additional comments (1)
pkg/controller/static-resource-controller/controller.go (1)
36-36: Condition type addition looks goodNew status key aligns with existing pattern and is scoped appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
bundle/manifests/zero-trust-workload-identity-manager.clusterserviceversion.yaml(2 hunks)config/rbac/role.yaml(1 hunks)pkg/controller/static-resource-controller/controller.go(5 hunks)pkg/controller/static-resource-controller/network-policy.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/static-resource-controller/controller.go
🔇 Additional comments (6)
bundle/manifests/zero-trust-workload-identity-manager.clusterserviceversion.yaml (2)
25-25: Metadata timestamp updated as expected.Routine regeneration of ClusterServiceVersion manifest.
173-182: NetworkPolicy RBAC rule added in sync with config/rbac/role.yaml.The new rule grants appropriate verbs for managing NetworkPolicy resources. Note that the
deleteverb is intentionally omitted—verify this is the intended design (e.g., to prevent accidental deletion of cluster-wide policies managed by the operator).config/rbac/role.yaml (1)
85-94: NetworkPolicy RBAC rule correctly positioned and configured.The rule is properly alphabetized among API groups and grants the necessary verbs (create, get, list, update, watch) for the operator to manage NetworkPolicy resources. This mirrors the ClusterServiceVersion changes and follows the existing RBAC pattern in the file.
pkg/controller/static-resource-controller/network-policy.go (3)
1-10: LGTM!The package structure and imports are clean and appropriate for NetworkPolicy management.
12-20: LGTM!The reconciliation loop is straightforward and handles errors correctly by returning early on failure.
25-26: Verify the timeline for enabling the default deny NetworkPolicy.The default deny policy is currently commented out. In zero-trust architectures, a default-deny policy is typically a foundational security control that blocks all traffic unless explicitly allowed by other policies. Since this PR is marked WIP, confirm whether:
- This is intentional for incremental rollout
- When it will be enabled
- What testing is planned before enabling it
Enabling default-deny without comprehensive allow policies can break workloads, so staged rollout may be appropriate.
| // Default deny network policy | ||
| func (r *StaticResourceReconciler) getDefaultDenyNetworkPolicy() *v1.NetworkPolicy { | ||
| defaultDenyNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.DefaultDenyNetworkPolicyAssetName)) | ||
| // Apply standardized labels - this policy is general, so use basic managed-by label | ||
| defaultDenyNetworkPolicy.Labels = utils.SetLabel(defaultDenyNetworkPolicy.Labels, utils.AppManagedByLabelKey, utils.AppManagedByLabelValue) | ||
| return defaultDenyNetworkPolicy | ||
| } | ||
|
|
||
| // Spire Agent network policies | ||
| func (r *StaticResourceReconciler) getSpireAgentAllowEgressToApiServerNetworkPolicy() *v1.NetworkPolicy { | ||
| spireAgentNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireAgentAllowEgressToApiServerNetworkPolicyAssetName)) | ||
| spireAgentNetworkPolicy.Labels = utils.SpireAgentLabels(spireAgentNetworkPolicy.Labels) | ||
| return spireAgentNetworkPolicy | ||
| } | ||
|
|
||
| func (r *StaticResourceReconciler) getSpireAgentAllowEgressToSpireServerNetworkPolicy() *v1.NetworkPolicy { | ||
| spireAgentNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireAgentAllowEgressToSpireServerNetworkPolicyAssetName)) | ||
| spireAgentNetworkPolicy.Labels = utils.SpireAgentLabels(spireAgentNetworkPolicy.Labels) | ||
| return spireAgentNetworkPolicy | ||
| } | ||
|
|
||
| func (r *StaticResourceReconciler) getSpireAgentAllowIngressToMetricsNetworkPolicy() *v1.NetworkPolicy { | ||
| spireAgentNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireAgentAllowIngressToMetricsNetworkPolicyAssetName)) | ||
| spireAgentNetworkPolicy.Labels = utils.SpireAgentLabels(spireAgentNetworkPolicy.Labels) | ||
| return spireAgentNetworkPolicy | ||
| } | ||
|
|
||
| // Spire OIDC Discovery Provider network policies | ||
| func (r *StaticResourceReconciler) getSpireOIDCDiscoveryProviderAllowIngressTo8443NetworkPolicy() *v1.NetworkPolicy { | ||
| spireOIDCNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireOIDCDiscoveryProviderAllowIngressTo8443NetworkPolicyAssetName)) | ||
| spireOIDCNetworkPolicy.Labels = utils.SpireOIDCDiscoveryProviderLabels(spireOIDCNetworkPolicy.Labels) | ||
| return spireOIDCNetworkPolicy | ||
| } | ||
|
|
||
| // Spire Server network policies | ||
| func (r *StaticResourceReconciler) getSpireServerAllowEgressIngressToFederationNetworkPolicy() *v1.NetworkPolicy { | ||
| spireServerNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireServerAllowEgressIngressToFederationNetworkPolicyAssetName)) | ||
| spireServerNetworkPolicy.Labels = utils.SpireServerLabels(spireServerNetworkPolicy.Labels) | ||
| return spireServerNetworkPolicy | ||
| } | ||
|
|
||
| func (r *StaticResourceReconciler) getSpireServerAllowEgressToApiServerNetworkPolicy() *v1.NetworkPolicy { | ||
| spireServerNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireServerAllowEgressToApiServerNetworkPolicyAssetName)) | ||
| spireServerNetworkPolicy.Labels = utils.SpireServerLabels(spireServerNetworkPolicy.Labels) | ||
| return spireServerNetworkPolicy | ||
| } | ||
|
|
||
| func (r *StaticResourceReconciler) getSpireServerAllowEgressToDNSNetworkPolicy() *v1.NetworkPolicy { | ||
| spireServerNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireServerAllowEgressToDNSNetworkPolicyAssetName)) | ||
| spireServerNetworkPolicy.Labels = utils.SpireServerLabels(spireServerNetworkPolicy.Labels) | ||
| return spireServerNetworkPolicy | ||
| } | ||
|
|
||
| func (r *StaticResourceReconciler) getSpireServerAllowIngressTo8081NetworkPolicy() *v1.NetworkPolicy { | ||
| spireServerNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireServerAllowIngressTo8081NetworkPolicyAssetName)) | ||
| spireServerNetworkPolicy.Labels = utils.SpireServerLabels(spireServerNetworkPolicy.Labels) | ||
| return spireServerNetworkPolicy | ||
| } | ||
|
|
||
| func (r *StaticResourceReconciler) getSpireServerAllowIngressToMetricsNetworkPolicy() *v1.NetworkPolicy { | ||
| spireServerNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireServerAllowIngressToMetricsNetworkPolicyAssetName)) | ||
| spireServerNetworkPolicy.Labels = utils.SpireServerLabels(spireServerNetworkPolicy.Labels) | ||
| return spireServerNetworkPolicy | ||
| } | ||
|
|
||
| func (r *StaticResourceReconciler) getSpireServerAllowIngressToWebhookNetworkPolicy() *v1.NetworkPolicy { | ||
| spireServerNetworkPolicy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(utils.SpireServerAllowIngressToWebhookNetworkPolicyAssetName)) | ||
| spireServerNetworkPolicy.Labels = utils.SpireServerLabels(spireServerNetworkPolicy.Labels) | ||
| return spireServerNetworkPolicy | ||
| } |
There was a problem hiding this comment.
Handle asset loading errors gracefully instead of panicking.
All getter functions use assets.MustAsset(), which panics if the asset is missing. This will crash the operator instead of allowing graceful error handling and recovery. While missing assets indicate a build/packaging issue, the operator should fail gracefully with a clear error message rather than panicking.
Consider one of these approaches:
- Use a non-panicking asset loader and return errors from getter functions
- Validate all required assets exist during controller initialization
- Wrap MustAsset calls with recover() and return appropriate errors
Additionally, these getter functions follow an identical pattern with only the asset name and label function varying. Consider refactoring to reduce duplication:
// Helper function to reduce duplication
func (r *StaticResourceReconciler) loadAndLabelNetworkPolicy(
assetName string,
labelFunc func(map[string]string) map[string]string,
) *v1.NetworkPolicy {
policy := utils.DecodeNetworkPolicyObjBytes(assets.MustAsset(assetName))
policy.Labels = labelFunc(policy.Labels)
return policy
}
// Example usage
func (r *StaticResourceReconciler) getSpireAgentAllowEgressToApiServerNetworkPolicy() *v1.NetworkPolicy {
return r.loadAndLabelNetworkPolicy(
utils.SpireAgentAllowEgressToApiServerNetworkPolicyAssetName,
utils.SpireAgentLabels,
)
}This consolidates the repeated pattern and makes the code more maintainable.
🤖 Prompt for AI Agents
In pkg/controller/static-resource-controller/network-policy.go (lines 53-122):
the current getters call assets.MustAsset(...) which panics on missing assets
and duplicate the same load-and-label pattern; change them to use a
non-panicking loader (or check asset existence) and return errors so callers can
handle failures gracefully, or validate all assets up-front during controller
initialization; also refactor the repeated code into a single helper method that
accepts the asset name and a label function, loads the asset (with error
propagation), decodes it, applies labels, and returns the *v1.NetworkPolicy and
error, then update each getter to call that helper and propagate/handle errors.
|
@PillaiManish: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
4 participants
No description provided.