-
Notifications
You must be signed in to change notification settings - Fork 366
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
bug: Route extension filters with different types but the same name and namespace aren't correctly cached #3388
bug: Route extension filters with different types but the same name and namespace aren't correctly cached #3388
Conversation
/retest |
1 similar comment
/retest |
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.
LGTM!
@@ -121,14 +121,14 @@ type resourceMappings struct { | |||
// extensionRefFilters is a map of filters managed by an extension. | |||
// The key is the namespaced name of the filter and the value is the | |||
// unstructured form of the resource. | |||
extensionRefFilters map[types.NamespacedName]unstructured.Unstructured | |||
extensionRefFilters map[message.NamespacedNameAndType]unstructured.Unstructured |
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 we reuse this structure or create a new similar one?
type ObjectKindNamespacedName struct { |
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.
all fields are string type should be okay
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.
The problem with
type ObjectKindNamespacedName struct { |
This means that if it is used as the key, then these two different objects will be considered the same:
---
apiVersion: some.group/v1
kind: Type1
metadata:
name: some-name
namespace: default
---
apiVersion: some.other.group/v1
kind: Type1
metadata:
name: some-name
namespace: default
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 we can define a new structure with group
in it, like ObjectGroupKindNamespacedName
, instead of defining at internal/message
, just to keep code consistency, since all your changes are related to k8s provider.
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.
Done.
caching them should be done with both the name and type as a key. Signed-off-by: Lior Okman <lior.okman@sap.com>
be clearer about what it has. Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
2c8327c
to
a70618e
Compare
/retest |
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.
Some comments on the naming, else LGTM
internal/utils/misc.go
Outdated
"k8s.io/apimachinery/pkg/types" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
) | ||
|
||
type NamespacedNameAndGroupKind struct { |
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.
type NamespacedNameAndGroupKind struct { | |
type NamespacedNameWithGroupKind struct { |
prefer with
internal/utils/misc.go
Outdated
schema.GroupKind | ||
} | ||
|
||
// NamespacedNameAndType creates and returns object's NamespacedNameAndType. |
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.
comments also need to update
Signed-off-by: Lior Okman <lior.okman@sap.com>
/retest |
1 similar comment
/retest |
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.
Nice work, thanks 👍
…nd namespace aren't correctly cached (envoyproxy#3388) * Route extension filters are unstructured.Unstructured instances, so caching them should be done with both the name and type as a key. Signed-off-by: Lior Okman <lior.okman@sap.com> * Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to be clearer about what it has. Signed-off-by: Lior Okman <lior.okman@sap.com> * Also renamed the helper function. Signed-off-by: Lior Okman <lior.okman@sap.com> * Moved to the 'utils' package to be beside NamespacedName. Signed-off-by: Lior Okman <lior.okman@sap.com> * Renamed structure according to review, and updated the comments Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com> (cherry picked from commit 95e2e35) Signed-off-by: Arko Dasgupta <arko@tetrate.io>
…nd namespace aren't correctly cached (envoyproxy#3388) * Route extension filters are unstructured.Unstructured instances, so caching them should be done with both the name and type as a key. Signed-off-by: Lior Okman <lior.okman@sap.com> * Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to be clearer about what it has. Signed-off-by: Lior Okman <lior.okman@sap.com> * Also renamed the helper function. Signed-off-by: Lior Okman <lior.okman@sap.com> * Moved to the 'utils' package to be beside NamespacedName. Signed-off-by: Lior Okman <lior.okman@sap.com> * Renamed structure according to review, and updated the comments Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com> (cherry picked from commit 95e2e35) Signed-off-by: Arko Dasgupta <arko@tetrate.io>
* Use <proto>-<port> for naming service and container ports (#3130) * Use <proto>-<port> for naming service and container ports Takes inspiration from #2973 to name port, not off the listener but off the port-proto ensuring that patch (during updates) also works Fixes: #3111 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix test Signed-off-by: Arko Dasgupta <arko@tetrate.io> * move to helper pkg Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> * lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit c41247b) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * bug: Tests are failing due to an expired certificate in one of the translator tests (#3476) Replaced a certificate in the test that had expired. The old certificate expired May 24 2024: Certificate: Data: Version: 1 (0x0) Serial Number: ca:7c:5c:b7:25:5d:bb:f9 Signature Algorithm: ecdsa-with-SHA256 Issuer: CN=test.example.com Validity Not Before: May 25 14:10:42 2023 GMT Not After : May 24 14:10:42 2024 GMT Subject: CN=test.example.com Subject Public Key Info: Public Key Algorithm: id-ecPublicKey Public-Key: (384 bit) pub: 04:78:cb:47:0b:78:48:7a:ad:90:b1:d9:2d:4a:2f: d9:35:1f:cc:28:d6:af:4a:6d:c7:36:7e:ed:1a:88: 1f:a9:aa:a7:f0:04:a0:1c:86:bb:c9:45:3e:f8:fb: 28:0c:3e:a4:7f:ef:82:7b:bb:ac:77:49:90:3b:54: a7:75:82:16:8f:64:0b:88:8c:f4:35:91:fc:07:f4: 2b:e2:2e:c9:d0:82:b0:b1:09:54:9e:e9:d9:aa:fe: 4a:63:d4:cb:41:ad:27 ASN1 OID: secp384r1 NIST CURVE: P-384 Signature Algorithm: ecdsa-with-SHA256 Signature Value: 30:65:02:31:00:86:4e:33:e4:86:37:4c:26:a7:be:57:51:44: 8e:6c:88:ea:3c:03:58:00:a3:5e:7a:53:9e:2c:54:b3:ab:82: 25:fe:4c:e4:be:4d:8c:56:e2:da:d8:de:d2:20:ca:13:55:02: 30:0c:2a:27:a7:fd:2b:a9:87:4f:06:ea:4e:2d:cc:48:4d:9d: d7:cf:73:88:6d:98:54:18:83:6d:e5:a9:c3:84:75:c9:ee:c6: 0d:1a:15:a2:8c:68:86:88:83:17:b9:7a:9b The new certificate is good for 10 years. Certificate: Data: Version: 3 (0x2) Serial Number: 42:29:94:01:e1:cb:32:dc:f8:b4:64:6d:9e:1e:28:8d:7b:5a:53:3b Signature Algorithm: ecdsa-with-SHA256 Issuer: CN=test.example.com Validity Not Before: May 25 09:11:37 2024 GMT Not After : May 23 09:11:37 2034 GMT Subject: CN=test.example.com Subject Public Key Info: Public Key Algorithm: id-ecPublicKey Public-Key: (384 bit) pub: 04:78:cb:47:0b:78:48:7a:ad:90:b1:d9:2d:4a:2f: d9:35:1f:cc:28:d6:af:4a:6d:c7:36:7e:ed:1a:88: 1f:a9:aa:a7:f0:04:a0:1c:86:bb:c9:45:3e:f8:fb: 28:0c:3e:a4:7f:ef:82:7b:bb:ac:77:49:90:3b:54: a7:75:82:16:8f:64:0b:88:8c:f4:35:91:fc:07:f4: 2b:e2:2e:c9:d0:82:b0:b1:09:54:9e:e9:d9:aa:fe: 4a:63:d4:cb:41:ad:27 ASN1 OID: secp384r1 NIST CURVE: P-384 X509v3 extensions: X509v3 Subject Key Identifier: DA:49:EA:13:99:CA:DE:10:D2:70:2B:27:E2:60:AA:E0:F4:7B:EA:50 X509v3 Authority Key Identifier: DA:49:EA:13:99:CA:DE:10:D2:70:2B:27:E2:60:AA:E0:F4:7B:EA:50 X509v3 Basic Constraints: critical CA:TRUE Signature Algorithm: ecdsa-with-SHA256 Signature Value: 30:65:02:30:6d:4e:25:4f:84:f4:38:7e:c4:de:c8:d1:55:0c: af:4b:e4:c0:a1:f3:59:de:fb:48:0a:96:07:65:29:9f:fe:7c: 3c:ee:f0:c9:ca:17:bc:cd:bd:a4:31:38:24:4f:c6:e5:02:31: 00:e6:9a:ce:52:60:4b:b8:0e:e7:23:6d:8a:69:a0:21:e5:d1: bb:e8:e9:09:6a:32:d6:8c:58:49:f4:76:86:e6:c1:b8:24:d3: 44:08:fa:1c:ef:34:70:c1:24:76:a9:35:8f Signed-off-by: Lior Okman <lior.okman@sap.com> (cherry picked from commit c2c9b43) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: use Patch API for infra-client (#3034) * fix(infrastructure): use Patch API instead Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: add interceptor for ApplyPatch on fake client Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: trigger make generate Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: remove update verb Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: SetUID no longer needed Signed-off-by: Ardika Bagus <me@ardikabs.com> --------- Signed-off-by: Ardika Bagus <me@ardikabs.com> (cherry picked from commit cc01bf5) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * refactor: infra client CreateOrUpdate to ServerSideApply (#3134) * refactor(infra-client): CreateOrUpdate to ServerSideApply Signed-off-by: Ardika Bagus <me@ardikabs.com> * test(infra-client): add e2e test for ServerSideApply Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: remove comment Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: fix linter Signed-off-by: Ardika Bagus <me@ardikabs.com> --------- Signed-off-by: Ardika Bagus <me@ardikabs.com> (cherry picked from commit 81108f2) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: duplicated xroutes are added to gatewayapi.Resources (#3282) fix duplicated xroutes Signed-off-by: Dingkang Li <dingkang1743@gmail.com> (cherry picked from commit 32c6876) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: add proxy protocol always as first listenerFilter (#3332) * add proxy protocol always as first listenerFilter Signed-off-by: Jesse Haka <haka.jesse@gmail.com> * add test Signed-off-by: Jesse Haka <haka.jesse@gmail.com> --------- Signed-off-by: Jesse Haka <haka.jesse@gmail.com> (cherry picked from commit 6d8f2dc) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: security policy reference grant from field type (#3386) fix: security policy reference grant from field Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com> (cherry picked from commit bd72474) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * bug: Route extension filters with different types but the same name and namespace aren't correctly cached (#3388) * Route extension filters are unstructured.Unstructured instances, so caching them should be done with both the name and type as a key. Signed-off-by: Lior Okman <lior.okman@sap.com> * Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to be clearer about what it has. Signed-off-by: Lior Okman <lior.okman@sap.com> * Also renamed the helper function. Signed-off-by: Lior Okman <lior.okman@sap.com> * Moved to the 'utils' package to be beside NamespacedName. Signed-off-by: Lior Okman <lior.okman@sap.com> * Renamed structure according to review, and updated the comments Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com> (cherry picked from commit 95e2e35) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix(translator): set ignoreCase for header matchers in extAuth (#3420) fix: set ignoreCase for header matchers in extAuth Signed-off-by: haoqixu <hq.xu0o0@gmail.com> (cherry picked from commit 8206e11) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix secrets/configmap updates do not trigger a controller reconcile (#3499) * ensure both secrets and config map reconcile upon changes ensure secret/config map changes trigger a reconcile Signed-off-by: Alex Volchok <alex.volchok@sap.com> * Update controller.go Signed-off-by: Alex Volchok <alex.volchok@sap.com> * Update controller.go Signed-off-by: Alex Volchok <alex.volchok@sap.com> --------- Signed-off-by: Alex Volchok <alex.volchok@sap.com> (cherry picked from commit ff2c598) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * feat: backend TLS SAN validation (#3507) * BTLS: enforce SAN validation Signed-off-by: Guy Daich <guy.daich@sap.com> * use dedicated cert for ext-proc e2e test Signed-off-by: Guy Daich <guy.daich@sap.com> * fix ext-proc server client tls settings Signed-off-by: Guy Daich <guy.daich@sap.com> --------- Signed-off-by: Guy Daich <guy.daich@sap.com> (cherry picked from commit dc201ba) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: ReplaceFullPath not working for root path (/) (#3530) * fix: ReplaceFullPath not working for root path (/) Takes #2817 forward Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 8f83c3d) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * chore: Remove namespace restriction for EnvoyProxy parametersRef reso… (#3544) chore: Remove namespace restriction for EnvoyProxy parametersRef resource (cherry picked from commit b870e39) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix test rm invalid diff related to #3134 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix GatewayInfraResourceTest Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Lior Okman <lior.okman@sap.com> Signed-off-by: Ardika Bagus <me@ardikabs.com> Signed-off-by: Dingkang Li <dingkang1743@gmail.com> Signed-off-by: Jesse Haka <haka.jesse@gmail.com> Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com> Signed-off-by: haoqixu <hq.xu0o0@gmail.com> Signed-off-by: Alex Volchok <alex.volchok@sap.com> Signed-off-by: Guy Daich <guy.daich@sap.com> Co-authored-by: Lior Okman <lior.okman@sap.com> Co-authored-by: Ardika <me@ardikabs.com> Co-authored-by: Dingkang Li <dingkang1743@gmail.com> Co-authored-by: Jesse Haka <haka.jesse@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> Co-authored-by: xu0o0 <hqcat6@gmail.com> Co-authored-by: Alex Volchok <alex.volchok@sap.com> Co-authored-by: Guy Daich <guy.daich@sap.com> Co-authored-by: zou rui <xiaorui.zou@gmail.com>
What this PR does / why we need it:
The provider caches
extensionRef
instances based on their name and namespace. But the actual object stored is an Unstructured instance, which means that if two instances of two different objects have the same name and namespace, then the cache would override the first with the second.This PR adds a unit test to include this situation and fixes the current cache to also consider the extensionRef's type when caching it.