From d5323964c629fd0b362e01ab90015cf8371e19d9 Mon Sep 17 00:00:00 2001
From: Brian Picciano <me@mediocregopher.com>
Date: Thu, 26 Dec 2024 19:36:39 +0100
Subject: [PATCH] Fix default garage ports not being used in 'storage add'

---
 go/cmd/entrypoint/main_test.go                |  4 +--
 go/cmd/entrypoint/storage_allocation_test.go  |  9 ++++--
 go/cmd/entrypoint/vpn_firewall_test.go        |  4 +--
 go/daemon/daecommon/config.go                 | 32 +++++++++++++++----
 ...et-config-dont-commit-new-config-on-err.md |  8 +++++
 tasks/soon/docs/clarify-firewalls.md          | 12 +++++++
 6 files changed, 55 insertions(+), 14 deletions(-)
 create mode 100644 tasks/bugs/set-config-dont-commit-new-config-on-err.md
 create mode 100644 tasks/soon/docs/clarify-firewalls.md

diff --git a/go/cmd/entrypoint/main_test.go b/go/cmd/entrypoint/main_test.go
index 2a825f4..260e7b3 100644
--- a/go/cmd/entrypoint/main_test.go
+++ b/go/cmd/entrypoint/main_test.go
@@ -97,9 +97,7 @@ func (h *runHarness) runAssertErrorContains(
 	t *testing.T, want string, args ...string,
 ) {
 	err := h.run(t, args...)
-	if assert.Error(t, err) {
-		assert.Contains(t, err.Error(), want)
-	}
+	assert.ErrorContains(t, err, want)
 }
 
 func (h *runHarness) assertChangeStaged(
diff --git a/go/cmd/entrypoint/storage_allocation_test.go b/go/cmd/entrypoint/storage_allocation_test.go
index 2180351..83f34d3 100644
--- a/go/cmd/entrypoint/storage_allocation_test.go
+++ b/go/cmd/entrypoint/storage_allocation_test.go
@@ -25,9 +25,12 @@ func TestStorageAllocationAdd(t *testing.T) {
 				"--data-path", "foo", "--meta-path=bar", "--capacity", "1",
 			},
 			wantAlloc: daecommon.ConfigStorageAllocation{
-				DataPath: "foo",
-				MetaPath: "bar",
-				Capacity: 1,
+				DataPath:  "foo",
+				MetaPath:  "bar",
+				Capacity:  1,
+				S3APIPort: 3901,
+				RPCPort:   3900,
+				AdminPort: 3902,
 			},
 		},
 		{
diff --git a/go/cmd/entrypoint/vpn_firewall_test.go b/go/cmd/entrypoint/vpn_firewall_test.go
index e72aff1..524560f 100644
--- a/go/cmd/entrypoint/vpn_firewall_test.go
+++ b/go/cmd/entrypoint/vpn_firewall_test.go
@@ -101,7 +101,7 @@ func TestVPNFirewallAdd(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			var (
 				h      = newRunHarness(t)
-				config daecommon.NetworkConfig
+				config = daecommon.NewNetworkConfig(nil)
 			)
 
 			args := append([]string{"vpn", "firewall", "add"}, test.flags...)
@@ -193,7 +193,7 @@ func TestVPNFirewallCommit(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			var (
 				h      = newRunHarness(t)
-				config daecommon.NetworkConfig
+				config = daecommon.NewNetworkConfig(nil)
 			)
 
 			args := []string{"vpn", "firewall", "commit"}
diff --git a/go/daemon/daecommon/config.go b/go/daemon/daecommon/config.go
index 5c60933..8d3197c 100644
--- a/go/daemon/daecommon/config.go
+++ b/go/daemon/daecommon/config.go
@@ -2,6 +2,7 @@ package daecommon
 
 import (
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"io"
 	"isle/bootstrap"
@@ -70,11 +71,7 @@ type ConfigFirewallRule struct {
 	Code   string   `yaml:"code,omitempty"`
 	Proto  string   `yaml:"proto,omitempty"`
 	Host   string   `yaml:"host,omitempty"`
-	Group  string   `yaml:"group,omitempty"`
 	Groups []string `yaml:"groups,omitempty"`
-	CIDR   string   `yaml:"cidr,omitempty"`
-	CASha  string   `yaml:"ca_sha,omitempty"`
-	CAName string   `yaml:"ca_name,omitempty"`
 }
 
 // ConfigStorageAllocation describes the structure of each storage allocation
@@ -186,7 +183,19 @@ func (c NetworkConfig) Validate() error {
 func (c *NetworkConfig) UnmarshalYAML(n *yaml.Node) error {
 	type wrap NetworkConfig
 	if err := n.Decode((*wrap)(c)); err != nil {
-		return fmt.Errorf("decoding into %T: %w", c, err)
+		return fmt.Errorf("yaml decoding into %T: %w", c, err)
+	}
+
+	c.fillDefaults()
+	return nil
+}
+
+// UnmarshalJSON implements the json.Unmarshaler interface. It will attempt to
+// fill in default values where it can.
+func (c *NetworkConfig) UnmarshalJSON(b []byte) error {
+	type wrap NetworkConfig
+	if err := json.Unmarshal(b, (*wrap)(c)); err != nil {
+		return fmt.Errorf("json decoding into %T: %w", c, err)
 	}
 
 	c.fillDefaults()
@@ -263,7 +272,18 @@ func (c *Config) UnmarshalYAML(n *yaml.Node) error {
 
 	type wrap Config
 	if err := n.Decode((*wrap)(c)); err != nil {
-		return fmt.Errorf("yaml unmarshaling back into Config struct: %w", err)
+		return fmt.Errorf("yaml decoding into %T: %w", c, err)
+	}
+
+	return c.Validate()
+}
+
+// UnmarshalJSON implements the json.Unmarshaler interface. It will attempt to
+// fill in default values where it can.
+func (c *Config) UnmarshalJSON(b []byte) error {
+	type wrap Config
+	if err := json.Unmarshal(b, (*wrap)(c)); err != nil {
+		return fmt.Errorf("json decoding into %T: %w", c, err)
 	}
 
 	return c.Validate()
diff --git a/tasks/bugs/set-config-dont-commit-new-config-on-err.md b/tasks/bugs/set-config-dont-commit-new-config-on-err.md
new file mode 100644
index 0000000..5f2d8a8
--- /dev/null
+++ b/tasks/bugs/set-config-dont-commit-new-config-on-err.md
@@ -0,0 +1,8 @@
+---
+type: task
+---
+
+When SetConfig is called, but ends up erroring, the new config should not end up
+getting stored in the state directory.
+
+This could be tricky if the reload call succeeds but the postInit fails.
diff --git a/tasks/soon/docs/clarify-firewalls.md b/tasks/soon/docs/clarify-firewalls.md
new file mode 100644
index 0000000..3c0eeb6
--- /dev/null
+++ b/tasks/soon/docs/clarify-firewalls.md
@@ -0,0 +1,12 @@
+---
+type: task
+---
+
+The Firewalls doc page should be extra clear that adding the line
+
+```
+-A INPUT --source <network CIDR> --jump ACCEPT
+```
+
+will not expose the host to the network entirely, as the nebula firewall will
+still block all traffic by default.