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

Adds BFF support for CORS #710

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

alexcreasy
Copy link
Contributor

@alexcreasy alexcreasy commented Jan 17, 2025

NOTE: This PR is stacked on top of my logging PR, that will need to be merged first.

Description

Adds W3C standards compliant support for CORS for the BFF

How Has This Been Tested?

This can be tested by running the BFF in full mock mode as the headers are generated by a middleware that will run in all scenarios. The tests involve merely querying the health check endpoint with a different combination of headers.

This to note:

  • CORS is only enforced by browsers, so all requests should be successful, the headers returned will vary and the browser will either accept or deny the request based on this.
  • When the frontend is served by the BFF, CORS will not be required as it will be served from the same origin - as such the default behaviour is to not return any CORS headers, this disallows cross origin requests and is the most secure.

Scenario 1: Testing with CORS disabled
Start the server:

 make run PORT=4000 MOCK_K8S_CLIENT=true MOCK_MR_CLIENT=true

Run a curl against the health check endpoint:

curl -i -H "kubeflow-userid: [email protected]" "localhost:4000/api/v1/healthcheck"

Expected outcome: The endpoint should function as normal and there should be no CORS headers in the response.

Scenario 2: Test with a single allowed origin
Start the server:

 make run PORT=4000 MOCK_K8S_CLIENT=true MOCK_MR_CLIENT=true ALLOWED_ORIGINS="http://example.com"

2.1 Make a request with an allowed origin header

curl -i -H "Origin: http://example.com" -H "kubeflow-userid: [email protected]" "localhost:4000/api/v1/healthcheck"

Expected outcome: Successful request with the following CORS specific headers:

Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: http://example.com
Vary: Origin

2.2 Make a request with a disallowed origin header

curl -i -H "Origin: http://shady-as-duck.com" -H "kubeflow-userid: [email protected]" "localhost:4000/api/v1/healthcheck"

Expected outcome: The request should be successful, but the only CORS header in the response should be:

Vary: Origin

If there are any other Access-Control-* headers present the test has failed!

2.3 Simulate a CORS pre-flight request for an allowed origin:

curl -i -X OPTIONS -H "Origin: http://example.com" -H "Access-Control-Request-Method: POST" -H "Access-Control-Request-Headers: kubeflow-userid" "localhost:4000/api/v1/healthcheck"

Expected outcome: A response like below - note the CORS headers.

HTTP/1.1 204 No Content
Access-Control-Allow-Credentials: true
Access-Control-Allow-Headers: kubeflow-userid
Access-Control-Allow-Methods: POST
Access-Control-Allow-Origin: http://example.com
Vary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers
Date: Fri, 17 Jan 2025 18:12:13 GMT

2.4 Simulate a CORS pre-flight request for a disallowed origin

curl -i -X OPTIONS -H "Origin: http://shady-as-duck.com" -H "Access-Control-Request-Method: POST" -H "Access-Control-Request-Headers: kubeflow-userid" "localhost:4000/api/v1/healthcheck"

Expected outcome: A response like below - note the lack of CORS headers aside from the Vary header

HTTP/1.1 204 No Content
Vary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers
Date: Fri, 17 Jan 2025 18:15:10 GMT

Additional tests:
You can repeat the above tests using an allow list of multiple origins, simply add comma separated ones to the make command e.g.

 make run PORT=4000 MOCK_K8S_CLIENT=true MOCK_MR_CLIENT=true ALLOWED_ORIGINS="http://example1.com,http://example2.com"

NOTE: Only a single origin is allowed in the Access-Control-Allow-Origin header, so the BFF will return only a single origin if the Origin request header matches the allow list.

Also you can test that allowing all origins (wildcard) works as expected:

 make run PORT=4000 MOCK_K8S_CLIENT=true MOCK_MR_CLIENT=true ALLOWED_ORIGINS="*"

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.
  • Code changes follow the kubeflow contribution guidelines.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

…dds debug logging statements for upstream requests

Signed-off-by: Alex Creasy <[email protected]>
Signed-off-by: Alex Creasy <[email protected]>
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from alexcreasy. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Alex Creasy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant