fix: validate k3s profile names against derived release name

`scheduler-k3s:profiles:add` accepted profile names it could not later derive a valid helm release name from, so a profile was created successfully and only failed on commands with no obvious relationship to it: `profiles:remove` could not delete the profile it created, and a single bad name broke every subsequent node sysctls change, including the global scope and unrelated profiles. Names are now refused at add time against the release name they derive rather than against the profile name alone, and the paths that have to cope with a profile stored before this check keep working instead of erroring.

docs/deployment/schedulers/k3s.md

@@ -204,7 +204,7 @@ dokku scheduler-k3s:profiles:add edge-workers \
   --kubelet-args protect-kernel-defaults=true,eviction-hard=memory.available<200Mi
 ```
 
-Profile names must be alphanumeric, may include internal dashes, cannot start/end with a dash, and must be ≤32 characters. Other than the `--server-ip` flag, all flags used for `scheduler-k3s:cluster:add` are valid for the `scheduler-k3s:profiles:add` command.
+Profile names must be lowercase alphanumeric, may include internal dashes, cannot start/end with a dash, and must be ≤26 characters. The limit is not arbitrary: the name becomes part of the helm release backing that profile's [non-namespaced sysctls](#non-namespaced-sysctls), and helm caps a release name at 53 characters. Other than the `--server-ip` flag, all flags used for `scheduler-k3s:cluster:add` are valid for the `scheduler-k3s:profiles:add` command.
 
 #### scheduler-k3s:profiles:remove
 
@@ -906,6 +906,8 @@ dokku scheduler-k3s:node-sysctls:set --profile edge-workers vm.max_map_count 524
 
 A profile scope inherits everything set globally and overrides it on conflict, so each node is covered by exactly one DaemonSet and no two ever write the same value. Note that the server node created by `scheduler-k3s:initialize` never carries a profile label, so only globally-scoped sysctls reach it.
 
+A profile created by an older Dokku may carry a name outside the [current rules](#adding-profiles), such as one containing uppercase or longer than 26 characters. Dokku cannot name a helm release after such a profile, so it is skipped with a warning and `node-sysctls:set --profile` refuses it. Recreate the profile under a valid name to give it sysctls; `scheduler-k3s:profiles:remove` still removes the old one.
+
 Use `node-sysctls:report` to see the resolved set for every scope.
 
 ```shell

plugins/scheduler-k3s/node_profiles.go

@@ -0,0 +1,75 @@
+package scheduler_k3s
+
+import (
+	"errors"
+	"fmt"
+	"regexp"
+
+	"helm.sh/helm/v3/pkg/chartutil"
+)
+
+// helmMaxReleaseNameLength mirrors the release name cap chartutil.ValidateReleaseName
+// enforces, which helm does not export
+const helmMaxReleaseNameLength = 53
+
+// maxNodeProfileNameLength is the longest node profile name that still derives a helm
+// release name for its node sysctls chart that helm will accept
+const maxNodeProfileNameLength = helmMaxReleaseNameLength - len(nodeSysctlsProfileReleasePrefix)
+
+// legacyMaxNodeProfileNameLength is the length limit node profiles were created under
+// before their names were validated against the helm release name they derive
+const legacyMaxNodeProfileNameLength = 32
+
+// nodeProfileNamePattern matches the node profile names dokku can derive every downstream
+// name from. It is lowercase-only because a helm release name is, and dokku appends a
+// profile name to a release name prefix verbatim.
+var nodeProfileNamePattern = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`)
+
+// legacyNodeProfileNamePattern matches the node profile names dokku accepted before it
+// validated them against the helm release name they derive
+var legacyNodeProfileNamePattern = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?$`)
+
+// validateNodeProfileName rejects node profile names dokku cannot derive a valid helm
+// release name from. The node sysctls release is the tightest constraint a profile name
+// feeds, so the name is checked against that rather than on its own: a name accepted here
+// but rejected by helm would create a profile that only fails later, on commands with no
+// obvious relationship to it.
+func validateNodeProfileName(profileName string) error {
+	if profileName == "" {
+		return errors.New("Missing profile name")
+	}
+
+	if !nodeProfileNamePattern.MatchString(profileName) {
+		return fmt.Errorf("Invalid profile name, must only contain lowercase alphanumeric characters and dashes and cannot start or end with a dash: %s", profileName)
+	}
+
+	if len(profileName) > maxNodeProfileNameLength {
+		return fmt.Errorf("Profile name is too long, must be at most %d characters: %s", maxNodeProfileNameLength, profileName)
+	}
+
+	if err := chartutil.ValidateReleaseName(getNodeSysctlsReleaseName(profileName)); err != nil {
+		return fmt.Errorf("Invalid profile name %s, unable to derive a node sysctls release name from it: %w", profileName, err)
+	}
+
+	return nil
+}
+
+// validateStoredNodeProfileName checks a node profile name against the rules it may have
+// been stored under. It stays looser than validateNodeProfileName so a profile created
+// before names were validated against their helm release name is still removable by the
+// command that created it.
+func validateStoredNodeProfileName(profileName string) error {
+	if profileName == "" {
+		return errors.New("Missing profile name")
+	}
+
+	if !legacyNodeProfileNamePattern.MatchString(profileName) {
+		return fmt.Errorf("Invalid profile name, must only contain alphanumeric characters and dashes and cannot start or end with a dash: %s", profileName)
+	}
+
+	if len(profileName) > legacyMaxNodeProfileNameLength {
+		return fmt.Errorf("Profile name is too long, must be at most %d characters: %s", legacyMaxNodeProfileNameLength, profileName)
+	}
+
+	return nil
+}

plugins/scheduler-k3s/node_profiles_test.go

