MK8S-286: Add nodeport healthchecks and hardening on healthchecks

DESIGN.md

@@ -48,9 +48,13 @@ pkg/domain             ── VirtualIPConfig, Address, sentinel errors
 Pure data and error definitions, no behavior and no outward dependencies:
 
 - `types.go` — `VirtualIPConfig` (apiVersion, kind, addresses, healthcheck) and `Address`
-  (`ip`, `node`, `vrId`). The metadata pointer fields (`apiVersion`, `kind`) distinguish "absent"
-  from "empty" during validation. Also holds the `EXPECTED_KIND` and `SUPPORTED_API_VERSION`
-  validation constants.
+  (`ip`, `node`, `vrId`). The optional `healthcheck` / `healthcheckNodePort` fields are pointers so
+  `CleanHealthchecks` can normalize a present-but-empty key to absent; `Validate` then requires each
+  one that survives to be an `http(s)` URL with a host and free of characters that would break out
+  of the quoted `script "…"` string in the generated config (`NODE_IP_TOKEN` is swapped for a
+  placeholder host before parsing, since it is only resolved at render time). The metadata pointer
+  fields (`apiVersion`, `kind`) distinguish "absent" from "empty" during validation. Also holds the
+  `EXPECTED_KIND`, `SUPPORTED_API_VERSION`, and `NODE_IP_TOKEN` constants.
 - `errors.go` — sentinel errors (`ErrInputFileReading`, `ErrInputFileParsing`,
   `ErrMissingInputParameter`, `ErrInvalidInputParameter`, `ErrTemplating`, `ErrInterfaceNotFound`,
   …) used as wrap targets.
@@ -73,13 +77,16 @@ configured output path, and wraps every failure with context (`WithDetail` / `Wi
 ### `pkg/infrastructure` (adapters)
 
 - `configgenerator/keepalived.go` — the `ConfigGenerator` implementation.
-  - `ParseInputData` unmarshals YAML and validates: `kind` must equal `VirtualIPConfiguration`,
-    `apiVersion` must be in the supported list, `addresses` must be present and non-empty.
+  - `ParseInputData` unmarshals YAML, normalizes the healthchecks (`CleanHealthchecks`), then
+    validates: `kind` must equal `VirtualIPConfiguration`, `apiVersion` must be in the supported
+    list, `addresses` must be present and non-empty, and each healthcheck must be a safe `http(s)`
+    URL. Normalization runs first so an empty healthcheck key reads as absent rather than as an
+    invalid URL.
   - `GenerateConfiguration` renders `keepalived.tmpl` (embedded with `//go:embed`). Template
     helpers (`templateFuncs`) expose `add`, `replace`, and `getInterfaceFromIP` (delegated to the
-    `InterfaceGetter`). The node identity (`NodeIP`, `NodeName`) is passed in via the template
-    data rather than read from the environment by the template; MASTER/BACKUP state and priority
-    are decided by comparing each address's `node` to `NodeName`.
+    `InterfaceGetter`). The node identity (`NodeIP`, `NodeName`) and `NodeIPToken` are passed in
+    via the template data rather than read from the environment by the template; MASTER/BACKUP
+    state and priority are decided by comparing each address's `node` to `NodeName`.
 - `interfacegetter/hostnetwork.go` — the production `InterfaceGetter`; iterates `net.Interfaces()`
   and returns the interface whose configured subnet contains the target IP.
 - `interfacegetter/mock.go` — a static mock used by tests (maps the fixture IPs to `eth0/1/2`).
