feat(web): add ability to skip X-SPINNAKER-ACCOUNTS in SpinnakerRequestInterceptor and SpinnakerRequestHeaderInterceptor #1148
Conversation
…to src/main/java since that's where java files belong
to reduce duplication
…ConfigurationProperties to pave the way for multiple SpinnakerRequestInterceptor instances, some configured differently than others.
…stInterceptor for uses of retrofit to communicate outside of spinnaker. Skipping this header avoids possible HTTP 431 response codes where http servers aren't configured to receive large request headers when X-SPINNAKER-ACCOUNTS is large.
…m groovy to java because the less groovy we've got, the better. This also makes converting the test to use WireMock in preference to mockStatic that much easier. While we're at it, move the test to the same package as SpinnakerRequestHeaderInterceptor: com.netflix.spinnaker.okhttp.
…use wiremock and real requests, instead of static mocking of AuthenticatedRequest since it seems like a more thorough test. Matching the structure of SpinnakerRequestInterceptorTest also seems like a win.
…ClientConfigurationProperties to pave the way for multiple SpinnakerRequestHeaderInterceptor instances, some configured differently than others.
…stHeaderInterceptor for uses of retrofit2 to communicate outside of spinnaker. Skipping this header avoids possible HTTP 431 response codes where http servers aren't configured to receive large request headers when X-SPINNAKER-ACCOUNTS is large.
|
@Mergifyio update |
✅ Branch has been successfully updated |
There was a problem hiding this comment.
I think this is fine, but I did have another suggested approach in case custom webhooks, for whatever reason, is relying on that header. However, if that is the case, we probably want to at least make that portion of it configurable which seems to not be via how the bean is constructed.
Either way, let me know your thoughts. I think if we do go with this approach and want this to be merged, allow for it to be configured. Or the other suggestion of choosing a header value size which we could even extend to other headers.
| import com.netflix.spinnaker.security.AuthenticatedRequest | ||
| import retrofit.RequestInterceptor | ||
|
|
||
| class SpinnakerRequestInterceptor implements RequestInterceptor { | ||
| private final OkHttpClientConfigurationProperties okHttpClientConfigurationProperties | ||
| private final boolean propagateSpinnakerHeaders; |
There was a problem hiding this comment.
This is one thing I really dislike about the HTTP spec is that they don't define a limit for header size, so servers just arbitrarily choose one. One thing we can do is also only send the header if the size is manageable (whatever that may be). So rather than needing to configure this, it can just send this header when the header values is less than 8kb or something which should work on most servers.
| @@ -82,12 +82,12 @@ public class OkHttpClientComponents { | |||
|
|
|||
| @Bean | |||
| public SpinnakerRequestInterceptor spinnakerRequestInterceptor() { | |||
| return new SpinnakerRequestInterceptor(clientProperties); | |||
| return new SpinnakerRequestInterceptor(clientProperties.getPropagateSpinnakerHeaders()); | |||
There was a problem hiding this comment.
There seems to be no way to configure sending the account headers?
There was a problem hiding this comment.
Right....sort of. The SpinnakerRequestInterceptor bean keeps the same behavior -- to send the account headers. But now it's possible to construct another SpinnakerRequestInterceptor bean that doesn't.
I was figuring that existing uses of the SpinnakerRequestInterceptor bean need to continue sending the accounts header, since they're used to communicate among the various spinnaker microservices. At the moment parts of spinnaker would break without it, so I don't think it needs to be configurable, at least not that way. As far as configuring a size and only sending the accounts header (or other headers) when they satisfy some size threshold...I can see the appeal of that...except that it seems like the receiving code either needs a complete header, or it doesn't need it at all, so sending a partial one might do more harm than good. |
for uses of retrofit to communicate outside of spinnaker. Skipping this header avoids possible HTTP 431 response codes where http servers aren't configured to receive large request headers when X-SPINNAKER-ACCOUNTS is large.
There are changes required in orca tests to keep up with the SpinnakerRequestInterceptor constructor changes: