Skip to content

fix: confusion when substituting ENV in config file#11545

Merged
nic-chen merged 4 commits into
apache:masterfrom
zhoujiexiong:fix-11121
Sep 12, 2024
Merged

fix: confusion when substituting ENV in config file#11545
nic-chen merged 4 commits into
apache:masterfrom
zhoujiexiong:fix-11121

Conversation

@zhoujiexiong

Copy link
Copy Markdown
Contributor

Description

Fixes #11121
Fixes #11239

  1. Import environment variables with the same prefix to Nginx using assignment style
  2. In the init_by_lua phase
  3. There will be confusion issues
use Test::Nginx::Socket 'no_plan';

master_on();
repeat_each(1);

our $prefix = "prefix";
our $longer = "longer";
$ENV{TEST_ENV_SAME_PREFIX} = $prefix;
$ENV{TEST_ENV_SAME_PREFIX_LONGER} = $longer;

add_block_preprocessor(sub {
    my ($block) = @_;

    if (!$block->request) {
        $block->set_value("request", "GET /t");
    }

    my $config = $block->config // "";
    $config .= <<_EOC_;
    location = /t {
    	content_by_lua_block {
            local prefix = os.getenv("TEST_ENV_SAME_PREFIX")                 
            local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
            assert(prefix == "$::prefix" and longer == "$::longer")
        }
	}
_EOC_

    $block->set_value("config", $config);
});

run_tests();

__DATA__

=== TEST 1: assignment style, the PREFIX 1st - incorrect
--- main_config eval
qq/
env TEST_ENV_SAME_PREFIX=$::prefix;
env TEST_ENV_SAME_PREFIX_LONGER=$::longer;
/
--- http_config eval
qq/
    init_by_lua_block {
        local prefix = os.getenv("TEST_ENV_SAME_PREFIX")                 
        local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
        assert(prefix == "$::prefix" and longer == "$::prefix")
    }
/



=== TEST 2: assignment style, the LONGER 1st - correct
--- main_config eval
qq/
env TEST_ENV_SAME_PREFIX_LONGER=$::longer;
env TEST_ENV_SAME_PREFIX=$::prefix;
/
--- http_config eval
qq/
    init_by_lua_block {
        local prefix = os.getenv("TEST_ENV_SAME_PREFIX")                 
        local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
        assert(prefix == "$::prefix" and longer == "$::longer")
    }
/

    

=== TEST 3: declaration style, the LONGER 1st - correct
--- main_config
env TEST_ENV_SAME_PREFIX_LONGER;
env TEST_ENV_SAME_PREFIX;
--- http_config eval
qq/
    init_by_lua_block {
        local prefix = os.getenv("TEST_ENV_SAME_PREFIX")                 
        local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
        assert(prefix == "$::prefix" and longer == "$::longer")
    }
/
    


=== TEST 4: declaration style, the PREFIX 1st - also correct
--- main_config
env TEST_ENV_SAME_PREFIX;
env TEST_ENV_SAME_PREFIX_LONGER;
--- http_config eval    
qq/
    init_by_lua_block {
        local prefix = os.getenv("TEST_ENV_SAME_PREFIX")                 
        local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
        assert(prefix == "$::prefix" and longer == "$::longer")
    }
/

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Sep 2, 2024

@shreemaan-abhishek shreemaan-abhishek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but the test cases are sophisticated. I would suggest you to simplify if you can, but after a close look I understood them.

@shreemaan-abhishek shreemaan-abhishek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread t/cli/test_main.sh
make init

if ! grep "env TEST_ENV=very-long-domain-with-many-symbols.absolutely-non-exists-123ss.com:1234/path?param1=value1;" conf/nginx.conf > /dev/null; then
if ! grep "env TEST_ENV;" conf/nginx.conf > /dev/null; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this could lead to a breaking change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, because when rendering nginx.conf, apisix cli decides to insert the relevant env directives based on whether they exist or are valid. The declarative method is sufficient. The final reading still needs to use os.getenv, env FOO=bar; this method is not necessary and can be replaced by the declarative method to ensure compatibility.

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@nic-chen
nic-chen merged commit bffa9c8 into apache:master Sep 12, 2024
shreemaan-abhishek pushed a commit to shreemaan-abhishek/apisix that referenced this pull request Jan 2, 2026
## Description
Fix confusion when substituting ENV in config files.
The PR will be ported from apache#11545
and will cause related test cases to fail, so copy the test case files
to EE first.

   
## Checklist
- [x] I have explained **WHY** we need this PR and **HOW** the problem
it solves
- [x] The title of the PR concisely describes the code modification and
complies with APISIX specifications.
 - [x] I have explained the changes or the new features added to this PR
 - [ ] I have added **e2e test cases** corresponding to this change
 - [ ] I have updated the documentation to reflect this change

**NOTE: This PR will not be reviewed and merged until the contents of
these checklists are completed.**
shreemaan-abhishek pushed a commit to shreemaan-abhishek/apisix that referenced this pull request Jan 2, 2026
## Description
Fix confusion when substituting ENV in config files.
The PR will be ported from apache#11545.

   
## Checklist
- [x] I have explained **WHY** we need this PR and **HOW** the problem
it solves
- [x] The title of the PR concisely describes the code modification and
complies with APISIX specifications.
 - [x] I have explained the changes or the new features added to this PR
 - [x] I have added **e2e test cases** corresponding to this change
 - [ ] I have updated the documentation to reflect this change

**NOTE: This PR will not be reviewed and merged until the contents of
these checklists are completed.**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Configuration errors due to environment variables sharing prefixes bug: environment variable values mixed in proxy-rewrite plugin

4 participants