Skip to content

bug: Route extension filters with different types but the same name and namespace aren't correctly cached#3388

Merged
arkodg merged 5 commits intoenvoyproxy:mainfrom
liorokman:route-filter-bad-resource-cache
May 20, 2024
Merged

bug: Route extension filters with different types but the same name and namespace aren't correctly cached#3388
arkodg merged 5 commits intoenvoyproxy:mainfrom
liorokman:route-filter-bad-resource-cache

Conversation

@liorokman
Copy link
Copy Markdown
Contributor

@liorokman liorokman commented May 13, 2024

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.

@liorokman liorokman requested a review from a team as a code owner May 13, 2024 16:44
@liorokman
Copy link
Copy Markdown
Contributor Author

liorokman commented May 13, 2024

/retest

1 similar comment
@liorokman
Copy link
Copy Markdown
Contributor Author

liorokman commented May 13, 2024

/retest

@arkodg arkodg requested a review from Alice-Lilith May 13, 2024 19:02
guydc
guydc previously approved these changes May 17, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@guydc guydc left a comment

Choose a reason for hiding this comment

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

LGTM!

shawnh2
shawnh2 reviewed May 17, 2024
View reviewed changes
internal/provider/kubernetes/controller.go Outdated
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 May 17, 2024

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?

gateway/internal/provider/kubernetes/helpers.go

Line 28 in 02ff9eb

