Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 11, 2025

The nginx_config resource currently uses a single folder_mode property (default '0750') for all directories it creates, including both configuration directories and the log directory. This causes issues with logrotate on Debian-like systems, which expect more permissive log directory permissions to access log files.

Changes Made

This PR introduces three new properties to provide fine-grained control over log directory permissions:

  • log_dir_mode (platform-specific default) - Sets log directory permissions separately from config directories. Defaults to '0750' to match nginx repo package defaults, but uses '0755' for Debian-like platforms
  • log_dir_owner (default: lazy { owner }) - Allows independent log directory ownership
  • log_dir_group (default: lazy { group }) - Allows independent log directory group

Example Usage

Default behavior (platform-aware):

nginx_config 'nginx' do
  action :create
  # log directory will use 0750 on RHEL/CentOS, 0755 on Debian/Ubuntu
end

Custom log directory permissions:

nginx_config 'nginx' do
  log_dir_mode '0750'
  log_dir_owner 'nginx'
  log_dir_group 'nginx'
  action :create
end

Platform-Specific Behavior

The log_dir_mode default is now platform-aware:

  • RHEL/CentOS/Fedora: '0750' (matches nginx repo package defaults)
  • Debian/Ubuntu: '0755' (accommodates logrotate and system requirements)

This reflects the different approaches these distributions take for nginx installation and configuration.

Backward Compatibility

This change is fully backward compatible:

  • Existing cookbooks continue to work without modification
  • Configuration directories still use folder_mode (0750) for security
  • Only the log directory behavior changes, using platform-appropriate defaults

Testing

  • Unit tests verify both default behavior and custom property usage for different platforms
  • Added separate test contexts for Debian and RHEL platforms
  • Integration tests already expected '0755' permissions for log directories on Debian-based systems
  • All syntax validation passes

Fixes #622.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@Copilot Copilot AI changed the title [WIP] Default folder_mode permissions in nginx_config resource may fail logrotate Add fine-grained log directory permissions to nginx_config resource Sep 11, 2025
Copilot finished work on behalf of damacus September 11, 2025 23:17
@Copilot Copilot AI requested a review from damacus September 11, 2025 23:17
@damacus damacus marked this pull request as ready for review September 12, 2025 21:55
@damacus damacus requested a review from a team as a code owner September 12, 2025 21:55
@damacus damacus added the Release: Patch Release to Chef Supermarket as a version patch when merged label Sep 12, 2025
@damacus damacus enabled auto-merge (squash) September 14, 2025 00:59
@damacus damacus disabled auto-merge September 14, 2025 01:44
@bmhughes
Copy link
Contributor

I think this would be best being platform specific as (from distant memory) the cookbook reflects the default install permissions from the nginx repo package and it's the same on Redhat based systems. Debian makes a lot of changes to make nginx work the same way they did with apache.


property :log_dir_mode, String,
description: 'Log directory mode',
default: '0755'
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot This should be platform specific. Using 0750 by default and 0755 for Debian-like platforms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented platform-specific log_dir_mode - defaults to '0750' with '0755' for Debian-like platforms using platform_family?('debian') check. Commit: 82ba2d8

Copy link
Contributor Author

Copilot AI commented Sep 14, 2025

I think this would be best being platform specific as (from distant memory) the cookbook reflects the default install permissions from the nginx repo package and it's the same on Redhat based systems....

Made the log_dir_mode platform-specific as requested. Now defaults to '0750' for consistency with nginx repo package defaults, but uses '0755' for Debian-like platforms to accommodate their specific requirements. Updated tests to verify both platform behaviors. Commit: 82ba2d8

@Copilot Copilot AI changed the title Add fine-grained log directory permissions to nginx_config resource Add platform-specific log directory permissions to nginx_config resource Sep 14, 2025
@Copilot Copilot AI requested a review from bmhughes September 14, 2025 17:38
Copilot finished work on behalf of bmhughes September 14, 2025 17:38
Copy link

Slowest examples

Top 10 slowest examples (0.92 seconds, 26.25% of total time)
Example Description Time in seconds
spec/unit/resources/install_spec.rb:15 nginx_install with default properties with epel source with centos 8 platform is expected to create template "(?-mix:/ohai/plugins/nginx.rb)" 0.22919
spec/unit/resources/install_spec.rb:52 nginx_install with default properties with distro source with ubuntu platform is expected to notify "ohai[nginx]" with action :reload immediately 0.14096
spec/unit/resources/config_spec.rb:17 nginx_config with default properties is expected to create template "/etc/nginx/nginx.conf" 0.07698
spec/unit/resources/config_spec.rb:52 nginx_config with default properties is expected to create directory "/etc/nginx/conf.http.d" 0.07536
spec/unit/resources/install_spec.rb:51 nginx_install with default properties with distro source with ubuntu platform is expected to install package "nginx-full" 0.0729
spec/unit/resources/install_spec.rb:15 nginx_install with default properties with repo source with opensuse platform is expected to create template "(?-mix:/ohai/plugins/nginx.rb)" 0.0718
spec/unit/resources/install_spec.rb:72 nginx_install with default properties with repo source with ubuntu platform is expected to add apt_repository "nginx" 0.06876
spec/unit/resources/install_spec.rb:22 nginx_install with default properties with epel source with amazon platform is expected to notify "ohai[nginx]" with action :reload immediately 0.06607
spec/unit/resources/config_spec.rb:37 nginx_config with default properties is expected to create template "/etc/nginx/conf.http.d/default-site.conf" 0.05926
spec/unit/resources/install_spec.rb:21 nginx_install with default properties with epel source with centos-stream 9 platform is expected to install package "nginx" 0.0589

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release: Patch Release to Chef Supermarket as a version patch when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default folder_mode permissions in nginx_config resource may fail logrotate
3 participants