Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .ainav/config/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ Generated docs: `doc/api_dump/*.md`

Admission-webhook validators implement the `Validator` interface (`validator.go`), are
listed per-CRD in `registry.go`, and get a default severity in `admission/defaults.go`.
Add a rule = implement + register + add to the defaults table. clusterCapacity validators:
Add a rule = implement + register + add to the defaults table.
Pod-spec syntax validators (`*_podspec_syntax.go` + shared `podspec_syntax.go`): k8s
pod-level syntax rules at WekaCluster/WekaClient admission (OP-361). clusterCapacity validators:
`cluster_capacity_chunk_feasibility.go` (greenfield per-FD TLC share ≥ 384 GiB; skipped once the
cluster has TLC-bearing drive containers) and `cluster_capacity_protection.go` (min SW≥3, RL≥2, HS≥0 /
hotSpare optional — the `3+2+0` floor from `allocator.MinProtectionFloor`).
Expand Down
13 changes: 13 additions & 0 deletions charts/weka-operator/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -728,10 +728,23 @@ admissionPolicies:
# # violation is reported per unmet dependency.
# cluster_skip_default_fs: default # strict: warn | relaxed: warn
#
# # Scheduling-related fields (tolerations, nodeSelector, per-role
# # label/annotation maps, failureDomain topology keys/skew, podConfig
# # affinity/topologySpreadConstraints) must have syntax the API server
# # would accept — invalid values are caught at CR apply instead of
# # pod create.
# cluster_podspec_syntax: default # strict: error | relaxed: error
#
# # WekaClient.spec.targetCluster must reference an existing
# # WekaCluster.
# client_target_cluster_exists: default # strict: error | relaxed: warn
#
# # Scheduling-related fields (tolerations, nodeSelector, csiConfig
# # advanced labels/tolerations) must have syntax the API server
# # would accept — invalid values are caught at CR apply instead of
# # pod create.
# client_podspec_syntax: default # strict: error | relaxed: error
#
# # Decreasing any cores field on a WekaCluster (spec.dynamicTemplate.*Cores)
# # is denied — reducing cores on a running cluster can destabilize
# # active workloads. Applies on Update only; also blocks unsetting an
Expand Down
7 changes: 4 additions & 3 deletions doc/operator/operations/admission-control.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@

## Overview

The operator runs a validating admission webhook for `WekaCluster` and `WekaClient`
resources. On every `kubectl apply` (or Helm/GitOps equivalent) it runs a battery
The operator runs a validating admission webhook for `WekaCluster`, `WekaClient`, and
`WekaContainer` resources. On every `kubectl apply` (or Helm/GitOps equivalent) it runs a battery
of policies and either admits the request, attaches a `kubectl Warning:` line, or
rejects it. The default posture is non-blocking — most policies emit warnings;
rejects it (`WekaContainer` carries update-only policies, e.g. cores decrease).
Comment on lines +5 to +8
The default posture is non-blocking — most policies emit warnings;
only feasibility-breaking specs are rejected.

## Configuration
Expand Down
2 changes: 2 additions & 0 deletions internal/admission/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ var (
"cluster_capacity_protection": {Strict: Error, Relaxed: Error},
"cluster_capacity_chunk_feasibility": {Strict: Error, Relaxed: Error},
"cluster_skip_default_fs": {Strict: Warn, Relaxed: Warn},
"cluster_podspec_syntax": {Strict: Error, Relaxed: Error},
}

wekaClientDefaults = map[string]PolicyDefaults{
"client_target_cluster_exists": {Strict: Error, Relaxed: Warn},
"client_podspec_syntax": {Strict: Error, Relaxed: Error},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relaxed: Error here (and line 20) is defensible — a syntactically invalid podspec can never schedule, so warning about it is pointless. But note the interaction with the grandfathering behaviour: ValidateUpdate short-circuits only while spec is byte-identical (wekacluster.go:57), so the first unrelated edit to an existing bad CR (image bump, core count change) gets denied with a toleration/nodeSelector error the user didn't touch in that apply.

For a relaxed-posture fleet that's a rollout surprise on operator upgrade. Two mitigations worth considering: ship Relaxed: Warn for one release and flip to Error after, or make sure the release note explicitly tells relaxed-posture operators to pre-scan existing CRs (the per-policy override in admissionPolicies is the escape hatch either way — maybe mention it in the note).

}

// Update-only defaults: cores-decrease checks are always Error regardless
Expand Down
40 changes: 40 additions & 0 deletions internal/validation/client_podspec_syntax.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package validation

