chore: add test to ensure fatal state on forbidden error (#756)

[alexandraoberaigner]

This PR

• Adds a new "permission denied" response pathway for the Envoy proxy service
• and a corresponding gherkin test to ensure the client handles forbidden responses correctly

Related Issues

makes sure flagd providers fix this bug #756

Follow-up Tasks

• incorporate the test in #783 to test the bug fix

How to test

run the inprocess e2e tests of any flagd provider that uses the gherkin suite. make sure @forbidden is an enabled tag

[gemini-code-assist]

Summary of Changes

Hello @alexandraoberaigner, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the client's error handling by introducing a mechanism to simulate and test forbidden responses from the flagd provider. It configures the Envoy proxy to serve a 403 Forbidden error and adds an end-to-end Gherkin test to ensure that clients properly detect this state and transition to a fatal error, improving the client's resilience to authorization issues.

Highlights

• Envoy Configuration: A new permission-denied-listener has been added to the docker-compose.yaml for the Envoy proxy, configured to return a 403 Forbidden response on port 9212.
• Gherkin Test: A new Gherkin scenario, "Provider forbidden," has been introduced to verify that clients correctly enter a fatal state when encountering a forbidden response from the flagd provider.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively adds a test to ensure the client enters a fatal state on a 'permission denied' (403) error. This is accomplished by introducing a new Envoy proxy listener in docker-compose.yaml to simulate the error, and a new Gherkin scenario in connection.feature to test the client's reaction. The implementation is correct and focused. My only feedback is a suggestion to improve the maintainability of the Envoy configuration by extracting it to a separate file, as the current inline approach is becoming unwieldy.

[toddbaert]

Looks like this is the only new gherkin clause?

[alexandraoberaigner]

yes it is

[aepfli]

Does the client have a state? Or do we throw an fatal error?

[aepfli]

Something like "and the error should be fatal" might be better suited, because we are specifically requesting an error, and the connection is with the error and not specifically with the client. Or am I wrong here? For flagd it is important that we throw a fatal event. What the SDK does is not as important for us. At least in this testbed

[toddbaert]

I think either way we are testing something on the client, but I agree that if we can do this with fewer new steps that's better. Do we have a step that we can use to check the error in the error handler and verify it's FATAL?

If so I would support that change for sure!

[alexandraoberaigner]

I'm checking the client.State() in the implementation. (See code line)
The event doesn't have the fatal error code, its just a normal error event thats produced. However, the state of the client is in fatal.
Do you think I need to adapt the event mocking logic to include also the fatal error code? or add a handler for fatal?

[aepfli]

I am not sure if the ErrorHandler should know about if it is an error state or fatal state, maybe also something worth defining. But most importantly we should normalize the naming, currently every then-step uses the phrasing should be and we should stay consistent here.

[toddbaert]

Approved with one nit.