feat: add ability to configure cors in the config #59
Conversation
internal/cmd/config/cmd.go
Outdated
| type CorsConfig struct { | ||
| AllowedOrigins []string `yaml:"allowed-origins"` | ||
| AllowOriginFunc func(origin string) bool `yaml:"allow-origin-func"` | ||
| AllowOriginRequestFunc func(r *http.Request, origin string) bool `yaml:"allow-origin-request-func"` |
There was a problem hiding this comment.
April 2021 - Software engineer Namit invented new revolutionary way to deserialize functions from json
There was a problem hiding this comment.
April 2021 - Software engineer Namit invented new revolutionary way to deserialize functions from json
Lol, I saw this coming 😂
The thing is that this PR is still Work in Progress and there are a couple of more changes I wanted to make in this PR before requesting for a review.
Since it was bedtime for me back then, I left those changes for today (sorry, forgot to mention this in the PR description).
The first change that I had planned for today was to fix the function extraction part from yaml (which is what you have pointed out too) and the second was to change the default values of some of the fields, like currently the AllowedOrigins field takes a default value of [ ] instead of ["*"], which needs to be changed.
Will push a commit that fixes both of these in some time.
internal/cmd/config/cmd.go
Outdated
| type Config struct { | ||
| Server *httptest.Server `yaml:"-"` | ||
| Gateway *gateway.Gateway `yaml:"-"` | ||
| Listen string `yaml:"listen"` | ||
| gateway.Config `yaml:"-,inline"` | ||
| CorsConfig `yaml:"cors"` |
There was a problem hiding this comment.
Maybe we can put this into some better structure - cors is usually part of http gateway config
There was a problem hiding this comment.
Resolved in commit: 801544f
| c := cors.New(cors.Options{ | ||
| AllowedOrigins: []string{"*"}, | ||
| }) | ||
| c := cors.New((cors.Options)(config.CorsConfig)) |
There was a problem hiding this comment.
This will crash when config is missing or not properly deserialized
| @@ -191,9 +191,7 @@ func watchFile(filename string, onChange func(in fsnotify.Event)) (*fsnotify.Wat | |||
|
|
|||
| func startServer(config *config.Config) error { | |||
| postProcess(config) | |||
| c := cors.New(cors.Options{ | |||
| AllowedOrigins: []string{"*"}, | |||
There was a problem hiding this comment.
Missing defaults that were applied here
There was a problem hiding this comment.
The New function of the rs/cors package handles all the default values for us.
So even if a field of config.CorsConfig has not been assigned any value in the yaml file, the New function will consider the default value for it (without the need of us explicitly assigning it) and then return the cors handler.
Please have a look at this for reference: https://github.com/rs/cors/blob/f9bce55a4e61e3d1a061993e3453eb9848fcdc4d/cors.go#L122-L127
internal/cmd/config/cmd.go
Outdated
| MaxAge int `yaml:"max-age"` | ||
| AllowCredentials bool `yaml:"allow-credentials"` | ||
| OptionsPassthrough bool `yaml:"options-passthrough"` | ||
| Debug bool `yaml:"debug"` |
There was a problem hiding this comment.
I'm not sure if all of those options are needed.
We should only expose ones we want
There was a problem hiding this comment.
Resolved in commit: f8eb6d5
internal/cmd/config/cmd.go
Outdated
| AllowOriginRequestFunc func(r *http.Request, origin string) bool `yaml:"allow-origin-request-func"` | ||
| AllowedMethods []string `yaml:"allowed-methods"` | ||
| AllowedHeaders []string `yaml:"allowed-headers"` | ||
| ExposedHeaders []string `yaml:"exposed-headers"` |
There was a problem hiding this comment.
Each option should be documented as part of the config file
|
Is this WIP? Looks like some untested experiment. We are missing couple things like:
|
@wtrocki Yes, this is WIP (sorry for not mentioning this in the PR description). Please read this comment of mine: #59 (comment) Please give me some time, I'll make all the changes that I had planned and also look into the review comments that you have added. |
|
No rush. Sorry for premature review |
Fixes #58
Through this PR, I have tried to add the functionality that enables CORS to be configured through the
graphql-link.yamlfile.Todo (as suggested in #59 (comment)):