Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ permissions:

env:
NFPM_VERSION: 'v2.35.3'
GOPROXY: "direct"
GOPROXY: "https://${{ secrets.ARTIFACTORY_USER }}:${{ secrets.ARTIFACTORY_TOKEN }}@azr.artifactory.f5net.com/artifactory/api/go/f5-nginx-go-dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can revert this change, main branch will use the dev goproxy by default


jobs:
proxy-sanity-check:
Expand Down
34 changes: 34 additions & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,40 @@ func Execute(ctx context.Context) error {
func Init(version, commit string) {
setVersion(version, commit)
registerFlags()
checkDeprecatedEnvVars()
}

func checkDeprecatedEnvVars() {
allViperKeys := make(map[string]struct{})
for _, key := range viperInstance.AllKeys() {
allViperKeys[key] = struct{}{}
}

const v3Prefix = EnvPrefix + KeyDelimiter

for _, env := range os.Environ() {
parts := strings.SplitN(env, "=", KeyValueNumber)
if len(parts) != KeyValueNumber {
continue
}
envKey := parts[0]

if !strings.HasPrefix(envKey, v3Prefix) {
continue
}

viperKey := strings.TrimPrefix(envKey, v3Prefix)

viperKey = strings.ToLower(viperKey)

if _, exists := allViperKeys[viperKey]; !exists {
slog.Warn("Detected deprecated or unknown environment variables. "+
"Please update to use the latest environment variables. For more information, visit "+
"https://docs.nginx.com/nginx-one/agent/configure-instances/configuration-overview/.",
"deprecated_env_var", envKey,
)
}
}
}

func RegisterConfigFile() error {
Expand Down
96 changes: 96 additions & 0 deletions internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
package config

import (
"bytes"
_ "embed"
"errors"
"log/slog"
"os"
"path"
"sort"
Expand Down Expand Up @@ -163,6 +165,100 @@ func TestNormalizeFunc(t *testing.T) {
assert.Equal(t, expected, result)
}

type deprecatedEnvVarsTest struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be moved into the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to define it outside as the linter gives error if we have more than 5 parameter in a function call.

name string
expectedLogContent string
unexpectedLogContent string
envVars map[string]string
viperKeys []string
expectWarning bool
}

func TestCheckDeprecatedEnvVars(t *testing.T) {
tests := []deprecatedEnvVarsTest{
{
name: "Test 1: should log warning for deprecated env var",
envVars: map[string]string{
"NGINX_AGENT_SERVER_HOST": "value",
},
viperKeys: []string{"some_other_key"},
expectedLogContent: "NGINX_AGENT_SERVER_HOST",
expectWarning: true,
},
{
name: "Test 2: should not log warning for valid env var",
envVars: map[string]string{
"NGINX_AGENT_LOG_LEVEL": "info",
},
viperKeys: []string{"log_level"},
unexpectedLogContent: "NGINX_AGENT_LOG_LEVEL",
expectWarning: false,
},
{
name: "Test 3: should handle mixed valid and deprecated env vars",
envVars: map[string]string{
"NGINX_AGENT_LOG_LEVEL": "info",
"NGINX_AGENT_DEPRECATED_VAR": "value",
},
viperKeys: []string{"log_level"},
expectedLogContent: "NGINX_AGENT_DEPRECATED_VAR",
unexpectedLogContent: "NGINX_AGENT_LOG_LEVEL",
expectWarning: true,
},
{
name: "Test 4: should ignore non-agent env vars",
envVars: map[string]string{
"NGINX_LICENSE": "value",
},
viperKeys: []string{},
expectWarning: false,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
runDeprecatedEnvVarsTest(t, tc)
})
}
}

func runDeprecatedEnvVarsTest(t *testing.T, tc deprecatedEnvVarsTest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this instead just be done inside the unit tests instead of splitting it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That increases the Cyclomatic complexity of the Test function.

t.Helper()

originalViper := viperInstance
viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter))
defer func() { viperInstance = originalViper }()

for key, value := range tc.envVars {
t.Setenv(key, value)
}

for _, key := range tc.viperKeys {
viperInstance.Set(key, "any-value")
}

var logBuffer bytes.Buffer
handler := slog.NewTextHandler(&logBuffer, nil)
slog.SetDefault(slog.New(handler))

checkDeprecatedEnvVars()

logOutput := logBuffer.String()

if tc.expectWarning {
require.NotEmpty(t, logOutput, "Expected a warning log, but got none")
assert.Contains(t, logOutput, "Detected deprecated or unknown environment variables")
if tc.expectedLogContent != "" {
assert.Contains(t, logOutput, tc.expectedLogContent)
}
if tc.unexpectedLogContent != "" {
assert.NotContains(t, logOutput, tc.unexpectedLogContent)
}
} else {
assert.Empty(t, logOutput, "Expected no warning logs")
}
}

func TestResolveAllowedDirectories(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading