Fix uniq for nil:NilClass error introduced in 3.2.2

[brentm5]

• Add default values for listen_ports and listen_addresses
• Add attribute to test for the error introduced

[b-dean]

Why is it even getting in there unless you have some values in node['apache']['listen_addresses'] or node['apache']['listen_ports']? And if one of those is set, why aren't they both?

[brentm5]

Not entirely sure what we are doing to cause this, but I know this caused everything that includes apache2 to fail in our infrastructure. I simply went back to 3.2.1 to resolve the issue for now. I can only assume we arent the only other ones to run into this issue. I mean by default both of those values are nil so we should either check them both with an && or there should be default attribute for this. If either of them are empty array then nothing happens anyway...

[b-dean]

In the cookbook versions less than 3.2, the ports and addresses were each in their own array and what the ports.conf had was a combination of every port on every address. In 3.2 there's a new array node['apache']['listen'] that is an array of <address>:<port>. Such as:

{
  "apache": {
    "listen": [
      "*:80",
      "*:443",
      "127.0.0.1:12345"
    ]
  }
}

that libraries/listen.rb file is there to convert the old attributes into the new ones. I had originally had that so you had to have both node['apache']['listen_addresses'] and node['apache']['listen_ports'] in order for the conversion to work. This seemed reasonable to me since they were both default attributes on the apache2 cookbook v3.1.x and should be there. But in changeset 52372bb which was in v3.2.2, they allowed you to have either the old listen_ports or listen_addresses. But that won't work because what would it combine them with? If you have only ports, what addresses should listen on those ports? If you have only address, what ports should apache listen to for those addresses?

My guess is that the reason it's failing for you is you have some override attributes that are setting either just the ports or just the attributes. On an existing node that still shouldn't be a problem since the old default attributes wouldn't have gone away (unless you did a knife node edit -a and deleted them). But on a new node, using only the new 3.2.2 cookbook, they won't be there. Does that make sense?

[b-dean]

Also, there should probably be some tests that fail because of this. I'd like this PR better if it had some tests that show the bug and expected behavior.

[sdadh01]

Mea culpa on not testing in all conditions for this PR. The situation I
came across was that I had a cookbook that had
node['apache']['listen_ports'] set but not listen_addresses - a valid
situation in 2.x.x. On upgrading the cookbook to 3.2.1 the self conversion
failed because it required both to be set to do so. Using || fixed that
situation in the case that listen_ports was set but listen_addresses was
not. I failed to test the reverse or check for defaults which I should have
done. A case of it works in my environment and looking for a quick fix.

Next time I'll do a more thorough job....

• adh

[brentm5]

I did add an attribute in the test cookbook so that this code path is now hit and tested @b-dean I wasn't sure of the exact fix that was wanted, I assumed that since this was an older syntax the reason there were no health defaults was because the new syntax was used in favor of this and people didn't want to add default values to confuse people. Thats why I just added a way to not have the nil class, but added an attribute in the test cookbook so the code path was executed.

[b-dean]

When I first changed from the old listen_ports and listen_addresses in #378 and then @jrosenboom improved it in #409, I had removed the older attributes from attributes/default.rb for the reason you mention, that I didn't want it to be confusing which ones to use. I didn't have to worry about them being nil because the conversion only did something if both attributes were there.

So when it was changed to be an || in the conversion for these attributes, I think that conversion might need to have the old defaults. I'm not the maintainer of this cookbook so I can't really say what they would want, but if I were using this and had a node that specified only node['apache']['listen_ports'] (like @sdadh01 mentioned), I think I would be surprised if my ports didn't end up in the ports.conf file. If the defaults are empty arrays, and I have node['apache']['listen_ports'] == ["1234"] when it does two loops over ["1234"] and [] the resulting listen array will be empty and ports.conf wouldn't have *:1234 which is probably what was intended.

[brentm5]

Then maybe it shouldn't be an || but rather an && ? Not really sure of the reasoning around that change TBH

[sdadh01]

@bigbam505 I did that because it was valid to only have one of node['apache']['listen_ports'] or node['apache']['listen_addresses'] in a cookbook. For instance in the case I was working on node['apache']['listen_ports'] was set but node['apache']['listen_addresses'] was not and relied on the apache2 cookbook defaults and so the self conversion to node['apache']['listen'] never occurred.

As mentioned above, I did fail to look at all circumstances but && has problems. Just replacing with || was an over simplification on my part but if we're trying to be backwardly compatible with 3.1.x then && does not fully work either. If we're presenting backward compatibility we need to move forward with something that does work for all circumstances - I realize in retrospect || breaks things but it did fix my particular circumstance where && failed.

Hope that explains the reasoning...

[brentm5]

I think we understand the reasoning, just with the current code its broken for that use case that you are referring to however so the next question is how do we want to go about fixing it. I have supplied one way which simply has no default values as was the current implementation. I think added in default values (whether in the attributes or hidden in this library method) changes the behavior so its not a very safe change for people using this cookbook. The logical sense for me was above which would keep the existing behavior of a no op. When one value wasn't specified other than making assumptions of what everyone using this cookbook would want. Maybe @svanzoest should be the one to answer this question?

[svanzoest]

The apache default has always been *:80 with the addition of *:443 when mod_ssl was enabled. So even if you look at c712650 and see that apache.listen_addresses is never set anywhere. The apache binary will make that default to *, even if you leave it off of the Listen directive.

In the majority of cases using * even in <VirtualHost *> should be the default as that generally resolves to the expected name-based behavior. IP-based virtual hosting is rarely needed these days.

[brentm5]

So from what I see we should just let this be nothing by default and the config would have no entry and therefore use the default apache config. If I am understanding you correctly then the above solution that I have provided is correct. If there has to be a value in the config file then what @b-dean stated (copied below) might make sense so that the attributes are not listed and confusing people but we provide the default value apache would use.

  (node['apache']['listen_addresses'] || %w(*)).uniq.each_with_object([]) do |address, listen|
        (node['apache']['listen_ports'] || %w(80)).uniq.each do |port|

[svanzoest]

It feels fine to me also if we don't know of any addresses to just set the default to *., so we end up with *:80 and *:443 by default.

[brentm5]

@svanzoest Assumed you meant adding some sane defaults in the library instead of in the attributes like @b-dean suggested. Went ahead and made those changes and added a comment because its sort of hidden by not being in the attributes.

[brentm5]

@svanzoest does this look good from your perspective / can be merged or are there other things I should change before that point?

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.