import (
"context"

wekav1alpha1 "github.com/weka/weka-k8s-api/api/v1alpha1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
"sigs.k8s.io/controller-runtime/pkg/client"
)

// clientPodspecSyntax rejects WekaClients whose scheduling-related fields
// would produce pods the API server rejects at create time (invalid
// toleration keys/enums, label syntax). Pure spec math; see
// podspec_syntax.go for the shared checks.
type clientPodspecSyntax struct{}

func (clientPodspecSyntax) ID() string { return "client_podspec_syntax" }

func (clientPodspecSyntax) Validate(_ context.Context, _ client.Client, obj runtime.Object) field.ErrorList {
wc, ok := obj.(*wekav1alpha1.WekaClient)
if !ok {
return nil
}
spec := field.NewPath("spec")
var errs field.ErrorList

errs = append(errs, validateSimpleTolerations(spec.Child("tolerations"), wc.Spec.Tolerations)...)
errs = append(errs, validateRawTolerations(spec.Child("rawTolerations"), wc.Spec.RawTolerations)...)
errs = append(errs, validateLabelMap(spec.Child("nodeSelector"), wc.Spec.NodeSelector)...)

if csi := wc.Spec.CsiConfig; csi != nil && csi.Advanced != nil {
adv := spec.Child("csiConfig", "advanced")
errs = append(errs, validateLabelMap(adv.Child("nodeLabels"), csi.Advanced.NodeLabels)...)
errs = append(errs, validateLabelMap(adv.Child("controllerLabels"), csi.Advanced.ControllerLabels)...)
errs = append(errs, validateRawTolerations(adv.Child("nodeTolerations"), csi.Advanced.NodeTolerations)...)
errs = append(errs, validateRawTolerations(adv.Child("controllerTolerations"), csi.Advanced.ControllerTolerations)...)
}
return errs
}
88 changes: 88 additions & 0 deletions internal/validation/client_podspec_syntax_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package validation

import (
"context"
"strings"
"testing"

weka "github.com/weka/weka-k8s-api/api/v1alpha1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestClientPodspecSyntax(t *testing.T) {
v := clientPodspecSyntax{}
base := func() *weka.WekaClient {
return &weka.WekaClient{ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}}
}

if errs := v.Validate(context.Background(), nil, base()); len(errs) != 0 {
t.Fatalf("empty spec should pass: %v", errs)
}

badTol := base()
badTol.Spec.Tolerations = []string{"scitix.ai/nodecheck:NoSchedule"}
if errs := v.Validate(context.Background(), nil, badTol); len(errs) != 1 {
t.Fatalf("bad string toleration: got %v", errs)
} else if !strings.HasPrefix(errs[0].Field, "spec.tolerations") {
t.Fatalf("wrong field path: %s", errs[0].Field)
} else if !strings.Contains(errs[0].Detail, "rawTolerations") {
t.Fatalf("missing rawTolerations hint: %s", errs[0].Detail)
}

badRawTol := base()
badRawTol.Spec.RawTolerations = []corev1.Toleration{{Key: "k", Operator: corev1.TolerationOpExists, Effect: "NoSchedul"}}

Check warning on line 34 in internal/validation/client_podspec_syntax_test.go

View workflow job for this annotation

GitHub Actions / Lint

"Schedul" should be "Schedule".
if errs := v.Validate(context.Background(), nil, badRawTol); len(errs) != 1 {
t.Fatalf("bad rawToleration effect: got %v", errs)
} else if !strings.HasPrefix(errs[0].Field, "spec.rawTolerations") {
t.Fatalf("wrong field path: %s", errs[0].Field)
}

badSel := base()
badSel.Spec.NodeSelector = map[string]string{"bad key!": "x"}
if errs := v.Validate(context.Background(), nil, badSel); len(errs) != 1 {
t.Fatalf("bad nodeSelector: got %v", errs)
} else if !strings.HasPrefix(errs[0].Field, "spec.nodeSelector") {
t.Fatalf("wrong field path: %s", errs[0].Field)
}

badNodeLabels := base()
badNodeLabels.Spec.CsiConfig = &weka.ClientCsiConfig{
Advanced: &weka.AdvancedCsiConfig{NodeLabels: map[string]string{"k": "bad value!"}},
}
if errs := v.Validate(context.Background(), nil, badNodeLabels); len(errs) != 1 {
t.Fatalf("bad csiConfig.advanced.nodeLabels: got %v", errs)
} else if !strings.HasPrefix(errs[0].Field, "spec.csiConfig.advanced.nodeLabels") {
t.Fatalf("wrong field path: %s", errs[0].Field)
}

