From 61ac72dc470c8da9a95f4dcda49cc8ee48180953 Mon Sep 17 00:00:00 2001 From: Anthony TREUILLIER Date: Fri, 28 Aug 2026 08:29:03 +0200 Subject: [PATCH 1/2] feat: Add specific healthcheck to bind healthcheckNodePort feature on Kubernetes Refs: MK8S-286 Signed-off-by: Anthony TREUILLIER --- DESIGN.md | 9 +- README.md | 12 +- pkg/domain/types.go | 14 +- .../configgenerator/keepalived.go | 2 +- .../configgenerator/keepalived.tmpl | 18 ++- tests/integration/generate_config_test.go | 59 +++++++++ tests/integration/inputData.go | 43 ++++++ tests/integration/outputData.go | 123 +++++++++++++++++- 8 files changed, 262 insertions(+), 18 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 5fedda5..18dec85 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -48,7 +48,8 @@ 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" + (`ip`, `node`, `vrId`). The optional `healthcheck` / `healthcheckNodePort` fields are pointers so + `CleanHealthchecks` can normalize a present-but-empty key to absent. The metadata pointer fields (`apiVersion`, `kind`) distinguish "absent" from "empty" during validation. Also holds the `EXPECTED_KIND` and `SUPPORTED_API_VERSION` validation constants. - `errors.go` — sentinel errors (`ErrInputFileReading`, `ErrInputFileParsing`, @@ -109,10 +110,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_` 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 diff --git a/README.md b/README.md index 14d487b..393e129 100644 --- a/README.md +++ b/README.md @@ -40,12 +40,17 @@ 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. ## Check script @@ -83,6 +88,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 diff --git a/pkg/domain/types.go b/pkg/domain/types.go index eb1819f..a8e20d4 100644 --- a/pkg/domain/types.go +++ b/pkg/domain/types.go @@ -25,8 +25,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 { @@ -87,8 +88,15 @@ func (v *VirtualIPConfig) Validate() error { 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 + } } diff --git a/pkg/infrastructure/configgenerator/keepalived.go b/pkg/infrastructure/configgenerator/keepalived.go index 653cdd0..da8677c 100644 --- a/pkg/infrastructure/configgenerator/keepalived.go +++ b/pkg/infrastructure/configgenerator/keepalived.go @@ -117,7 +117,7 @@ func (k *Keepalived) ParseInputData(inputData []byte) (*domain.VirtualIPConfig, ) } - parsedInputData.CleanHealthcheck() + parsedInputData.CleanHealthchecks() return parsedInputData, nil } diff --git a/pkg/infrastructure/configgenerator/keepalived.tmpl b/pkg/infrastructure/configgenerator/keepalived.tmpl index fe29a76..90837f3 100644 --- a/pkg/infrastructure/configgenerator/keepalived.tmpl +++ b/pkg/infrastructure/configgenerator/keepalived.tmpl @@ -3,6 +3,7 @@ global_defs { script_user keepalived keepalived } {{- $healthcheck := .Healthcheck }} +{{- $healthcheckNodePort := .HealthCheckNodePort }} {{- if $healthcheck }} vrrp_script check_get { @@ -11,20 +12,33 @@ vrrp_script check_get { weight 60 } {{- end }} +{{- if $healthcheckNodePort }} + +vrrp_script check_get_nodeport { + script "/etc/keepalived/check-get.sh {{ replace $healthcheckNodePort "__NODE_IP__" $.NodeIP }}" + interval 5 + weight 60 +} +{{- end }} {{- range $index, $addr := .Addresses }} {{- $isMaster := eq $addr.Node $.NodeName }} 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 }} } diff --git a/tests/integration/generate_config_test.go b/tests/integration/generate_config_test.go index a9cda7c..9a5d030 100644 --- a/tests/integration/generate_config_test.go +++ b/tests/integration/generate_config_test.go @@ -34,6 +34,27 @@ var _ = Describe("Parse Input Data", func() { Expect(testResource.Healthcheck).To(BeNil()) }) + It("should successfully load a input file with only a healthcheckNodePort", func() { + testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputNodePortOnly) + Expect(err).NotTo(HaveOccurred()) + Expect(testResource.Healthcheck).To(BeNil()) + Expect(testResource.HealthCheckNodePort).NotTo(BeNil()) + Expect(*testResource.HealthCheckNodePort).To(Equal("http://localhost:31846")) + }) + + It("should successfully load a input file with both healthchecks", func() { + testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputBothHealthchecks) + Expect(err).NotTo(HaveOccurred()) + Expect(testResource.Healthcheck).NotTo(BeNil()) + Expect(testResource.HealthCheckNodePort).NotTo(BeNil()) + }) + + It("should successfully load a input file with empty healthcheckNodePort", func() { + testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputEmptyNodePort) + Expect(err).NotTo(HaveOccurred()) + Expect(testResource.HealthCheckNodePort).To(BeNil()) + }) + It("should return an error if the input file is a YAML file but malformed", func() { testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputMalformed) Expect(err).To(HaveOccurred()) @@ -127,6 +148,44 @@ var _ = Describe("Generate Output Configuration", func() { Expect(outputData).To(Equal(outputNoHealthcheck)) }) + It("should generate a check_get_nodeport script when only healthcheckNodePort is set", func() { + By("loading the input file") + testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputNodePortOnly) + Expect(err).NotTo(HaveOccurred()) + Expect(testResource.Healthcheck).To(BeNil()) + Expect(testResource.HealthCheckNodePort).NotTo(BeNil()) + + By("generating the output configuration") + outputData, err := testingSuite.container.GetConfigGenerator().GenerateConfiguration(testResource) + Expect(err).NotTo(HaveOccurred()) + Expect(outputData).To(Equal(outputNodePortOnly)) + }) + + It("should generate both scripts and track both when the two healthchecks are set", func() { + By("loading the input file") + testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputBothHealthchecks) + Expect(err).NotTo(HaveOccurred()) + Expect(testResource.Healthcheck).NotTo(BeNil()) + Expect(testResource.HealthCheckNodePort).NotTo(BeNil()) + + By("generating the output configuration") + outputData, err := testingSuite.container.GetConfigGenerator().GenerateConfiguration(testResource) + Expect(err).NotTo(HaveOccurred()) + Expect(outputData).To(Equal(outputBothHealthchecks)) + }) + + It("should not generate any track_script section when no healthcheck is set", func() { + By("loading the input file") + testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputNoHealthcheck) + Expect(err).NotTo(HaveOccurred()) + + By("generating the output configuration") + outputData, err := testingSuite.container.GetConfigGenerator().GenerateConfiguration(testResource) + Expect(err).NotTo(HaveOccurred()) + Expect(outputData).NotTo(ContainSubstring("track_script")) + Expect(outputData).NotTo(ContainSubstring("vrrp_script")) + }) + It("should return an error if no interfaces are found", func() { By("loading the input file") testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputNoMatchingInterfaces) diff --git a/tests/integration/inputData.go b/tests/integration/inputData.go index bcd121f..ede0fc5 100644 --- a/tests/integration/inputData.go +++ b/tests/integration/inputData.go @@ -16,6 +16,49 @@ addresses: healthcheck: https://__NODE_IP__:443/healthz `) +var inputNodePortOnly = []byte(`--- +apiVersion: loadbalancer.scality.com/v1alpha1 +kind: VirtualIPConfiguration +addresses: +- ip: 172.17.0.15 + node: bootstrap + vrId: 51 +- ip: 172.17.0.16 + node: node1 + vrId: 52 +- ip: 172.17.0.17 + node: node2 + vrId: 53 +healthcheckNodePort: http://localhost:31846 +`) + +var inputBothHealthchecks = []byte(`--- +apiVersion: loadbalancer.scality.com/v1alpha1 +kind: VirtualIPConfiguration +addresses: +- ip: 172.17.0.15 + node: bootstrap + vrId: 51 +- ip: 172.17.0.16 + node: node1 + vrId: 52 +- ip: 172.17.0.17 + node: node2 + vrId: 53 +healthcheck: https://__NODE_IP__:443/healthz +healthcheckNodePort: http://localhost:31846 +`) + +var inputEmptyNodePort = []byte(`--- +apiVersion: loadbalancer.scality.com/v1alpha1 +kind: VirtualIPConfiguration +addresses: +- ip: 172.17.0.15 + node: bootstrap + vrId: 51 +healthcheckNodePort: +`) + var inputNoHealthcheck = []byte(`--- apiVersion: loadbalancer.scality.com/v1alpha1 kind: VirtualIPConfiguration diff --git a/tests/integration/outputData.go b/tests/integration/outputData.go index 498ac21..71f7968 100644 --- a/tests/integration/outputData.go +++ b/tests/integration/outputData.go @@ -14,7 +14,7 @@ vrrp_script check_get { vrrp_instance VI_1 { state MASTER interface eth0 - priority 150 + priority 130 virtual_router_id 51 virtual_ipaddress { 172.17.0.15 @@ -27,7 +27,7 @@ vrrp_instance VI_1 { vrrp_instance VI_2 { state BACKUP interface eth1 - priority 100 + priority 80 virtual_router_id 52 virtual_ipaddress { 172.17.0.16 @@ -40,7 +40,7 @@ vrrp_instance VI_2 { vrrp_instance VI_3 { state BACKUP interface eth2 - priority 100 + priority 80 virtual_router_id 53 virtual_ipaddress { 172.17.0.17 @@ -59,7 +59,7 @@ var outputNoHealthcheck = `global_defs { vrrp_instance VI_1 { state MASTER interface eth0 - priority 150 + priority 130 virtual_router_id 51 virtual_ipaddress { 172.17.0.15 @@ -69,7 +69,7 @@ vrrp_instance VI_1 { vrrp_instance VI_2 { state BACKUP interface eth1 - priority 100 + priority 80 virtual_router_id 52 virtual_ipaddress { 172.17.0.16 @@ -79,10 +79,121 @@ vrrp_instance VI_2 { vrrp_instance VI_3 { state BACKUP interface eth2 - priority 100 + priority 80 virtual_router_id 53 virtual_ipaddress { 172.17.0.17 } } ` + +var outputNodePortOnly = `global_defs { + enable_script_security + script_user keepalived keepalived +} + +vrrp_script check_get_nodeport { + script "/etc/keepalived/check-get.sh http://localhost:31846" + interval 5 + weight 60 +} + +vrrp_instance VI_1 { + state MASTER + interface eth0 + priority 130 + virtual_router_id 51 + virtual_ipaddress { + 172.17.0.15 + } + track_script { + check_get_nodeport + } +} + +vrrp_instance VI_2 { + state BACKUP + interface eth1 + priority 80 + virtual_router_id 52 + virtual_ipaddress { + 172.17.0.16 + } + track_script { + check_get_nodeport + } +} + +vrrp_instance VI_3 { + state BACKUP + interface eth2 + priority 80 + virtual_router_id 53 + virtual_ipaddress { + 172.17.0.17 + } + track_script { + check_get_nodeport + } +} +` + +var outputBothHealthchecks = `global_defs { + enable_script_security + script_user keepalived keepalived +} + +vrrp_script check_get { + script "/etc/keepalived/check-get.sh https://1.1.1.1:443/healthz" + interval 5 + weight 60 +} + +vrrp_script check_get_nodeport { + script "/etc/keepalived/check-get.sh http://localhost:31846" + interval 5 + weight 60 +} + +vrrp_instance VI_1 { + state MASTER + interface eth0 + priority 130 + virtual_router_id 51 + virtual_ipaddress { + 172.17.0.15 + } + track_script { + check_get + check_get_nodeport + } +} + +vrrp_instance VI_2 { + state BACKUP + interface eth1 + priority 80 + virtual_router_id 52 + virtual_ipaddress { + 172.17.0.16 + } + track_script { + check_get + check_get_nodeport + } +} + +vrrp_instance VI_3 { + state BACKUP + interface eth2 + priority 80 + virtual_router_id 53 + virtual_ipaddress { + 172.17.0.17 + } + track_script { + check_get + check_get_nodeport + } +} +` From 6f98b6e1191a3cdc6fca2d6242e74ef643d30c8b Mon Sep 17 00:00:00 2001 From: Anthony TREUILLIER Date: Fri, 28 Aug 2026 08:50:21 +0200 Subject: [PATCH 2/2] feat: Add healthcheck entries validation for hardening Refs: MK8S-286 Signed-off-by: Anthony TREUILLIER --- DESIGN.md | 25 +++++---- README.md | 8 ++- cmd/config/environment.go | 10 ++++ pkg/domain/types.go | 54 +++++++++++++++++++ .../configgenerator/keepalived.go | 12 +++-- .../configgenerator/keepalived.tmpl | 4 +- tests/integration/generate_config_test.go | 42 +++++++++++++++ tests/integration/inputData.go | 50 +++++++++++++++++ tests/integration/outputData.go | 25 +++++++++ 9 files changed, 214 insertions(+), 16 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 18dec85..c117494 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -49,9 +49,12 @@ Pure data and error definitions, no behavior and no outward dependencies: - `types.go` — `VirtualIPConfig` (apiVersion, kind, addresses, healthcheck) and `Address` (`ip`, `node`, `vrId`). The optional `healthcheck` / `healthcheckNodePort` fields are pointers so - `CleanHealthchecks` can normalize a present-but-empty key to absent. The metadata pointer fields (`apiVersion`, `kind`) distinguish "absent" - from "empty" during validation. Also holds the `EXPECTED_KIND` and `SUPPORTED_API_VERSION` - validation constants. + `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. @@ -74,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`). @@ -94,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 diff --git a/README.md b/README.md index 393e129..a5eba70 100644 --- a/README.md +++ b/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 @@ -51,6 +51,9 @@ healthcheckNodePort: http://localhost:31846 # optional; probes a local NodePo `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 @@ -66,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 ` (required) and `-output ` (defaults to stdout). ## Usage diff --git a/cmd/config/environment.go b/cmd/config/environment.go index 466972e..469cc82 100644 --- a/cmd/config/environment.go +++ b/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"), diff --git a/pkg/domain/types.go b/pkg/domain/types.go index a8e20d4..6d54e0c 100644 --- a/pkg/domain/types.go +++ b/pkg/domain/types.go @@ -1,7 +1,9 @@ package domain import ( + "net/url" "slices" + "strings" "github.com/scality/go-errors" ) @@ -9,8 +11,14 @@ import ( 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"` @@ -85,6 +93,52 @@ 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 } diff --git a/pkg/infrastructure/configgenerator/keepalived.go b/pkg/infrastructure/configgenerator/keepalived.go index da8677c..b427524 100644 --- a/pkg/infrastructure/configgenerator/keepalived.go +++ b/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.CleanHealthchecks() - return parsedInputData, nil } diff --git a/pkg/infrastructure/configgenerator/keepalived.tmpl b/pkg/infrastructure/configgenerator/keepalived.tmpl index 90837f3..487b33d 100644 --- a/pkg/infrastructure/configgenerator/keepalived.tmpl +++ b/pkg/infrastructure/configgenerator/keepalived.tmpl @@ -7,7 +7,7 @@ global_defs { {{- 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 } @@ -15,7 +15,7 @@ vrrp_script check_get { {{- if $healthcheckNodePort }} vrrp_script check_get_nodeport { - script "/etc/keepalived/check-get.sh {{ replace $healthcheckNodePort "__NODE_IP__" $.NodeIP }}" + script "/etc/keepalived/check-get.sh {{ replace $healthcheckNodePort $.NodeIPToken $.NodeIP }}" interval 5 weight 60 } diff --git a/tests/integration/generate_config_test.go b/tests/integration/generate_config_test.go index 9a5d030..1f3bb74 100644 --- a/tests/integration/generate_config_test.go +++ b/tests/integration/generate_config_test.go @@ -55,6 +55,34 @@ var _ = Describe("Parse Input Data", func() { Expect(testResource.HealthCheckNodePort).To(BeNil()) }) + It("should return an error if a healthcheck contains a forbidden character", func() { + testResource, err = testingSuite.container.GetConfigGenerator(). + ParseInputData(inputHealthcheckWithQuote) + Expect(err).To(MatchError(domain.ErrInvalidInputParameter)) + Expect(testResource).To(BeNil()) + }) + + It("should return an error if a healthcheck is not an http(s) URL", func() { + testResource, err = testingSuite.container.GetConfigGenerator(). + ParseInputData(inputHealthcheckBadScheme) + Expect(err).To(MatchError(domain.ErrInvalidInputParameter)) + Expect(testResource).To(BeNil()) + }) + + It("should return an error if a healthcheck has no host", func() { + testResource, err = testingSuite.container.GetConfigGenerator(). + ParseInputData(inputHealthcheckNoHost) + Expect(err).To(MatchError(domain.ErrInvalidInputParameter)) + Expect(testResource).To(BeNil()) + }) + + It("should return an error if the healthcheckNodePort is not an http(s) URL", func() { + testResource, err = testingSuite.container.GetConfigGenerator(). + ParseInputData(inputNodePortBadScheme) + Expect(err).To(MatchError(domain.ErrInvalidInputParameter)) + Expect(testResource).To(BeNil()) + }) + It("should return an error if the input file is a YAML file but malformed", func() { testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputMalformed) Expect(err).To(HaveOccurred()) @@ -161,6 +189,20 @@ var _ = Describe("Generate Output Configuration", func() { Expect(outputData).To(Equal(outputNodePortOnly)) }) + It("should substitute __NODE_IP__ in the healthcheckNodePort", func() { + By("loading the input file") + testResource, err = testingSuite.container.GetConfigGenerator(). + ParseInputData(inputNodePortWithNodeIPToken) + Expect(err).NotTo(HaveOccurred()) + Expect(*testResource.HealthCheckNodePort).To(ContainSubstring(domain.NODE_IP_TOKEN)) + + By("generating the output configuration") + outputData, err := testingSuite.container.GetConfigGenerator().GenerateConfiguration(testResource) + Expect(err).NotTo(HaveOccurred()) + Expect(outputData).NotTo(ContainSubstring(domain.NODE_IP_TOKEN)) + Expect(outputData).To(Equal(outputNodePortWithNodeIPToken)) + }) + It("should generate both scripts and track both when the two healthchecks are set", func() { By("loading the input file") testResource, err = testingSuite.container.GetConfigGenerator().ParseInputData(inputBothHealthchecks) diff --git a/tests/integration/inputData.go b/tests/integration/inputData.go index ede0fc5..84f82bd 100644 --- a/tests/integration/inputData.go +++ b/tests/integration/inputData.go @@ -32,6 +32,16 @@ addresses: healthcheckNodePort: http://localhost:31846 `) +var inputNodePortWithNodeIPToken = []byte(`--- +apiVersion: loadbalancer.scality.com/v1alpha1 +kind: VirtualIPConfiguration +addresses: +- ip: 172.17.0.15 + node: bootstrap + vrId: 51 +healthcheckNodePort: http://__NODE_IP__:31846 +`) + var inputBothHealthchecks = []byte(`--- apiVersion: loadbalancer.scality.com/v1alpha1 kind: VirtualIPConfiguration @@ -59,6 +69,46 @@ addresses: healthcheckNodePort: `) +var inputHealthcheckWithQuote = []byte(`--- +apiVersion: loadbalancer.scality.com/v1alpha1 +kind: VirtualIPConfiguration +addresses: +- ip: 172.17.0.15 + node: bootstrap + vrId: 51 +healthcheck: "https://__NODE_IP__:443/heal\"thz" +`) + +var inputHealthcheckBadScheme = []byte(`--- +apiVersion: loadbalancer.scality.com/v1alpha1 +kind: VirtualIPConfiguration +addresses: +- ip: 172.17.0.15 + node: bootstrap + vrId: 51 +healthcheck: ftp://__NODE_IP__/healthz +`) + +var inputHealthcheckNoHost = []byte(`--- +apiVersion: loadbalancer.scality.com/v1alpha1 +kind: VirtualIPConfiguration +addresses: +- ip: 172.17.0.15 + node: bootstrap + vrId: 51 +healthcheck: not-a-url +`) + +var inputNodePortBadScheme = []byte(`--- +apiVersion: loadbalancer.scality.com/v1alpha1 +kind: VirtualIPConfiguration +addresses: +- ip: 172.17.0.15 + node: bootstrap + vrId: 51 +healthcheckNodePort: localhost:31846 +`) + var inputNoHealthcheck = []byte(`--- apiVersion: loadbalancer.scality.com/v1alpha1 kind: VirtualIPConfiguration diff --git a/tests/integration/outputData.go b/tests/integration/outputData.go index 71f7968..55014e2 100644 --- a/tests/integration/outputData.go +++ b/tests/integration/outputData.go @@ -197,3 +197,28 @@ vrrp_instance VI_3 { } } ` + +var outputNodePortWithNodeIPToken = `global_defs { + enable_script_security + script_user keepalived keepalived +} + +vrrp_script check_get_nodeport { + script "/etc/keepalived/check-get.sh http://1.1.1.1:31846" + interval 5 + weight 60 +} + +vrrp_instance VI_1 { + state MASTER + interface eth0 + priority 130 + virtual_router_id 51 + virtual_ipaddress { + 172.17.0.15 + } + track_script { + check_get_nodeport + } +} +`