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

Return 500 when xPolicy translation fails #3873

Open
2 of 4 tasks
zhaohuabing opened this issue Jul 17, 2024 · 10 comments
Open
2 of 4 tasks

Return 500 when xPolicy translation fails #3873

zhaohuabing opened this issue Jul 17, 2024 · 10 comments
Milestone

Comments

@zhaohuabing
Copy link
Member

zhaohuabing commented Jul 17, 2024

Description:

Describe the issue.

The current behaviors when xPolicies translation fails:

Options that we have:

For SecurityPolicy, it's reasonable to default to fail close as fail open poses a security risk. A configuration knob can be added to each SecurityPolicy, allowing users to customize this behavior if needed.

For BackendTrafficPolicy and EnvoyExtensionPolicy, there are two strategies:

  • default to fail close: try the best to serve client traffic since service can still be functional to the user ends when these policies fail
  • default to fail open: this aligns with the SecurityPolicy approach, maintaining consistency across policy behaviors.

We may also need to decide the default behavior for the ClientTrafficPolicy and EnvoyPatchPolicy failure, should we fail the targeted Gateways/Listeners?

[optional Relevant Links:]

Any extra documentation required to understand the issue.

@zhaohuabing zhaohuabing added triage kind/decision A record of a decision made by the community. area/policy and removed triage labels Jul 17, 2024
@arkodg
Copy link
Contributor

arkodg commented Jul 17, 2024

im a +1 to return 500 if a CTP, BTP or EETP policy tied to a route cannot be translated . This would match the behavior of filters https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.HTTPRouteFilter

If a reference to a custom filter type cannot be resolved, the filter MUST NOT be skipped. Instead, requests that would have been processed by that filter MUST receive a HTTP error response.

@zirain
Copy link
Contributor

zirain commented Jul 17, 2024

+1 on this

@Xunzhuo
Copy link
Member

Xunzhuo commented Jul 17, 2024

+1

@arkodg arkodg added the help wanted Extra attention is needed label Aug 8, 2024
@arkodg arkodg changed the title decision: the default behaviors when xPolicy translation fails Return 500 when xPolicy translation fails Aug 8, 2024
@arkodg arkodg added this to the v1.2.0-rc1 milestone Aug 8, 2024
@arkodg arkodg removed the kind/decision A record of a decision made by the community. label Aug 8, 2024
@arkodg arkodg removed the help wanted Extra attention is needed label Aug 16, 2024
@guydc
Copy link
Contributor

guydc commented Aug 19, 2024

+1

@guydc
Copy link
Contributor

guydc commented Aug 20, 2024

Should we also consider BackendTLSPolicy here? I suppose that if a backend requires TLS and we fail to translate BTLSP, the upstream connection would just fail.

Maybe if/when gateway-level backend TLS configuration is supported, we will see a behavior of fallback to wrong gateway-level defaults that reduce security (e.g. use a system trust store instead of a specific trusted CA).

@alexwo alexwo removed their assignment Aug 27, 2024
@logan-hcg
Copy link

While not directly related, I think Extension Server should have a similar "fail close" option.

I'm using the Extension Server to automatically add a default Authz filter to all Listeners. If the Extension Server fails in some way (isn't available, fails during the processing), then the current behavior means that the Listener will be active without the filter (and without the security properties). With an option to "block" the Listener if the Extension Server fails, than the security boundary provided would be maintained.

@alexwo
Copy link
Contributor

alexwo commented Sep 4, 2024

While not directly related, I think Extension Server should have a similar "fail close" option.

I'm using the Extension Server to automatically add a default Authz filter to all Listeners. If the Extension Server fails in some way (isn't available, fails during the processing), then the current behavior means that the Listener will be active without the filter (and without the security properties). With an option to "block" the Listener if the Extension Server fails, than the security boundary provided would be maintained.

Hi @logan-hcg ,
There is a failOpen knob in the EnvoyExtensionPolicySpec, does it address this concern?
https://gateway.envoyproxy.io/docs/api/extension_types/#envoyextensionpolicyspec

@logan-hcg
Copy link

Hi @logan-hcg , There is a failOpen knob in the EnvoyExtensionPolicySpec, does it address this concern? https://gateway.envoyproxy.io/docs/api/extension_types/#envoyextensionpolicyspec

hi @alexwo , unfortunately that control knob is for Extension Polices, not Extension Manager / Extension Server (seems to be referred to interchangeably)

@liorokman
Copy link
Contributor

While not directly related, I think Extension Server should have a similar "fail close" option.

I'm using the Extension Server to automatically add a default Authz filter to all Listeners. If the Extension Server fails in some way (isn't available, fails during the processing), then the current behavior means that the Listener will be active without the filter (and without the security properties). With an option to "block" the Listener if the Extension Server fails, than the security boundary provided would be maintained.

@logan-hcg could you open a separate issue for the Extension Server

Copy link

github-actions bot commented Oct 5, 2024

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@github-actions github-actions bot added the stale label Oct 5, 2024
@arkodg arkodg modified the milestones: v1.2.0-rc1, Backlog Oct 18, 2024
@github-actions github-actions bot removed the stale label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants