Identity v2&v3: add RequestOpts.OmitHeaders, remove blank header#2315
Identity v2&v3: add RequestOpts.OmitHeaders, remove blank header#2315EmilienM merged 1 commit intogophercloud:masterfrom
Conversation
| // OmitHeaders specifies the HTTP headers which should be omitted. | ||
| OmitHeaders []string |
There was a problem hiding this comment.
In your implementation, OmitHeaders takes precedence over MoreHeaders. Please mention that in the docstring.
| } | ||
| } | ||
|
|
||
| for _, v := range options.OmitHeaders { |
There was a problem hiding this comment.
nit: please update the comment on lines 399-400
| } | ||
|
|
||
| for _, v := range options.OmitHeaders { | ||
| req.Header.Del(v) |
There was a problem hiding this comment.
Is there any concern that the headers from client.AuthenticatedHeaders() will override this setting (line 420-422)?
There was a problem hiding this comment.
Before commit 29581a2, client.AuthenticatedHeaders() is prior to options.MoreHeaders. In my case, options.OmitHeaders makes up for the previous options.MoreHeaders and should inherit the original order
There was a problem hiding this comment.
yeah, we expected that something could be broken, see #2215 (comment)
There was a problem hiding this comment.
I'm not sure that something is broken yet, though. I think there needs to be some investigation and comparison to @freedywu's use-case that is triggering this and the basic acceptance test that passes without changes required.
There was a problem hiding this comment.
Is there any concern that the headers from client.AuthenticatedHeaders() will override this setting
I think it should be okay given that the order of the existing code has MoreHeaders first adding an empty X-Auth-Token and then client.AuthenticatedHeaders adds headers on top of it. Definitely warrants running this PR through the Keystone suite of acceptance tests, though.
|
Build succeeded.
|
|
Per discussion in #2314, it looks like the root cause is a very old version of Keystone triggering this issue. In my opinion, if the Keystone suite of acceptance tests passes with this PR, it should be fine to merge. It's not a lot of work to help get this functionality fixed and adds a feature that may be useful in other areas. And right now, the only place |
|
@freedywu could you please rebase on master, so we get the new CI job for Keystone? Thanks |
Sure, thanks for noticing. |
|
Hi @EmilienM , my commit has been rebased, could you have a look at it? |
mandre
left a comment
There was a problem hiding this comment.
LGTM, the code looks safe, and the identity acceptance tests worked as expected.
Any objection to merge?
Fixes #2314