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

include apache::mod::alias in foreman_proxy_content::pub_dir #409

Merged
merged 2 commits into from
Apr 25, 2022

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Apr 4, 2022

Part of the effort that was originally theforeman/foreman-installer#754

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'd like to hold off on this until #326 (included in #405) is merged. That allows for more reliable testing.

@evgeni
Copy link
Member

evgeni commented Apr 5, 2022

#326 is merged, so you can go ahead here

@evgeni
Copy link
Member

evgeni commented Apr 5, 2022

I did re-kick the tests, and at least this doesn't seem to break anything? :)

@ekohl
Copy link
Member

ekohl commented Apr 5, 2022

Interesting:

   Info: /Stage[main]/Apache/File[/etc/httpd/conf.modules.d/00-systemd.conf]: Filebucketed /etc/httpd/conf.modules.d/00-systemd.conf to puppet with sum fd94264cd695af2ad86e7715c10e285d
  Notice: /Stage[main]/Apache/File[/etc/httpd/conf.modules.d/00-systemd.conf]/ensure: removed

This may actually something we do want as a base module.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Sorry for the spam, but looking closer: I do wonder, where does the alias come from. Looking at the contents here I don't see what creates it. It's actually in templates/httpd_pub.erb so it should be defined close to that. So manifests/pub_dir.pp is the correct file to place it in.

@wbclark
Copy link
Contributor Author

wbclark commented Apr 19, 2022

Even after working through #411 I've still been struggling to determine why in my testing, when passing apache::default_mods: false the list of loaded modules still looks like:

# httpd -M
Loaded Modules:
 core_module (static)
 so_module (static)
 http_module (static)
 alias_module (shared)
 authz_core_module (shared)
 authz_host_module (shared)
 filter_module (shared)
 headers_module (shared)
 log_config_module (shared)
 mime_module (shared)
 mpm_prefork_module (shared)
 proxy_module (shared)
 proxy_http_module (shared)
 socache_shmcb_module (shared)
 ssl_module (shared)
 systemd_module (shared)
 unixd_module (shared)

When for other projects I've been through this process already and was able to trim it down to an initially smaller list and build that back up by including specific modules as required by the configuration.

It finally occurred to me to check again what this looks when omitting the apache::default_modules parameter in hieradata, and in fact the list is quite a bit longer even in that case.

So the good news is that although the behavior is different from what I expected, my hieradata isn't actually being ignored. And it seems the explicit include apache::mod::alias is not necessary within the reverse proxy define. I suspect, though I haven't yet confirmed, that the specific parameter values passed to apache::vhost inside foreman_proxy_content::reverse_proxy are responsible for these automatic inclusions.

A good test will be to run spec/acceptance/content_with_foreman_spec.rb rather than spec/acceptance/content_standalone_mirror_spec.rb which would never declare any foreman_proxy_content::reverse_proxy resource. Then I'll be able to determine if any other classes here need to explicitly include any modules (as @ekohl previously suggested)

@ehelms
Copy link
Member

ehelms commented Apr 20, 2022

@wbclark Can you clarify then the state of this PR? Are you performing further tests or is it OK to merge?

@wbclark
Copy link
Contributor Author

wbclark commented Apr 22, 2022

@wbclark Can you clarify then the state of this PR? Are you performing further tests or is it OK to merge?

I'm testing the comparison between the apache modules installed by this module when apache::default_mods: false vs. the apache modules installed by luna when apache::default_mods: false

working around theforeman/forklift#1487 to do that

@wbclark
Copy link
Contributor Author

wbclark commented Apr 25, 2022

I've been having further issues with forklift in testing this, but I believe I've solved the problem. will update when my pipeline completes

@wbclark wbclark changed the title include apache::mod::alias with reverse proxy include apache::mod::alias in foreman_proxy_content::pub_dir Apr 25, 2022
@wbclark
Copy link
Contributor Author

wbclark commented Apr 25, 2022

Testing the latest version with https://github.com/wbclark/forklift/tree/default_mods

To test, first clean up the previous environment if any:

$ for id in $(vagrant global-status --prune 2>&1 | grep $(pwd) | awk '{print $1}') ; do vagrant destroy -f $id ; done

Then run the test (Katello in this example):

$ ansible-playbook pipelines/install_pipeline.yml -e forklift_state=up -e pipeline_os=centos8-stream -e pipeline_type=katello -e pipeline_version=nightly

@wbclark
Copy link
Contributor Author

wbclark commented Apr 25, 2022

Regarding the unit test failures here, I'm unable to reproduce in my environment:

$ SPEC_FACTS_OS=centos-8-x86_64 bundle exec rspec spec/classes/foreman_proxy_content_spec.rb 
..............................

Coverage Report:

Total resources:   20
Touched resources: 5
Resource coverage: 25.00%

Untouched resources:
  Apache::Vhost::Fragment[pulp-https-container]
  Apache::Vhost[rhsm-pulpcore-https-443]
  Apache::Vhost[rhsm-pulpcore-https-8443]
  Bootstrap_rpm[katello-ca-consumer-foo.example.com]
  Class[Foreman_proxy_content::Bootstrap_rpm]
  Class[Foreman_proxy_content::Container]
  Class[Foreman_proxy_content::Dispatch_router]
  Class[Foreman_proxy_content::Params]
  File[/var/www/html/pub/katello-server-ca.crt]
  File[/var/www/html/pub]
  Package[katello-client-bootstrap]
  Package[rpm-build]
  Pulpcore::Apache::Fragment[pub_dir]
  Rhsm_reconfigure_script[/var/www/html/pub/katello-rhsm-consumer]
  Trusted_ca::Ca[katello_server-host-cert]


Finished in 35.8 seconds (files took 5.28 seconds to load)
30 examples, 0 failures

@ehelms
Copy link
Member

ehelms commented Apr 25, 2022

The failures are unrelated to your change. This are result of dropping apipie:cache:index and I forgot that this work around lived in here. I've opened #414 to address it

@ehelms ehelms merged commit 4138967 into theforeman:master Apr 25, 2022
@wbclark
Copy link
Contributor Author

wbclark commented Apr 25, 2022

With this branch, I can now run foreman-installer nightly with apache::default_mods: false and it results in a successful install the following modules enabled for the Katello scenario:

# httpd -M
Loaded Modules:
 core_module (static)
 so_module (static)
 http_module (static)
 alias_module (shared)
 authz_core_module (shared)
 authz_host_module (shared)
 env_module (shared)
 filter_module (shared)
 headers_module (shared)
 log_config_module (shared)
 mime_module (shared)
 mpm_prefork_module (shared)
 proxy_module (shared)
 proxy_http_module (shared)
 proxy_wstunnel_module (shared)
 rewrite_module (shared)
 setenvif_module (shared)
 socache_shmcb_module (shared)
 ssl_module (shared)
 systemd_module (shared)
 unixd_module (shared)

And the following modules for the foreman-proxy-content scenario:

# httpd -M
Loaded Modules:
 core_module (static)
 so_module (static)
 http_module (static)
 alias_module (shared)
 authz_core_module (shared)
 authz_host_module (shared)
 filter_module (shared)
 headers_module (shared)
 log_config_module (shared)
 mime_module (shared)
 mpm_prefork_module (shared)
 proxy_module (shared)
 proxy_http_module (shared)
 socache_shmcb_module (shared)
 ssl_module (shared)
 systemd_module (shared)
 unixd_module (shared)

wbclark added a commit to wbclark/forklift that referenced this pull request Apr 25, 2022
This reverts commit 8dcbeb4. theforeman/puppet-foreman_proxy_content#409 is now merged, so pulling in the custom branch is no longer necessary for this testing.
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.

5 participants