Skip to content

Conversation

@bavshin-f5
Copy link
Member

Previously, we would prepend NGX_ACME_STATE_PREFIX to default paths and cycle->prefix to explicitly configured paths. This seems inconsistent and unnecesarily confusing.

Fixes 131a681.

@bavshin-f5 bavshin-f5 requested a review from Copilot October 15, 2025 22:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes inconsistent path prefix handling for the state_path configuration option. Previously, the code applied NGX_ACME_STATE_PREFIX to default paths but used cycle->prefix for explicitly configured paths. Now, relative paths consistently receive the NGX_ACME_STATE_PREFIX regardless of whether they're default or user-configured.

Key Changes:

  • Refactored NGX_ACME_STATE_PREFIX handling into a const function that normalizes trailing slashes
  • Modified cmd_issuer_set_state_path to apply the state prefix to user-configured relative paths before nginx processes them
  • Simplified default_state_path to use the normalized prefix constant

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/conf/issuer.rs Introduces get_state_prefix() const function to normalize the state prefix and updates default path construction to use it
src/conf/ext.rs Adds args_mut() method to enable modification of configuration arguments
src/conf.rs Applies state prefix to user-configured relative paths before nginx's path resolution

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

// Strip all the trailing slashes from the path.
const VAL_1: &str = match VAL_0 {
Some(x) => trim_trailing_slashes(x),
None => "", // unreachable
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

[nitpick] The unreachable branch in line 476 could be replaced with unreachable!() macro to make the intent explicit and provide better compile-time guarantees. This would help prevent future logic errors if the const evaluation changes.

Suggested change
None => "", // unreachable
None => unreachable!(),

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

VAL_0, and VAL_1 seems a bit odd

diff --git a/src/conf/issuer.rs b/src/conf/issuer.rs
index ccd8807..086d4ce 100644
--- a/src/conf/issuer.rs
+++ b/src/conf/issuer.rs
@@ -465,16 +465,16 @@ const fn get_state_prefix() -> Option<&'static str> {
         unsafe { core::str::from_utf8_unchecked(bytes) }
     }
 
-    const VAL_0: Option<&str> = core::option_env!("NGX_ACME_STATE_PREFIX");
-    if VAL_0.is_none() {
+    const RAW_ENV_PREFIX: Option<&str> = core::option_env!("NGX_ACME_STATE_PREFIX");
+    if RAW_ENV_PREFIX.is_none() {
         return None;
     }
 
     // Strip all the trailing slashes from the path.
-    const VAL_1: &str = match VAL_0 {
+    const TRIMMED_ENV_PREFIX: &str = match RAW_ENV_PREFIX {
         Some(x) => trim_trailing_slashes(x),
         None => "", // unreachable
     };
 
-    Some(constcat::concat!(VAL_1, "/"))
+    Some(constcat::concat!(TRIMMED_ENV_PREFIX, "/"))
 }

Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

Otherwise looks good.

// Strip all the trailing slashes from the path.
const VAL_1: &str = match VAL_0 {
Some(x) => trim_trailing_slashes(x),
None => "", // unreachable
Copy link
Contributor

Choose a reason for hiding this comment

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

VAL_0, and VAL_1 seems a bit odd

diff --git a/src/conf/issuer.rs b/src/conf/issuer.rs
index ccd8807..086d4ce 100644
--- a/src/conf/issuer.rs
+++ b/src/conf/issuer.rs
@@ -465,16 +465,16 @@ const fn get_state_prefix() -> Option<&'static str> {
         unsafe { core::str::from_utf8_unchecked(bytes) }
     }
 
-    const VAL_0: Option<&str> = core::option_env!("NGX_ACME_STATE_PREFIX");
-    if VAL_0.is_none() {
+    const RAW_ENV_PREFIX: Option<&str> = core::option_env!("NGX_ACME_STATE_PREFIX");
+    if RAW_ENV_PREFIX.is_none() {
         return None;
     }
 
     // Strip all the trailing slashes from the path.
-    const VAL_1: &str = match VAL_0 {
+    const TRIMMED_ENV_PREFIX: &str = match RAW_ENV_PREFIX {
         Some(x) => trim_trailing_slashes(x),
         None => "", // unreachable
     };
 
-    Some(constcat::concat!(VAL_1, "/"))
+    Some(constcat::concat!(TRIMMED_ENV_PREFIX, "/"))
 }

Previously, we would prepend NGX_ACME_STATE_PREFIX to default paths and
cycle->prefix to explicitly configured paths.  This seems inconsistent
and unnecesarily confusing.

Fixes 131a681.
Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

Looks good.

@bavshin-f5 bavshin-f5 merged commit 07941ef into nginx:main Oct 20, 2025
20 checks passed
@bavshin-f5 bavshin-f5 deleted the state-path branch October 20, 2025 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants