refactor: simplify filter patching with an unified interface #2210
Conversation
zhaohuabing
force-pushed
the
refactor-http-filters
branch
2 times, most recently
from
571b9f2
to
463d612
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2210 +/- ##
==========================================
+ Coverage 64.37% 64.43% +0.05%
==========================================
Files 110 110
Lines 15343 15377 +34
==========================================
+ Hits 9877 9908 +31
- Misses 4854 4856 +2
- Partials 612 613 +1 ☔ View full report in Codecov by Sentry. |
zhaohuabing
force-pushed
the
refactor-http-filters
branch
8 times, most recently
from
e461f08
to
b5d1a31
Compare
zhaohuabing
force-pushed
the
refactor-http-filters
branch
3 times, most recently
from
bf6d580
to
3b29d7e
Compare
zhaohuabing
force-pushed
the
refactor-http-filters
branch
from
3b29d7e
to
9a25f2f
Compare
| @@ -40,6 +40,28 @@ | |||
| outlierDetection: {} | |||
| perConnectionBufferLimitBytes: 32768 | |||
| type: EDS | |||
| - commonLbConfig: | |||
There was a problem hiding this comment.
why did this config change ?
There was a problem hiding this comment.
Nothing really changed, just the location of ratelimit_cluster moved down in its parent config. It's because of this line:
// Add all the other needed resources referenced by this filter to the
// resource version table.
if err := patchResources(tCtx, httpListener.Routes); err != nil {
return err
}
// RateLimit filter is handled separately because it relies on the global
// rate limit server configuration.
// Check if a ratelimit cluster exists, if not, add it, if it's needed.
if err := t.createRateLimitServiceCluster(tCtx, httpListener); err != nil {
errs = multierror.Append(errs, err)
}
internal/xds/translator/oidc.go
Outdated
| @@ -262,15 +278,15 @@ func createOAuth2Secrets(tCtx *types.ResourceVersionTable, routes []*ir.HTTPRout | |||
| } | |||
|
|
|||
| clientSecret := buildOAuth2ClientSecret(route) | |||
| if err := tCtx.AddXdsResource(resourcev3.SecretType, clientSecret); err != nil { | |||
| if err := addXdsSecretIfNotExist(tCtx, clientSecret); err != nil { | |||
There was a problem hiding this comment.
what happens when 2 different policies (attached to 2 different routes) access the same secret ?
There was a problem hiding this comment.
That might happen, but a separate secret has been created for each route, so they won't be shared across routes. I will add some comments so we (including me) won't get confused by the codes in the future.
| ) | ||
|
|
||
| var httpFilters = [...]httpFilter{&cors{}, &jwt{}, &oidc{}} |
There was a problem hiding this comment.
is this needed ? someone will always forget to add this
There was a problem hiding this comment.
The tests will never pass if someone forget this, but it will be better to just modify one file. I replaced this with a register method in each HTTPFilter.
| // The filter types that haven't native per-route support: oauth2, basic authn | ||
| // Note: The filter types that have native per-route configuration support should | ||
| // always se their own native per-route configuration. | ||
| type httpFilter interface { |
There was a problem hiding this comment.
❤️ nicely done, thanks for all the comments
Signed-off-by: huabing zhao <zhaohuabing@gmail.com> fix: check if secret exist before creat it Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
zhaohuabing
force-pushed
the
refactor-http-filters
branch
from
149b389
to
83ec36b
Compare
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
zhaohuabing
force-pushed
the
refactor-http-filters
branch
from
83ec36b
to
bfe21a1
Compare
|
/retest |
Why: as we continue to add more features such as CORS, JWT, OIDC, and other to XDS translator, the current HTTP filter patching code is growing increasingly complex and becoming harder to maintain.
This PR aims to unify the HTTP filter patching logics into an
httpFilterinterface to make them more modular and the code cleaner.Related: #882