search

teamhephy / router

MIT License
4 stars 10 forks source link

feat(router): Add ability to set X-Request-Start header per app #42 #43

Closed Cryptophobia closed 4 years ago

Cryptophobia commented 4 years ago

Closes https://github.com/teamhephy/router/issues/42

Still needs to be tested to make sure it works as intended for each application config.

Can you guys review @pdomagala and @felixbuenemann ?

felixbuenemann commented 4 years ago

@Cryptophobia I don't think it makes sense to have appConfig.DisableRequestStartHeader, it would be better to have appConfig.RequestStartHeader as a boolean.

You might want to have the routerConfig.RequestStartHeader = false and only enable appConfig.RequestStartHeader = true on some apps.

Cryptophobia commented 4 years ago

@felixbuenemann , should this annotation be enabled per cluster and for all apps by default? I thought turning it off per app was an edge case per the discussion with @pdomagala and with his particular environment where they have multiple internal loadbalancers (nginx, HAproxy, etc...)?

felixbuenemann commented 4 years ago

@Cryptophobia I don't think this setting should be enabled for all apps by default, since it could interfere with existing external headers.

After looking at the code I understand why you chose the DisableRequestStartHeader naming, but I still find it confusing.

As long as we can have an indeterminate state in appConfig.RequestStartHeader I think we can keep the names the same.

So if appConfig.RequestStartHeader is nil, we use the value from routerConfig.RequestStartHeader, but if it is either true or false it overrides the routerConfig value.

Does this make sense to you or am I missing something?

pdomagala commented 4 years ago

@Cryptophobia Exactly, I'm also worried that it'll interfere/override existing headers, so this solution (enable/disable per app) sounds legit.

Cryptophobia commented 4 years ago

Thank you for reviewing this @pdomagala and @felixbuenemann .

So let's go with what @felixbuenemann mentioned above:

So if appConfig.RequestStartHeader is nil, we use the value from routerConfig.RequestStartHeader, but if it is either true or false it overrides the routerConfig value.

Cryptophobia commented 4 years ago

I have tested this and it works perfectly the way we want. I have kept he old naming because it makes more sense if the annotation is called disableRequestStartHeader since the values of it can be either true or false. If the header is missing from the appConfig then we just use the global value override.

felixbuenemann commented 4 years ago

@Cryptophobia The thing I don't like about this implementation, is that if you want to enable the header for a single app, you would need to set routerConfig.requestStartHeader = true and then set appConfig.disableRequestStartHeader = false for every app, except the one app, where we want the header.

With the logic I proposed you would have just set appConfig.requestStartHeader = true for the one app, where you want it enabled.

I don't think it's a dealbreaker, since it most cases you would be fine with having the header active for all apps, even if they don't need it.

Cryptophobia commented 4 years ago

I don't think it's a dealbreaker, since it most cases you would be fine with having the header active for all apps, even if they don't need it.

This is what I thought from the beginning. Having this extra header even if not used by anything (like Datadog, etc.), is it actually a burden?

felixbuenemann commented 4 years ago

I don't think it's a dealbreaker, since it most cases you would be fine with having the header active for all apps, even if they don't need it.

This is what I thought from the beginning. Having this extra header even if not used by anything (like Datadog, etc.), is it actually a burden?

Nope, let's ship it like this!