Skip to content

Commit 1ecc3b0

Browse files
committed
fix(relay-server): make the config report describe the deployment it checks
Two cases where the report could validate a configuration different from the one that would actually run. `--env-file` is documented as inspecting a file instead of the process environment, but it only set the file's own keys. A relay variable absent from the file stayed inherited from the shell, and a higher-priority alias in the shell beat a value the file did supply — process AWS_REGION over file AWS_DEFAULT_REGION. Compose passes only the file, so the report could describe a different deployment than the one being checked. The file is now the whole environment for that pass: every name the deployment understands is cleared first, the file applied, and the previous environment restored afterwards. Inspecting the process environment, where isolation is not the intent, is unchanged. ACME was reported as enabled whenever the provider and credential looked valid, including when PORTAL_URL had a local-only host. acme.NewManager returns before it builds a DNS provider for a local base domain, so managed issuance never runs in that case and a development certificate is used instead. The report now treats a configured provider on a local host as blocked, which is what asking for automation that cannot start should look like. ens-gasless already defers to acmeFeature, so it inherits the state and the reason. Both are the mismatch this report exists to surface, so both are covered by tests that fail when either fix is reverted.
1 parent 3ab2b22 commit 1ecc3b0

2 files changed

Lines changed: 245 additions & 6 deletions

File tree

cmd/relay-server/config.go

Lines changed: 80 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,18 @@ func acmeFeature(cfg relayServerConfig) feature {
123123
return f
124124
}
125125

126+
// acme.NewManager returns before it builds a DNS provider when the base
127+
// domain is local-only, so managed issuance cannot run however well the
128+
// provider is configured. Reporting it as enabled here would describe an
129+
// automation that never starts.
130+
if host := portalURLHost(cfg.PortalURL); host == "" || utils.IsLocalRelayHost(host) {
131+
f.State, f.By = stateBlocked, "ACME_DNS_PROVIDER="+provider
132+
f.Missing = fmt.Sprintf(
133+
"PORTAL_URL host %q is local-only; managed issuance is skipped for local hosts and a development certificate is used instead",
134+
host)
135+
return f
136+
}
137+
126138
required, supported := dnsProviderCredential[provider]
127139
if !supported {
128140
f.State, f.By = stateBlocked, "ACME_DNS_PROVIDER="+provider
@@ -641,6 +653,70 @@ func writeCommentWrapped(w io.Writer, text string) {
641653
}
642654
}
643655

