-
Notifications
You must be signed in to change notification settings - Fork 34
generate network policy per operand #1246
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
base: main
Are you sure you want to change the base?
Conversation
7bd6f48 to
1d1cb4b
Compare
andreasgerstmayr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add unit tests with the generated policy? ideally in yaml format (to be less verbose than Golang), like here: https://github.com/grafana/tempo-operator/blob/main/internal/manifests/config/build_test.go#L23-L26
|
y, I will do once its working. |
pavolloffay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it is missing the policy for the gateway
| @@ -0,0 +1,171 @@ | |||
| package networking | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a similar comment as the previous one. I would be more specific and rename it to networkpolicies
| policies = append(policies, generatePolicyFor(tempo, manifestutils.QuerierComponentName)) | ||
| policies = append(policies, generatePolicyFor(tempo, manifestutils.QueryFrontendComponentName)) | ||
|
|
||
| return policies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So If I am not mistaken this creates 3+5policies(per component).
We might need to have percomponent policy, but those 3 generic one can be embeded into the policy per component, which would make it easier to understand what policy applies to a component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces network policy generation for Tempo operands to limit network access between components. The implementation adds a new networking configuration option to the TempoStack spec and generates NetworkPolicy resources for each component based on defined communication relationships.
- Adds NetworkingSpec to the TempoStack CRD with an enabled flag
- Implements per-component network policy generation with ingress/egress rules
- Defines component communication relationships and port mappings
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| api/tempo/v1alpha1/tempostack_types.go | Adds NetworkingSpec struct and networking field to TempoStackSpec |
| internal/manifests/networking/operands.go | Main function to generate network policies for all operands |
| internal/manifests/networking/components.go | Core logic for generating per-component policies and defining relationships |
| internal/manifests/networking/components_test.go | Test cases for network policy generation and relation reversal |
| config/crd/bases/tempo.grafana.com_tempostacks.yaml | CRD definition update for networking spec |
| bundle//manifests/ | Generated bundle files with CRD and CSV updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| s3Conn = []networkingv1.NetworkPolicyPort{ | ||
| { // TODO: get this from secret? | ||
| Protocol: ptr.To(corev1.ProtocolTCP), | ||
| Port: ptr.To(intstr.FromInt(443)), | ||
| }, | ||
| { // TODO: get this from secret? | ||
| Protocol: ptr.To(corev1.ProtocolTCP), | ||
| Port: ptr.To(intstr.FromInt(9000)), | ||
| }, |
Copilot
AI
Sep 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded ports 443 and 9000 for S3 connections should be configurable or derived from the storage configuration. The TODO comments indicate this is a known issue that should be addressed.
3e42611 to
7452f0a
Compare
Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
Downside of this approach is that we need to manually align the mapping when we change e.g. a service. But we could generate services based on this too. wdyt?