type ObjectKindNamespacedName struct {

Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 May 17, 2024

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

Copy link
Copy Markdown
Contributor Author

@liorokman liorokman May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The problem with

gateway/internal/provider/kubernetes/helpers.go

Line 28 in 02ff9eb

type ObjectKindNamespacedName struct {
is that it doesn't contain the object's group, only the kind.

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

Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 May 18, 2024 •
edited
Loading

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.

Copy link
Copy Markdown
Contributor Author

@liorokman liorokman May 18, 2024

Choose a reason for hiding this comment

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

Done.

liorokman added 4 commits May 18, 2024 09:12
@liorokman
Route extension filters are unstructured.Unstructured instances, so
9f18f53 
caching them should be done with both the name and type as a key.

Signed-off-by: Lior Okman <[email protected]>
@liorokman
Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to
347b4a5 
be clearer about what it has.

Signed-off-by: Lior Okman <[email protected]>
@liorokman
Also renamed the helper function.
a974355 
Signed-off-by: Lior Okman <[email protected]>
@liorokman
Moved to the 'utils' package to be beside NamespacedName.
a70618e 
Signed-off-by: Lior Okman <[email protected]>
@liorokman liorokman force-pushed the route-filter-bad-resource-cache branch from 2c8327c to a70618e Compare May 18, 2024 06:16
@liorokman
Copy link
Copy Markdown
Contributor Author

liorokman commented May 18, 2024

/retest

shawnh2
shawnh2 reviewed May 18, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 left a 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
"sigs.k8s.io/controller-runtime/pkg/client"
)

type NamespacedNameAndGroupKind struct {
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 May 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
type NamespacedNameAndGroupKind struct {
type NamespacedNameWithGroupKind struct {

prefer with

internal/utils/misc.go Outdated
schema.GroupKind
}

// NamespacedNameAndType creates and returns object's NamespacedNameAndType.
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 May 18, 2024

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

@liorokman
Copy link
Copy Markdown
Contributor Author

liorokman commented May 18, 2024

/retest

1 similar comment
@liorokman
Copy link
Copy Markdown
Contributor Author

liorokman commented May 18, 2024

/retest

Alice-Lilith
Alice-Lilith approved these changes May 20, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Alice-Lilith Alice-Lilith left a comment

Choose a reason for hiding this comment

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

Nice work, thanks 👍

@arkodg arkodg merged commit 95e2e35 into envoyproxy:main May 20, 2024
@liorokman liorokman deleted the route-filter-bad-resource-cache branch May 21, 2024 12:44
arkodg pushed a commit to arkodg/gateway that referenced this pull request Jun 11, 2024
@liorokman
bug: Route extension filters with different types but the same name a…
7b2943d 
…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 <[email protected]>

* Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to
be clearer about what it has.

Signed-off-by: Lior Okman <[email protected]>

* Also renamed the helper function.

Signed-off-by: Lior Okman <[email protected]>

* Moved to the 'utils' package to be beside NamespacedName.

Signed-off-by: Lior Okman <[email protected]>

* Renamed structure according to review, and updated the comments

Signed-off-by: Lior Okman <[email protected]>

---------

Signed-off-by: Lior Okman <[email protected]>
(cherry picked from commit 95e2e35)
Signed-off-by: Arko Dasgupta <[email protected]>
arkodg pushed a commit to arkodg/gateway that referenced this pull request Jun 12, 2024
@liorokman
bug: Route extension filters with different types but the same name a…
46de4bd 
…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 <[email protected]>

* Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to
be clearer about what it has.

Signed-off-by: Lior Okman <[email protected]>

* Also renamed the helper function.

Signed-off-by: Lior Okman <[email protected]>

* Moved to the 'utils' package to be beside NamespacedName.

Signed-off-by: Lior Okman <[email protected]>

* Renamed structure according to review, and updated the comments

Signed-off-by: Lior Okman <[email protected]>

---------

Signed-off-by: Lior Okman <[email protected]>
(cherry picked from commit 95e2e35)
Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg mentioned this pull request Jun 12, 2024
Merged
Xunzhuo pushed a commit that referenced this pull request Jun 12, 2024
@arkodg @liorokman
@ardikabs @aoledk @zetaab @eguzki @haoqixu @alexwo @guydc @phantooom
[release/v1.0] Cherry-pick fixes for v1.0.2 (#3593)
7ffa89f 
* 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 <[email protected]>

* testdata

Signed-off-by: Arko Dasgupta <[email protected]>

* fix test

Signed-off-by: Arko Dasgupta <[email protected]>

* move to helper pkg

Signed-off-by: Arko Dasgupta <[email protected]>

* fix e2e

Signed-off-by: Arko Dasgupta <[email protected]>

* lint

Signed-off-by: Arko Dasgupta <[email protected]>

* fix e2e

Signed-off-by: Arko Dasgupta <[email protected]>

---------

Signed-off-by: Arko Dasgupta <[email protected]>
(cherry picked from commit c41247b)
Signed-off-by: Arko Dasgupta <[email protected]>

* 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 <[email protected]>
(cherry picked from commit c2c9b43)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: use Patch API for infra-client (#3034)

* fix(infrastructure): use Patch API instead

Signed-off-by: Ardika Bagus <[email protected]>

* chore: add interceptor for ApplyPatch on fake client

Signed-off-by: Ardika Bagus <[email protected]>

* chore: trigger make generate

Signed-off-by: Ardika Bagus <[email protected]>

* chore: remove update verb

Signed-off-by: Ardika Bagus <[email protected]>

* chore: SetUID no longer needed

Signed-off-by: Ardika Bagus <[email protected]>

---------

Signed-off-by: Ardika Bagus <[email protected]>
(cherry picked from commit cc01bf5)
Signed-off-by: Arko Dasgupta <[email protected]>

* refactor: infra client CreateOrUpdate to ServerSideApply (#3134)

* refactor(infra-client): CreateOrUpdate to ServerSideApply

Signed-off-by: Ardika Bagus <[email protected]>

* test(infra-client): add e2e test for ServerSideApply

Signed-off-by: Ardika Bagus <[email protected]>

* chore: remove comment

Signed-off-by: Ardika Bagus <[email protected]>

* chore: fix linter

Signed-off-by: Ardika Bagus <[email protected]>

---------

Signed-off-by: Ardika Bagus <[email protected]>
(cherry picked from commit 81108f2)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: duplicated xroutes are added to gatewayapi.Resources (#3282)

fix duplicated xroutes

Signed-off-by: Dingkang Li <[email protected]>
(cherry picked from commit 32c6876)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: add proxy protocol always as first listenerFilter (#3332)

* add proxy protocol always as first listenerFilter

Signed-off-by: Jesse Haka <[email protected]>

* add test

Signed-off-by: Jesse Haka <[email protected]>

---------

Signed-off-by: Jesse Haka <[email protected]>
(cherry picked from commit 6d8f2dc)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: security policy reference grant from field type (#3386)

fix: security policy reference grant from field

Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
(cherry picked from commit bd72474)
Signed-off-by: Arko Dasgupta <[email protected]>

* 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 <[email protected]>

* Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to
be clearer about what it has.

Signed-off-by: Lior Okman <[email protected]>

* Also renamed the helper function.

Signed-off-by: Lior Okman <[email protected]>

* Moved to the 'utils' package to be beside NamespacedName.

Signed-off-by: Lior Okman <[email protected]>

* Renamed structure according to review, and updated the comments

Signed-off-by: Lior Okman <[email protected]>

---------

Signed-off-by: Lior Okman <[email protected]>
(cherry picked from commit 95e2e35)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix(translator): set ignoreCase for header matchers in extAuth (#3420)

fix: set ignoreCase for header matchers in extAuth

Signed-off-by: haoqixu <[email protected]>
(cherry picked from commit 8206e11)
Signed-off-by: Arko Dasgupta <[email protected]>

* 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 <[email protected]>

* Update controller.go

Signed-off-by: Alex Volchok <[email protected]>

* Update controller.go

Signed-off-by: Alex Volchok <[email protected]>

---------

Signed-off-by: Alex Volchok <[email protected]>
(cherry picked from commit ff2c598)
Signed-off-by: Arko Dasgupta <[email protected]>

* feat: backend TLS SAN validation (#3507)

* BTLS: enforce SAN validation

Signed-off-by: Guy Daich <[email protected]>

* use dedicated cert for ext-proc e2e test

Signed-off-by: Guy Daich <[email protected]>

* fix ext-proc server client tls settings

Signed-off-by: Guy Daich <[email protected]>

---------

Signed-off-by: Guy Daich <[email protected]>
(cherry picked from commit dc201ba)
Signed-off-by: Arko Dasgupta <[email protected]>

* fix: ReplaceFullPath not working for root path (/) (#3530)

* fix: ReplaceFullPath not working for root path (/)

Takes #2817 forward

Signed-off-by: Arko Dasgupta <[email protected]>
(cherry picked from commit 8f83c3d)
Signed-off-by: Arko Dasgupta <[email protected]>

* 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 <[email protected]>

* fix test

rm invalid diff related to #3134

Signed-off-by: Arko Dasgupta <[email protected]>

* fix GatewayInfraResourceTest

Signed-off-by: Arko Dasgupta <[email protected]>

---------

Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Lior Okman <[email protected]>
Signed-off-by: Ardika Bagus <[email protected]>
Signed-off-by: Dingkang Li <[email protected]>
Signed-off-by: Jesse Haka <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: haoqixu <[email protected]>
Signed-off-by: Alex Volchok <[email protected]>
Signed-off-by: Guy Daich <[email protected]>
Co-authored-by: Lior Okman <[email protected]>
Co-authored-by: Ardika <[email protected]>
Co-authored-by: Dingkang Li <[email protected]>
Co-authored-by: Jesse Haka <[email protected]>
Co-authored-by: Eguzki Astiz Lezaun <[email protected]>
Co-authored-by: xu0o0 <[email protected]>
Co-authored-by: Alex Volchok <[email protected]>
Co-authored-by: Guy Daich <[email protected]>
Co-authored-by: zou rui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shawnh2 shawnh2 shawnh2 approved these changes

@guydc guydc guydc left review comments

+1 more reviewer

@Alice-Lilith Alice-Lilith Alice-Lilith approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cherrypick/release-v1.0.2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@liorokman @guydc @Alice-Lilith @shawnh2 @arkodg