Conversation
hown3d
requested review from
dergeberl,
robinschneider and
timebertt
as code owners
hown3d
force-pushed
the
refactor/types
branch
2 times, most recently
from
859f14e to
9e7beaf
Compare
dergeberl
left a comment
dergeberl
left a comment
There was a problem hiding this comment.
Very nice to have this objects typed! 🎉
We added a few comments.
I will have a second look incl. the cluster on the ondemand after resolving our comments.
| if err := json.Unmarshal([]byte(originalFilter.Raw), &originalFilterMap); err != nil { | ||
| return admission.Errored(http.StatusInternalServerError, err) | ||
| var originalFilter *structpb.Struct | ||
| for _, configpatch := range filter.Spec.ConfigPatches { |
There was a problem hiding this comment.
In the gjson we only checked the ConfigPatches[0].
There was a problem hiding this comment.
We could break the outer loop when we find the first occurence of"envoy.filters.network.tcp_proxy" in a filterList. This way we are safe if the configPatches changes and something is added before the tcp_proxy filter
hown3d
force-pushed
the
refactor/types
branch
2 times, most recently
from
bdf5b81 to
3c0bb13
Compare
| - remote_ip: | ||
| address_prefix: 10.96.0.0 | ||
| prefix_len: 11 | ||
| stat_prefix: envoyrbac |
There was a problem hiding this comment.
This was previously a bug, there is not stat_prefix field in http.rbac.v3.RBAC message.
Ref: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/rbac/v3/rbac.proto
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
There was a problem hiding this comment.
Pull request overview
Refactors EnvoyFilter generation to use typed Envoy/Istio protobuf definitions (instead of map[string]interface{}), improving type safety and easing future maintenance.
Changes:
- Reworks EnvoyFilter builders to return typed
*istio.io/api/networking/v1alpha3.EnvoyFilterobjects and constructs typed RBAC filter configs usinggo-control-planeprotos. - Updates unit tests and golden testdata to compare rendered output via JSON equivalence (and adjusts field names like
rules_stat_prefix). - Wires new builder signatures through the actuator and updates module dependencies to include protobuf/Envoy proto packages.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/envoyfilters/envoyfilters.go | Core refactor: build typed EnvoyFilter + typed RBAC configs; introduces proto→structpb.Struct helper. |
| pkg/envoyfilters/envoyfilters_test.go | Updates tests to compare JSON-equivalent output; adapts to new return types and signatures. |
| pkg/envoyfilters/testdata/*.yaml | Adjusts golden specs for protojson defaults (e.g., action omitted) and rules_stat_prefix naming. |
| pkg/controller/actuator.go | Updates call sites to handle new (spec, error) builder signatures. |
| go.mod / go.sum | Adds dependencies for Envoy protos and protobuf tooling; adjusts YAML/protobuf deps. |
Comments suppressed due to low confidence (1)
pkg/envoyfilters/envoyfilters.go:416
ruleCIDRsToPrincipalappendsalwaysAllowedCIDRsonly whenrule.Action == "ALLOW", which is case-sensitive. Since validation accepts any casing, an action like "allow" would skip adding always-allowed CIDRs and could unintentionally block cluster-internal communication. Consider using a case-insensitive check (or reusingactionProto()/ normalized action) here as well.
// if the rule has action "ALLOW" (which means "limit the access to only the
// specified IPs", we need to insert the node CIDR range to not block
// cluster-internal communication)
if rule.Action == "ALLOW" {
for _, cidr := range alwaysAllowedCIDRs {
prefix, length, err := getPrefixAndPrefixLength(cidr)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (r *ACLRule) actionProto() (envoy_rbacv3.RBAC_Action, error) { | ||
| switch r.Action { | ||
| case "DENY": | ||
| return envoy_rbacv3.RBAC_DENY, nil | ||
| case "ALLOW": | ||
| return envoy_rbacv3.RBAC_ALLOW, nil | ||
| default: | ||
| return -1, fmt.Errorf("unknown action %s", r.Action) | ||
| } |
| } | ||
|
|
||
| // FilterPatch represents the object beneath EnvoyFilter.spec.configPatches.patch.value | ||
| // It holds the name of the filter and it's typed config to inject into the envoy config |
timebertt
left a comment
timebertt
left a comment
There was a problem hiding this comment.
Ufff, this is really hard to review. I can only judge high-level code quality, e.g., error handling, etc.
However, the tests comparing the marshalled results with the expected YAML give me confidence that everything will still work as expected.
| result, err := CreateInternalFilterPatchFromRule(rule, alwaysAllowedCIDRs, []string{}) | ||
|
|
||
| Expect(err).ToNot(HaveOccurred()) | ||
| checkIfMapEqualsYAML(result, "singleFiltersAllowEntry.yaml") |
There was a problem hiding this comment.
Can we remove the singleFiltersAllowEntry.yaml file?
What this PR does / why we need it:
Refactors the code to use the proto definitions of envoy instead of relying on map[string]interface{}.
This ensures type safety and increases developer experience a lot, since you no longer have to dig deep in the envoy documentation in regards to syntax.
Special notes for your reviewer:
/cc @timebertt
Tested on
ond-lambwith shootacl