feat(installer): Adds support for configuration via environment variables #906

mfulb · 2024-05-24T20:45:07Z

This PR adds a new feature for the newrelic-install.sh script which will scan environment variables for properly named variables and inject these values into the newrelic.ini file. This will only be available when freshly installing the agent and a fresh newrelic.ini is being created. For the feature to be triggered the environment variableNR_CONFIG_WITH_ENVIRON must be set to a non-empty string.

A manually maintained mapping file (newrelic-install-php-cfg-mappings.php) is used to track the mapping from INI entry name to the matching environment variable name. Whenever INI entries for the agent are added or removed this file must be updated as well.

A new test only API was added to the agent which returns a PHP array containing the INI entry names and the corresponding environment variable names. This list is built dynamically from the INI entry definitions in the agent.

A new test target called verify-inject-script is added which runs two tests:

verifies the agents view of INI entries and environment variables name mapping matching the manually maintained newrelic-install-php-cfg-mappings.php file
verifies the newrelic-install-inject-envvars.php script can properly inject INI values from environment variables

The verify-inject-script was added to the GHA workflow for PR runs so it is checked regularly.

Test API useful for getting all INI entries and matching environment variable names.

Gives a mapping from INI entry name to matching environment variable name.

Start of a script that will inject environment variable config values into a newrelic.ini config file.

newrelic-php-agent-bot · 2024-05-24T20:45:35Z

Test Suite	Status	Result
Multiverse	❌	4/9 passing
SOAK	❌	50/56 passing

codecov-commenter · 2024-05-28T21:03:42Z

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.

Please upload report for BASE (feat/env-configuration@787c147). Learn more about missing BASE report.

Files	Patch %	Lines
agent/php_nrini.c	76.92%	6 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                    @@
##             feat/env-configuration     #906   +/-   ##
=========================================================
  Coverage                          ?   79.00%           
=========================================================
  Files                             ?      193           
  Lines                             ?    27284           
  Branches                          ?        0           
=========================================================
  Hits                              ?    21555           
  Misses                            ?     5729           
  Partials                          ?        0

Flag	Coverage Δ
agent-for-php-7.0	`77.74% <80.00%> (?)`
agent-for-php-7.1	`77.50% <79.31%> (?)`
agent-for-php-7.2	`78.42% <79.31%> (?)`
agent-for-php-7.3	`78.44% <79.31%> (?)`
agent-for-php-7.4	`78.15% <79.31%> (?)`
agent-for-php-8.0	`78.21% <79.31%> (?)`
agent-for-php-8.1	`78.20% <79.31%> (?)`
agent-for-php-8.2	`77.80% <79.31%> (?)`
agent-for-php-8.3	`77.80% <79.31%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Needed all installer files to fit within a single glob so artifacts could be specfiied with simple wildcard.

lavarou · 2024-06-11T14:23:37Z

agent/php_nrini.c

@@ -7,6 +7,7 @@
 #include "php_globals.h"
 #include "php_hash.h"
 #include "php_internal_instrument.h"
+#include "php_nrini.h"


This header is already included on line 12:

Suggested change

#include "php_nrini.h"

lavarou · 2024-06-11T14:24:27Z

agent/newrelic-install.sh

@@ -420,6 +422,9 @@ fi
 #   If this is set, it must be set to a valid New Relic license key, and this
 #   key will be set in the daemon config file.
 #
+#   NOTE: If NR_CONFIG_WITH_ENVIRON is defined and NEW_RELIC_LICENSE is defined
+#         then the value of NEW_RELIC_LICENSE will be used.s


Suggested change

# then the value of NEW_RELIC_LICENSE will be used.s

# then the value of NEW_RELIC_LICENSE will be used instead of NR_INSTALL_KEY.

lavarou · 2024-06-11T14:26:25Z

agent/newrelic-install.sh

-      if sed -e "s/REPLACE_WITH_REAL_KEY/${nrkey}/" "${ilibdir}/scripts/newrelic.ini.template" > "${pi_inidir_cli}/newrelic.ini"; then
+      # check if doing old style injection of just "newrelic.license" or if injecting values from env vars
+      if [ -n "${NR_CONFIG_WITH_ENVIRON}" ]; then
+        if php "${ilibdir}/newrelic-install-inject-envvars.php" "${ilibdir}/scripts/newrelic.ini.template" "${pi_inidir_cli}/newrelic.ini"; then


Shouldn't starting the daemon be disabled here?

lavarou · 2024-06-11T14:35:53Z

tests/newrelic-installer/verify_inject_envvars_script_injection.php

@@ -0,0 +1,84 @@
+<?PHP


Suggested change

<?PHP

<?php

lavarou · 2024-06-11T14:36:02Z

tests/newrelic-installer/verify_inject_envvars_script_mapping.php

@@ -0,0 +1,61 @@
+<?PHP


Suggested change

<?PHP

<?php

bduranleau-nr · 2024-06-25T21:11:27Z

agent/php_nrini.c

+  char* ini_name = NULL;
+  char* env_name = NULL;
+
+  if (ini_entry->module_number != NR_PHP_PROCESS_GLOBALS(our_module_number)) {


are we safe to always assume that ini_entry will never be NULL?

bduranleau-nr · 2024-06-25T21:17:29Z

agent/newrelic-install.sh

+      istat="failed"
+      if [ -n "${istat}" ]; then


if istat is set to "failed" on the line above, won't this statement always resolve to true? Can't we remove this conditional entirely in that case?

Was this istat="failed" is intentional or was it accidentally copied from the else statement above?

mfulb added 14 commits May 22, 2024 16:29

feat(tests): Adds testing API to capture INI entries

6dbbc72

Test API useful for getting all INI entries and matching environment variable names.

feat(install): Adds mapping file for env vars

4b1aa4c

Gives a mapping from INI entry name to matching environment variable name.

feat(install): Start of env var inject script

60b0f93

Start of a script that will inject environment variable config values into a newrelic.ini config file.

chore(build): Adds rules for env var injection script

8ac76b1

feat(tests): Adds script to verify ini/envvar mappings

8943480

chore(install): Adds rules for ini/envvar script

ae77685

feat(install,tests): Adds progress for env var injection

707292e

fix(install): Fixes usage and message

ec92c50

fix(tests): Fixes call to inject script

ddc0144

fix(install): Handles newrelic.ini creation better

4c257e8

chore: Removes debugging output

bd2f729

chore: Removes set -x

bdd9f87

chore: Fixes spacing

341b346

chore: Removes trailing space

9263753

mfulb added 2 commits May 28, 2024 16:34

fix(agent): Uses macros that works with PHP 7.x as well

c1d55c3

chore(testing): Adds env var injection script tests to CI/CD

e5b1583

chore(testing): Removes unneeded code

ae05cd4

mfulb requested review from bduranleau-nr and zsistla May 29, 2024 18:34

mfulb added the ready for review label May 29, 2024

mfulb marked this pull request as ready for review May 29, 2024 18:35

mfulb changed the title ~~feat(installer): Adds support for configuration via environment variables [DRAFT]~~ feat(installer): Adds support for configuration via environment variables May 30, 2024

mfulb added 5 commits June 4, 2024 15:06

fix(agent): Adds make target for installer release

89719ba

fix(install): Adds proper path for running inject script

46a93f5

chore(installer): Renames mapping file to work with GHA limitations

ca0238f

Needed all installer files to fit within a single glob so artifacts could be specfiied with simple wildcard.

chore(installer): Adds renamed mapping file

d484f6c

fix(testing): Improves newrelic-install tests

a10f617

lavarou reviewed Jun 11, 2024

View reviewed changes

tests/newrelic-installer/verify_inject_envvars_script_injection.php

@@ -0,0 +1,84 @@

<?PHP

Copy link

Member

lavarou Jun 11, 2024

Choose a reason for hiding this comment

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

Suggested change

<?PHP

<?php

lavarou reviewed Jun 11, 2024

View reviewed changes

tests/newrelic-installer/verify_inject_envvars_script_mapping.php

@@ -0,0 +1,61 @@

<?PHP

Copy link

Member

lavarou Jun 11, 2024

Choose a reason for hiding this comment

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

Suggested change

<?PHP

<?php

lavarou approved these changes Jun 11, 2024

View reviewed changes

bduranleau-nr reviewed Jun 25, 2024

View reviewed changes

ZNeumann added on hold This issue or pull request is necessary, but better suited for the future and removed ready for review labels Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(installer): Adds support for configuration via environment variables #906

feat(installer): Adds support for configuration via environment variables #906

mfulb commented May 24, 2024 •

edited

Loading

newrelic-php-agent-bot commented May 24, 2024 •

edited

Loading

codecov-commenter commented May 28, 2024 •

edited

Loading

lavarou Jun 11, 2024

lavarou Jun 11, 2024

lavarou Jun 11, 2024

lavarou Jun 11, 2024

lavarou Jun 11, 2024

bduranleau-nr Jun 25, 2024

bduranleau-nr Jun 25, 2024

bduranleau-nr Jun 25, 2024

	# then the value of NEW_RELIC_LICENSE will be used.s
	# then the value of NEW_RELIC_LICENSE will be used instead of NR_INSTALL_KEY.

feat(installer): Adds support for configuration via environment variables #906

Are you sure you want to change the base?

feat(installer): Adds support for configuration via environment variables #906

Conversation

mfulb commented May 24, 2024 • edited Loading

newrelic-php-agent-bot commented May 24, 2024 • edited Loading

codecov-commenter commented May 28, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfulb commented May 24, 2024 •

edited

Loading

newrelic-php-agent-bot commented May 24, 2024 •

edited

Loading

codecov-commenter commented May 28, 2024 •

edited

Loading