Skip to content
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

feat: add ability to add custom netpols for prometheus-stack package #997

Merged
merged 8 commits into from
Nov 18, 2024

Conversation

joelmccoy
Copy link
Contributor

@joelmccoy joelmccoy commented Nov 9, 2024

Description

Adds a value to allow custom netpols for the prometheus-stack package. Following a similar pattern that we use for vector. Additionally added some documentation on how to override and add custom netpols to prometheus stack and vector.

Related Issue

Fixes #996

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@joelmccoy joelmccoy requested a review from a team as a code owner November 9, 2024 21:35
@noahpb
Copy link
Contributor

noahpb commented Nov 13, 2024

Based on the discussion here, I'm not sure if this is the preferred approach that we'd like to take for creating additional Network Policies - we have some more design to do here. Given that we currently don't have a solution, I think this is a reasonable approach that can work in the interim. cc @mjnagel and @UnicornChance.

@joelmccoy
Copy link
Contributor Author

joelmccoy commented Nov 13, 2024

@noahpb Good callout. Missed that issue. It sounds like the preferred approach would be to minimize the customizability of these netpols. The main use case I am trying to support is to allow alertmanager to egress to some place. This place is going to be different for each customer/environment. Would it be preferred to have a value that is more specific to where alertmanager can egress to?

A simple value like alertmanagerEgress: [] which provides a list of ip ranges / netpol targets that it can go to?

@mjnagel
Copy link
Contributor

mjnagel commented Nov 13, 2024

@noahpb / @joelmccoy yeah my main thoughts with that mention on the other issue are that we should try to capture specific use cases where we can. I don't think we necessarily should lock down all customizability, but where there are known needs to support it would be great to explicitly have a rule for those.

A specific value for alertmanager egress might make sense in this case, with conditionals for inCluster, cidr, or fallback for anywhere (in the future ideal state we could support specific hosts, i.e. slack.com or something). I also know that linked issue called out a few other needs for Thanos support and other alert sources so I could also see an argument to allow any custom policies.

Two other comments on this:

  • It might be good to try and document this somewhere that surfaces on the docs site, like a "networking configuration" page that gives an example + shows the value to override.
  • I don't like the custom value name personally. I know this is a pattern emerging with SWF and other apps but without looking at the template/having the tribal knowledge of what that does the name custom means nothing. I think we should be a lot more explicit with the name, i.e. customNetworking or additionalNetworkAllow which more directly connect to the field in the Package CR.

@joelmccoy
Copy link
Contributor Author

joelmccoy commented Nov 13, 2024

Good feedback and clarity. Do we have a preference on how flexible we want this too be? For the alertmanager use case, I am thinking of two routes we could take at this point:

  1. Add a field like customNetworking or additionalNetworkAllow and then give access to the end user to configure the network properties directly?
    or..
  2. Do we take a less flexible approach and add a field called something like alertmanagerEgressAllow which is only allowed to customize where alertmanager can send alerts.

Happy to document example overrides in the docs for whatever route we prefer

@mjnagel
Copy link
Contributor

mjnagel commented Nov 14, 2024

@joelmccoy I think option 1 for now. This provides the flexibility that people need and supports unknown end user needs. I think if/when we identify specific possible network targets we should add conditional templates for those specific pieces, but will likely still want this block to support other unknown needs.

@joelmccoy
Copy link
Contributor Author

Based on feedback added in an additionalNetworkAllow value for the prometheus stack. This is the same pattern currently used for vector. Additionally added some documentation to demonstrate how to add additional network allowances for both Prometheus Stack and Vector.

noahpb
noahpb previously approved these changes Nov 18, 2024
Copy link
Contributor

@noahpb noahpb left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks for adding a doc! :)

@mjnagel
Copy link
Contributor

mjnagel commented Nov 18, 2024

Will rebase and merge once #1020 merges.

@mjnagel mjnagel merged commit 472f9c5 into main Nov 18, 2024
28 checks passed
@mjnagel mjnagel deleted the add-custom-network-policies branch November 18, 2024 22:52
mjnagel pushed a commit that referenced this pull request Nov 22, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.32.0](v0.31.2...v0.32.0)
(2024-11-22)


### Features

* add ability to add custom netpols for prometheus-stack package
([#997](#997))
([472f9c5](472f9c5))
* add checkpoint uds-core slim package
([#818](#818))
([d95f6be](d95f6be))
* allow additional network rules for grafana and neuvector
([#1038](#1038))
([5c84007](5c84007))


### Bug Fixes

* keycloak upgrade wait
([#1037](#1037))
([1207812](1207812))


### Miscellaneous

* add variables for pepr memory requests in dev/demo bundles
([#1021](#1021))
([867501c](867501c))
* architecture diagrams
([#1024](#1024))
([d0bca43](d0bca43))
* **deps:** update grafana helm chart
([#998](#998))
([25d4c29](25d4c29))
* **deps:** update grafana to v11.3.1
([#1023](#1023))
([8d3cf3a](8d3cf3a))
* **deps:** update husky to v9.1.7
([#1014](#1014))
([0d9a854](0d9a854))
* **deps:** update kfc for jest to v3.3.3
([#1015](#1015))
([eba189e](eba189e))
* **deps:** update neuvector to 5.4.0
([#778](#778))
([ccd0a32](ccd0a32))
* **deps:** update pepr to v0.40.1
([#1025](#1025))
([871bdad](871bdad))
* **deps:** update support-deps
([#1006](#1006))
([bfb66a4](bfb66a4))
* **deps:** update support-deps
([#1019](#1019))
([82dfb32](82dfb32))
* **deps:** update velero helm chart to v8
([#999](#999))
([e8187be](e8187be))
* fix duplicative checkpoint publish location
([#1020](#1020))
([b497fc5](b497fc5))
* update diagrams
([#1035](#1035))
([cca5e2c](cca5e2c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Allow Custom Netpols for Prometheus Stack Package
3 participants