Add port 443 to listen_ports in apache2 (SOC-9172)

[zzaimeche]

This commit stops port clashes between horizon and haproxy on HA
deployments with SSL enabled.

Without this patch, the apache ssl recipe adds port 443 to the apache
listen.conf file without adding the port to the listen_ports array.
In a HA setup with ssl enabled, both HAProxy and Apache will
try to use port 443. To work around this, the horizon ssl recipe currently
checks to see what ports are listed in a listen_ports array, and
if either port 443 or port 80 is in there, it wipes the listen.conf
file. As port 443 is not currently in the listen_ports array, the
horizon recipe leaves the listen.conf file alone in cases where
port 443 is in there.

This commit adds port 443 to the listen_ports array at the point
where it is added as a listen port, so that the horizon recipe can
find it later.

More explanation in:
https://bugzilla.suse.com/show_bug.cgi?id=1141490#c34

[zzaimeche]

I need to check this new version actually works; I don't trust myself to have got the Ruby syntax right.

[jsuchome]

This actually looks right (once you fix the syntax). I'm really surprised we didn't see it before...

[zzaimeche]

This actually looks right (once you fix the syntax). I'm really surprised we didn't see it before...

Yeah, it made me wonder if there was a not-obvious but important reason for the current implementation, since it's easier to add 443 to the array.

[zzaimeche]

[I had an old comment here that said this shouldn't be merged, but actually it was wrong. Kept it here to avoid confusion from anyone who'd already read it, but it should be ignored.]

---

OLD INCORRECT COMMENT:
This shouldn't be merged, yet, since I've realised this means port 443 is always going to get added to listen_ports, and that means that the HA cookbook will always wipe the listen.conf. Though I think that means the HA cookbook implementation needs to change first.

(HA cookbook in the crowbar-openstack repo, that is: https://github.com/crowbar/crowbar-openstack/blob/master/chef/cookbooks/horizon/recipes/ha.rb#L52 should change to just remove the specified ports instead of clearing the array entirely)
EDIT: (Actually, looks like the listen.conf gets wiped and replaced either way; the contents of the file doesn't make it back into the listen_ports array. So that's unrelated to this, and might be desired behaviour.)

END OF OLD INCORRECT COMMENT

---

[jsuchome]

@rhafer @cmurphy @jgrassler Could you please take a look? This seems like a correct change, I'm just unsure if we can't break something else with it.
I'm also surprised we haven't seen it before...

[rhafer]

@rhafer @cmurphy @jgrassler Could you please take a look? This seems like a correct change, I'm just unsure if we can't break something else with it.

I'm also surprised we haven't seen it before...

I'm not really sure if this is the right fix. AFAIU the default for the node[:apache][:listen_ports] attribute is alway set to [80, 443]. (See here: https://github.com/crowbar/crowbar-core/blob/master/chef/cookbooks/apache2/attributes/default.rb#L90)

There seems to be just one place where the attribute is ever touched. And that's in https://github.com/crowbar/crowbar-openstack/blob/master/chef/cookbooks/horizon/recipes/ha.rb#L52

Now we'd be changing the attribute twice. First the port is added to the nodes attribute (in the mod_ssl recipe) and then its removed it again later (in the horizion/ha recipe). I am not exactly sure about the run order here, but depending on which other services are deployed on the node the recipes might actually change the order even. (Depending of which cookbook does the first include_recipe call for apache)

Given that node[:apache][:listen_ports] seems to need to be either the default ( [80, 443] ) or empty (when HA is enabled). The right fix for crowbar might just be to remove that whole
unless node[:apache][:listen_ports].include?("443") block from the mod_ssl recipe.

Or did I overlook something and node[:apache][:listen_ports] is updated somewhere else throughout the code?

[zzaimeche]

The right fix for crowbar might just be to remove that whole
unless node[:apache][:listen_ports].include?("443") block from the mod_ssl recipe.

I'd be happy to change it to that, I just don't know why that unless block was put there in the first place! So hoping someone who's more familiar with crowbar can weigh in on that. I didn't find another mention of listen_ports . ( https://github.com/search?p=1&q=org%3Acrowbar+node%5B%3Aapache%5D%5B%3Alisten_ports%5D&type=Code )

[rhafer]

The right fix for crowbar might just be to remove that whole
unless node[:apache][:listen_ports].include?("443") block from the mod_ssl recipe.

I'd be happy to change it to that, I just don't know why that unless block was put there in the first place! So hoping someone who's more familiar with crowbar can weigh in on that. I didn't find another mention of listen_ports . ( https://github.com/search?p=1&q=org%3Acrowbar+node%5B%3Aapache%5D%5B%3Alisten_ports%5D&type=Code )

I think that is still an artifact from the old "upstream" chef cookbook for apache. I.e. the apache cookbook in crowbar was forked from https://github.com/chefs/cookbooks at some point (which is archived here: https://github.com/chef-boneyard/cookbooks/tree/deprecated-master/apache2 if you're interested in archaeology ;-) ). I don't think crowbar needs that specific part as listen_ports is either [] or [80, 443]