bouncestorage / chaos-http-proxy

Introduce failures into HTTP requests via a proxy server
Apache License 2.0
144 stars 12 forks source link

Extended configuration to allow json configuration via api #12

Closed castlec closed 8 years ago

castlec commented 8 years ago

Changed jetty handler to HandlerList and added a second handler to parse json configuration messages.

POST { "http_301":1, "http_302":1, "http_303":1, "http_307":1, "http_308":1, "http_408":1, "http_500":1, "http_503":1, "http_504":1, "change_header_case":1, "corrupt_request_content_md5":1, "corrupt_response_content_md5":1, "partial_data":1, "reorder_headers":1, "slow_response":1, "timeout":1, "success":83 }

gaul commented 8 years ago

This looks like a great feature! Rather than use JSON for the request, perhaps you could use the same format as the existing configuration, i.e., Java properties? Also could you revert all the style changes and squash the commits?

castlec commented 8 years ago

which style changes? I was only trying to make the code pass the checkstyle config (my first commit didn't have any style changes at all. I decided to be a good citizen.). I changed anything and everything required to do so with the exception of the line lengths. The code I pulled from master wasn't passing line length in places so I opted to increase the line length on the check.

I thought about using the property configuration as is but then thought it would be better to use a json configuration that non-java devs would be used to seeing. My basic thought process is that the only person that will see the property file is the system administrator. Initially, I was going to do query parameters but then realized it would be a nasty parameter set since, with the way the Supplier is structured, there's no way to do individual updates cleanly; it's all or nothing. I'm willing to do it. I agree that it would be nice to have the same configuration. I just don't think it's ideal for the usecase. It could easily be unified if desired since the only real differences are the com.bouncestorage.chaoshttpproxy. string and error handling (I opted to let it fail when a bad call is made).

gaul commented 8 years ago

The code on master passes all Checkstyle checks including line lengths. This pull request introduces dozens of spurious whitespace and line length changes which need to be reverted.

The configuration format should be the same as the request format so that users can use the same files to both invoke the proxy and configure it via RPC, e.g.,

chaos-http-proxy --properties configs/http_500.conf
curl --request POST http://localhost:1080/chaos/api < configs/http_500.conf

Finally this needs some kind of unit test. I cannot get the POST endpoint to work with the following:

cat http_500.json
{   
    "http_500":1,
    "success":1
}

curl --request POST http://localhost:1080/chaos/api < http_500.json

I encounter the following exception:

java.lang.NullPointerException: null
        at com.bouncestorage.chaoshttpproxy.ChaosHttpProxy$1.handle(ChaosHttpProxy.java:80) ~[chaos-http-proxy:1.1.0-SNAPSHOT]
        at org.eclipse.jetty.server.handler.HandlerList.handle(HandlerList.java:52) ~[chaos-http-proxy:1.1.0-SNAPSHOT]
        at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:97) ~[chaos-http-proxy:1.1.0-SNAPSHOT]
        at org.eclipse.jetty.server.Server.handle(Server.java:499) ~[chaos-http-proxy:1.1.0-SNAPSHOT]
        at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:310) ~[chaos-http-proxy:1.1.0-SNAPSHOT]
        at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:257) [chaos-http-proxy:1.1.0-SNAPSHOT]
        at org.eclipse.jetty.io.AbstractConnection$2.run(AbstractConnection.java:540) [chaos-http-proxy:1.1.0-SNAPSHOT]
        at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:635) [chaos-http-proxy:1.1.0-SNAPSHOT]
        at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:555) [chaos-http-proxy:1.1.0-SNAPSHOT]
        at java.lang.Thread.run(Thread.java:745) [na:1.8.0_77]
gaul commented 8 years ago

In terms of a unit test, we probably want to add a GET endpoint as well which the test can check both the initial static configuration and dynamic configuration with.

castlec commented 8 years ago

No problem. It'll probably sit until next weekend.