OADP-659 Avoid overwriting good hostname. #1177
Conversation
SanitizeHostForHeader puts the stripped host name into the Host field of the HTTP request, but only if it actually needed to strip the port. If there is already no port, that Host field can be blank, and there is no reason to overwrite the URL's host field. Signed-off-by: Matthew Arnold <marnold@redhat.com>
| @@ -213,6 +213,8 @@ func StripDefaultPorts(fromUrl string) (string, error) { | |||
| URL: u, | |||
| } | |||
| request.SanitizeHostForHeader(&r) | |||
| r.URL.Host = r.Host | |||
| if r.Host != "" { | |||
There was a problem hiding this comment.
I would add unit tests to some scenarios (IBM, Noobaa, etc) here https://github.com/openshift/oadp-operator/blob/master/pkg/common/common_test.go to avoid regressions
There was a problem hiding this comment.
+1 but not blocking. could be follow up issue.
There was a problem hiding this comment.
Tested, and this fixes my use case 😄
Only concern I have with follow up PR is that we need this in 1.2 branch right? then will be 3 cherry-picks
There was a problem hiding this comment.
3 cherry-picks to oadp-1.2 and oadp 1.1 and what's the other one?
But yeah follow up pr is fine.
There was a problem hiding this comment.
oh no, I think no need to 1.1, only for 1.2
it would be form this PR, the previous (#1169) and the follow up (3, but for the same branch)
https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_oadp-operator/1177/pull-ci-openshift-oadp-operator-master-4.12-operator-e2e-aws/1711749018561810432#1:build-log.txt%3A76 /test 4.13-operator-e2e-azure |
|
@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/4.12-operator-e2e-aws In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, mrnold, shubham-pampattiwar, sseago 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 |
|
/cherry-pick oadp-1.2 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of oadp-1.2 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
|
@mrnold: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@kaovilai: #1177 failed to apply on top of branch "oadp-1.2": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@mrnold cherrypick #1169 and #1177 into oadp-1.2 :) and move https://issues.redhat.com/browse/OADP-659 status accordingly. |
SanitizeHostForHeader puts the stripped host name into the Host field of the HTTP request, but only if it actually needed to strip the port. If there is already no port, that Host field can be blank, and there is no reason to overwrite the URL's host field. Signed-off-by: Matthew Arnold <marnold@redhat.com>
… AWS S3 URLs. (#1180) * OADP-659 Remove HTTP/HTTPS port numbers from AWS S3 URLs. (#1169) * OADP-659 Remove port numbers if HTTP or HTTPS. Avoids a signature mismatch from S3-compatible services. Signed-off-by: Matthew Arnold <marnold@redhat.com> * OADP-659 Move StripDefaultPorts to pkg/common. Signed-off-by: Matthew Arnold <marnold@redhat.com> * OADP-659 Fix port number in comment. Signed-off-by: Matthew Arnold <marnold@redhat.com> --------- Signed-off-by: Matthew Arnold <marnold@redhat.com> * OADP-659 Avoid overwriting good hostname. (#1177) SanitizeHostForHeader puts the stripped host name into the Host field of the HTTP request, but only if it actually needed to strip the port. If there is already no port, that Host field can be blank, and there is no reason to overwrite the URL's host field. Signed-off-by: Matthew Arnold <marnold@redhat.com> --------- Signed-off-by: Matthew Arnold <marnold@redhat.com>
Fix an issue with StripDefaultPorts that breaks a new test case being added for #1169.