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

[FINE] Disable connection reuse for WebSocket connections in Apache #219

Merged
merged 1 commit into from Jul 6, 2017

Conversation

skateman
Copy link
Member

This is a temporary workaround for the issue described here:
https://bugzilla.redhat.com/show_bug.cgi?id=1404354

This can be reverted after httpd is updated to 2.4.25 or newer

@miq-bot miq-bot changed the title Disable connection reuse for WebSocket connections in Apache [FINE] Disable connection reuse for WebSocket connections in Apache Jun 28, 2017
@simaishi simaishi requested a review from jrafanie June 28, 2017 17:46
@skateman skateman force-pushed the fine-websocket-fix branch 2 times, most recently from 22b31ec to 0f5e5c8 Compare June 28, 2017 18:18
end
ports
end

def remove_ports(ports, protocol)
ports = Array(ports)
ports.each do |port|
@raw_lines.delete_if { |line| line =~ /BalancerMember\s+#{protocol}:\/\/0\.0\.0\.0:#{port}$/ }
@raw_lines.delete_if { |line| line =~ /BalancerMember\s+#{protocol}:\/\/0\.0\.0\.0:#{port}( disablereuse=on)?$/ }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skateman does this mean we only remove the ws protocol BalancerMembers? What about non-ws? I guess I'm wondering why we are checking for disablereuse at all if no other line will match the protocol, 0.0.0.0 and port... Right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, AFAIK the ? at the end of the regexp makes the disablereuse part optional, but maybe I'm wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I believe so, but if it's not required, I wouldn't include it. I think it's not required so I'd prefer we keep the regexp as simple as possible. Maybe I'm wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I saw the $ at the end and did not wanted to drop it.


it 'remove_ports should remove the line with a suffix' do
before = @conf.raw_lines.dup
@conf.raw_lines = ["<Proxy balancer://evmcluster/ lbmethod=byrequests>\n", "BalancerMember ws://0.0.0.0:3000 disablereuse=on\n", "</Proxy>\n"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skateman can you add a non-ws remove_ports test to confirm the regexp works for both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrafanie done

@simaishi simaishi self-assigned this Jun 29, 2017
This is a temporary workaround for the issue described here:
https://bugzilla.redhat.com/show_bug.cgi?id=1404354

This can be reverted after httpd is updated to 2.4.25 or newer
@miq-bot
Copy link
Member

miq-bot commented Jun 29, 2017

Checked commit skateman@65842a4 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @simaishi 👍

@simaishi simaishi merged commit 73a5739 into ManageIQ:fine Jul 6, 2017
@simaishi simaishi added this to the Sprint 64 Ending Jul 10, 2017 milestone Jul 6, 2017
@skateman skateman deleted the fine-websocket-fix branch July 7, 2017 08:49
@simaishi
Copy link
Contributor

simaishi commented Jul 7, 2017

@skateman this can be euwe/yes?

@skateman
Copy link
Member Author

skateman commented Jul 7, 2017

We aren't using websocket notifications in euwe, but backporting this would definitely not do any harm.

@simaishi
Copy link
Contributor

simaishi commented Jul 7, 2017

Then... what change will fix the issue for Euwe branch??

@skateman
Copy link
Member Author

skateman commented Jul 7, 2017

@simaishi uh, I got confused by the names, sure, backport it to euwe, all the things I said was ment for darga 😉 sorry

@simaishi
Copy link
Contributor

simaishi commented Jul 7, 2017

@skateman Thanks 😄

Euwe backport (to manageiq repo) details:

$ git log -1
commit eee139331f831587ee62d3979c074eb201520c84
Author: Satoe Imaishi <simaishi@redhat.com>
Date:   Thu Jul 6 09:56:11 2017 -0400

    Merge pull request #219 from skateman/fine-websocket-fix
    
    [FINE] Disable connection reuse for WebSocket connections in Apache
    (cherry picked from commit 73a5739b13fc460b4a339ea80571111940dbb808)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1468633

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants