Skip to content
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

feat: remove AuthenticationFilter in favor of SecurityPolicy #2086

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Oct 27, 2023

This PR removes AuthenticationFilter. The existing JWT authentication in AuthenticationFilter has been replaced by SecurityPolicy. The other planned authn features (OIDC, Basic Auth, etc.) will be incorporated into `SecurityPolicy as well in the future.

Related: #2079
Fix: #2082

@zhaohuabing zhaohuabing requested a review from a team as a code owner October 27, 2023 02:57
@zhaohuabing zhaohuabing marked this pull request as draft October 27, 2023 02:57
@zhaohuabing
Copy link
Member Author

The code is ready for review while I'm working on the docs.

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #2086 (9ea96a9) into main (845e874) will decrease coverage by 0.20%.
The diff coverage is 37.00%.

@@            Coverage Diff             @@
##             main    #2086      +/-   ##
==========================================
- Coverage   64.41%   64.22%   -0.20%     
==========================================
  Files         109      107       -2     
  Lines       15094    14626     -468     
==========================================
- Hits         9723     9393     -330     
+ Misses       4768     4666     -102     
+ Partials      603      567      -36     
Files Coverage Δ
internal/gatewayapi/filters.go 80.46% <ø> (-0.73%) ⬇️
internal/gatewayapi/helpers.go 90.55% <ø> (+3.51%) ⬆️
internal/gatewayapi/resource.go 67.27% <ø> (+1.20%) ⬆️
internal/gatewayapi/route.go 88.28% <100.00%> (-0.03%) ⬇️
internal/gatewayapi/zz_generated.deepcopy.go 0.00% <ø> (ø)
internal/ir/xds.go 74.37% <ø> (+2.98%) ⬆️
internal/ir/zz_generated.deepcopy.go 12.14% <ø> (+0.41%) ⬆️
internal/provider/kubernetes/filters.go 32.00% <ø> (-14.67%) ⬇️
internal/provider/kubernetes/predicates.go 54.94% <ø> (+1.37%) ⬆️
internal/xds/translator/httpfilters.go 78.26% <ø> (+5.89%) ⬆️
... and 4 more

... and 1 file with indirect coverage changes

@zirain
Copy link
Member

zirain commented Oct 27, 2023

kindly reminder: you need rebase once #2081 merged.

@arkodg
Copy link
Contributor

arkodg commented Oct 27, 2023

@zhaohuabing you will also need to delete and recreate the crds in https://github.com/envoyproxy/gateway/tree/main/charts/gateway-helm since our tooling does not clean, only adds

@zhaohuabing zhaohuabing marked this pull request as ready for review October 27, 2023 07:04
zirain
zirain previously approved these changes Oct 27, 2023
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.

LGTM

@Xunzhuo
Copy link
Member

Xunzhuo commented Oct 27, 2023

@zhaohuabing you will also need to delete and recreate the crds in https://github.com/envoyproxy/gateway/tree/main/charts/gateway-helm since our tooling does not clean, only adds

How about raising an issue for it ? @arkodg

Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

Well done, some docs lint errors need to be resolved.

@zhaohuabing
Copy link
Member Author

zhaohuabing commented Oct 27, 2023

This PR only adds docs for JWT, CORS docs will be taken care of in a follow-up PR.

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

fix e2e

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

fix test and add policies to egctl translate command

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

add docs for jwt

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

remove crd

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

clear docs

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

address comments

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

update security policy status

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

fix e2e test

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing
Copy link
Member Author

@zirain @Xunzhuo @arkodg please hold other PRs until this one get merged. It's a bit hard to rebase such a huge PR.

@zhaohuabing zhaohuabing added area/api API-related issues release-note Indicates a required release note and removed area/api API-related issues labels Oct 27, 2023
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 merged commit 364710f into envoyproxy:main Oct 27, 2023
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Indicates a required release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cors policy is not automatically cleared when deleting CRD Security Policy
4 participants