HAProxy: Extend configuration options #193

matrixik · 2017-04-07T12:11:19Z

We configure Monasca using the ansible-based monasca-installer, which is run from Crowbar. This means that the Monasca API services we want to put behind haproxy are not directly under Crowbar's aegis. To put them behind Crowbar-managed haproxy instances we need these new options.

houndci-bot · 2017-04-07T12:11:29Z

chef/cookbooks/haproxy/resources/loadbalancer.rb

+attribute :address,         kind_of: String,  default: "0.0.0.0"
+attribute :port,            kind_of: Integer, default: 0
+attribute :mode,            kind_of: String,  default: "http", equal_to: ["http", "tcp", "health"]
+attribute :balance,         kind_of: String,  default: "", equal_to: ["", "roundrobin", "static-rr", "leastconn", "first", "source"]


Metrics/LineLength: Line is too long. [132/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

houndci-bot · 2017-04-07T12:11:29Z

chef/cookbooks/haproxy/resources/loadbalancer.rb

-attribute :options, kind_of: Array,   default: []
-attribute :servers, kind_of: Array,   default: []
+attribute :name,            kind_of: String,  name_attribute: true
+attribute :type,            kind_of: String,  default: "listen", equal_to: ["listen", "backend", "frontend"]


Metrics/LineLength: Line is too long. [108/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

Any chance you could break the equal_to: ... onto a continuation line?

Done, but hound complain now about Style/ExtraSpacing: Unnecessary spacing detected. in next line...
#193 (comment)

houndci-bot · 2017-04-07T12:11:29Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

+      raise "One of the servers has no name." if server["name"].nil?
+      raise "Server #{server["name"]} has no address." if server["address"].nil?
+      raise "Server #{server["name"]} has no port." if server["port"].nil?
+      raise "Server #{server["name"]} has invalid port." if server["port"] < 1 || server["port"] > 65535


Metrics/LineLength: Line is too long. [104/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

matrixik · 2017-04-07T12:11:52Z

Sorry about previous PR

aspiers · 2017-04-07T12:20:38Z

@matrixik No problem :) I'm missing context so I could be completely wrong, but you might find this blog post useful?

houndci-bot · 2017-04-10T07:06:55Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

+  unless new_resource.default_backend.empty?
+    section["default_backend"] = new_resource.default_backend
+  end
+  unless new_resource.servers.empty?


Style/IfUnlessModifier: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||. (https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier)

houndci-bot · 2017-04-10T07:06:56Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

  section["use_ssl"] = new_resource.use_ssl
  if new_resource.use_ssl
    section["mode"] = "tcp"
  else
    section["mode"] = new_resource.mode
  end
+  unless new_resource.balance.empty?


Style/IfUnlessModifier: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||. (https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier)

houndci-bot · 2017-04-10T07:06:56Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

+  unless new_resource.address.empty?
+    section["address"] = new_resource.address
+  end
+  unless new_resource.port == 0


Style/IfUnlessModifier: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||. (https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier)
Style/NumericPredicate: Use new_resource.port.zero? instead of new_resource.port == 0. (https://github.com/bbatsov/ruby-style-guide#predicate-methods)

houndci-bot · 2017-04-10T07:06:56Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

  end

  section = {}
-  section["address"] = new_resource.address
-  section["port"] = new_resource.port
+  unless new_resource.address.empty?


Style/IfUnlessModifier: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||. (https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier)

houndci-bot · 2017-04-10T07:06:56Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

+      raise "One of the servers has no name." if server["name"].nil?
+      raise "Server #{server['name']} has no address." if server["address"].nil?
+      raise "Server #{server['name']} has no port." if server["port"].nil?
+      raise "Server #{server['name']} has invalid port." if (server["port"] < 1 || server["port"] > 65535)


Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliteralsininterpolation)
Style/ParenthesesAroundCondition: Don't use parentheses around the condition of an if. (https://github.com/bbatsov/ruby-style-guide#no-parens-around-condition)
Metrics/LineLength: Line is too long. [106/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

I disagree with Hound CI regarding Style/StringLiteralsInInterpolation, but I agree regarding Style/ParenthesesAroundCondition and Metrics/LineLength.

Should I change back and allow hound to complain about Style/StringLiteralsInInterpolation? Rest done.

I don't think you need to change back :-) Thanks!

houndci-bot · 2017-04-10T07:06:56Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

+    new_resource.servers.each do |server|
+      raise "One of the servers has no name." if server["name"].nil?
+      raise "Server #{server['name']} has no address." if server["address"].nil?
+      raise "Server #{server['name']} has no port." if server["port"].nil?


Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliteralsininterpolation)

houndci-bot · 2017-04-10T07:06:56Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

+  else
+    new_resource.servers.each do |server|
+      raise "One of the servers has no name." if server["name"].nil?
+      raise "Server #{server['name']} has no address." if server["address"].nil?


Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliteralsininterpolation)

houndci-bot · 2017-04-10T07:06:56Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

-    raise "Server #{server['name']} has no address." if server["address"].nil?
-    raise "Server #{server['name']} has no port." if server["port"].nil?
-    raise "Server #{server['name']} has invalid port." if (server["port"] < 1 || server["port"] > 65535)
+    if new_resource.type != "frontend"


Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||. (https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier)

houndci-bot · 2017-04-10T07:06:56Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

@@ -23,30 +23,42 @@
  # While there is no way to have an include directive for haproxy
  # configuration file, this provider will only modify attributes !

-  if new_resource.port < 1 || new_resource.port > 65535
-    raise "Invalid port: #{new_resource.port}."
+  if ! new_resource.port == 0 && new_resource.type == "backend"


Style/NumericPredicate: Use ! new_resource.port.zero? instead of ! new_resource.port == 0. (https://github.com/bbatsov/ruby-style-guide#predicate-methods)
Style/SpaceAfterNot: Do not leave space between ! and its argument. (https://github.com/bbatsov/ruby-style-guide#no-space-bang)

aspiers

Looks OK modulo the Hound CI style warnings.

aspiers · 2017-05-17T11:49:11Z

chef/cookbooks/haproxy/resources/loadbalancer.rb

-attribute :options, kind_of: Array,   default: []
-attribute :servers, kind_of: Array,   default: []
+attribute :name,            kind_of: String,  name_attribute: true
+attribute :type,            kind_of: String,  default: "listen", equal_to: ["listen", "backend", "frontend"]


Any chance you could break the equal_to: ... onto a continuation line?

aspiers · 2017-05-17T11:49:19Z

chef/cookbooks/haproxy/resources/loadbalancer.rb

+attribute :address,         kind_of: String,  default: "0.0.0.0"
+attribute :port,            kind_of: Integer, default: 0
+attribute :mode,            kind_of: String,  default: "http", equal_to: ["http", "tcp", "health"]
+attribute :balance,         kind_of: String,  default: "", equal_to: ["", "roundrobin", "static-rr", "leastconn", "first", "source"]


aspiers · 2017-05-17T11:50:49Z

chef/cookbooks/haproxy/providers/loadbalancer.rb

+      raise "One of the servers has no name." if server["name"].nil?
+      raise "Server #{server['name']} has no address." if server["address"].nil?
+      raise "Server #{server['name']} has no port." if server["port"].nil?
+      raise "Server #{server['name']} has invalid port." if (server["port"] < 1 || server["port"] > 65535)


I disagree with Hound CI regarding Style/StringLiteralsInInterpolation, but I agree regarding Style/ParenthesesAroundCondition and Metrics/LineLength.

houndci-bot · 2017-05-19T08:13:34Z

chef/cookbooks/haproxy/resources/loadbalancer.rb

+attribute :mode,            kind_of: String,  default: "http", equal_to: [
+  "http", "tcp", "health"
+]
+attribute :balance,         kind_of: String,  default: "", equal_to: [


Style/ExtraSpacing: Unnecessary spacing detected.

aspiers

Thanks, I can't see anything obviously wrong with the aggregate changes now. It's a bit hard for me to give a confident +1 however, as I don't know the context surrounding the PR. Normally I would get this from the PR description and the commit messages, but the functionality changes are split across several commits and there is no PR description explaining all the changes. In particular I'm a bit confused why some checks are introduced and then removed again in a later commit. Maybe someone else who knows more about the context can +1.

matrixik · 2017-05-23T11:00:53Z

It was long time ago and I'm not sure who suggested me to remove this checks. I think they was removed because they are already checked/handled in chef/cookbooks/haproxy/providers/loadbalancer.rb.
Also regarding this change it was needed for when Monasca was supposed to be installed in HA mode and we needed more advanced HAProxy configuration to handle relaying communication with influxdb and influxdb-relay in consistent manner so that from the outside this two looks exactly the same as communicating with only influxdb.

matrixik · 2017-05-23T12:26:43Z

I also wanted to have separate commits for each functionality but I can squash all if you want.

houndci-bot · 2017-06-02T09:15:12Z

chef/cookbooks/haproxy/resources/loadbalancer.rb

+attribute :mode,            kind_of: String,  default: "http", equal_to: [
+  "http", "tcp", "health"
+]
+attribute :balance,         kind_of: String,  default: "", equal_to: [


Layout/ExtraSpacing: Unnecessary spacing detected.

matrixik · 2017-06-02T09:28:02Z

@aspiers Squashed and rebased.

jgrassler · 2017-06-02T09:56:07Z

Some high level context on why we need this: we configure Monasca using the ansible based monasca-installer, which is run from Crowbar. This means that the Monasca API services we want to put behind haproxy are not directly under Crowbar's aegis. To put them behind Crowbar managed haproxy instances we need these new options.

aspiers · 2017-06-16T11:37:47Z

@jgrassler Thanks a lot, that's really helpful. In the future please just add this kind of context to the PR description :-) It's hard to find it when it's buried in a stream of comments. (On this occasion I've just copied your info to the description.)

Now it's possible to configure separately frontend and backend and hide many different services with one frontend based on access path. E.g. `/write` can go to writable backend and `/read` could go only to read-only backend. Add `acl`, `use_backend` and `default_backend` to configurable attributes. Does not require server for frontend and does not require bind configuration, frontent services does not need to bind to any address when they are only used for backend services.

matrixik · 2017-07-17T09:04:36Z

Rebased.

matrixik · 2017-10-06T10:35:32Z

I no longer actively work on this so closing.

houndci-bot reviewed Apr 7, 2017

View reviewed changes

matrixik force-pushed the update_haproxy branch from e60e449 to 83002e4 Compare April 10, 2017 07:06

houndci-bot reviewed Apr 10, 2017

View reviewed changes

matrixik force-pushed the update_haproxy branch from b2c5a7e to 46dde2e Compare April 10, 2017 07:32

sjamgade mentioned this pull request Apr 10, 2017

Rework haproxy config for stickiness and balance strategy #179

Open

matrixik force-pushed the update_haproxy branch from 9b14da3 to a9d0618 Compare April 10, 2017 08:54

sjamgade previously approved these changes Apr 10, 2017

View reviewed changes

matrixik force-pushed the update_haproxy branch 2 times, most recently from a9d0618 to bc4402e Compare April 12, 2017 06:53

dirkmueller requested review from vuntz and aspiers April 13, 2017 13:33

aspiers reviewed May 17, 2017

View reviewed changes

matrixik force-pushed the update_haproxy branch from bc4402e to 50b4226 Compare May 19, 2017 08:13

houndci-bot reviewed May 19, 2017

View reviewed changes

aspiers reviewed May 19, 2017

View reviewed changes

matrixik force-pushed the update_haproxy branch from 50b4226 to 772dd38 Compare June 2, 2017 09:15

houndci-bot reviewed Jun 2, 2017

View reviewed changes

matrixik force-pushed the update_haproxy branch from 772dd38 to e61012f Compare June 2, 2017 09:27

jsuchome added the needs backport to SOC7 (stable/4.0) label Jun 15, 2017

matrixik dismissed sjamgade’s stale review via ae06e65 July 17, 2017 09:03

matrixik force-pushed the update_haproxy branch from e61012f to ae06e65 Compare July 17, 2017 09:03

dirkmueller added needs backport to SOC7 and removed needs backport to SOC7 (stable/4.0) labels Oct 5, 2017

matrixik closed this Oct 6, 2017

HAProxy: Extend configuration options #193

HAProxy: Extend configuration options #193

Conversation

matrixik commented Apr 7, 2017 • edited by aspiers Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matrixik commented Apr 7, 2017

aspiers commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aspiers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aspiers left a comment

Choose a reason for hiding this comment

matrixik commented May 23, 2017 • edited Loading

matrixik commented May 23, 2017

Choose a reason for hiding this comment

matrixik commented Jun 2, 2017

jgrassler commented Jun 2, 2017

aspiers commented Jun 16, 2017 • edited Loading

matrixik commented Jul 17, 2017

matrixik commented Oct 6, 2017

matrixik commented Apr 7, 2017 •

edited by aspiers

Loading

matrixik commented May 23, 2017 •

edited

Loading

aspiers commented Jun 16, 2017 •

edited

Loading