@@ -93,7 +100,8 @@ configured output path, and wraps every failure with context (`WithDetail` / `Wi
 - `main.go` — parses `-input`/`-output`, loads the environment config, builds the container, and
   runs the use case.
 - `config/environment.go` — loads the `Environment` from env vars via `go-envconfig`. `NODE_IP`
-  and `NODE_NAME` are required; `LOGGER_LOG_LEVEL` defaults to `info`. Also defines
+  and `NODE_NAME` are required, and `NODE_IP` must parse as an IP address (`net.ParseIP`) since it
+  is interpolated into the generated config; `LOGGER_LOG_LEVEL` defaults to `info`. Also defines
   `ApplicationName` and `ApplicationVersion` (injected at build time via `-ldflags`).
 
 ## Error handling
@@ -109,10 +117,12 @@ cause. This keeps failures structured and greppable from the logs.
 generated config. It emits:
 
 - a `global_defs` block with script security enabled;
-- an optional `vrrp_script check_get` block when `healthcheck` is set (probing via
+- an optional `vrrp_script check_get` block when `healthcheck` is set, and an optional
+  `vrrp_script check_get_nodeport` block when `healthcheckNodePort` is set (both probing via
   `/etc/keepalived/check-get.sh`, with `__NODE_IP__` substituted from the `NODE_IP` env var);
 - one `vrrp_instance VI_<n>` per address, with `state`, `priority`, `interface` (resolved from
-  the IP), `virtual_router_id`, and `virtual_ipaddress`.
+  the IP), `virtual_router_id`, and `virtual_ipaddress`, plus a `track_script` block listing the
+  enabled scripts — emitted only when at least one of the two healthchecks is set.
 
 ## Packaging
 

README.md

@@ -22,7 +22,7 @@ The container entrypoint runs two steps:
 2. `exec keepalived …` — starts keepalived against the generated config.
 
 For each address, the node whose `NODE_NAME` matches the address's `node` becomes the VRRP
-`MASTER` (priority 150); every other node is a `BACKUP` (priority 100).
+`MASTER` (priority 130); every other node is a `BACKUP` (priority 80).
 
 ## Input spec
 
@@ -40,12 +40,20 @@ addresses:
     node: node1
     vrId: 52
 healthcheck: https://__NODE_IP__:443/healthz   # optional; __NODE_IP__ is substituted at runtime
+healthcheckNodePort: http://localhost:31846    # optional; probes a local NodePort
 ```
 
 - `addresses` is required and must be non-empty. Each entry needs `ip`, `node`, and `vrId`.
-- `healthcheck` is optional. When set, a `vrrp_script` is added that probes the URL every 5s via
-  `/etc/keepalived/check-get.sh` (shipped from `scripts/check-get.sh`); the `__NODE_IP__` token is
-  replaced with the `NODE_IP` env var.
+- `healthcheck` is optional. When set, a `vrrp_script check_get` is added that probes the URL every
+  5s via `/etc/keepalived/check-get.sh` (shipped from `scripts/check-get.sh`); the `__NODE_IP__`
+  token is replaced with the `NODE_IP` env var.
+- `healthcheckNodePort` is optional and follows the same rules, emitting a
+  `vrrp_script check_get_nodeport`. It is meant to probe a service exposed locally on a NodePort.
+- Each `vrrp_instance` gets a `track_script` block listing the scripts that are enabled; when
+  neither healthcheck is set, no `vrrp_script` and no `track_script` block is generated.
+- Both healthcheck fields are validated at startup: the value must be an `http` or `https` URL with
+  a host, and must not contain characters that would break the generated keepalived config
+  (quotes, whitespace, and shell metacharacters). A key present but left empty counts as absent.
 
 ## Check script
 
@@ -61,6 +69,9 @@ The check script is used by keepalived to check that the local node, where the k
 | `NODE_NAME`        | yes      | —       | This node's name; decides MASTER vs BACKUP.        |
 | `LOGGER_LOG_LEVEL` | no       | `info`  | Log level for the structured (slog) logger.        |
 
+`NODE_IP` must parse as an IP address; it is interpolated into the generated keepalived config, so
+anything else is rejected at startup.
+
 Flags: `-input <path>` (required) and `-output <path>` (defaults to stdout).
 
 ## Usage
@@ -83,6 +94,7 @@ addresses:
   node: node2
   vrId: 53
 healthcheck: https://__NODE_IP__:443/healthz
+healthcheckNodePort: http://localhost:31846
 EOF
 NODE_NAME=bootstrap NODE_IP=1.1.1.1 \
   go run ./cmd -input ./spec.yaml

cmd/config/environment.go

@@ -2,6 +2,7 @@ package config
 
 import (
 	"context"
+	"net"
 
 	"github.com/scality/go-errors"
 	"github.com/scality/virtualip-manager/pkg/domain"
@@ -62,6 +63,15 @@ func (cfg *Environment) Load(ctx context.Context) error {
 		)
 	}
 
+	// NodeIP is interpolated into the generated keepalived config, so reject
+	// anything that is not strictly an IP address.
+	if net.ParseIP(cfg.NodeIP) == nil {
+		return errors.Wrap(domain.ErrInvalidIPAddress,
+			errors.WithDetail("NODE_IP is not a valid IP address"),
+			errors.WithProperty("nodeIP", cfg.NodeIP),
+		)
+	}
+
 	if cfg.NodeName == "" {
 		return errors.Wrap(domain.ErrConfigurationLoading,
 			errors.WithDetail("NODE_NAME environment variable is required"),

pkg/domain/types.go

@@ -1,16 +1,24 @@
 package domain
 
 import (
+	"net/url"
 	"slices"
+	"strings"
 
 	"github.com/scality/go-errors"
 )
 
 var (
 	EXPECTED_KIND         string   = "VirtualIPConfiguration"
 	SUPPORTED_API_VERSION []string = []string{"loadbalancer.scality.com/v1alpha1"}
+	NODE_IP_TOKEN         string   = "__NODE_IP__"
 )
 
+// healthcheckForbiddenChars are characters that would either break out of the
+// quoted `script "…"` string in the generated keepalived config or be
+// interpreted by a shell if keepalived ever falls back to one.
+const healthcheckForbiddenChars = "\"'`$;&|<>\\ \t\n\r"
+
 type Address struct {
 	Ip   string `yaml:"ip"`
 	Node string `yaml:"node"`
@@ -25,8 +33,9 @@ type VirtualIPConfigMetadata struct {
 type VirtualIPConfig struct {
 	VirtualIPConfigMetadata `yaml:",inline"`
 
-	Addresses   []Address `yaml:"addresses,omitempty"`
-	Healthcheck *string   `yaml:"healthcheck,omitempty"`
+	Addresses           []Address `yaml:"addresses,omitempty"`
+	Healthcheck         *string   `yaml:"healthcheck,omitempty"`
+	HealthCheckNodePort *string   `yaml:"healthcheckNodePort,omitempty"`
 }
 
 func (v *VirtualIPConfig) Validate() error {
@@ -84,11 +93,64 @@ func (v *VirtualIPConfig) Validate() error {
 		}
 	}
 
+	if err := validateHealthcheckURL("healthcheck", v.Healthcheck); err != nil {
+		return err
+	}
+
+	if err := validateHealthcheckURL("healthcheckNodePort", v.HealthCheckNodePort); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// validateHealthcheckURL rejects a healthcheck that is not an http(s) URL, or
+// that carries characters unsafe to interpolate into the keepalived config. The
+// NODE_IP_TOKEN is substituted with a placeholder host so the value is
+// parseable before the template resolves it.
+func validateHealthcheckURL(field string, value *string) error {
+	if value == nil {
+		return nil
+	}
+
+	if strings.ContainsAny(*value, healthcheckForbiddenChars) {
+		return errors.Wrap(ErrInvalidInputParameter,
+			errors.WithDetail("healthcheck contains forbidden characters"),
+			errors.WithProperty("field", field),
+			errors.WithProperty("value", *value),
+		)
+	}
+
+	parsed, err := url.Parse(strings.ReplaceAll(*value, NODE_IP_TOKEN, "0.0.0.0"))
+	if err != nil {
+		return errors.Wrap(ErrInvalidInputParameter,
+			errors.WithDetail("healthcheck is not a valid URL"),
+			errors.WithProperty("field", field),
+			errors.WithProperty("value", *value),
+			errors.CausedBy(err),
+		)
+	}
+
+	if (parsed.Scheme != "http" && parsed.Scheme != "https") || parsed.Host == "" {
+		return errors.Wrap(ErrInvalidInputParameter,
+			errors.WithDetail("healthcheck must be an http or https URL with a host"),
+			errors.WithProperty("field", field),
+			errors.WithProperty("value", *value),
+		)
+	}
+
 	return nil
 }
 
-func (v *VirtualIPConfig) CleanHealthcheck() {
+// CleanHealthchecks normalizes the optional healthcheck fields: a key present
+// in the input but left empty is treated as absent, so the template only has
+// to test for nil.
+func (v *VirtualIPConfig) CleanHealthchecks() {
 	if v.Healthcheck != nil && *v.Healthcheck == "" {
 		v.Healthcheck = nil
 	}
+
+	if v.HealthCheckNodePort != nil && *v.HealthCheckNodePort == "" {
+		v.HealthCheckNodePort = nil
+	}
 }

pkg/infrastructure/configgenerator/keepalived.go

@@ -49,8 +49,9 @@ var templateContent string
 // the node identity so the template no longer reads the environment directly.
 type templateData struct {
 	*domain.VirtualIPConfig
-	NodeIP   string
-	NodeName string
+	NodeIP      string
+	NodeName    string
+	NodeIPToken string
 }
 
 // GenerateConfiguration generates the output configuration from the input data.
@@ -70,6 +71,7 @@ func (k *Keepalived) GenerateConfiguration(inputData *domain.VirtualIPConfig) (s
 		VirtualIPConfig: inputData,
 		NodeIP:          k.nodeIP,
 		NodeName:        k.nodeName,
+		NodeIPToken:     domain.NODE_IP_TOKEN,
 	}
 
 	outputData := strings.Builder{}
@@ -109,6 +111,10 @@ func (k *Keepalived) ParseInputData(inputData []byte) (*domain.VirtualIPConfig,
 		)
 	}
 
+	// Normalize before validating so an empty healthcheck key reads as absent
+	// rather than as an invalid URL.
+	parsedInputData.CleanHealthchecks()
+
 	err = parsedInputData.Validate()
 	if err != nil {
 		return nil, errors.Wrap(err,
@@ -117,8 +123,6 @@ func (k *Keepalived) ParseInputData(inputData []byte) (*domain.VirtualIPConfig,
 		)
 	}
 
-	parsedInputData.CleanHealthcheck()
-
 	return parsedInputData, nil
 }
 

pkg/infrastructure/configgenerator/keepalived.tmpl

@@ -3,10 +3,19 @@ global_defs {
     script_user keepalived keepalived
 }
 {{- $healthcheck := .Healthcheck }}
+{{- $healthcheckNodePort := .HealthCheckNodePort }}
 {{- if $healthcheck }}
 
 vrrp_script check_get {
-    script "/etc/keepalived/check-get.sh {{ replace $healthcheck "__NODE_IP__" $.NodeIP }}"
+    script "/etc/keepalived/check-get.sh {{ replace $healthcheck $.NodeIPToken $.NodeIP }}"
+    interval 5
+    weight 60
+}
+{{- end }}
+{{- if $healthcheckNodePort }}
+
+vrrp_script check_get_nodeport {
+    script "/etc/keepalived/check-get.sh {{ replace $healthcheckNodePort $.NodeIPToken $.NodeIP }}"
     interval 5
     weight 60
 }
@@ -17,14 +26,19 @@ vrrp_script check_get {
 vrrp_instance VI_{{ add $index 1 }} {
     state {{ if $isMaster }}MASTER{{ else }}BACKUP{{ end }}
     interface {{ getInterfaceFromIP $addr.Ip }}
-    priority {{ if $isMaster }}150{{ else }}100{{ end }}
+    priority {{ if $isMaster }}130{{ else }}80{{ end }}
     virtual_router_id {{ $addr.VrId }}
     virtual_ipaddress {
         {{ $addr.Ip }}
     }
-    {{- if $healthcheck }}
+    {{- if or $healthcheck $healthcheckNodePort }}
     track_script {
+        {{- if $healthcheck }}
         check_get
+        {{- end }}
+        {{- if $healthcheckNodePort }}
+        check_get_nodeport
+        {{- end }}
     }
     {{- end }}
 }