badControllerTol := base()
badControllerTol.Spec.CsiConfig = &weka.ClientCsiConfig{
Advanced: &weka.AdvancedCsiConfig{ControllerTolerations: []corev1.Toleration{{Key: "k", Operator: "Bogus"}}},
}
if errs := v.Validate(context.Background(), nil, badControllerTol); len(errs) != 1 {
t.Fatalf("bad csiConfig.advanced.controllerTolerations: got %v", errs)
} else if !strings.HasPrefix(errs[0].Field, "spec.csiConfig.advanced.controllerTolerations") {
t.Fatalf("wrong field path: %s", errs[0].Field)
}

badControllerLabels := base()
badControllerLabels.Spec.CsiConfig = &weka.ClientCsiConfig{
Advanced: &weka.AdvancedCsiConfig{ControllerLabels: map[string]string{"bad key!": "x"}},
}
if errs := v.Validate(context.Background(), nil, badControllerLabels); len(errs) != 1 {
t.Fatalf("bad csiConfig.advanced.controllerLabels: got %v", errs)
} else if !strings.HasPrefix(errs[0].Field, "spec.csiConfig.advanced.controllerLabels") {
t.Fatalf("wrong field path: %s", errs[0].Field)
}

badNodeTol := base()
badNodeTol.Spec.CsiConfig = &weka.ClientCsiConfig{
Advanced: &weka.AdvancedCsiConfig{NodeTolerations: []corev1.Toleration{{Key: "k", Operator: "Bogus"}}},
}
if errs := v.Validate(context.Background(), nil, badNodeTol); len(errs) != 1 {
t.Fatalf("bad csiConfig.advanced.nodeTolerations: got %v", errs)
} else if !strings.HasPrefix(errs[0].Field, "spec.csiConfig.advanced.nodeTolerations") {
t.Fatalf("wrong field path: %s", errs[0].Field)
}
}
136 changes: 136 additions & 0 deletions internal/validation/cluster_podspec_syntax.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
package validation

import (
"context"

wekav1alpha1 "github.com/weka/weka-k8s-api/api/v1alpha1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
"sigs.k8s.io/controller-runtime/pkg/client"
)

// clusterPodspecSyntax rejects WekaClusters whose scheduling-related fields
// would produce pods the API server rejects at create time (invalid
// toleration keys/enums, label syntax, topology keys, affinity). Pure spec
// math; see podspec_syntax.go for the shared checks.
type clusterPodspecSyntax struct{}

func (clusterPodspecSyntax) ID() string { return "cluster_podspec_syntax" }

