Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1852728: haproxy 503 error page: replace all LF by CRLF to be fully RFC compliant #140

Conversation

felixkrohn
Copy link

rfc2616 and rfc7230 both demand that in HTTP messages lines be ended with CRLF.
Although both RFCs stipulate that LF-only line endings MAY or even SHOULD be accepted, It's better to be fully compliant. Very picky HTTP clients, such as web access firewalls, may trip over this and blame Openshift's router.
This PR is whitespace-only and converts all LF line terminators in this file to CRLF.

@openshift-ci-robot
Copy link
Contributor

Hi @felixkrohn. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 15, 2020
@felixkrohn
Copy link
Author

P.S. Checking the 503 error file of upstream HAproxy, I noticed they have the RFC-compliant CRLF, so it should be no big issue for Openshift-HAproxy either:

$ wget -q "http://git.haproxy.org/?p=haproxy-1.8.git;a=blob_plain;f=examples/errorfiles/503.http;hb=HEAD" -O- | xxd | head -n3
00000000: 4854 5450 2f31 2e30 2035 3033 2053 6572  HTTP/1.0 503 Ser
00000010: 7669 6365 2055 6e61 7661 696c 6162 6c65  vice Unavailable
00000020: 0d0a 4361 6368 652d 436f 6e74 726f 6c3a  ..Cache-Control:

(0d0a is CRLF)

@felixkrohn
Copy link
Author

@JacobTanenbaum @pecameron Do you have any objections or see other blockers to get this one merged?

@sgreene570
Copy link

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 8, 2020
@felixkrohn
Copy link
Author

/retest

5 similar comments
@felixkrohn
Copy link
Author

/retest

@felixkrohn
Copy link
Author

/retest

@felixkrohn
Copy link
Author

/retest

@felixkrohn
Copy link
Author

/retest

@felixkrohn
Copy link
Author

/retest

@frobware
Copy link
Contributor

frobware commented Sep 9, 2020

This is LGTM for me. Adding @openshift/openshift-team-network-edge in case we deliberately had this as LF only for specific reasons.

@danehans
Copy link
Contributor

danehans commented Sep 9, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2020
@danehans
Copy link
Contributor

danehans commented Sep 9, 2020

@felixkrohn a BZ will need to be created and linked if you need this fix in 4.6.

@felixkrohn
Copy link
Author

@danehans Thanks! BZ #1852728 already exists, can you somehow link it?

@sgreene570
Copy link

/retitle Bug 1852728: haproxy 503 error page: replace all LF by CRLF to be fully RFC compliant

@openshift-ci-robot openshift-ci-robot changed the title haproxy 503 error page: replace all LF by CRLF to be fully RFC compliant Bug 1852728: haproxy 503 error page: replace all LF by CRLF to be fully RFC compliant Sep 9, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Sep 9, 2020
@openshift-ci-robot
Copy link
Contributor

@felixkrohn: This pull request references Bugzilla bug 1852728, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1852728: haproxy 503 error page: replace all LF by CRLF to be fully RFC compliant

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.

@sgreene570
Copy link

hm, @danehans isn't in the approvers list
/approve

@felixkrohn we will backport this PR for 4.5, 4.4, and 3.11 later this week most likely.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans, felixkrohn, sgreene570

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2020
@felixkrohn
Copy link
Author

@sgreene570 Wonderful! Thanks, this is very much apreciated.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 9, 2020

@felixkrohn: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws fadab45 link /test e2e-aws

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.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit d5bc671 into openshift:master Sep 9, 2020
@openshift-ci-robot
Copy link
Contributor

@felixkrohn: All pull requests linked via external trackers have merged:

Bugzilla bug 1852728 has been moved to the MODIFIED state.

In response to this:

Bug 1852728: haproxy 503 error page: replace all LF by CRLF to be fully RFC compliant

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.

@sgreene570
Copy link

/cherry-pick release.4-5

@sgreene570
Copy link

/cherry-pick release.4-4

@openshift-cherrypick-robot

@sgreene570: cannot checkout release.4-5: error checking out release.4-5: exit status 1. output: error: pathspec 'release.4-5' did not match any file(s) known to git

In response to this:

/cherry-pick release.4-5

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-cherrypick-robot

@sgreene570: cannot checkout release.4-4: error checking out release.4-4: exit status 1. output: error: pathspec 'release.4-4' did not match any file(s) known to git

In response to this:

/cherry-pick release.4-4

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.

@sgreene570
Copy link

whoops, try that again.
/cherry-pick release-4.5

@openshift-cherrypick-robot

@sgreene570: new pull request created: #181

In response to this:

whoops, try that again.
/cherry-pick release-4.5

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.

@sgreene570
Copy link

/cherry-pick release-4.4

@openshift-cherrypick-robot

@sgreene570: new pull request created: #182

In response to this:

/cherry-pick release-4.4

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants