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
8 changes: 8 additions & 0 deletions managed/utils/envvars/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,14 @@ func ParseEnvVars(envs []string) (*models.ChangeSettingsParams, []error, []strin
}
}

// Nomad needs the public address to build the URL agents connect back to, so enabling it
// without one leaves the Nomad server silently not started.
Comment on lines +326 to +327

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The first clause holds — advertise.rpc in managed/services/nomad/server.hcl is the public address, and the server certificate is named after it. It is the second clause I would reword. "Silently" is what this PR removes, and "without one" claims more than the condition below can support: it sees only the environment, so an address already stored in Settings keeps Nomad running while this still fires. 1b8f7b4 already taught the message that distinction; the comment is the last place holding the unhedged version, and it is the version a future reader will trust when tidying the string.

Suggested change
// Nomad needs the public address to build the URL agents connect back to, so enabling it
// without one leaves the Nomad server silently not started.
// Nomad needs the public address to build the URL agents connect back to. Only the
// environment is visible here, so an address held only in Settings warns anyway.

Writing the guard as pointer.GetBool(envSettings.EnableNomad) && pointer.GetString(envSettings.PMMPublicAddress) == "" would help too — it makes this the visible mirror of IsNomadEnabled() rather than a second spelling of the same question.

if envSettings.EnableNomad != nil && *envSettings.EnableNomad &&
(envSettings.PMMPublicAddress == nil || *envSettings.PMMPublicAddress == "") {
warns = append(warns, "PMM_ENABLE_NOMAD is set but PMM_PUBLIC_ADDRESS is not; "+
"Nomad will not start unless a public address is configured in PMM settings")
}

return envSettings, errs, warns
}

Expand Down
69 changes: 69 additions & 0 deletions managed/utils/envvars/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,75 @@ func TestEnvVarValidator(t *testing.T) {
})
}

func TestNomadWithoutPublicAddress(t *testing.T) {
t.Parallel()

const warning = "PMM_ENABLE_NOMAD is set but PMM_PUBLIC_ADDRESS is not; " +
"Nomad will not start unless a public address is configured in PMM settings"

for _, tt := range []struct {
name string
envs []string
expected bool
}{
{
name: "Nomad enabled without public address",
envs: []string{"PMM_ENABLE_NOMAD=1"},
expected: true,
},
{
name: "Nomad enabled with non-numeric truthy value",
envs: []string{"PMM_ENABLE_NOMAD=TRUE"},
expected: true,
},
{
name: "Nomad enabled with empty public address",
envs: []string{"PMM_ENABLE_NOMAD=1", "PMM_PUBLIC_ADDRESS="},
expected: true,
},
{
name: "Nomad enabled with public address",
envs: []string{"PMM_ENABLE_NOMAD=1", "PMM_PUBLIC_ADDRESS=1.2.3.4:5678"},
expected: false,
},
{
name: "Nomad explicitly disabled without public address",
envs: []string{"PMM_ENABLE_NOMAD=0"},
expected: false,
},
{
name: "neither variable set",
envs: []string{"PMM_DATA_RETENTION=72h"},
expected: false,
},
{
name: "only public address set",
envs: []string{"PMM_PUBLIC_ADDRESS=1.2.3.4:5678"},
expected: false,
},
} {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

_, gotErrs, gotWarns := ParseEnvVars(tt.envs)
assert.Nil(t, gotErrs)
if tt.expected {
assert.Contains(t, gotWarns, warning)
} else {
assert.NotContains(t, gotWarns, warning)
}
})
}

t.Run("invalid Nomad value reports an error without the warning", func(t *testing.T) {
t.Parallel()

_, gotErrs, gotWarns := ParseEnvVars([]string{"PMM_ENABLE_NOMAD=maybe"})
assert.Len(t, gotErrs, 1)
assert.NotContains(t, gotWarns, warning)
})
}

func TestRedactSecretEnvVar(t *testing.T) {
t.Parallel()

Expand Down
Loading