func (clusterPodspecSyntax) Validate(_ context.Context, _ client.Client, obj runtime.Object) field.ErrorList {
wc, ok := obj.(*wekav1alpha1.WekaCluster)
if !ok {
return nil
}
spec := field.NewPath("spec")
var errs field.ErrorList

errs = append(errs, validateSimpleTolerations(spec.Child("tolerations"), wc.Spec.Tolerations)...)
errs = append(errs, validateRawTolerations(spec.Child("rawTolerations"), wc.Spec.RawTolerations)...)
errs = append(errs, validateLabelMap(spec.Child("nodeSelector"), wc.Spec.NodeSelector)...)

// RoleNodeSelector and RoleAnnotations have six roles (compute, drive, s3,
// nfs, smbw, dataServices); RoleAffinity below has only five — there is no
// dataServices field on that struct.
roleNodeSelectors := []struct {
role string
sel *map[string]string
}{
{"compute", wc.Spec.RoleNodeSelector.Compute},
{"drive", wc.Spec.RoleNodeSelector.Drive},
{"s3", wc.Spec.RoleNodeSelector.S3},
{"nfs", wc.Spec.RoleNodeSelector.Nfs},
{"smbw", wc.Spec.RoleNodeSelector.Smbw},
{"dataServices", wc.Spec.RoleNodeSelector.DataServices},
}
Comment on lines +35 to +45

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The four role tables here (and the mirrored role lists in cluster_podspec_syntax_test.go:178-179) are hand-maintained. If someone adds an envoy/dataServices field to RoleNodeSelector / RoleAffinity / RoleTopologySpreadConstraints in weka-k8s-api, that field is silently unvalidated and no test fails — the tests enumerate the same hardcoded list, so they agree with the bug.

Cheap guard: a reflection assertion in the test, e.g.

if got := reflect.TypeOf(weka.RoleNodeSelector{}).NumField(); got != len(sixRoles) {
    t.Fatalf("RoleNodeSelector has %d fields, table covers %d — update the validator", got, len(sixRoles))
}

(one per struct). That turns a silent coverage hole into a compile-time-ish failure at the next API bump.

for _, r := range roleNodeSelectors {
if r.sel != nil {
errs = append(errs, validateLabelMap(spec.Child("roleNodeSelector", r.role), *r.sel)...)
}
}

roleAnnotations := []struct {
role string
ann *map[string]string
}{
{"compute", wc.Spec.RoleAnnotations.Compute},
{"drive", wc.Spec.RoleAnnotations.Drive},
{"s3", wc.Spec.RoleAnnotations.S3},
{"nfs", wc.Spec.RoleAnnotations.Nfs},
{"smbw", wc.Spec.RoleAnnotations.Smbw},
{"dataServices", wc.Spec.RoleAnnotations.DataServices},
}
for _, r := range roleAnnotations {
if r.ann != nil {
errs = append(errs, validateAnnotationMap(spec.Child("roleAnnotations", r.role), *r.ann)...)
}
}

if fd := wc.Spec.FailureDomain; fd != nil {
// mirror getDefaultRoleTopologySpreadConstraints precedence: label
// wins over compositeLabels; skew is used only with label
if fd.Label != nil {
if *fd.Label == "" {
errs = append(errs, field.Required(spec.Child("failureDomain", "label"), "failureDomain label may not be empty when set"))
} else {
errs = append(errs, validateTopologyKey(spec.Child("failureDomain", "label"), *fd.Label)...)
}
// skew becomes the generated spread constraint's maxSkew (must be > 0)
if fd.Skew != nil && *fd.Skew <= 0 {
errs = append(errs, field.Invalid(spec.Child("failureDomain", "skew"), *fd.Skew, "must be greater than zero"))
}
} else {
for i, l := range fd.CompositeLabels {
p := spec.Child("failureDomain", "compositeLabels").Index(i)
if l == "" {
errs = append(errs, field.Required(p, "failureDomain compositeLabels entries may not be empty"))
} else {
errs = append(errs, validateTopologyKey(p, l)...)
}
}
}
}
Comment on lines +69 to +92

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The precedence mirrored here matches the code (container_factory.go:220-240: Label != nil wins, CompositeLabels only in the else), which is the right thing to mirror. But the published API doc says the opposite — doc/api_dump/wekacluster.md:185: "If compositeLabels is set, label and skew will be ignored."

Consequence: a user who follows the doc and sets both gets their compositeLabels neither used nor validated, and a typo there stays invisible. Worth fixing the field comment in weka-k8s-api (source of the generated dump) as a follow-up so doc and validator agree — otherwise this validator quietly cements the undocumented precedence.


if pc := wc.Spec.PodConfig; pc != nil {
errs = append(errs, validateRawAffinity(spec.Child("podConfig", "affinity"), pc.Affinity)...)
if ra := pc.RoleAffinity; ra != nil {
roleAffinities := []struct {
role string
raw *runtime.RawExtension
}{
{"compute", ra.Compute},
{"drive", ra.Drive},
{"s3", ra.S3},
{"nfs", ra.Nfs},
{"smbw", ra.Smbw},
}
for _, r := range roleAffinities {
if r.raw != nil {
errs = append(errs, validateRawAffinity(spec.Child("podConfig", "roleAffinity", r.role), r.raw)...)
}
}
}

errs = append(errs, validateRawTopologySpreadConstraints(spec.Child("podConfig", "topologySpreadConstraints"), pc.TopologySpreadConstraints)...)
if rtsc := pc.RoleTopologySpreadConstraints; rtsc != nil {
// RoleTopologySpreadConstraints has five roles (no dataServices field),
// same set as RoleAffinity above.
roleTopologySpreadConstraints := []struct {
role string
raw *runtime.RawExtension
}{
{"compute", rtsc.Compute},
{"drive", rtsc.Drive},
{"s3", rtsc.S3},
{"nfs", rtsc.Nfs},
{"smbw", rtsc.Smbw},
}
for _, r := range roleTopologySpreadConstraints {
if r.raw != nil {
errs = append(errs, validateRawTopologySpreadConstraints(spec.Child("podConfig", "roleTopologySpreadConstraints", r.role), r.raw)...)
}
}
}
}
return errs
}
Loading
Loading