add mesh conformance for request header modifier #3812
Conversation
k8s-ci-robot
added
release-note
LiorLieberman
force-pushed
the
request-header-modifier-mesh-test
branch
from
c4d30f5 to
ffb897e
Compare
| for name, val := range resp.RequestHeaders { | ||
| resp.RequestHeaders[strings.ToLower(name)] = val | ||
| } | ||
| for name, expectedVal := range wantReq.Headers { |
There was a problem hiding this comment.
you're not checking if length of want and got headers match and you are missing the case here where got has some extra header. Is this expected?
There was a problem hiding this comment.
hmmm, if got has an extra header does it mean its not conformant? I dont think thats necessarily true
There was a problem hiding this comment.
I think it's fine for implementations to set custom headers, I know at least some set custom headers. If we want to add a requirement that no additional headers are set, that should probably result in some clarification on API spec first. We do have the AbsentHeaders check to ensure that headers we want to have been removed from a request have been.
There was a problem hiding this comment.
right, thats what I thought as well. Thanks for clarifying though.
|
Thanks @LiorLieberman! /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiorLieberman, robscott The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/area conformance-test
Add mesh tests for request header modifier
This is a follow up to #3729
What this PR does / why we need it:
Add mesh tests coverage
Which issue(s) this PR fixes:
Related #3581
Does this PR introduce a user-facing change?:
/cc @robscott