656+
// applyEnvFileInIsolation makes the file the whole environment for the pass
657+
// that follows, and returns a function restoring what was there before.
658+
//
659+
// Setting only the file's own keys is not enough. A relay variable absent from
660+
// the file would stay inherited from the shell, and a higher-priority alias in
661+
// the shell would beat a value the file does supply — process AWS_REGION over
662+
// file AWS_DEFAULT_REGION, for instance. Either way the report would describe a
663+
// configuration different from the one Compose is going to deploy, which is the
664+
// opposite of what checking a file is for.
665+
func applyEnvFileInIsolation(entries []envFileEntry) (func(), error) {
666+
// A first pass populates the registry, which is how the set of names the
667+
// deployment understands is known at all.
668+
if _, err := resolveRelayServerConfig(nil); err != nil {
669+
return nil, err
670+
}
671+
672+
names := make([]string, 0, len(externalEnvVars))
673+
for _, entry := range utils.EnvVars() {
674+
names = append(names, entry.Name)
675+
names = append(names, entry.Aliases...)
676+
}
677+
for name := range externalEnvVars {
678+
names = append(names, name)
679+
}
680+
for _, entry := range entries {
681+
names = append(names, entry.Name)
682+
}
683+
684+
type saved struct {
685+
value string
686+
set bool
687+
}
688+
previous := make(map[string]saved, len(names))
689+
restore := func() {
690+
for name, prior := range previous {
691+
if prior.set {
692+
_ = os.Setenv(name, prior.value)
693+
continue
694+
}
695+
_ = os.Unsetenv(name)
696+
}
697+
}
698+
699+
for _, name := range names {
700+
if _, recorded := previous[name]; recorded {
701+
continue
702+
}
703+
value, set := os.LookupEnv(name)
704+
previous[name] = saved{value: value, set: set}
705+
if err := os.Unsetenv(name); err != nil {
706+
restore()
707+
return nil, fmt.Errorf("isolate %s: %w", name, err)
708+
}
709+
}
710+
711+
for _, entry := range entries {
712+
if err := os.Setenv(entry.Name, entry.Value); err != nil {
713+
restore()
714+
return nil, fmt.Errorf("apply %s: %w", entry.Name, err)
715+
}
716+
}
717+
return restore, nil
718+
}
719+
644720
func runConfigCommand(args []string) error {
645721
var (
646722
envFilePath string
@@ -661,20 +737,18 @@ func runConfigCommand(args []string) error {
661737
return err
662738
}
663739

664-
// Load the file before registering flags: flag defaults resolve from the
665-
// process environment, which is exactly how Compose delivers env_file.
666740
var entries []envFileEntry
667741
source := "process environment"
668742
if strings.TrimSpace(envFilePath) != "" {
669743
loaded, err := loadEnvFile(envFilePath)
670744
if err != nil {
671745
return fmt.Errorf("read env file: %w", err)
672746
}
673-
for _, entry := range loaded {
674-
if err := os.Setenv(entry.Name, entry.Value); err != nil {
675-
return fmt.Errorf("apply %s: %w", entry.Name, err)
676-
}
747+
restore, err := applyEnvFileInIsolation(loaded)
748+
if err != nil {
749+
return err
677750
}
751+
defer restore()
678752
entries = loaded
679753
source = envFilePath
680754
}

cmd/relay-server/config_test.go

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
package main
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
)
9+
10+
// writeEnvFile writes lines to a temporary env file and returns its path.
11+
func writeEnvFile(t *testing.T, lines ...string) string {
12+
t.Helper()
13+
path := filepath.Join(t.TempDir(), "test.env")
14+
body := strings.Join(lines, "\n") + "\n"
15+
if err := os.WriteFile(path, []byte(body), 0o600); err != nil {
16+
t.Fatalf("write env file: %v", err)
17+
}
18+
return path
19+
}
20+
21+
// resolveWithEnvFile runs the same isolation the config subcommand performs and
22+
// returns the resulting configuration.
23+
func resolveWithEnvFile(t *testing.T, path string) relayServerConfig {
24+
t.Helper()
25+
entries, err := loadEnvFile(path)
26+
if err != nil {
27+
t.Fatalf("load env file: %v", err)
28+
}
29+
restore, err := applyEnvFileInIsolation(entries)
30+
if err != nil {
31+
t.Fatalf("isolate env file: %v", err)
32+
}
33+
defer restore()
34+
35+
cfg, err := resolveRelayServerConfig(nil)
36+
if err != nil {
37+
t.Fatalf("resolve config: %v", err)
38+
}
39+
return cfg
40+
}
41+
42+
func featureByName(t *testing.T, cfg relayServerConfig, name string) feature {
43+
t.Helper()
44+
for _, f := range evaluateFeatures(cfg) {
45+
if f.Name == name {
46+
return f
47+
}
48+
}
49+
t.Fatalf("feature %q not reported", name)
50+
return feature{}
51+
}
52+
53+
// A variable absent from the env file must not leak in from the surrounding
54+
// shell: Compose passes only the file, so a report that saw the shell would be
55+
// describing a different deployment.
56+
func TestEnvFileIsolationIgnoresInheritedValue(t *testing.T) {
57+
t.Setenv("DISCOVERY", "true")
58+
59+
cfg := resolveWithEnvFile(t, writeEnvFile(t, "PORTAL_URL=https://relay.example.com"))
60+
61+
if cfg.DiscoveryEnabled {
62+
t.Fatal("DISCOVERY was inherited from the process environment; the file did not set it")
63+
}
64+
}
65+
66+
// A higher-priority alias in the shell must not beat a value the file supplies
67+
// through a lower-priority name.
68+
func TestEnvFileIsolationBeatsHigherPriorityAlias(t *testing.T) {
69+
t.Setenv("AWS_REGION", "us-east-1")
70+
71+
cfg := resolveWithEnvFile(t, writeEnvFile(t, "AWS_DEFAULT_REGION=ap-northeast-2"))
72+
73+
if cfg.AWSRegion != "ap-northeast-2" {
74+
t.Fatalf("AWS region = %q, want the file value ap-northeast-2", cfg.AWSRegion)
75+
}
76+
}
77+
78+
func TestEnvFileIsolationRestoresEnvironment(t *testing.T) {
79+
t.Setenv("DISCOVERY", "true")
80+
if err := os.Unsetenv("BOOTSTRAPS"); err != nil {
81+
t.Fatalf("unset BOOTSTRAPS: %v", err)
82+
}
83+
84+
entries, err := loadEnvFile(writeEnvFile(t, "BOOTSTRAPS=https://seed.example.com"))
85+
if err != nil {
86+
t.Fatalf("load env file: %v", err)
87+
}
88+
restore, err := applyEnvFileInIsolation(entries)
89+
if err != nil {
90+
t.Fatalf("isolate env file: %v", err)
91+
}
92+
restore()
93+
94+
if got := os.Getenv("DISCOVERY"); got != "true" {
95+
t.Fatalf("DISCOVERY = %q after restore, want true", got)
96+
}
97+
if _, set := os.LookupEnv("BOOTSTRAPS"); set {
98+
t.Fatal("BOOTSTRAPS is set after restore; it was unset before")
99+
}
100+
}
101+
102+
// acme.NewManager returns before building a DNS provider for a local-only base
103+
// domain, so a configured provider still yields no managed issuance.
104+
func TestACMEFeatureBlockedForLocalHost(t *testing.T) {
105+
cfg := relayServerConfig{
106+
PortalURL: "https://localhost",
107+
ACMEDNSProvider: "cloudflare",
108+
CloudflareToken: "token",
109+
}
110+
111+
f := featureByName(t, cfg, "acme")
112+
if f.State != stateBlocked {
113+
t.Fatalf("acme state = %q, want %q", f.State, stateBlocked)
114+
}
115+
if !strings.Contains(f.Missing, "local-only") {
116+
t.Fatalf("acme missing = %q, want it to name the local-only host", f.Missing)
117+
}
118+
}
119+
120+
// ens-gasless drives the same provider, so it must inherit the blocked state
121+
// rather than repeating half of the check.
122+
func TestENSGaslessFollowsBlockedACME(t *testing.T) {
123+
cfg := relayServerConfig{
124+
PortalURL: "https://localhost",
125+
ACMEDNSProvider: "cloudflare",
126+
CloudflareToken: "token",
127+
ENSGaslessEnabled: true,
128+
}
129+
130+
f := featureByName(t, cfg, "ens-gasless")
131+
if f.State != stateBlocked {
132+
t.Fatalf("ens-gasless state = %q, want %q", f.State, stateBlocked)
133+
}
134+
if !strings.Contains(f.Missing, "local-only") {
135+
t.Fatalf("ens-gasless missing = %q, want the ACME reason propagated", f.Missing)
136+
}
137+
}
138+
139+
func TestACMEFeatureEnabledForPublicHost(t *testing.T) {
140+
cfg := relayServerConfig{
141+
PortalURL: "https://relay.example.com",
142+
ACMEDNSProvider: "cloudflare",
143+
CloudflareToken: "token",
144+
}
145+
146+
f := featureByName(t, cfg, "acme")
147+
if f.State != stateEnabled {
148+
t.Fatalf("acme state = %q, want %q (missing: %s)", f.State, stateEnabled, f.Missing)
149+
}
150+
}
151+
152+
func TestACMEFeatureBlockedWithoutCredential(t *testing.T) {
153+
cfg := relayServerConfig{
154+
PortalURL: "https://relay.example.com",
155+
ACMEDNSProvider: "cloudflare",
156+
}
157+
158+
f := featureByName(t, cfg, "acme")
159+
if f.State != stateBlocked {
160+
t.Fatalf("acme state = %q, want %q", f.State, stateBlocked)
161+
}
162+
if !strings.Contains(f.Missing, "CLOUDFLARE_TOKEN") {
163+
t.Fatalf("acme missing = %q, want it to name the credential", f.Missing)
164+
}
165+
}

0 commit comments

Comments
 (0)