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

Make cors middleware optional + tests #2285

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Make cors middleware optional + tests #2285

merged 2 commits into from
Oct 19, 2020

Conversation

yaron2
Copy link
Member

@yaron2 yaron2 commented Oct 19, 2020

This PR does the following:

  1. Adds unit tests for the CORS middleware
  2. Does not add the fasthttp handler middleware unless needed. Profiling has shown this code consumes a significant amount of memory under load (~1MB).

Release Note

RELEASE NOTE: Changed Cors middleware to optional + added tests

@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #2285 into master will increase coverage by 0.14%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2285      +/-   ##
==========================================
+ Coverage   48.08%   48.23%   +0.14%     
==========================================
  Files          70       70              
  Lines        6143     6145       +2     
==========================================
+ Hits         2954     2964      +10     
+ Misses       2931     2923       -8     
  Partials      258      258              
Impacted Files Coverage Δ
pkg/runtime/cli.go 0.00% <0.00%> (ø)
pkg/runtime/config.go 100.00% <ø> (ø)
pkg/http/server.go 31.57% <100.00%> (+17.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 985a5bf...eeccb95. Read the comment docs.

@youngbupark
Copy link

is there a reason why we make cors middleware ? is this for testing?

@yaron2
Copy link
Member Author

yaron2 commented Oct 19, 2020

is there a reason why we make cors middleware ? is this for testing?

It's a feature we had from the very beginning, presumably to make sure that the Dapr API can only be accessed from certain origins. (helpful in the case where you expose the Dapr API publicly).

@youngbupark
Copy link

is there a reason why we make cors middleware ? is this for testing?

It's a feature we had from the very beginning, presumably to make sure that the Dapr API can only be accessed from certain origins. (helpful in the case where you expose the Dapr API publicly).

From the code I am reading, we only control Access-Control-Allow-Origin header for CORS set by cmd flag. no knobs for the other CORS headers such as Access-Control-Allow-Methods.

New cors package includes only one constant so I was wonder if we need this change.

Also do we plan to support the other CORS headers ?

@yaron2
Copy link
Member Author

yaron2 commented Oct 19, 2020

is there a reason why we make cors middleware ? is this for testing?

It's a feature we had from the very beginning, presumably to make sure that the Dapr API can only be accessed from certain origins. (helpful in the case where you expose the Dapr API publicly).

From the code I am reading, we only control Access-Control-Allow-Origin header for CORS set by cmd flag. no knobs for the other CORS headers such as Access-Control-Allow-Methods.

New cors package includes only one constant so I was wonder if we need this change.

Also do we plan to support the other CORS headers ?

The change just refactored the location of the const, didn't add a new one.

Also do we plan to support the other CORS headers ?

I don't know, maybe. we need to probably make CORS more visible to users and get feedback.

@yaron2 yaron2 merged commit 4164950 into dapr:master Oct 19, 2020
@yaron2 yaron2 deleted the corsoptional-1 branch October 19, 2020 19:10
@yaron2 yaron2 self-assigned this Oct 20, 2020
@yaron2 yaron2 added this to In progress in 1.0.0 Milestone 1 via automation Oct 20, 2020
@yaron2 yaron2 moved this from In progress to Review in 1.0.0 Milestone 1 Oct 20, 2020
@artursouza artursouza moved this from Review to Done in 1.0.0 Milestone 1 Oct 20, 2020
wcs1only pushed a commit to wcs1only/dapr that referenced this pull request Oct 22, 2020
@artursouza artursouza added this to Release in 1.0.0 Milestone Oct 27, 2020
vinayada1 pushed a commit to vinayada1/dapr that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants