Conversation
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8538 +/- ##
==========================================
+ Coverage 74.16% 74.21% +0.04%
==========================================
Files 242 242
Lines 37624 37708 +84
==========================================
+ Hits 27903 27984 +81
+ Misses 7773 7771 -2
- Partials 1948 1953 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| - port: 9001 | ||
| name: http | ||
| protocol: TCP | ||
| referenceGrants: |
There was a problem hiding this comment.
this PR fixes unnecessary refGrant.
Gateway Policy (ns/envoy-gateway) has extAuth setting and auth-service exist in envoy-gateway.
Previously, during the merge, it was treated as spec of route policy, which meant that references from different namespaces.
| type: Opaque | ||
| data: | ||
| .htpasswd: dXNlcjE6e1NIQX15LzJzWUFqNXlyUUlONFRMMFlkUGRtR05LcGM9 | ||
| - apiVersion: v1 |
There was a problem hiding this comment.
this PR fixes unnecessary secret in Route ns.
233556d to
3a7dc97
Compare
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
3a7dc97 to
c9abe3a
Compare
| prefix: /foo | ||
| security: | ||
| basicAuth: | ||
| name: securitypolicy/envoy-gateway/policy-for-gateway |
There was a problem hiding this comment.
merge policy can resolve parent policy's auth ref and generate IR name correctly.
| grpc: | ||
| authority: grpc-backend.envoy-gateway:9000 | ||
| destination: | ||
| metadata: |
There was a problem hiding this comment.
merge policy can resolve parent policy's ext auth backendRef and generate IR name correctly.
| cookieSuffix: 811c9dc5 | ||
| hmacSecret: '[redacted]' | ||
| logoutPath: /logout | ||
| name: securitypolicy/envoy-gateway/policy-for-gateway |
There was a problem hiding this comment.
merge policy can resolve parent policy's oidc refs and generate IR name correctly.
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
|
|
||
| if contextExtensions, err = t.buildContextExtensions(policy.Spec.ExtAuth.ContextExtensions, policy.Namespace); err != nil { | ||
| ownerPolicy := resolvePolicyFieldOwner(fieldOwners, spFieldExtAuthContextExtensions, policy) | ||
| if contextExtensions, err = t.buildContextExtensions(policy.Spec.ExtAuth.ContextExtensions, ownerPolicy.Namespace); err != nil { |
There was a problem hiding this comment.
We'll need a per-entry owner map for ContextExtensions. It may contain ContextExtension valueRefs from different namespaces after merging.
There was a problem hiding this comment.
thanks for comment :)
Yes, while checking the CRD markers, I initially thought the same thing as your comment.
https://github.com/envoyproxy/gateway/blob/main/api/v1alpha1/ext_auth_types.go#L73-L78
But, while adding SecurityPolicy to tests for the policy merge function, I confirmed that ContextExtensions is merged by replacing the entire array.
(add test file utils/testdata/securitypolicy_all in this PR)
Let me know if I missing sometheing. Or is the entire-array replacement a separate issue?
There was a problem hiding this comment.
On second thought, full-array replacement might be the correct behavior, since this is part of ExtAuth.
cc @maxbrunet
| routePolicy *egv1a1.SecurityPolicy, | ||
| parentPolicy *egv1a1.SecurityPolicy, | ||
| ) PolicyFieldOwners[*egv1a1.SecurityPolicy] { | ||
| // Route policy owners are applied last so they override parent owners when both define the same field key. |
There was a problem hiding this comment.
Isn't it possible that some of the resources under a path that is a list might belong to route while some others might belong to parent? For example if Spec.JWT.Providers are merged from both route and parent who will be the owner of this field?
There was a problem hiding this comment.
thanks, that’s a good point.
I had the same concern, so I checked how merging behaves in SP (utils/testdata/securitypolicy_all in this PR).
I confirmed that all list fields are merged by replacing the entire array.
If we will add any list field that is not merged via full-array replacement, we would need a per-entry ownership map.
There was a problem hiding this comment.
Full-array replacement within an auth method seems the right default for me. Merging inner properties across different levels is error-prone, as the fields are often interrelated and shouldn’t be merged implicitly.
What this PR does / why we need it:
follow up #7918
introduce
PolicyFieldOwnersto fixes below issues when parent/route Policy mergeLocalObjectReference/BackendRefcorrectlyWhich issue(s) this PR fixes:
Fixes #
Release Notes: No