fix(fb_apache): render arrays inside of hash configs

[ericnorris]

Description

This PR fixes a bug where the template_hash_handler in fb_apache silently skips over Array values. Now these values are rendered as if they were in the top-level VirtualHost directive.

For example:

{
  ...
  'Location /foo' => {
    'Header' => [
      'unset Bar',
      'unset Baz',
    ],
    ...

...will now properly include multiple Header ... lines in the Location /foo directive.

Impact

These values were ignored previously, so it is possible that it would cause changes in hosts where this was happening.

[facebook-github-bot]

Hi @ericnorris!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[jaymzh]

Thanks for contributing!

This is interesting. It doesn't handle hashes because of https://github.com/facebook/chef-cookbooks/blob/main/cookbooks/fb_apache/templates/default/apache_conf.erb#L4 where we handle arrays. However, if there's an array embedded in a Hash, you're right, we don't handle it.

So, let's DRY this up a bit. Update apache_conf.erb to call this function if it's array or a hash, and rename the method to template_data_handler (or if you have a better name, I'm open).

And finally, you'll need to sign the CLA.

[ericnorris]

Hey @jaymzh, thanks for the quick review! I hope you don't mind but I went a step further and moved all of the template logic into the function. This had the added benefit of allowing the _rewrites config in both the virtual host and directory contexts; if I understand correctly it could only be inside a directory context previously.

Edit: I'm still looking into the CLA on my side, waiting for some information from legal.

[jaymzh]

Can you even have rewrites inside a directory? I thought they only worked inside a VirtualHost

[ericnorris]

I believe so! I tested this locally, and per https://httpd.apache.org/docs/current/mod/mod_rewrite.html#rewriterule:

Context: server config, virtual host, directory, .htaccess

Also, if I understand correctly my change actually enables this for virtual hosts, whereas before it was only allowed inside a directory. This is because the top-level apache_conf.erb didn't handle the _rewrite key, so it could only be called when you had specified a Location or Directory sub-hash, since those were processed by the template_hash_handler.

Edit: I reread the original apache_conf.erb, and now I think that my understanding about how it worked before was wrong; if in the top-level you had _rewrites it would call the template_hash_handler because it was a hash, and that would call the template_rewrite_helper. It actually also semi-worked if you had it inside of a Location or Directory sub-hash, but the indentation was hardcoded to 1 and would have looked funny. Either way, it works cleanly now in both contexts, and according to mod_rewrite it's supported in both.

[jaymzh]

Yeah I was gonna say, it definitely worked at the top level, we use it at SCALE:

https://github.com/socallinuxexpo/scale-chef/blob/main/cookbooks/scale_apache/recipes/default.rb#L203

[jaymzh]

This looks reasonable, but you need to rebase. I kicked off the tests... I know a lot are broken (sorry), but lets make sure any Apache ones pass.

[ericnorris]

@jaymzh I'm still waiting on getting the CLA signed, apologies for that.

That said, I've rebased the branch and fixed a rubocop lint check (the copy-pasted logic was looking at Fixnum instead of Integer, which was caught by the lint). All of the other failing tests appear unrelated to my change; while there is something about fb_apache in the ruby (2.6) check, I've seen that failing in other builds (e.g. the current commit on main: https://github.com/facebook/chef-cookbooks/actions/runs/4640529922/jobs/8212557841).

[jaymzh]

LGTM. Once you've signed the CLA, someone at Meta can pull this in internally, test it, and merge it.

[ericnorris]

I'm still stuck on trying to get the CLA signed; we already had an existing CLA and we've sent an email to [email protected] to update it but have not yet received a response. We're interested in potentially submitting other PRs in the future so we're motivated to figure this out.

[jaymzh]

Hey @davide125 or @joshuamiller01 - maybe one of you can poke the opensource folks on the status of Eric's request?

[dafyddcrosby]

I'm still stuck on trying to get the CLA signed; we already had an existing CLA and we've sent an email to [email protected] to update it but have not yet received a response. We're interested in potentially submitting other PRs in the future so we're motivated to figure this out.

Taking a look into the CLA issue now - did you try https://code.facebook.com/cla as well?

[ericnorris]

Thanks @jaymzh and @dafyddcrosby! I was considering signing a separate CLA, but it seems that someone responded soon after I left the earlier GitHub message. I believe my coworkers and myself are now squared away with the CLA, although I see the check hasn't yet updated.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[facebook-github-bot]

@dafyddcrosby has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.