-
Notifications
You must be signed in to change notification settings - Fork 360
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
api: support failOpen in ext auth #2908
Conversation
Signed-off-by: Dennis Zhou <idennis.zhou@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2908 +/- ##
==========================================
+ Coverage 64.05% 64.45% +0.39%
==========================================
Files 124 122 -2
Lines 20931 21037 +106
==========================================
+ Hits 13407 13559 +152
+ Misses 6671 6630 -41
+ Partials 853 848 -5 ☔ View full report in Codecov by Sentry. |
/retest |
api/v1alpha1/ext_auth_types.go
Outdated
// | ||
// +optional | ||
// +kubebuilder:default=true | ||
FailClosed *bool `json:"failClosed,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this is less intuitive
prefer failOpen
, @envoyproxy/gateway-maintainers @envoyproxy/gateway-reviewers can you share your preferences ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, failOpen
feels better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Dennis Zhou <idennis.zhou@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks!
quick question, (I am just starting to get more familiar with the code base, sorry if I am not accurate) here https://github.com/envoyproxy/gateway/blob/main/internal/xds/translator/extauth.go#L98 it is set explicitly to false, should this be changed as well? |
@piotrmsc this PR only handles the API change, a follow up PR will implement it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks !
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
failOpen
field for ext auth, set its default value to falseFixes #2897