Skip to content

Conversation

slayer321
Copy link
Contributor

@slayer321 slayer321 commented Jul 30, 2025

What type of PR is this?

feat: add cacheDuration and asyncFetch support for remoteJWKS in SecurityPolicy

What this PR does / why we need it:
Adds support for two new field under remoteJWKS i.e cacheDuration and asyncFetch

Which issue(s) this PR fixes:

Fixes #6520

Release Notes: Yes

@slayer321 slayer321 requested a review from a team as a code owner July 30, 2025 10:27
Copy link

codecov bot commented Jul 30, 2025

Codecov Report

❌ Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.03%. Comparing base (5c0df92) to head (d8d4847).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
internal/gatewayapi/securitypolicy.go 80.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6641      +/-   ##
==========================================
+ Coverage   70.97%   71.03%   +0.06%     
==========================================
  Files         227      227              
  Lines       40461    40470       +9     
==========================================
+ Hits        28716    28747      +31     
+ Misses      10039    10022      -17     
+ Partials     1706     1701       -5     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@slayer321 slayer321 force-pushed the adds-new-field-to-remotejwks-securitypolicy branch from d2ae94e to 76240ec Compare July 30, 2025 11:50
// +kubebuilder:default="300s"
// +kubebuilder:validation:Format=duration
// +optional
CacheDuration *metav1.Duration `json:"cacheDuration,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

lets use gwapiv1.Duration here


// Fetch Jwks asynchronously in the main thread before the listener is activated. Fetched Jwks can be used by all worker threads.
// +optional
AsyncFetch *JwksAsyncFetch `json:"asyncFetch,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rm AsyncFetch from this API PR, since its already enabled by default
prefer if we address retry semantics as part of #6525
this should help make this PR move faster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so if I understand it correctly, in this PR, we just want to add the CacheDuration field and not the AsyncFetch field right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah thanks

@slayer321 slayer321 force-pushed the adds-new-field-to-remotejwks-securitypolicy branch from 76240ec to 7804d1f Compare August 17, 2025 15:20
@slayer321
Copy link
Contributor Author

slayer321 commented Aug 18, 2025

Hey @arkodg , I see that conformance-test is failing for v1.32.5 . I tried runing it locally and it passed.

Edit: I see it been passed now

@arkodg arkodg changed the title feat: add cacheDuration and asyncFetch support for remoteJWKS in SecurityPolicy feat: add cacheDuration for remoteJWKS in SecurityPolicy Sep 15, 2025
// JwksAsyncFetch is used to Fetch Jwks asynchronously in the main thread before the listener is activated.
//
// +k8s:deepcopy-gen=true
type JwksAsyncFetch struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rm this from this PR


var duration *gwapiv1.Duration
if jwks.CacheDuration != nil {
duration = jwks.CacheDuration
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are relying on a ptr value, preference here would be to wrap it around

if jwks.CacheDuration != nil {

cur, err := time.ParseDuration(string(*jwks.CacheDuration))
				if err != nil {
					return nil, err
				}
remote.CacheDuration = cDur
}

// Duration after which the cached JWKS should be expired. If not specified, default cache duration is 5 minutes.

// +kubebuilder:default="300s"
// +kubebuilder:validation:Format=duration
Copy link
Contributor

Choose a reason for hiding this comment

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

lets rm // +kubebuilder:validation:Format=duration, we are relying on CEL validations of gwapiv1.Duration

@slayer321 slayer321 force-pushed the adds-new-field-to-remotejwks-securitypolicy branch 2 times, most recently from f594d42 to 98ce496 Compare September 18, 2025 08:47
@arkodg arkodg added this to the v1.6.0-rc.1 Release milestone Sep 19, 2025
@arkodg
Copy link
Contributor

arkodg commented Sep 19, 2025

thanks @slayer321 can you fix make gen, by running make generate && make manifests and committing those changes

@arkodg
Copy link
Contributor

arkodg commented Sep 23, 2025

can you fix the CI errors @slayer321 ?

@slayer321 slayer321 force-pushed the adds-new-field-to-remotejwks-securitypolicy branch from 94fae34 to 80d9bda Compare September 24, 2025 04:14
URI string `json:"uri"`

// Duration after which the cached JWKS should be expired. If not specified, default cache duration is 5 minutes.
CacheDuration *gwapiv1.Duration `json:"cacheDuration,omitempty"`
Copy link
Contributor

@arkodg arkodg Sep 24, 2025

Choose a reason for hiding this comment

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

can you follow similar time fields in this file, which use metav1.Duration here to simplify test comparison

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eariler metav1.Duration was used but as mentioned here I updated it to gwapiv1.Duration

Copy link
Contributor

Choose a reason for hiding this comment

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

Meta is under in the IR

@slayer321 slayer321 force-pushed the adds-new-field-to-remotejwks-securitypolicy branch from 36e15c7 to c800b8c Compare September 24, 2025 13:12
@slayer321
Copy link
Contributor Author

Hey @arkodg , I see two of the e2e tests are failing .. I check the logs and can see in v1.33.1, ipv4 test TestE2E/RateLimitHeaderMatch/all_matched_headers_can_got_limited is failing and in v1.32.5, ipv6 test TestE2E/RateLimitGlobalMergeTest/shared_no_client_selectors is failing .. I checked there are no changes releated to this in this PR. Please let me know if I'm missing something.

arkodg
arkodg previously approved these changes Sep 24, 2025
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@arkodg arkodg requested review from a team September 24, 2025 22:54
zirain
zirain previously approved these changes Sep 25, 2025
Copy link
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

can you add a release notes?

@slayer321 slayer321 dismissed stale reviews from zirain and arkodg via 6c27f59 September 25, 2025 04:15
@slayer321 slayer321 force-pushed the adds-new-field-to-remotejwks-securitypolicy branch from 6c27f59 to ab1467d Compare September 25, 2025 04:16
@zirain zirain force-pushed the adds-new-field-to-remotejwks-securitypolicy branch from ca3f501 to d8d4847 Compare September 27, 2025 00:15
@arkodg arkodg enabled auto-merge (squash) September 27, 2025 01:13
@arkodg arkodg disabled auto-merge September 27, 2025 16:16
@arkodg arkodg merged commit 276ba9f into envoyproxy:main Sep 27, 2025
64 of 69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add cacheDuration and asyncFetch support for remoteJWKS in SecurityPolicy
3 participants