diff --git a/pkg/clouds/k8s/kube_run.go b/pkg/clouds/k8s/kube_run.go index 9a590133..f8e799a4 100644 --- a/pkg/clouds/k8s/kube_run.go +++ b/pkg/clouds/k8s/kube_run.go @@ -133,8 +133,8 @@ type VPAConfig struct { // MaxAllowed specifies maximum allowed resources MaxAllowed *VPAResourceRequirements `json:"maxAllowed" yaml:"maxAllowed"` // ControlledResources specifies which resources VPA should control. - // Per the VPA CRD this is a per-container field; SC places it inside each - // containerPolicy entry, not at resourcePolicy level. + // Per the VPA CRD this is a per-container field; SC places it inside the + // catch-all "*" containerPolicy entry, not at resourcePolicy level. ControlledResources []string `json:"controlledResources" yaml:"controlledResources"` // ControlledValues specifies which resource values VPA should control. // One of "RequestsAndLimits" (default) or "RequestsOnly". Use "RequestsOnly" @@ -143,6 +143,9 @@ type VPAConfig struct { // proportionally with a lowered request — the proportional shrink causes // CPU-throttle-induced startup probe failures. ControlledValues *string `json:"controlledValues" yaml:"controlledValues"` + // ContainerPolicies are per-container overrides; the top-level fields render + // the catch-all "*". Common use: a sidecar with mode "Off" to exclude it. + ContainerPolicies []VPAContainerPolicy `json:"containerPolicies" yaml:"containerPolicies"` } // VPAResourceRequirements defines resource requirements for VPA @@ -152,6 +155,17 @@ type VPAResourceRequirements struct { EphemeralStorage *string `json:"ephemeral-storage" yaml:"ephemeral-storage"` } +// VPAContainerPolicy mirrors a VPA CRD containerPolicies entry; mode "Off" +// excludes the container from VPA. +type VPAContainerPolicy struct { + ContainerName string `json:"containerName" yaml:"containerName"` + Mode *string `json:"mode" yaml:"mode"` + MinAllowed *VPAResourceRequirements `json:"minAllowed" yaml:"minAllowed"` + MaxAllowed *VPAResourceRequirements `json:"maxAllowed" yaml:"maxAllowed"` + ControlledResources []string `json:"controlledResources" yaml:"controlledResources"` + ControlledValues *string `json:"controlledValues" yaml:"controlledValues"` +} + func (i *KubeRunInput) DependsOnResources() []api.StackConfigDependencyResource { return i.Deployment.StackConfig.Dependencies } diff --git a/pkg/clouds/pulumi/kubernetes/simple_container.go b/pkg/clouds/pulumi/kubernetes/simple_container.go index b7eaf95e..d32a8bfb 100644 --- a/pkg/clouds/pulumi/kubernetes/simple_container.go +++ b/pkg/clouds/pulumi/kubernetes/simple_container.go @@ -1029,9 +1029,41 @@ ${proto}://${domain} { return sc, nil } +// ValidateVPAConfiguration rejects per-container policies the VPA CRD would +// reject at apply time, so a bad config fails at plan time instead. +func ValidateVPAConfiguration(vpa *k8s.VPAConfig) error { + if vpa == nil || !vpa.Enabled { + return nil + } + seen := make(map[string]bool, len(vpa.ContainerPolicies)) + for i, cp := range vpa.ContainerPolicies { + switch { + case cp.ContainerName == "": + return errors.Errorf("containerPolicies[%d]: containerName must not be empty", i) + case cp.ContainerName == "*": + return errors.Errorf("containerPolicies[%d]: %q is reserved; set the catch-all via the top-level fields", i, "*") + case seen[cp.ContainerName]: + return errors.Errorf("containerPolicies: duplicate containerName %q", cp.ContainerName) + } + seen[cp.ContainerName] = true + if cp.Mode != nil && *cp.Mode != "Off" && *cp.Mode != "Auto" { + return errors.Errorf("containerPolicies[%q]: mode must be \"Off\" or \"Auto\", got %q", cp.ContainerName, *cp.Mode) + } + } + return nil +} + func createVPA(ctx *sdk.Context, args *SimpleContainerArgs, deploymentName string, namespace sdk.StringInput, labels, annotations map[string]string, opts ...sdk.ResourceOption) error { vpaName := fmt.Sprintf("%s-vpa", deploymentName) + if err := ValidateVPAConfiguration(args.VPA); err != nil { + return errors.Wrapf(err, "invalid VPA configuration for %q", deploymentName) + } + if len(args.VPA.ContainerPolicies) > 0 && args.VPA.MinAllowed == nil && args.VPA.MaxAllowed == nil && + len(args.VPA.ControlledResources) == 0 && args.VPA.ControlledValues == nil { + args.Log.Warn(ctx.Context(), "VPA for %q has containerPolicies but no top-level floor; unlisted containers autoscale without a minAllowed", deploymentName) + } + // Build VPA spec content vpaSpec := map[string]interface{}{ "targetRef": map[string]interface{}{ @@ -1048,61 +1080,72 @@ func createVPA(ctx *sdk.Context, args *SimpleContainerArgs, deploymentName strin } } - // Add resource policy if specified - if args.VPA.MinAllowed != nil || args.VPA.MaxAllowed != nil || len(args.VPA.ControlledResources) > 0 || args.VPA.ControlledValues != nil { - resourcePolicy := map[string]interface{}{} - - // Build the container policy. Per the VPA CRD, controlledResources and - // controlledValues are per-container fields and live inside the - // containerPolicy entry — not at the resourcePolicy level. Placing them - // at resourcePolicy level (the previous behavior) caused k8s to silently - // drop them. - containerPolicy := map[string]interface{}{ - "containerName": "*", - } - - if len(args.VPA.ControlledResources) > 0 { - containerPolicy["controlledResources"] = args.VPA.ControlledResources + // Top-level fields render the catch-all "*"; ContainerPolicies append + // per-container entries (an exact containerName wins over "*"). + hasTopLevel := args.VPA.MinAllowed != nil || args.VPA.MaxAllowed != nil || len(args.VPA.ControlledResources) > 0 || args.VPA.ControlledValues != nil + if hasTopLevel || len(args.VPA.ContainerPolicies) > 0 { + resMap := func(r *k8s.VPAResourceRequirements) map[string]interface{} { + if r == nil { + return nil + } + m := map[string]interface{}{} + if r.CPU != nil { + m["cpu"] = lo.FromPtr(r.CPU) + } + if r.Memory != nil { + m["memory"] = lo.FromPtr(r.Memory) + } + if r.EphemeralStorage != nil { + m["ephemeral-storage"] = lo.FromPtr(r.EphemeralStorage) + } + if len(m) == 0 { + return nil + } + return m } - if args.VPA.ControlledValues != nil { - containerPolicy["controlledValues"] = lo.FromPtr(args.VPA.ControlledValues) - } + containerPolicies := make([]interface{}, 0, 1+len(args.VPA.ContainerPolicies)) - if args.VPA.MinAllowed != nil { - minAllowed := map[string]interface{}{} - if args.VPA.MinAllowed.CPU != nil { - minAllowed["cpu"] = lo.FromPtr(args.VPA.MinAllowed.CPU) + if hasTopLevel { + star := map[string]interface{}{"containerName": "*"} + if len(args.VPA.ControlledResources) > 0 { + star["controlledResources"] = args.VPA.ControlledResources } - if args.VPA.MinAllowed.Memory != nil { - minAllowed["memory"] = lo.FromPtr(args.VPA.MinAllowed.Memory) + if args.VPA.ControlledValues != nil { + star["controlledValues"] = lo.FromPtr(args.VPA.ControlledValues) } - if args.VPA.MinAllowed.EphemeralStorage != nil { - minAllowed["ephemeral-storage"] = lo.FromPtr(args.VPA.MinAllowed.EphemeralStorage) + if m := resMap(args.VPA.MinAllowed); m != nil { + star["minAllowed"] = m } - if len(minAllowed) > 0 { - containerPolicy["minAllowed"] = minAllowed + if m := resMap(args.VPA.MaxAllowed); m != nil { + star["maxAllowed"] = m } + containerPolicies = append(containerPolicies, star) } - if args.VPA.MaxAllowed != nil { - maxAllowed := map[string]interface{}{} - if args.VPA.MaxAllowed.CPU != nil { - maxAllowed["cpu"] = lo.FromPtr(args.VPA.MaxAllowed.CPU) + for _, cp := range args.VPA.ContainerPolicies { + policy := map[string]interface{}{"containerName": cp.ContainerName} + if cp.Mode != nil { + policy["mode"] = lo.FromPtr(cp.Mode) } - if args.VPA.MaxAllowed.Memory != nil { - maxAllowed["memory"] = lo.FromPtr(args.VPA.MaxAllowed.Memory) + if len(cp.ControlledResources) > 0 { + policy["controlledResources"] = cp.ControlledResources } - if args.VPA.MaxAllowed.EphemeralStorage != nil { - maxAllowed["ephemeral-storage"] = lo.FromPtr(args.VPA.MaxAllowed.EphemeralStorage) + if cp.ControlledValues != nil { + policy["controlledValues"] = lo.FromPtr(cp.ControlledValues) } - if len(maxAllowed) > 0 { - containerPolicy["maxAllowed"] = maxAllowed + if m := resMap(cp.MinAllowed); m != nil { + policy["minAllowed"] = m } + if m := resMap(cp.MaxAllowed); m != nil { + policy["maxAllowed"] = m + } + containerPolicies = append(containerPolicies, policy) } - resourcePolicy["containerPolicies"] = []interface{}{containerPolicy} - vpaSpec["resourcePolicy"] = resourcePolicy + vpaSpec["resourcePolicy"] = map[string]interface{}{ + "containerPolicies": containerPolicies, + } } // Build the complete VPA resource with proper spec nesting diff --git a/pkg/clouds/pulumi/kubernetes/vpa_container_policies_test.go b/pkg/clouds/pulumi/kubernetes/vpa_container_policies_test.go new file mode 100644 index 00000000..1015ed6f --- /dev/null +++ b/pkg/clouds/pulumi/kubernetes/vpa_container_policies_test.go @@ -0,0 +1,177 @@ +package kubernetes + +import ( + "testing" + + "github.com/samber/lo" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + corev1 "github.com/pulumi/pulumi-kubernetes/sdk/v4/go/kubernetes/core/v1" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" + + "github.com/simple-container-com/api/pkg/api/logger" + "github.com/simple-container-com/api/pkg/clouds/k8s" +) + +// renderVPAContainerPolicies runs NewSimpleContainer with the given VPA config +// and returns the captured spec.resourcePolicy.containerPolicies array. +func renderVPAContainerPolicies(t *testing.T, vpa *k8s.VPAConfig) []resource.PropertyValue { + t.Helper() + mocks := NewSimpleContainerMocks() + err := sdk.RunErr(func(ctx *sdk.Context) error { + args := &SimpleContainerArgs{ + Namespace: "myapp", + Service: "myapp", + Deployment: "myapp", + ScEnv: "production", + Domain: "myapp.example.com", + Prefix: "/", + Replicas: 2, + IngressContainer: &k8s.CloudRunContainer{ + Name: "myapp", MainPort: lo.ToPtr(8080), Ports: []int{8080}, + }, + Log: logger.New(), + Containers: []corev1.ContainerArgs{ + {Name: sdk.String("myapp"), Image: sdk.String("nginx:latest")}, + {Name: sdk.String("cloudsql-proxy"), Image: sdk.String("cloud-sql-proxy:latest")}, + }, + VPA: vpa, + } + _, err := NewSimpleContainer(ctx, args) + return err + }, sdk.WithMocks("test", "test", mocks)) + require.NoError(t, err) + + mocks.mu.RLock() + defer mocks.mu.RUnlock() + for _, props := range mocks.createdResources { + kind, ok := props["kind"] + if !ok || !kind.IsString() || kind.StringValue() != "VerticalPodAutoscaler" { + continue + } + require.True(t, props["spec"].IsObject(), "VPA spec should be an object") + rp := props["spec"].ObjectValue()["resourcePolicy"] + require.True(t, rp.IsObject(), "VPA spec.resourcePolicy should be an object") + cps := rp.ObjectValue()["containerPolicies"] + require.True(t, cps.IsArray(), "resourcePolicy.containerPolicies should be an array") + return cps.ArrayValue() + } + t.Fatal("VPA should have been created and captured by the mock") + return nil +} + +// VPA.ContainerPolicies must render per-container entries alongside the "*" +// catch-all. The motivating case: an injected sidecar (cloudsql-proxy) set to +// mode "Off" so it keeps its small template request instead of being floored at +// the app container's minAllowed (the "*" policy). +func TestCreateVPA_PerContainerPolicies(t *testing.T) { + arr := renderVPAContainerPolicies(t, &k8s.VPAConfig{ + Enabled: true, + UpdateMode: lo.ToPtr("Auto"), + MinAllowed: &k8s.VPAResourceRequirements{CPU: lo.ToPtr("250m"), Memory: lo.ToPtr("64Mi")}, + ControlledResources: []string{"cpu", "memory"}, + ControlledValues: lo.ToPtr("RequestsOnly"), + ContainerPolicies: []k8s.VPAContainerPolicy{ + {ContainerName: "cloudsql-proxy", Mode: lo.ToPtr("Off")}, + }, + }) + require.Len(t, arr, 2, "expected exactly the '*' catch-all + the cloudsql-proxy override") + + sawStar, sawProxy := false, false + for _, e := range arr { + require.True(t, e.IsObject()) + o := e.ObjectValue() + require.True(t, o["containerName"].IsString()) + switch o["containerName"].StringValue() { + case "*": + sawStar = true + assert.Equal(t, "RequestsOnly", o["controlledValues"].StringValue()) + require.True(t, o["minAllowed"].IsObject()) + assert.Equal(t, "250m", o["minAllowed"].ObjectValue()["cpu"].StringValue()) + case "cloudsql-proxy": + sawProxy = true + require.True(t, o["mode"].IsString()) + assert.Equal(t, "Off", o["mode"].StringValue(), "sidecar must be excluded from VPA via mode Off") + _, hasMin := o["minAllowed"] + assert.False(t, hasMin, "sidecar policy must not inherit the app minAllowed floor") + default: + t.Fatalf("unexpected containerPolicy entry: %s", o["containerName"].StringValue()) + } + } + assert.True(t, sawStar, "expected a '*' catch-all containerPolicy") + assert.True(t, sawProxy, "expected a per-container policy for cloudsql-proxy") +} + +// Backward compat: with no ContainerPolicies, the render is the historical +// single "*" entry carrying the top-level floor/controlled* fields. Locks the +// guarantee that existing stacks re-render byte-identical (no Pulumi diff). +func TestCreateVPA_NoContainerPolicies_BackwardCompat(t *testing.T) { + arr := renderVPAContainerPolicies(t, &k8s.VPAConfig{ + Enabled: true, + UpdateMode: lo.ToPtr("Auto"), + MinAllowed: &k8s.VPAResourceRequirements{CPU: lo.ToPtr("250m"), Memory: lo.ToPtr("64Mi")}, + MaxAllowed: &k8s.VPAResourceRequirements{CPU: lo.ToPtr("2"), Memory: lo.ToPtr("2560Mi")}, + ControlledResources: []string{"cpu", "memory"}, + ControlledValues: lo.ToPtr("RequestsOnly"), + }) + require.Len(t, arr, 1, "no ContainerPolicies must render exactly one '*' entry") + o := arr[0].ObjectValue() + assert.Equal(t, "*", o["containerName"].StringValue()) + assert.Equal(t, "RequestsOnly", o["controlledValues"].StringValue()) + require.True(t, o["controlledResources"].IsArray()) + assert.Len(t, o["controlledResources"].ArrayValue(), 2) + assert.Equal(t, "250m", o["minAllowed"].ObjectValue()["cpu"].StringValue()) + assert.Equal(t, "2560Mi", o["maxAllowed"].ObjectValue()["memory"].StringValue()) +} + +// ContainerPolicies with no top-level fields renders ONLY the per-container +// entries — no "*" catch-all (hasTopLevel == false branch). Also exercises a +// per-container minAllowed override (not just mode Off). +func TestCreateVPA_ContainerPoliciesOnly_NoStar(t *testing.T) { + arr := renderVPAContainerPolicies(t, &k8s.VPAConfig{ + Enabled: true, + UpdateMode: lo.ToPtr("Auto"), + ContainerPolicies: []k8s.VPAContainerPolicy{ + {ContainerName: "cloudsql-proxy", Mode: lo.ToPtr("Off")}, + {ContainerName: "myapp", MinAllowed: &k8s.VPAResourceRequirements{CPU: lo.ToPtr("100m")}}, + }, + }) + require.Len(t, arr, 2, "containerPolicies-only must render exactly the listed entries, no '*'") + for _, e := range arr { + o := e.ObjectValue() + assert.NotEqual(t, "*", o["containerName"].StringValue(), "no '*' catch-all when no top-level fields are set") + if o["containerName"].StringValue() == "myapp" { + require.True(t, o["minAllowed"].IsObject()) + assert.Equal(t, "100m", o["minAllowed"].ObjectValue()["cpu"].StringValue()) + } + } +} + +func TestValidateVPAConfiguration(t *testing.T) { + cases := []struct { + name string + vpa *k8s.VPAConfig + wantErr string + }{ + {"nil", nil, ""}, + {"disabled", &k8s.VPAConfig{Enabled: false, ContainerPolicies: []k8s.VPAContainerPolicy{{ContainerName: ""}}}, ""}, + {"valid", &k8s.VPAConfig{Enabled: true, ContainerPolicies: []k8s.VPAContainerPolicy{{ContainerName: "cloudsql-proxy", Mode: lo.ToPtr("Off")}}}, ""}, + {"empty name", &k8s.VPAConfig{Enabled: true, ContainerPolicies: []k8s.VPAContainerPolicy{{ContainerName: ""}}}, "must not be empty"}, + {"star reserved", &k8s.VPAConfig{Enabled: true, ContainerPolicies: []k8s.VPAContainerPolicy{{ContainerName: "*"}}}, "reserved"}, + {"duplicate", &k8s.VPAConfig{Enabled: true, ContainerPolicies: []k8s.VPAContainerPolicy{{ContainerName: "x"}, {ContainerName: "x"}}}, "duplicate"}, + {"bad mode", &k8s.VPAConfig{Enabled: true, ContainerPolicies: []k8s.VPAContainerPolicy{{ContainerName: "x", Mode: lo.ToPtr("off")}}}, "mode must be"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateVPAConfiguration(tc.vpa) + if tc.wantErr == "" { + assert.NoError(t, err) + } else { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + } + }) + } +}