@@ -0,0 +1,99 @@
+package scheduler_k3s
+
+import (
+	"strings"
+	"testing"
+
+	"helm.sh/helm/v3/pkg/chartutil"
+)
+
+func TestValidateNodeProfileName(t *testing.T) {
+	tests := []struct {
+		name        string
+		profileName string
+		wantErr     bool
+	}{
+		{name: "typical name", profileName: "edge-workers"},
+		{name: "single character", profileName: "a"},
+		{name: "digits", profileName: "pool0"},
+		{name: "maximum length", profileName: strings.Repeat("a", maxNodeProfileNameLength)},
+		{name: "empty", profileName: "", wantErr: true},
+		{name: "uppercase", profileName: "EdgePool", wantErr: true},
+		{name: "one over the maximum length", profileName: strings.Repeat("a", maxNodeProfileNameLength+1), wantErr: true},
+		{name: "twenty seven characters", profileName: "a-twenty-seven-char-profile", wantErr: true},
+		{name: "leading dash", profileName: "-edge", wantErr: true},
+		{name: "trailing dash", profileName: "edge-", wantErr: true},
+		{name: "dot", profileName: "edge.workers", wantErr: true},
+		{name: "underscore", profileName: "edge_workers", wantErr: true},
+		{name: "space", profileName: "edge workers", wantErr: true},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			err := validateNodeProfileName(test.profileName)
+			if test.wantErr && err == nil {
+				t.Errorf("validateNodeProfileName(%q) = nil, want an error", test.profileName)
+			}
+			if !test.wantErr && err != nil {
+				t.Errorf("validateNodeProfileName(%q) = %v, want nil", test.profileName, err)
+			}
+		})
+	}
+}
+
+// TestMaxNodeProfileNameLengthMatchesHelm asserts the limit validateNodeProfileName
+// reports is the same one helm enforces. Without it the local arithmetic could drift
+// from chartutil.ValidateReleaseName, which is the failure this limit exists to prevent.
+func TestMaxNodeProfileNameLengthMatchesHelm(t *testing.T) {
+	longest := getNodeSysctlsReleaseName(strings.Repeat("a", maxNodeProfileNameLength))
+	if err := chartutil.ValidateReleaseName(longest); err != nil {
+		t.Errorf("chartutil.ValidateReleaseName(%q) = %v, want nil", longest, err)
+	}
+
+	tooLong := getNodeSysctlsReleaseName(strings.Repeat("a", maxNodeProfileNameLength+1))
+	if err := chartutil.ValidateReleaseName(tooLong); err == nil {
+		t.Errorf("chartutil.ValidateReleaseName(%q) = nil, want an error", tooLong)
+	}
+}
+
+// TestValidateStoredNodeProfileNameAcceptsLegacyNames asserts profiles:remove can still
+// address a profile stored before names were validated against their helm release name
+func TestValidateStoredNodeProfileNameAcceptsLegacyNames(t *testing.T) {
+	tests := []struct {
+		name        string
+		profileName string
+		wantErr     bool
+	}{
+		{name: "uppercase", profileName: "EdgePool"},
+		{name: "twenty seven characters", profileName: "a-twenty-seven-char-profile"},
+		{name: "legacy maximum length", profileName: strings.Repeat("a", legacyMaxNodeProfileNameLength)},
+		{name: "empty", profileName: "", wantErr: true},
+		{name: "one over the legacy maximum length", profileName: strings.Repeat("a", legacyMaxNodeProfileNameLength+1), wantErr: true},
+		{name: "underscore", profileName: "edge_workers", wantErr: true},
+		{name: "path traversal", profileName: "../config", wantErr: true},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			err := validateStoredNodeProfileName(test.profileName)
+			if test.wantErr && err == nil {
+				t.Errorf("validateStoredNodeProfileName(%q) = nil, want an error", test.profileName)
+			}
+			if !test.wantErr && err != nil {
+				t.Errorf("validateStoredNodeProfileName(%q) = %v, want nil", test.profileName, err)
+			}
+		})
+	}
+}
+
+// TestVerifyNodeProfileSysctlsSupported asserts node-sysctls:set refuses a legacy profile
+// name rather than storing sysctls the reconcile loop would have to skip
+func TestVerifyNodeProfileSysctlsSupported(t *testing.T) {
+	if err := verifyNodeProfileSysctlsSupported("edge-workers"); err != nil {
+		t.Errorf("verifyNodeProfileSysctlsSupported(\"edge-workers\") = %v, want nil", err)
+	}
+
+	if err := verifyNodeProfileSysctlsSupported("EdgePool"); err == nil {
+		t.Error("verifyNodeProfileSysctlsSupported(\"EdgePool\") = nil, want an error")
+	}
+}

plugins/scheduler-k3s/node_sysctls.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	"github.com/dokku/dokku/plugins/common"
+	"helm.sh/helm/v3/pkg/chartutil"
 )
 
 // NodeSysctlsValues contains the values for a dokku-managed node sysctls helm chart
@@ -45,13 +46,18 @@ func getNodeSysctlsProperty(profileName string) string {
 	return fmt.Sprintf("node-sysctls.profile.%s", profileName)
 }
 
+// nodeSysctlsProfileReleasePrefix prefixes the helm release name of every profile-scoped
+// node sysctls chart. A profile name is appended to it verbatim, so this is what bounds
+// how long a profile name may be.
+const nodeSysctlsProfileReleasePrefix = "dokku-node-sysctls-profile-"
+
 // getNodeSysctlsReleaseName returns the helm release name for a node sysctls scope
 func getNodeSysctlsReleaseName(profileName string) string {
 	if profileName == "" {
 		return "dokku-node-sysctls-global"
 	}
 
-	return fmt.Sprintf("dokku-node-sysctls-profile-%s", profileName)
+	return nodeSysctlsProfileReleasePrefix + profileName
 }
 
 // getNodeSysctls returns the sysctls stored against a single scope
@@ -175,6 +181,11 @@ func CreateOrUpdateNodeSysctls(ctx context.Context) error {
 	}
 
 	for _, scope := range scopes {
+		if err := chartutil.ValidateReleaseName(scope.ReleaseName); err != nil {
+			common.LogWarn(fmt.Sprintf("Skipping node sysctls for %s, its name predates profile name validation and cannot back a helm release: %s", nodeSysctlsScopeLabel(scope.ProfileName), err))
+			continue
+		}
+
 		if len(scope.Sysctls) == 0 {
 			if err := deleteNodeSysctlsRelease(scope.ReleaseName); err != nil {
 				return err
@@ -278,6 +289,11 @@ func installNodeSysctlsChart(ctx context.Context, scope nodeSysctlScope) error {
 // to an empty set. The kernel values it wrote are not reverted; they persist on the
 // affected nodes until those nodes reboot.
 func deleteNodeSysctlsRelease(releaseName string) error {
+	if err := chartutil.ValidateReleaseName(releaseName); err != nil {
+		common.LogDebug(fmt.Sprintf("skipping node sysctls release deletion, helm could never have installed %s: %s", releaseName, err))
+		return nil
+	}
+
 	helmAgent, err := NewHelmAgent(NodeSysctlsNamespace, DeployLogPrinter)
 	if err != nil {
 		return fmt.Errorf("error creating helm agent: %w", err)

plugins/scheduler-k3s/subcommands.go

@@ -9,7 +9,6 @@ import (
 	"net/url"
 	"os"
 	"os/signal"
-	"regexp"
 	"slices"
 	"sort"
 	"strings"
@@ -18,6 +17,7 @@ import (
 	"github.com/dokku/dokku/plugins/common"
 	resty "github.com/go-resty/resty/v2"
 	"github.com/ryanuber/columnize"
+	"helm.sh/helm/v3/pkg/chartutil"
 )
 
 // CommandAnnotationsSet set or clear a scheduler-k3s annotation for an app
@@ -55,6 +55,10 @@ func CommandNodeSysctlsSet(profileName string, key string, value string) error {
 		if err := verifyNodeProfileExists(profileName); err != nil {
 			return err
 		}
+
+		if err := verifyNodeProfileSysctlsSupported(profileName); err != nil {
+			return err
+		}
 	}
 
 	property := getNodeSysctlsProperty(profileName)
@@ -134,6 +138,17 @@ func verifyNodeProfileExists(profileName string) error {
 	return nil
 }
 
+// verifyNodeProfileSysctlsSupported returns an error when a node profile is stored under a
+// name dokku cannot derive a node sysctls release name from. Such a profile predates the
+// validation in profiles:add and must be recreated before it can carry node sysctls.
+func verifyNodeProfileSysctlsSupported(profileName string) error {
+	if err := chartutil.ValidateReleaseName(getNodeSysctlsReleaseName(profileName)); err != nil {
+		return fmt.Errorf("Node profile %s cannot carry node sysctls, recreate it with a name of at most %d lowercase alphanumeric characters and dashes: %w", profileName, maxNodeProfileNameLength, err)
+	}
+
+	return nil
+}
+
 // CommandAutoscalingAuthSet set or clear a scheduler-k3s autoscaling keda trigger authentication object for an app
 func CommandAutoscalingAuthSet(appName string, trigger string, metadata map[string]string, global bool) error {
 	if global {
@@ -1253,18 +1268,8 @@ func CommandProfilesAdd(profileName string, role string, allowUknownHosts bool,
 		return fmt.Errorf("Invalid role: %s", role)
 	}
 
-	if profileName == "" {
-		return fmt.Errorf("Missing profile name")
-	}
-
-	// profile names must only contain alphanumeric characters and dashes and cannot start with a dash
-	if !regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?$`).MatchString(profileName) {
-		return fmt.Errorf("Invalid profile name, must only contain alphanumeric characters and dashes and cannot start with a dash: %s", profileName)
-	}
-
-	// ensure profile names are no longer than 32 characters
-	if len(profileName) > 32 {
-		return fmt.Errorf("Profile name is too long, must be less than 32 characters: %s", profileName)
+	if err := validateNodeProfileName(profileName); err != nil {
+		return err
 	}
 
 	profile := NodeProfile{
@@ -1338,18 +1343,8 @@ func CommandProfilesList(format string) error {
 
 // CommandProfilesRemove removes a node profile from the k3s cluster
 func CommandProfilesRemove(profileName string) error {
-	if profileName == "" {
-		return fmt.Errorf("Missing profile name")
-	}
-
-	// profile names must only contain alphanumeric characters and dashes and cannot start with a dash
-	if !regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?$`).MatchString(profileName) {
-		return fmt.Errorf("Invalid profile name, must only contain alphanumeric characters and dashes and cannot start with a dash: %s", profileName)
-	}
-
-	// ensure profile names are no longer than 32 characters
-	if len(profileName) > 32 {
-		return fmt.Errorf("Profile name is too long, must be less than 32 characters: %s", profileName)
+	if err := validateStoredNodeProfileName(profileName); err != nil {
+		return err
 	}
 
 	if err := common.PropertyDelete("scheduler-k3s", "--global", fmt.Sprintf("node-profile-%s.json", profileName)); err != nil {

tests/unit/scheduler-k3s-profiles.bats

@@ -0,0 +1,94 @@
+#!/usr/bin/env bats
+
+load test_helper
+
+PROFILE_PROPERTY_PATH="/var/lib/dokku/config/scheduler-k3s/--global"
+
+setup() {
+  global_setup
+}
+
+teardown() {
+  rm -f "$PROFILE_PROPERTY_PATH"/node-profile-*.json || true
+  global_teardown
+}
+
+@test "(scheduler-k3s:profiles:add) rejects names that cannot derive a valid helm release name" {
+  run /bin/bash -c "dokku scheduler-k3s:profiles:add a-twenty-seven-char-profile --role worker"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "Profile name is too long, must be at most 26 characters"
+
+  run /bin/bash -c "dokku scheduler-k3s:profiles:add EdgePool --role worker"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "must only contain lowercase alphanumeric characters"
+
+  run /bin/bash -c "dokku scheduler-k3s:profiles:list --format json | jq -r '. | length'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "0"
+}
+
+@test "(scheduler-k3s:profiles:add) accepts a name at the maximum length" {
+  run /bin/bash -c "dokku scheduler-k3s:profiles:add aaaaaaaaaaaaaaaaaaaaaaaaaa --role worker"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku scheduler-k3s:profiles:list --format json | jq -r '.[0].name'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "aaaaaaaaaaaaaaaaaaaaaaaaaa"
+
+  run /bin/bash -c "dokku scheduler-k3s:profiles:remove aaaaaaaaaaaaaaaaaaaaaaaaaa"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku scheduler-k3s:profiles:list --format json | jq -r '. | length'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "0"
+}
+
+@test "(scheduler-k3s:profiles:remove) removes a profile stored under a legacy name" {
+  seed_legacy_node_profile EdgePool
+
+  run /bin/bash -c "dokku scheduler-k3s:profiles:list --format json | jq -r '.[0].name'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "EdgePool"
+
+  run /bin/bash -c "dokku scheduler-k3s:node-sysctls:set --profile EdgePool vm.max_map_count 262144"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "cannot carry node sysctls"
+
+  run /bin/bash -c "dokku scheduler-k3s:profiles:remove EdgePool"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku scheduler-k3s:profiles:list --format json | jq -r '. | length'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "0"
+}
+
+seed_legacy_node_profile() {
+  declare desc="writes a node profile property under a name profiles:add no longer accepts"
+  declare PROFILE_NAME="$1"
+
+  mkdir -p "$PROFILE_PROPERTY_PATH"
+  echo "{\"name\":\"$PROFILE_NAME\",\"role\":\"worker\"}" >"$PROFILE_PROPERTY_PATH/node-profile-$PROFILE_NAME.json"
+  chown dokku:dokku "$PROFILE_PROPERTY_PATH/node-profile-$PROFILE_NAME.json"
+}