Skip to content

fix secrets/configmap updates do not trigger a controller reconcile#3499

Merged
guydc merged 4 commits intoenvoyproxy:mainfrom
alexwo:ensure_secrets_trigger_reconciles
May 30, 2024
Merged

fix secrets/configmap updates do not trigger a controller reconcile#3499
guydc merged 4 commits intoenvoyproxy:mainfrom
alexwo:ensure_secrets_trigger_reconciles

Conversation

@alexwo
Copy link
Copy Markdown
Contributor

@alexwo alexwo commented May 29, 2024 •
edited
Loading

What this PR does / why we need it:
Ensure that update to secret and configmas will tigger a reconcile, currently updates are ignored.

Since secrets and configmaps don’t have a spec or generation field, the “TypedGenerationChangedPredicate” will prevent any reconciles from being triggered.

Which issue(s) this PR fixes:
Fixes # #3496

Affected resources that may use outdated secret / configmaps:
EG CRs: Gateway, SecurityPolicy, ClienTraffiPolicies and BackendTLSPolicies.

@alexwo
ensure both secrets and config map reconcile upon changes
a19da43 
ensure secret/config map changes trigger a reconcile

Signed-off-by: Alex Volchok <alex.volchok@sap.com>
@alexwo alexwo requested a review from a team as a code owner May 29, 2024 06:09
@alexwo alexwo changed the title Fix: secrets/config map updates do not trigger reconciles Fix: secrets/config map changes do not trigger reconciles May 29, 2024
alexwo added 2 commits May 29, 2024 08:21
@alexwo
Update controller.go
ca22865 
Signed-off-by: Alex Volchok <alex.volchok@sap.com>
@alexwo
Update controller.go
fdf6726 
Signed-off-by: Alex Volchok <alex.volchok@sap.com>
@codecov
Copy link
Copy Markdown

codecov bot commented May 29, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.52%. Comparing base (6f30397) to head (5707253).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3499      +/-   ##
==========================================
- Coverage   67.53%   67.52%   -0.02%     
==========================================
  Files         168      168              
  Lines       20154    20152       -2     
==========================================
- Hits        13612    13607       -5     
- Misses       5558     5560       +2     
- Partials      984      985       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexwo alexwo changed the title Fix: secrets/config map changes do not trigger reconciles Fix: secrets/config map updates do not trigger reconciles May 29, 2024
@alexwo alexwo changed the title Fix: secrets/config map updates do not trigger reconciles Fix: secrets/config map updates do not trigger a controller reconcile May 29, 2024
@alexwo
Copy link
Copy Markdown
Contributor Author

alexwo commented May 29, 2024

/retest

@alexwo alexwo changed the title Fix: secrets/config map updates do not trigger a controller reconcile Fix: secrets/config map updates do not trigger a controller reconcile May 29, 2024
@alexwo alexwo changed the title Fix: secrets/config map updates do not trigger a controller reconcile fix: secrets/config map updates do not trigger a controller reconcile May 29, 2024
@alexwo alexwo changed the title fix: secrets/config map updates do not trigger a controller reconcile fix secrets/config map updates do not trigger a controller reconcile May 29, 2024
@zirain
Copy link
Copy Markdown
Member

zirain commented May 29, 2024

gateway/internal/provider/kubernetes/controller.go

Line 1095 in 72c0cc7

predicate.GenerationChangedPredicate{},

same exists in v1.0.0?

@alexwo
Copy link
Copy Markdown
Contributor Author

alexwo commented May 29, 2024

/retest

@alexwo alexwo changed the title fix secrets/config map updates do not trigger a controller reconcile fix secrets/configmap updates do not trigger a controller reconcile May 29, 2024
liorokman
liorokman approved these changes May 29, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@liorokman liorokman left a comment

Choose a reason for hiding this comment

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

LGTM

@alexwo
Copy link
Copy Markdown
Contributor Author

alexwo commented May 29, 2024 •
edited
Loading

gateway/internal/provider/kubernetes/controller.go

Line 1095 in 72c0cc7

predicate.GenerationChangedPredicate{},

same exists in v1.0.0?

Yes, its about 7 month old.

@zirain
Copy link
Copy Markdown
Member

zirain commented May 29, 2024

Can you add a test?

@alexwo
Copy link
Copy Markdown
Contributor Author

alexwo commented May 29, 2024 •
edited
Loading

Can you add a test?

Do you think we should add a component test?

I have a larger change that will also ensure we can perform secret rotations for backend mTLS.
If this makes sense, we can combine this fix with my larger change, which will also include an end-to-end (e2e) test to cover secret updates.

Let me know if this makes sense or we keep this PR small?
(#3500)

@zirain
Copy link
Copy Markdown
Member

zirain commented May 29, 2024 •
edited
Loading

I'm talking something like TestValidateServiceForReconcile if possible.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented May 29, 2024

looking at https://pkg.go.dev/k8s.io/api/core/v1#Secret , it contains ObjectMeta which contains the Generation field
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#ObjectMeta , does k8s not bump this val when the secret changes ?

@alexwo
Copy link
Copy Markdown
Contributor Author

alexwo commented May 29, 2024

looking at https://pkg.go.dev/k8s.io/api/core/v1#Secret , it contains ObjectMeta which contains the Generation field https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#ObjectMeta , does k8s not bump this val when the secret changes ?

The generation field is typically used in resources with a spec section to track changes to the spec. Each time the spec of a resource changes, the generation is incremented by the Kubernetes API server.

For resources like Secrets and ConfigMaps, changes are made directly to the data, and these resources do not have a spec field or a generation field. Therefore, updates to Secrets and ConfigMaps do not automatically trigger reconciliations in controllers that rely on generation changes.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented May 29, 2024

thanks for the explaination @alexwo !

@guydc
Copy link
Copy Markdown
Contributor

guydc commented May 29, 2024

/retest

3 similar comments
@guydc
Copy link
Copy Markdown
Contributor

guydc commented May 30, 2024

/retest

@alexwo
Copy link
Copy Markdown
Contributor Author

alexwo commented May 30, 2024

/retest

@quyenhoang96
Copy link
Copy Markdown
Contributor

quyenhoang96 commented May 30, 2024

/retest

@quyenhoang96
Copy link
Copy Markdown
Contributor

quyenhoang96 commented May 30, 2024

Hi @arkodg. I'm happy that Envoy gateway supports extauth integration in a simple and fast way. I am using extauth feature for authentication. But it is not working with a service with a different namespace than the extauth service. When do you plan to release this cherrypick/release-v1.0.2 version?

@guydc guydc merged commit ff2c598 into envoyproxy:main May 30, 2024
@shawnh2 shawnh2 mentioned this pull request May 30, 2024
Closed
arkodg pushed a commit to arkodg/gateway that referenced this pull request Jun 12, 2024
@alexwo
fix secrets/configmap updates do not trigger a controller reconcile (e…
7bf1f48 
…nvoyproxy#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>
@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 <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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arkodg arkodg arkodg approved these changes

@guydc guydc guydc approved these changes

+1 more reviewer

@liorokman liorokman liorokman 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.

6 participants

@alexwo @zirain @arkodg @guydc @quyenhoang96 @liorokman