feat: Introduce "default_application" config param #36
Conversation
|
I love the idea! I just started trying out the experimental branch and this was exactly the feature I wish I had. Another idea (maybe for a different PR) might be to allow the values in the default_application to act as the defaults for all the other applications if they don't specify a value. That way, you can define most all your config in the default_application and only put overrides in each real application section. |
internal/request.go
Outdated
| @@ -50,7 +50,15 @@ func (s *SPOA) processRequest(msg spoe.Message) ([]spoe.Action, error) { | |||
| var ok bool | |||
| app, ok = s.applications[arg.Value.(string)] | |||
| if !ok { | |||
| return nil, fmt.Errorf("application %q not found", arg.Value.(string)) | |||
| if len(s.defaultApplication) > 0 { | |||
| fmt.Printf("application %q not found, using default application %q\n", arg.Value.(string), s.defaultApplication) | |||
There was a problem hiding this comment.
I imagine this might be a very noisy log, especially for people using hdr(host) as the app name since every request would trigger this message. Maybe it could be app.logger.Debug instead?
There was a problem hiding this comment.
I second that, something like this after reassigning app var after the inner if stmt: app.logger.Debug("application not found, using default", zap.Any("application", arg.Value), zap.String("default", s.defaultApplication))
There was a problem hiding this comment.
Thank you @mac-chaffee for idea and @jcmoraisjr for solution. Updated message severity.
Edit: tagging right people.
|
Do you mind adding some tests? |
|
@jcchavezs, I'm sorry, I'm not experienced Go dev at all and will not be able to introduce testing in this project. Edit: add mention. |
| } | ||
| } else { | ||
| return nil, fmt.Errorf("application %q not found", arg.Value.(string)) | ||
| } |
There was a problem hiding this comment.
Better if using the parameter in the end of the sentence, without type casting, which follows the component patterns. E.g.:
return nil, fmt.Errorf("default application not found: %s", s.defaultApplication)
...
return nil, fmt.Errorf("application not found: %v", arg.Value)
There was a problem hiding this comment.
Thanks for suggestion. Updated error messages.
|
@jcchavezs Since there's no testing harness in this repo yet, could we defer the testing for another PR? I could try my hand at adding tests in a separate PR, but I might need some guidance on the kind of testing you prefer. I'm personally a fan of "Write tests. Not too many. Mostly integration." but I'd say whatever is best for you is best for the project |
|
I've started on a test here, stop me if I look like I'm going off the rails :) https://github.com/corazawaf/coraza-spoa/compare/experimental...mac-chaffee:coraza-spoa:testing?expand=1 |
|
@NullIsNot0 could you change this PR to be based off the main branch? The experimental branch got merged into main in #45 |
|
I'm closing this PR as #53 is successor of this one. |
I use
hdr(host)asappinspoe-messagefor more dynamic configuration. For every hostname I define an application incoraza-spoe, but I noticed, if i open my web by typing an IP address, the request is also processed by SPOA and it throws an error that "an application<MY_IP_ADDRESS>is not found". That's why I thought that it would be a great idea to define default application for requests that has no application defined. If somebody does not want a default application behaviour, then config parameterdefault_applicationcan be commented out.