compiler: claude can we have a kubernetes reporter #3
Conversation
holy |
| // Clean and recreate the output directory. | ||
| if root.exists() { | ||
| if root.is_dir() { | ||
| std::fs::remove_dir_all(root) |
There was a problem hiding this comment.
like it's not too bad in theory, but the .env files kind of invite you to edit them so
There was a problem hiding this comment.
yeah, maybe let's make people rm -rf <whatever> && amber --k8s <whatever>. not that bad compared to overwriting a .env, which can be really annoying
nhynes
left a comment
nhynes
left a comment
There was a problem hiding this comment.
looks mostly reasonable. we should strongly consider whether it makes sense to include the cni checker which is more weighty than anticipated. an ignored rust test that exercises the new reporter in KinD and runs in CI would be cool. also a hoisting and cleanup pass. I'll run this through codex for completeness
| #[arg(long = "disable-networkpolicy-check", requires = "kubernetes")] | ||
| disable_networkpolicy_check: bool, |
There was a problem hiding this comment.
Interesting. So far I haven't had a good answer for how to add reporter-specific args. Thanks for making a movement on that.
| // Clean and recreate the output directory. | ||
| if root.exists() { | ||
| if root.is_dir() { | ||
| std::fs::remove_dir_all(root) |
There was a problem hiding this comment.
yeah, maybe let's make people rm -rf <whatever> && amber --k8s <whatever>. not that bad compared to overwriting a .env, which can be really annoying
| .collect::<Vec<_>>() | ||
| .join(" -> "); | ||
| return Err(ReporterError::new(format!( | ||
| "kubernetes reporter requires an acyclic dependency graph (ignoring weak bindings). \ |
There was a problem hiding this comment.
I wonder if this error shouldn't be some non-string error or something since the compose reporter also prefers acyclic. I also wonder if not all scenarios should be required to be acyclic, hoisting the check to scenario construction (linking) time.
| Ok(endpoint.port) | ||
| } | ||
|
|
||
| fn compose_templates_dfs( |
|
|
||
| /// Try to resolve a config query against a composed template. | ||
| /// Returns Static if the value is fully resolved, Runtime if it contains config refs. | ||
| fn resolve_config_query_for_program( |
There was a problem hiding this comment.
This PR could use a pass to collect all of the shared code into one place. I can nominate myself for that
| /// - Phase 1: Try to connect to allowed server. If this fails, networking is broken - exit with error. | ||
| /// - Phase 2: Try to connect to blocked server. If this succeeds, NetworkPolicy isn't enforced - exit with error. | ||
| /// - Success: Both checks pass, stay alive. | ||
| fn generate_netpol_enforcement_check( |
There was a problem hiding this comment.
I wonder if we really care that much that we would add 300 lines of code for it. This should be replaced at some point by L7 enforcement, so maybe we don't bother
| miette = { workspace = true, features = ["derive"] } | ||
| serde = { workspace = true, features = ["derive"] } | ||
| serde_json = { workspace = true } | ||
| serde_yaml = { workspace = true } |
There was a problem hiding this comment.
controversial opinion: yaml is a superset of json. write out json instead of bringing in yaml
| - `linker`: schema validation, binding resolution, and export verification. | ||
| - `passes`: graph rewrites that must preserve scenario invariants. | ||
| - `reporter`: transforms `CompileOutput` into artifacts (e.g., scenario IR JSON, DOT, Docker Compose YAML). | ||
| - `reporter`: transforms `CompileOutput` into artifacts (e.g., scenario IR JSON, DOT, Docker Compose YAML, Kubernetes YAML). |
There was a problem hiding this comment.
and then leave it described as kubernetes YAML to mess with people
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da969e8784
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Add ingress rules for bound consumers. | ||
| if let Some(allowed) = inbound_allow.get(id) { | ||
| // Group by port. |
There was a problem hiding this comment.
The NetworkPolicy here only adds ingress rules based on inbound_allow (bindings). If a component is only exported (no internal bindings), inbound_allow is empty so the generated policy has zero ingress rules, which is a default‑deny for all ingress to that pod. That makes scenario exports unreachable from any external client (other namespaces, ingress, or utility pods) despite the service being created. Consider adding rules for exported ports (or skipping the policy) so exported provides remain accessible.
Useful? React with 👍 / 👎.
|
Vague suggestion: docker-compose and k8s work in much the same way with programs and sidecars being turned into a "workload" and then rendered. There's a lot of duplicated code to prove this point. Would it be valuable to move the shared logic into one or more compose+k8s passes so that they can both be thin renderers on top of the same postprocessed IR? If you don't think that makes sense to put in this PR, I'm happy to try a refactoring pass after it's in. |
--kubernetes <output dir>writes a directory of kubernetes config files and a kustomization.yaml. usekubectl apply -k <ouptut_dir>to put up the scenarioamber-root-configConfigMap out of itamber-root-configvalues into places OR use the amber-compose-helper to exec a program with string interpolations using those values