From 798d599956200565b94df1bb57034b1b6788e8ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:41:50 +0000 Subject: [PATCH 01/28] Add ServiceSpecEditor for mutating Kubernetes ServiceSpec Provides typed methods for managing service ports, selectors, session affinity, traffic policies, and other ServiceSpec fields. Includes full test coverage for all editor methods. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/mutation/editors/servicespec.go | 108 +++++++++++++ pkg/mutation/editors/servicespec_test.go | 188 +++++++++++++++++++++++ 2 files changed, 296 insertions(+) create mode 100644 pkg/mutation/editors/servicespec.go create mode 100644 pkg/mutation/editors/servicespec_test.go diff --git a/pkg/mutation/editors/servicespec.go b/pkg/mutation/editors/servicespec.go new file mode 100644 index 00000000..5663cf5d --- /dev/null +++ b/pkg/mutation/editors/servicespec.go @@ -0,0 +1,108 @@ +package editors + +import ( + corev1 "k8s.io/api/core/v1" +) + +// ServiceSpecEditor provides a typed API for mutating a Kubernetes ServiceSpec. +type ServiceSpecEditor struct { + spec *corev1.ServiceSpec +} + +// NewServiceSpecEditor creates a new ServiceSpecEditor for the given ServiceSpec. +func NewServiceSpecEditor(spec *corev1.ServiceSpec) *ServiceSpecEditor { + return &ServiceSpecEditor{spec: spec} +} + +// Raw returns the underlying *corev1.ServiceSpec. +// +// This is an escape hatch for cases where the typed API is insufficient. +func (e *ServiceSpecEditor) Raw() *corev1.ServiceSpec { + return e.spec +} + +// SetType sets the service type (ClusterIP, NodePort, LoadBalancer, ExternalName). +func (e *ServiceSpecEditor) SetType(t corev1.ServiceType) { + e.spec.Type = t +} + +// EnsurePort upserts a service port. If a port with the same Name exists (or the same +// Port number when Name is empty), it is replaced; otherwise the port is appended. +func (e *ServiceSpecEditor) EnsurePort(port corev1.ServicePort) { + for i, existing := range e.spec.Ports { + if port.Name != "" && existing.Name == port.Name { + e.spec.Ports[i] = port + return + } + if port.Name == "" && existing.Port == port.Port { + e.spec.Ports[i] = port + return + } + } + e.spec.Ports = append(e.spec.Ports, port) +} + +// RemovePort removes a service port by name. It is a no-op if the port does not exist. +func (e *ServiceSpecEditor) RemovePort(name string) { + for i, existing := range e.spec.Ports { + if existing.Name == name { + e.spec.Ports = append(e.spec.Ports[:i], e.spec.Ports[i+1:]...) + return + } + } +} + +// SetSelector replaces the entire service selector map. +func (e *ServiceSpecEditor) SetSelector(selector map[string]string) { + e.spec.Selector = selector +} + +// EnsureSelector adds or updates a single selector key-value pair. +func (e *ServiceSpecEditor) EnsureSelector(key, value string) { + if e.spec.Selector == nil { + e.spec.Selector = make(map[string]string) + } + e.spec.Selector[key] = value +} + +// RemoveSelector removes a single selector key. It is a no-op if the key does not exist. +func (e *ServiceSpecEditor) RemoveSelector(key string) { + if e.spec.Selector != nil { + delete(e.spec.Selector, key) + } +} + +// SetSessionAffinity sets the session affinity type (None or ClientIP). +func (e *ServiceSpecEditor) SetSessionAffinity(affinity corev1.ServiceAffinity) { + e.spec.SessionAffinity = affinity +} + +// SetSessionAffinityConfig sets the session affinity configuration. +func (e *ServiceSpecEditor) SetSessionAffinityConfig(cfg *corev1.SessionAffinityConfig) { + e.spec.SessionAffinityConfig = cfg +} + +// SetPublishNotReadyAddresses controls whether endpoints for not-ready pods are published. +func (e *ServiceSpecEditor) SetPublishNotReadyAddresses(v bool) { + e.spec.PublishNotReadyAddresses = v +} + +// SetExternalTrafficPolicy sets the external traffic policy for the service. +func (e *ServiceSpecEditor) SetExternalTrafficPolicy(policy corev1.ServiceExternalTrafficPolicyType) { + e.spec.ExternalTrafficPolicy = policy +} + +// SetInternalTrafficPolicy sets the internal traffic policy for the service. +func (e *ServiceSpecEditor) SetInternalTrafficPolicy(policy *corev1.ServiceInternalTrafficPolicy) { + e.spec.InternalTrafficPolicy = policy +} + +// SetLoadBalancerSourceRanges sets the allowed source ranges for a LoadBalancer service. +func (e *ServiceSpecEditor) SetLoadBalancerSourceRanges(ranges []string) { + e.spec.LoadBalancerSourceRanges = ranges +} + +// SetExternalName sets the external name for an ExternalName service type. +func (e *ServiceSpecEditor) SetExternalName(name string) { + e.spec.ExternalName = name +} diff --git a/pkg/mutation/editors/servicespec_test.go b/pkg/mutation/editors/servicespec_test.go new file mode 100644 index 00000000..8f3f8691 --- /dev/null +++ b/pkg/mutation/editors/servicespec_test.go @@ -0,0 +1,188 @@ +package editors + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/intstr" +) + +func TestServiceSpecEditor(t *testing.T) { + t.Run("SetType", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + editor.SetType(corev1.ServiceTypeLoadBalancer) + assert.Equal(t, corev1.ServiceTypeLoadBalancer, spec.Type) + }) + + t.Run("EnsurePort appends new port", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + editor.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + assert.Len(t, spec.Ports, 1) + assert.Equal(t, "http", spec.Ports[0].Name) + assert.Equal(t, int32(80), spec.Ports[0].Port) + }) + + t.Run("EnsurePort upserts by name", func(t *testing.T) { + spec := &corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + } + editor := NewServiceSpecEditor(spec) + editor.EnsurePort(corev1.ServicePort{Name: "http", Port: 8080, TargetPort: intstr.FromInt32(8080)}) + assert.Len(t, spec.Ports, 1) + assert.Equal(t, int32(8080), spec.Ports[0].Port) + }) + + t.Run("EnsurePort upserts by port number when name is empty", func(t *testing.T) { + spec := &corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Port: 80}, + }, + } + editor := NewServiceSpecEditor(spec) + editor.EnsurePort(corev1.ServicePort{Port: 80, TargetPort: intstr.FromInt32(8080)}) + assert.Len(t, spec.Ports, 1) + assert.Equal(t, intstr.FromInt32(8080), spec.Ports[0].TargetPort) + }) + + t.Run("EnsurePort appends when no match", func(t *testing.T) { + spec := &corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + } + editor := NewServiceSpecEditor(spec) + editor.EnsurePort(corev1.ServicePort{Name: "https", Port: 443}) + assert.Len(t, spec.Ports, 2) + }) + + t.Run("RemovePort removes existing", func(t *testing.T) { + spec := &corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + {Name: "https", Port: 443}, + }, + } + editor := NewServiceSpecEditor(spec) + editor.RemovePort("http") + assert.Len(t, spec.Ports, 1) + assert.Equal(t, "https", spec.Ports[0].Name) + }) + + t.Run("RemovePort no-op when absent", func(t *testing.T) { + spec := &corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + } + editor := NewServiceSpecEditor(spec) + editor.RemovePort("missing") + assert.Len(t, spec.Ports, 1) + }) + + t.Run("SetSelector replaces entire selector", func(t *testing.T) { + spec := &corev1.ServiceSpec{ + Selector: map[string]string{"old": "value"}, + } + editor := NewServiceSpecEditor(spec) + editor.SetSelector(map[string]string{"app": "test"}) + assert.Equal(t, map[string]string{"app": "test"}, spec.Selector) + }) + + t.Run("EnsureSelector adds key", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + editor.EnsureSelector("app", "test") + assert.Equal(t, "test", spec.Selector["app"]) + }) + + t.Run("EnsureSelector overwrites key", func(t *testing.T) { + spec := &corev1.ServiceSpec{ + Selector: map[string]string{"app": "old"}, + } + editor := NewServiceSpecEditor(spec) + editor.EnsureSelector("app", "new") + assert.Equal(t, "new", spec.Selector["app"]) + }) + + t.Run("RemoveSelector removes key", func(t *testing.T) { + spec := &corev1.ServiceSpec{ + Selector: map[string]string{"app": "test", "env": "prod"}, + } + editor := NewServiceSpecEditor(spec) + editor.RemoveSelector("app") + assert.NotContains(t, spec.Selector, "app") + assert.Equal(t, "prod", spec.Selector["env"]) + }) + + t.Run("RemoveSelector no-op on nil selector", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + editor.RemoveSelector("missing") // should not panic + }) + + t.Run("SetSessionAffinity", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + editor.SetSessionAffinity(corev1.ServiceAffinityClientIP) + assert.Equal(t, corev1.ServiceAffinityClientIP, spec.SessionAffinity) + }) + + t.Run("SetSessionAffinityConfig", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + timeout := int32(10800) + cfg := &corev1.SessionAffinityConfig{ + ClientIP: &corev1.ClientIPConfig{TimeoutSeconds: &timeout}, + } + editor.SetSessionAffinityConfig(cfg) + assert.Equal(t, cfg, spec.SessionAffinityConfig) + }) + + t.Run("SetPublishNotReadyAddresses", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + editor.SetPublishNotReadyAddresses(true) + assert.True(t, spec.PublishNotReadyAddresses) + }) + + t.Run("SetExternalTrafficPolicy", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + editor.SetExternalTrafficPolicy(corev1.ServiceExternalTrafficPolicyLocal) + assert.Equal(t, corev1.ServiceExternalTrafficPolicyLocal, spec.ExternalTrafficPolicy) + }) + + t.Run("SetInternalTrafficPolicy", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + local := corev1.ServiceInternalTrafficPolicyLocal + editor.SetInternalTrafficPolicy(&local) + assert.Equal(t, &local, spec.InternalTrafficPolicy) + }) + + t.Run("SetLoadBalancerSourceRanges", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + ranges := []string{"10.0.0.0/8", "192.168.0.0/16"} + editor.SetLoadBalancerSourceRanges(ranges) + assert.Equal(t, ranges, spec.LoadBalancerSourceRanges) + }) + + t.Run("SetExternalName", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + editor.SetExternalName("my.external.service.example.com") + assert.Equal(t, "my.external.service.example.com", spec.ExternalName) + }) + + t.Run("Raw returns underlying spec", func(t *testing.T) { + spec := &corev1.ServiceSpec{} + editor := NewServiceSpecEditor(spec) + assert.Equal(t, spec, editor.Raw()) + }) +} From e142f7b3e58a62fd968ad8de81c32f3709e8b207 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:41:56 +0000 Subject: [PATCH 02/28] Add Service primitive (Integration lifecycle) Implements the Service primitive with: - Builder with fluent API for mutations, flavors, status handlers - Mutator with plan-and-apply pattern (metadata then spec edits) - DefaultFieldApplicator preserving immutable clusterIP/clusterIPs - DefaultOperationalStatusHandler (LoadBalancer pending until ingress assigned) - DefaultDeleteOnSuspendHandler (true - Services deleted on suspend) - PreserveCurrentLabels and PreserveCurrentAnnotations flavors - Full test coverage for builder, mutator, handlers, and flavors Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/service/builder.go | 185 ++++++++++++++++ pkg/primitives/service/builder_test.go | 270 ++++++++++++++++++++++++ pkg/primitives/service/flavors.go | 25 +++ pkg/primitives/service/flavors_test.go | 125 +++++++++++ pkg/primitives/service/handlers.go | 77 +++++++ pkg/primitives/service/handlers_test.go | 149 +++++++++++++ pkg/primitives/service/mutator.go | 106 ++++++++++ pkg/primitives/service/mutator_test.go | 181 ++++++++++++++++ pkg/primitives/service/resource.go | 106 ++++++++++ 9 files changed, 1224 insertions(+) create mode 100644 pkg/primitives/service/builder.go create mode 100644 pkg/primitives/service/builder_test.go create mode 100644 pkg/primitives/service/flavors.go create mode 100644 pkg/primitives/service/flavors_test.go create mode 100644 pkg/primitives/service/handlers.go create mode 100644 pkg/primitives/service/handlers_test.go create mode 100644 pkg/primitives/service/mutator.go create mode 100644 pkg/primitives/service/mutator_test.go create mode 100644 pkg/primitives/service/resource.go diff --git a/pkg/primitives/service/builder.go b/pkg/primitives/service/builder.go new file mode 100644 index 00000000..8c412cfe --- /dev/null +++ b/pkg/primitives/service/builder.go @@ -0,0 +1,185 @@ +package service + +import ( + "fmt" + + "github.com/sourcehawk/operator-component-framework/internal/generic" + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + "github.com/sourcehawk/operator-component-framework/pkg/feature" + corev1 "k8s.io/api/core/v1" +) + +// Builder is a configuration helper for creating and customizing a Service Resource. +// +// It provides a fluent API for registering mutations, status handlers, and +// data extractors. This builder ensures that the resulting Resource is +// properly initialized and validated before use in a reconciliation loop. +type Builder struct { + base *generic.IntegrationBuilder[*corev1.Service, *Mutator] +} + +// NewBuilder initializes a new Builder with the provided Service object. +// +// The Service object passed here serves as the "desired base state". During +// reconciliation, the Resource will attempt to make the cluster's state match +// this base state, modified by any registered mutations. +// +// The provided Service must have at least a Name and Namespace set, which +// is validated during the Build() call. +func NewBuilder(svc *corev1.Service) *Builder { + identityFunc := func(s *corev1.Service) string { + return fmt.Sprintf("v1/Service/%s/%s", s.Namespace, s.Name) + } + + base := generic.NewIntegrationBuilder[*corev1.Service, *Mutator]( + svc, + identityFunc, + DefaultFieldApplicator, + NewMutator, + ) + + base. + WithCustomOperationalStatus(DefaultOperationalStatusHandler). + WithCustomSuspendStatus(DefaultSuspensionStatusHandler). + WithCustomSuspendMutation(DefaultSuspendMutationHandler). + WithCustomSuspendDeletionDecision(DefaultDeleteOnSuspendHandler) + + return &Builder{ + base: base, + } +} + +// WithMutation registers a feature-based mutation for the Service. +// +// Mutations are applied sequentially during the Mutate() phase of reconciliation. +// They are typically used by Features to modify ports, selectors, or other +// service configuration. +// +// Since mutations are often version-gated, the provided feature.Mutation +// should contain the logic to determine if and how the mutation is applied +// based on the component's current version or configuration. +func (b *Builder) WithMutation(m Mutation) *Builder { + b.base.WithMutation(feature.Mutation[*Mutator](m)) + return b +} + +// WithCustomFieldApplicator sets a custom strategy for applying the desired +// state to the existing Service in the cluster. +// +// The default applicator (DefaultFieldApplicator) replaces the current object +// with a deep copy of the desired object while preserving Kubernetes-assigned +// immutable fields (spec.clusterIP and spec.clusterIPs). +// +// The applicator receives the current object from the API server and the desired +// object from the Resource, and is responsible for merging the desired changes +// into the current object. +func (b *Builder) WithCustomFieldApplicator( + applicator func(current, desired *corev1.Service) error, +) *Builder { + b.base.WithCustomFieldApplicator(applicator) + return b +} + +// WithFieldApplicationFlavor registers a post-baseline field application flavor. +// +// Flavors run after the baseline applicator (default or custom) in registration +// order. They are typically used to preserve fields from the live cluster object +// that should not be overwritten by the desired state. +// +// A nil flavor is ignored. +func (b *Builder) WithFieldApplicationFlavor(flavor FieldApplicationFlavor) *Builder { + b.base.WithFieldApplicationFlavor(generic.FieldApplicationFlavor[*corev1.Service](flavor)) + return b +} + +// WithCustomOperationalStatus overrides the default logic for determining if the +// Service is operational. +// +// The default behavior uses DefaultOperationalStatusHandler, which considers +// LoadBalancer services pending until an ingress IP or hostname is assigned, +// and all other service types immediately operational. +// +// If you want to augment the default behavior, you can call DefaultOperationalStatusHandler +// within your custom handler. +func (b *Builder) WithCustomOperationalStatus( + handler func(concepts.ConvergingOperation, *corev1.Service) (concepts.OperationalStatusWithReason, error), +) *Builder { + b.base.WithCustomOperationalStatus(handler) + return b +} + +// WithCustomSuspendStatus overrides how the progress of suspension is reported. +// +// The default behavior uses DefaultSuspensionStatusHandler, which always reports +// Suspended because Services are deleted on suspend. +// +// If you want to augment the default behavior, you can call DefaultSuspensionStatusHandler +// within your custom handler. +func (b *Builder) WithCustomSuspendStatus( + handler func(*corev1.Service) (concepts.SuspensionStatusWithReason, error), +) *Builder { + b.base.WithCustomSuspendStatus(handler) + return b +} + +// WithCustomSuspendMutation defines how the Service should be modified when +// the component is suspended. +// +// The default behavior uses DefaultSuspendMutationHandler, which is a no-op +// because Services are deleted on suspend rather than mutated. +// +// If you want to augment the default behavior, you can call DefaultSuspendMutationHandler +// within your custom handler. +func (b *Builder) WithCustomSuspendMutation( + handler func(*Mutator) error, +) *Builder { + b.base.WithCustomSuspendMutation(handler) + return b +} + +// WithCustomSuspendDeletionDecision overrides the decision of whether to delete +// the Service when the component is suspended. +// +// The default behavior uses DefaultDeleteOnSuspendHandler, which returns true +// because Services have no meaningful "scaled down" state. +// +// If you want to augment the default behavior, you can call DefaultDeleteOnSuspendHandler +// within your custom handler. +func (b *Builder) WithCustomSuspendDeletionDecision( + handler func(*corev1.Service) bool, +) *Builder { + b.base.WithCustomSuspendDeletionDecision(handler) + return b +} + +// WithDataExtractor registers a function to harvest information from the +// Service after it has been successfully reconciled. +// +// This is useful for capturing auto-generated fields (like assigned ClusterIP +// or LoadBalancer ingress) and making them available to other components or +// resources via the framework's data extraction mechanism. +func (b *Builder) WithDataExtractor( + extractor func(corev1.Service) error, +) *Builder { + if extractor != nil { + b.base.WithDataExtractor(func(s *corev1.Service) error { + return extractor(*s) + }) + } + return b +} + +// Build validates the configuration and returns the initialized Resource. +// +// It ensures that: +// - A base Service object was provided. +// - The Service has both a name and a namespace set. +// +// If validation fails, an error is returned and the Resource should not be used. +func (b *Builder) Build() (*Resource, error) { + genericRes, err := b.base.Build() + if err != nil { + return nil, err + } + return &Resource{base: genericRes}, nil +} diff --git a/pkg/primitives/service/builder_test.go b/pkg/primitives/service/builder_test.go new file mode 100644 index 00000000..c22cd186 --- /dev/null +++ b/pkg/primitives/service/builder_test.go @@ -0,0 +1,270 @@ +package service + +import ( + "errors" + "testing" + + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestBuilder(t *testing.T) { + t.Parallel() + + t.Run("Build validation", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + svc *corev1.Service + expectedErr string + }{ + { + name: "nil service", + svc: nil, + expectedErr: "object cannot be nil", + }, + { + name: "empty name", + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-ns", + }, + }, + expectedErr: "object name cannot be empty", + }, + { + name: "empty namespace", + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + }, + }, + expectedErr: "object namespace cannot be empty", + }, + { + name: "valid service", + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + }, + expectedErr: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res, err := NewBuilder(tt.svc).Build() + if tt.expectedErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedErr) + assert.Nil(t, res) + } else { + require.NoError(t, err) + require.NotNil(t, res) + assert.Equal(t, "v1/Service/test-ns/test-svc", res.Identity()) + } + }) + } + }) + + t.Run("WithMutation", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + m := Mutation{ + Name: "test-mutation", + } + res, err := NewBuilder(svc). + WithMutation(m). + Build() + require.NoError(t, err) + assert.Len(t, res.base.Mutations, 1) + assert.Equal(t, "test-mutation", res.base.Mutations[0].Name) + }) + + t.Run("WithCustomFieldApplicator", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + applied := false + applicator := func(_ *corev1.Service, _ *corev1.Service) error { + applied = true + return nil + } + res, err := NewBuilder(svc). + WithCustomFieldApplicator(applicator). + Build() + require.NoError(t, err) + require.NotNil(t, res.base.CustomFieldApplicator) + _ = res.base.CustomFieldApplicator(nil, nil) + assert.True(t, applied) + }) + + t.Run("WithFieldApplicationFlavor", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + res, err := NewBuilder(svc). + WithFieldApplicationFlavor(PreserveCurrentLabels). + WithFieldApplicationFlavor(nil). // nil must be ignored + Build() + require.NoError(t, err) + assert.Len(t, res.base.FieldFlavors, 1) + }) + + t.Run("WithCustomOperationalStatus", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + handler := func(_ concepts.ConvergingOperation, _ *corev1.Service) (concepts.OperationalStatusWithReason, error) { + return concepts.OperationalStatusWithReason{Status: concepts.OperationalStatusPending}, nil + } + res, err := NewBuilder(svc). + WithCustomOperationalStatus(handler). + Build() + require.NoError(t, err) + require.NotNil(t, res.base.OperationalStatusHandler) + status, err := res.base.OperationalStatusHandler(concepts.ConvergingOperationUpdated, nil) + require.NoError(t, err) + assert.Equal(t, concepts.OperationalStatusPending, status.Status) + }) + + t.Run("WithCustomSuspendStatus", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + handler := func(_ *corev1.Service) (concepts.SuspensionStatusWithReason, error) { + return concepts.SuspensionStatusWithReason{Status: concepts.SuspensionStatusSuspended}, nil + } + res, err := NewBuilder(svc). + WithCustomSuspendStatus(handler). + Build() + require.NoError(t, err) + require.NotNil(t, res.base.SuspendStatusHandler) + status, err := res.base.SuspendStatusHandler(nil) + require.NoError(t, err) + assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) + }) + + t.Run("WithCustomSuspendMutation", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + handler := func(_ *Mutator) error { + return errors.New("suspend error") + } + res, err := NewBuilder(svc). + WithCustomSuspendMutation(handler). + Build() + require.NoError(t, err) + require.NotNil(t, res.base.SuspendMutationHandler) + err = res.base.SuspendMutationHandler(nil) + assert.EqualError(t, err, "suspend error") + }) + + t.Run("WithCustomSuspendDeletionDecision", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + handler := func(_ *corev1.Service) bool { + return false + } + res, err := NewBuilder(svc). + WithCustomSuspendDeletionDecision(handler). + Build() + require.NoError(t, err) + require.NotNil(t, res.base.DeleteOnSuspendHandler) + assert.False(t, res.base.DeleteOnSuspendHandler(nil)) + }) + + t.Run("WithDataExtractor", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + called := false + extractor := func(_ corev1.Service) error { + called = true + return nil + } + res, err := NewBuilder(svc). + WithDataExtractor(extractor). + Build() + require.NoError(t, err) + assert.Len(t, res.base.DataExtractors, 1) + err = res.base.DataExtractors[0](&corev1.Service{}) + require.NoError(t, err) + assert.True(t, called) + }) + + t.Run("WithDataExtractor nil", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + res, err := NewBuilder(svc). + WithDataExtractor(nil). + Build() + require.NoError(t, err) + assert.Len(t, res.base.DataExtractors, 0) + }) + + t.Run("WithDataExtractor error propagated", func(t *testing.T) { + t.Parallel() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + } + res, err := NewBuilder(svc). + WithDataExtractor(func(_ corev1.Service) error { + return errors.New("extractor error") + }). + Build() + require.NoError(t, err) + err = res.base.DataExtractors[0](&corev1.Service{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "extractor error") + }) +} diff --git a/pkg/primitives/service/flavors.go b/pkg/primitives/service/flavors.go new file mode 100644 index 00000000..ca97903e --- /dev/null +++ b/pkg/primitives/service/flavors.go @@ -0,0 +1,25 @@ +package service + +import ( + "github.com/sourcehawk/operator-component-framework/pkg/flavors" + corev1 "k8s.io/api/core/v1" +) + +// FieldApplicationFlavor defines a function signature for applying flavors to a +// Service resource. A flavor is called after the baseline field applicator has +// run and can be used to preserve or merge fields from the live cluster object. +type FieldApplicationFlavor flavors.FieldApplicationFlavor[*corev1.Service] + +// PreserveCurrentLabels ensures that any labels present on the current live +// Service but missing from the applied (desired) object are preserved. +// If a label exists in both, the applied value wins. +func PreserveCurrentLabels(applied, current, desired *corev1.Service) error { + return flavors.PreserveCurrentLabels[*corev1.Service]()(applied, current, desired) +} + +// PreserveCurrentAnnotations ensures that any annotations present on the current +// live Service but missing from the applied (desired) object are preserved. +// If an annotation exists in both, the applied value wins. +func PreserveCurrentAnnotations(applied, current, desired *corev1.Service) error { + return flavors.PreserveCurrentAnnotations[*corev1.Service]()(applied, current, desired) +} diff --git a/pkg/primitives/service/flavors_test.go b/pkg/primitives/service/flavors_test.go new file mode 100644 index 00000000..bc19f816 --- /dev/null +++ b/pkg/primitives/service/flavors_test.go @@ -0,0 +1,125 @@ +package service + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestPreserveCurrentLabels(t *testing.T) { + t.Run("adds missing labels from current", func(t *testing.T) { + applied := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"keep": "applied"}}} + current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"extra": "current"}}} + + require.NoError(t, PreserveCurrentLabels(applied, current, nil)) + assert.Equal(t, "applied", applied.Labels["keep"]) + assert.Equal(t, "current", applied.Labels["extra"]) + }) + + t.Run("applied value wins on overlap", func(t *testing.T) { + applied := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "applied"}}} + current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "current"}}} + + require.NoError(t, PreserveCurrentLabels(applied, current, nil)) + assert.Equal(t, "applied", applied.Labels["key"]) + }) + + t.Run("handles nil applied labels", func(t *testing.T) { + applied := &corev1.Service{} + current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"extra": "current"}}} + + require.NoError(t, PreserveCurrentLabels(applied, current, nil)) + assert.Equal(t, "current", applied.Labels["extra"]) + }) +} + +func TestPreserveCurrentAnnotations(t *testing.T) { + t.Run("adds missing annotations from current", func(t *testing.T) { + applied := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"keep": "applied"}}} + current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"extra": "current"}}} + + require.NoError(t, PreserveCurrentAnnotations(applied, current, nil)) + assert.Equal(t, "applied", applied.Annotations["keep"]) + assert.Equal(t, "current", applied.Annotations["extra"]) + }) + + t.Run("applied value wins on overlap", func(t *testing.T) { + applied := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"key": "applied"}}} + current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"key": "current"}}} + + require.NoError(t, PreserveCurrentAnnotations(applied, current, nil)) + assert.Equal(t, "applied", applied.Annotations["key"]) + }) +} + +func TestFlavors_Integration(t *testing.T) { + // Flavors run after baseline applicator via Mutate, in registration order. + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "default", + Labels: map[string]string{"app": "desired"}, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + }, + } + + t.Run("PreserveCurrentLabels via Mutate", func(t *testing.T) { + res, err := NewBuilder(desired). + WithFieldApplicationFlavor(PreserveCurrentLabels). + Build() + require.NoError(t, err) + + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"external": "keep", "app": "old"}, + }, + } + require.NoError(t, res.Mutate(current)) + + assert.Equal(t, "desired", current.Labels["app"]) + assert.Equal(t, "keep", current.Labels["external"]) + }) + + t.Run("flavors run in registration order", func(t *testing.T) { + var order []string + flavor1 := func(_, _, _ *corev1.Service) error { + order = append(order, "flavor1") + return nil + } + flavor2 := func(_, _, _ *corev1.Service) error { + order = append(order, "flavor2") + return nil + } + + res, err := NewBuilder(desired). + WithFieldApplicationFlavor(flavor1). + WithFieldApplicationFlavor(flavor2). + Build() + require.NoError(t, err) + + require.NoError(t, res.Mutate(&corev1.Service{})) + assert.Equal(t, []string{"flavor1", "flavor2"}, order) + }) + + t.Run("flavor error is returned", func(t *testing.T) { + flavorErr := errors.New("flavor boom") + res, err := NewBuilder(desired). + WithFieldApplicationFlavor(func(_, _, _ *corev1.Service) error { + return flavorErr + }). + Build() + require.NoError(t, err) + + err = res.Mutate(&corev1.Service{}) + require.Error(t, err) + assert.True(t, errors.Is(err, flavorErr)) + }) +} diff --git a/pkg/primitives/service/handlers.go b/pkg/primitives/service/handlers.go new file mode 100644 index 00000000..19e7feab --- /dev/null +++ b/pkg/primitives/service/handlers.go @@ -0,0 +1,77 @@ +package service + +import ( + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + corev1 "k8s.io/api/core/v1" +) + +// DefaultOperationalStatusHandler is the default logic for determining if a Service is operational. +// +// For LoadBalancer services, it reports OperationalStatusPending until at least one ingress +// entry (IP or hostname) is assigned to Status.LoadBalancer.Ingress, then reports +// OperationalStatusOperational. +// +// For all other service types (ClusterIP, NodePort, ExternalName, headless), it +// immediately reports OperationalStatusOperational. +// +// This function is used as the default handler by the Resource if no custom handler is +// registered via Builder.WithCustomOperationalStatus. It can be reused within custom +// handlers to augment the default behavior. +func DefaultOperationalStatusHandler( + _ concepts.ConvergingOperation, svc *corev1.Service, +) (concepts.OperationalStatusWithReason, error) { + if svc.Spec.Type == corev1.ServiceTypeLoadBalancer { + if len(svc.Status.LoadBalancer.Ingress) == 0 { + return concepts.OperationalStatusWithReason{ + Status: concepts.OperationalStatusPending, + Reason: "Awaiting load balancer IP assignment", + }, nil + } + return concepts.OperationalStatusWithReason{ + Status: concepts.OperationalStatusOperational, + Reason: "Load balancer IP address assigned", + }, nil + } + + return concepts.OperationalStatusWithReason{ + Status: concepts.OperationalStatusOperational, + Reason: "Service is operational", + }, nil +} + +// DefaultDeleteOnSuspendHandler provides the default decision of whether to delete the Service +// when the parent component is suspended. +// +// It always returns true because Services have no meaningful "scaled down" state and should +// be deleted when the component is suspended. +// +// This function is used as the default handler by the Resource if no custom handler is registered via +// Builder.WithCustomSuspendDeletionDecision. It can be reused within custom handlers. +func DefaultDeleteOnSuspendHandler(_ *corev1.Service) bool { + return true +} + +// DefaultSuspendMutationHandler provides the default mutation applied to a Service when the +// component is suspended. +// +// It is a no-op because Services are deleted on suspend rather than mutated. +// +// This function is used as the default handler by the Resource if no custom handler is registered via +// Builder.WithCustomSuspendMutation. It can be reused within custom handlers. +func DefaultSuspendMutationHandler(_ *Mutator) error { + return nil +} + +// DefaultSuspensionStatusHandler reports the progress of the suspension process. +// +// It always reports Suspended because the Service is deleted on suspend. This handler +// is called before the deletion to confirm that the resource is ready to be removed. +// +// This function is used as the default handler by the Resource if no custom handler is registered via +// Builder.WithCustomSuspendStatus. It can be reused within custom handlers. +func DefaultSuspensionStatusHandler(_ *corev1.Service) (concepts.SuspensionStatusWithReason, error) { + return concepts.SuspensionStatusWithReason{ + Status: concepts.SuspensionStatusSuspended, + Reason: "Service deleted on suspend", + }, nil +} diff --git a/pkg/primitives/service/handlers_test.go b/pkg/primitives/service/handlers_test.go new file mode 100644 index 00000000..6ed56829 --- /dev/null +++ b/pkg/primitives/service/handlers_test.go @@ -0,0 +1,149 @@ +package service + +import ( + "testing" + + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" +) + +func TestDefaultOperationalStatusHandler(t *testing.T) { + tests := []struct { + name string + svc *corev1.Service + wantStatus concepts.OperationalStatus + wantReason string + }{ + { + name: "ClusterIP is immediately operational", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + }, + }, + wantStatus: concepts.OperationalStatusOperational, + wantReason: "Service is operational", + }, + { + name: "NodePort is immediately operational", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + }, + }, + wantStatus: concepts.OperationalStatusOperational, + wantReason: "Service is operational", + }, + { + name: "ExternalName is immediately operational", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeExternalName, + }, + }, + wantStatus: concepts.OperationalStatusOperational, + wantReason: "Service is operational", + }, + { + name: "headless service (empty type) is immediately operational", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{}, + }, + wantStatus: concepts.OperationalStatusOperational, + wantReason: "Service is operational", + }, + { + name: "LoadBalancer pending (no ingress)", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + Status: corev1.ServiceStatus{}, + }, + wantStatus: concepts.OperationalStatusPending, + wantReason: "Awaiting load balancer IP assignment", + }, + { + name: "LoadBalancer pending (empty ingress slice)", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{}, + }, + }, + }, + wantStatus: concepts.OperationalStatusPending, + wantReason: "Awaiting load balancer IP assignment", + }, + { + name: "LoadBalancer operational (IP assigned)", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + {IP: "203.0.113.10"}, + }, + }, + }, + }, + wantStatus: concepts.OperationalStatusOperational, + wantReason: "Load balancer IP address assigned", + }, + { + name: "LoadBalancer operational (hostname assigned)", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + {Hostname: "abc123.elb.amazonaws.com"}, + }, + }, + }, + }, + wantStatus: concepts.OperationalStatusOperational, + wantReason: "Load balancer IP address assigned", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := DefaultOperationalStatusHandler(concepts.ConvergingOperationNone, tt.svc) + require.NoError(t, err) + assert.Equal(t, tt.wantStatus, got.Status) + assert.Equal(t, tt.wantReason, got.Reason) + }) + } +} + +func TestDefaultDeleteOnSuspendHandler(t *testing.T) { + svc := &corev1.Service{} + assert.True(t, DefaultDeleteOnSuspendHandler(svc)) +} + +func TestDefaultSuspendMutationHandler(t *testing.T) { + svc := &corev1.Service{} + mutator := NewMutator(svc) + err := DefaultSuspendMutationHandler(mutator) + require.NoError(t, err) + // No-op, so service should be unchanged + require.NoError(t, mutator.Apply()) +} + +func TestDefaultSuspensionStatusHandler(t *testing.T) { + svc := &corev1.Service{} + got, err := DefaultSuspensionStatusHandler(svc) + require.NoError(t, err) + assert.Equal(t, concepts.SuspensionStatusSuspended, got.Status) + assert.Equal(t, "Service deleted on suspend", got.Reason) +} diff --git a/pkg/primitives/service/mutator.go b/pkg/primitives/service/mutator.go new file mode 100644 index 00000000..a39d5645 --- /dev/null +++ b/pkg/primitives/service/mutator.go @@ -0,0 +1,106 @@ +package service + +import ( + "github.com/sourcehawk/operator-component-framework/pkg/feature" + "github.com/sourcehawk/operator-component-framework/pkg/mutation/editors" + corev1 "k8s.io/api/core/v1" +) + +// Mutation defines a mutation that is applied to a service Mutator +// only if its associated feature gate is enabled. +type Mutation feature.Mutation[*Mutator] + +type featurePlan struct { + metadataEdits []func(*editors.ObjectMetaEditor) error + serviceSpecEdits []func(*editors.ServiceSpecEditor) error +} + +// Mutator is a high-level helper for modifying a Kubernetes Service. +// +// It uses a "plan-and-apply" pattern: mutations are recorded first, then +// applied to the Service in a single controlled pass when Apply() is called. +// +// The Mutator maintains feature boundaries: each feature's mutations are planned +// together and applied in the order the features were registered. +// +// Mutator implements editors.ObjectMutator. +type Mutator struct { + svc *corev1.Service + + plans []featurePlan + active *featurePlan +} + +// NewMutator creates a new Mutator for the given Service. +func NewMutator(svc *corev1.Service) *Mutator { + m := &Mutator{svc: svc} + m.beginFeature() + return m +} + +// beginFeature starts a new feature planning scope. All subsequent mutation +// registrations will be grouped into this feature's plan. +func (m *Mutator) beginFeature() { + m.plans = append(m.plans, featurePlan{}) + m.active = &m.plans[len(m.plans)-1] +} + +// EditObjectMetadata records a mutation for the Service's own metadata. +// +// Metadata edits are applied before service spec edits within the same feature. +// A nil edit function is ignored. +func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) { + if edit == nil { + return + } + m.active.metadataEdits = append(m.active.metadataEdits, edit) +} + +// EditServiceSpec records a mutation for the Service's spec. +// +// The editor provides structured operations (SetType, EnsurePort, SetSelector, +// etc.) as well as Raw() for free-form access. Service spec edits are applied +// after metadata edits within the same feature, in registration order. +// +// A nil edit function is ignored. +func (m *Mutator) EditServiceSpec(edit func(*editors.ServiceSpecEditor) error) { + if edit == nil { + return + } + m.active.serviceSpecEdits = append(m.active.serviceSpecEdits, edit) +} + +// Apply executes all recorded mutation intents on the underlying Service. +// +// Execution order across all registered features: +// +// 1. Metadata edits (in registration order within each feature) +// 2. Service spec edits (in registration order within each feature) +// +// Features are applied in the order they were registered. Later features observe +// the Service as modified by all previous features. +func (m *Mutator) Apply() error { + for _, plan := range m.plans { + // 1. Metadata edits + if len(plan.metadataEdits) > 0 { + editor := editors.NewObjectMetaEditor(&m.svc.ObjectMeta) + for _, edit := range plan.metadataEdits { + if err := edit(editor); err != nil { + return err + } + } + } + + // 2. Service spec edits + if len(plan.serviceSpecEdits) > 0 { + editor := editors.NewServiceSpecEditor(&m.svc.Spec) + for _, edit := range plan.serviceSpecEdits { + if err := edit(editor); err != nil { + return err + } + } + } + } + + return nil +} diff --git a/pkg/primitives/service/mutator_test.go b/pkg/primitives/service/mutator_test.go new file mode 100644 index 00000000..b7687432 --- /dev/null +++ b/pkg/primitives/service/mutator_test.go @@ -0,0 +1,181 @@ +package service + +import ( + "errors" + "testing" + + "github.com/sourcehawk/operator-component-framework/pkg/mutation/editors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func newTestService() *corev1.Service { + return &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "default", + }, + } +} + +// --- EditObjectMetadata --- + +func TestMutator_EditObjectMetadata(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("app", "myapp") + return nil + }) + require.NoError(t, m.Apply()) + assert.Equal(t, "myapp", svc.Labels["app"]) +} + +func TestMutator_EditObjectMetadata_Nil(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditObjectMetadata(nil) + assert.NoError(t, m.Apply()) +} + +func TestMutator_EditObjectMetadata_Error(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditObjectMetadata(func(_ *editors.ObjectMetaEditor) error { + return errors.New("metadata error") + }) + err := m.Apply() + require.Error(t, err) + assert.Contains(t, err.Error(), "metadata error") +} + +// --- EditServiceSpec --- + +func TestMutator_EditServiceSpec(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.SetType(corev1.ServiceTypeNodePort) + return nil + }) + require.NoError(t, m.Apply()) + assert.Equal(t, corev1.ServiceTypeNodePort, svc.Spec.Type) +} + +func TestMutator_EditServiceSpec_Nil(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditServiceSpec(nil) + assert.NoError(t, m.Apply()) +} + +func TestMutator_EditServiceSpec_Error(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditServiceSpec(func(_ *editors.ServiceSpecEditor) error { + return errors.New("spec error") + }) + err := m.Apply() + require.Error(t, err) + assert.Contains(t, err.Error(), "spec error") +} + +func TestMutator_EditServiceSpec_Ports(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + e.EnsurePort(corev1.ServicePort{Name: "https", Port: 443}) + return nil + }) + require.NoError(t, m.Apply()) + assert.Len(t, svc.Spec.Ports, 2) +} + +func TestMutator_EditServiceSpec_Selector(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsureSelector("app", "myapp") + return nil + }) + require.NoError(t, m.Apply()) + assert.Equal(t, "myapp", svc.Spec.Selector["app"]) +} + +func TestMutator_EditServiceSpec_RawAccess(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.Raw().ExternalName = "external.example.com" + return nil + }) + require.NoError(t, m.Apply()) + assert.Equal(t, "external.example.com", svc.Spec.ExternalName) +} + +// --- Execution order --- + +func TestMutator_OperationOrder(t *testing.T) { + // Within a feature: metadata edits run before service spec edits. + svc := newTestService() + m := NewMutator(svc) + // Register in reverse logical order to confirm Apply() enforces category ordering. + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + return nil + }) + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("order", "tested") + return nil + }) + require.NoError(t, m.Apply()) + + assert.Equal(t, "tested", svc.Labels["order"]) + assert.Len(t, svc.Spec.Ports, 1) + assert.Equal(t, "http", svc.Spec.Ports[0].Name) +} + +func TestMutator_MultipleFeatures(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + return nil + }) + m.beginFeature() + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "https", Port: 443}) + return nil + }) + require.NoError(t, m.Apply()) + + assert.Len(t, svc.Spec.Ports, 2) +} + +func TestMutator_MultipleFeatures_LaterSeesEarlier(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsureSelector("app", "myapp") + return nil + }) + m.beginFeature() + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + // Later feature should see the selector set by the earlier feature. + e.EnsureSelector("env", "prod") + return nil + }) + require.NoError(t, m.Apply()) + + assert.Equal(t, "myapp", svc.Spec.Selector["app"]) + assert.Equal(t, "prod", svc.Spec.Selector["env"]) +} + +// --- ObjectMutator interface --- + +func TestMutator_ImplementsObjectMutator(_ *testing.T) { + var _ editors.ObjectMutator = (*Mutator)(nil) +} diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go new file mode 100644 index 00000000..8e0f2ba0 --- /dev/null +++ b/pkg/primitives/service/resource.go @@ -0,0 +1,106 @@ +// Package service provides a builder and resource for managing Kubernetes Services. +package service + +import ( + "github.com/sourcehawk/operator-component-framework/internal/generic" + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// DefaultFieldApplicator replaces current with a deep copy of desired, preserving +// the immutable spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns +// after creation. +func DefaultFieldApplicator(current, desired *corev1.Service) error { + clusterIP := current.Spec.ClusterIP + clusterIPs := current.Spec.ClusterIPs + *current = *desired.DeepCopy() + if clusterIP != "" { + current.Spec.ClusterIP = clusterIP + current.Spec.ClusterIPs = clusterIPs + } + return nil +} + +// Resource is a high-level abstraction for managing a Kubernetes Service within +// a controller's reconciliation loop. +// +// It implements the following component interfaces: +// - component.Resource: for basic identity and mutation behaviour. +// - component.Operational: for tracking whether the Service is operational. +// - component.Suspendable: for controlled deletion when the component is suspended. +// - component.DataExtractable: for exporting values after successful reconciliation. +type Resource struct { + base *generic.IntegrationResource[*corev1.Service, *Mutator] +} + +// Identity returns a unique identifier for the Service in the format +// "v1/Service//". +func (r *Resource) Identity() string { + return r.base.Identity() +} + +// Object returns a deep copy of the underlying Kubernetes Service object. +// +// The returned object implements client.Object, making it compatible with +// controller-runtime's Client for Create, Update, and Patch operations. +func (r *Resource) Object() (client.Object, error) { + return r.base.Object() +} + +// Mutate transforms the current state of a Kubernetes Service into the desired state. +// +// The mutation process follows this order: +// 1. Field application: the current object is updated to reflect the desired base state, +// using either DefaultFieldApplicator or a custom applicator if one is configured. +// The default applicator preserves spec.clusterIP and spec.clusterIPs. +// 2. Field application flavors: any registered flavors are applied in registration order. +// 3. Feature mutations: all registered feature-gated mutations are applied in order. +// 4. Suspension: if the resource is in a suspending state, the suspension mutation is applied. +func (r *Resource) Mutate(current client.Object) error { + return r.base.Mutate(current) +} + +// ConvergingStatus reports the operational status of the Service. +// +// By default, it uses DefaultOperationalStatusHandler, which considers LoadBalancer +// services pending until an ingress IP or hostname is assigned, and all other service +// types immediately operational. +func (r *Resource) ConvergingStatus(op concepts.ConvergingOperation) (concepts.OperationalStatusWithReason, error) { + return r.base.ConvergingStatus(op) +} + +// DeleteOnSuspend determines whether the Service should be deleted from the +// cluster when the parent component is suspended. +// +// By default, it uses DefaultDeleteOnSuspendHandler, which returns true — Services +// are deleted on suspend because they have no meaningful "scaled down" state. +func (r *Resource) DeleteOnSuspend() bool { + return r.base.DeleteOnSuspend() +} + +// Suspend triggers the suspension of the Service. +// +// The default handler is a no-op because Services are deleted on suspend +// rather than mutated. +func (r *Resource) Suspend() error { + return r.base.Suspend() +} + +// SuspensionStatus reports the progress of the suspension process. +// +// By default, it uses DefaultSuspensionStatusHandler, which always reports +// Suspended with a reason indicating the Service is deleted on suspend. +func (r *Resource) SuspensionStatus() (concepts.SuspensionStatusWithReason, error) { + return r.base.SuspensionStatus() +} + +// ExtractData executes all registered data extractor functions against a deep copy +// of the reconciled Service. +// +// This is called by the framework after successful reconciliation, allowing the +// component to read generated or updated values such as assigned ClusterIP or +// LoadBalancer ingress from the Service. +func (r *Resource) ExtractData() error { + return r.base.ExtractData() +} From 363ba66cf3008bc54dcc70b9bc256eabffe13343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:43:00 +0000 Subject: [PATCH 03/28] Add Service primitive documentation Covers building, mutations, editors, flavors, operational status, suspension behavior, data extraction, and usage guidance. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 342 +++++++++++++++++++++++++++++++++++++ 1 file changed, 342 insertions(+) create mode 100644 docs/primitives/service.md diff --git a/docs/primitives/service.md b/docs/primitives/service.md new file mode 100644 index 00000000..b782deed --- /dev/null +++ b/docs/primitives/service.md @@ -0,0 +1,342 @@ +# Service Primitive + +The `service` primitive is the framework's built-in integration abstraction for managing Kubernetes `Service` resources. It integrates with the component lifecycle and provides a structured mutation API for managing ports, selectors, and service configuration. + +## Capabilities + +| Capability | Detail | +|-------------------------|-----------------------------------------------------------------------------------------------------| +| **Operational tracking** | Monitors LoadBalancer ingress assignment; reports `Operational` or `Pending` | +| **Suspension** | Deletes the Service on suspend (no meaningful "scaled down" state) | +| **Mutation pipeline** | Typed editors for metadata and service spec, with a raw escape hatch for free-form access | +| **Flavors** | Preserves externally-managed fields (labels, annotations) | +| **Data extraction** | Reads generated or updated values (ClusterIP, LoadBalancer ingress) after each sync cycle | + +## Building a Service Primitive + +```go +import "github.com/sourcehawk/operator-component-framework/pkg/primitives/service" + +base := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "app-service", + Namespace: owner.Namespace, + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{"app": owner.Name}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, TargetPort: intstr.FromInt32(8080)}, + }, + }, +} + +resource, err := service.NewBuilder(base). + WithFieldApplicationFlavor(service.PreserveCurrentAnnotations). + WithMutation(MyFeatureMutation(owner.Spec.Version)). + Build() +``` + +## Default Field Application + +`DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving the immutable `spec.clusterIP` and `spec.clusterIPs` fields that Kubernetes assigns after creation. + +This prevents the operator from clearing the cluster-assigned IP on every reconciliation cycle while ensuring all other fields converge to the desired state. + +Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: + +```go +resource, err := service.NewBuilder(base). + WithCustomFieldApplicator(func(current, desired *corev1.Service) error { + // Only synchronise owned fields; leave other fields untouched. + current.Spec.Ports = desired.DeepCopy().Spec.Ports + return nil + }). + Build() +``` + +## Mutations + +Mutations are the primary mechanism for modifying a `Service` beyond its baseline. Each mutation is a named function that receives a `*Mutator` and records edit intent through typed editors. + +The `Feature` field controls when a mutation applies. Leaving it nil applies the mutation unconditionally. A feature with no version constraints and no `When()` conditions is also always enabled: + +```go +func MyFeatureMutation(version string) service.Mutation { + return service.Mutation{ + Name: "my-feature", + Feature: feature.NewResourceFeature(version, nil), // always enabled + Mutate: func(m *service.Mutator) error { + // record edits here + return nil + }, + } +} +``` + +Mutations are applied in the order they are registered with the builder. If one mutation depends on a change made by another, register the dependency first. + +### Boolean-gated mutations + +Use `When(bool)` to gate a mutation on a runtime condition: + +```go +func NodePortMutation(version string, enabled bool) service.Mutation { + return service.Mutation{ + Name: "nodeport", + Feature: feature.NewResourceFeature(version, nil).When(enabled), + Mutate: func(m *service.Mutator) error { + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.SetType(corev1.ServiceTypeNodePort) + return nil + }) + return nil + }, + } +} +``` + +### Version-gated mutations + +Pass a `[]feature.VersionConstraint` to gate on a semver range: + +```go +var legacyConstraint = mustSemverConstraint("< 2.0.0") + +func LegacyPortMutation(version string) service.Mutation { + return service.Mutation{ + Name: "legacy-port", + Feature: feature.NewResourceFeature( + version, + []feature.VersionConstraint{legacyConstraint}, + ), + Mutate: func(m *service.Mutator) error { + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "legacy", Port: 9090}) + return nil + }) + return nil + }, + } +} +``` + +All version constraints and `When()` conditions must be satisfied for a mutation to apply. + +## Internal Mutation Ordering + +Within a single mutation, edit operations are grouped into categories and applied in a fixed sequence regardless of the order they are recorded: + +| Step | Category | What it affects | +|------|--------------------|--------------------------------------------| +| 1 | Metadata edits | Labels and annotations on the `Service` | +| 2 | ServiceSpec edits | Ports, selectors, type, traffic policies | + +Within each category, edits are applied in their registration order. Later features observe the Service as modified by all previous features. + +## Editors + +### ServiceSpecEditor + +Controls service-level settings via `m.EditServiceSpec`. + +Available methods: `SetType`, `EnsurePort`, `RemovePort`, `SetSelector`, `EnsureSelector`, `RemoveSelector`, `SetSessionAffinity`, `SetSessionAffinityConfig`, `SetPublishNotReadyAddresses`, `SetExternalTrafficPolicy`, `SetInternalTrafficPolicy`, `SetLoadBalancerSourceRanges`, `SetExternalName`, `Raw`. + +```go +m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.SetType(corev1.ServiceTypeLoadBalancer) + e.EnsurePort(corev1.ServicePort{ + Name: "https", + Port: 443, + TargetPort: intstr.FromInt32(8443), + }) + e.SetExternalTrafficPolicy(corev1.ServiceExternalTrafficPolicyLocal) + return nil +}) +``` + +#### Port Management + +`EnsurePort` upserts a port: if a port with the same `Name` exists (or the same `Port` number when `Name` is empty), it is replaced; otherwise it is appended. `RemovePort` removes a port by name. + +```go +m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + e.RemovePort("legacy") + return nil +}) +``` + +#### Selector Management + +`SetSelector` replaces the entire selector map. `EnsureSelector` adds or updates a single key-value pair. `RemoveSelector` removes a single key. + +```go +m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsureSelector("app", "myapp") + e.EnsureSelector("env", "production") + return nil +}) +``` + +For fields not covered by the typed API, use `Raw()`: + +```go +m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.Raw().HealthCheckNodePort = 30000 + return nil +}) +``` + +### ObjectMetaEditor + +Modifies labels and annotations via `m.EditObjectMetadata`. + +Available methods: `EnsureLabel`, `RemoveLabel`, `EnsureAnnotation`, `RemoveAnnotation`, `Raw`. + +```go +m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("app.kubernetes.io/version", version) + e.EnsureAnnotation("service.beta.kubernetes.io/aws-load-balancer-type", "nlb") + return nil +}) +``` + +## Flavors + +Flavors run after the baseline applicator and before mutations. They are used to preserve fields managed by external controllers or other tools. + +### PreserveCurrentLabels + +Preserves labels present on the live object but absent from the applied desired state. Applied labels win on overlap. + +```go +resource, err := service.NewBuilder(base). + WithFieldApplicationFlavor(service.PreserveCurrentLabels). + Build() +``` + +### PreserveCurrentAnnotations + +Preserves annotations present on the live object but absent from the applied desired state. Applied annotations win on overlap. + +```go +resource, err := service.NewBuilder(base). + WithFieldApplicationFlavor(service.PreserveCurrentAnnotations). + Build() +``` + +Multiple flavors can be registered and run in registration order. + +## Operational Status + +The Service primitive implements the `Operational` concept to track whether the Service is ready to accept traffic. + +### DefaultOperationalStatusHandler + +| Service Type | Behaviour | +|----------------|---------------------------------------------------------------------------| +| `LoadBalancer` | Reports `OperationPending` until `Status.LoadBalancer.Ingress` has entries; then `Operational` | +| `ClusterIP` | Immediately `Operational` | +| `NodePort` | Immediately `Operational` | +| `ExternalName` | Immediately `Operational` | +| Headless | Immediately `Operational` | + +Override with `WithCustomOperationalStatus` to add custom checks: + +```go +resource, err := service.NewBuilder(base). + WithCustomOperationalStatus(func(op concepts.ConvergingOperation, svc *corev1.Service) (concepts.OperationalStatusWithReason, error) { + // Custom logic, e.g. check for specific annotations + return service.DefaultOperationalStatusHandler(op, svc) + }). + Build() +``` + +## Suspension + +By default, Services are **deleted** when the parent component is suspended (`DefaultDeleteOnSuspendHandler` returns `true`). This is because Services have no meaningful "scaled down" state — they are either present and routing traffic, or absent. + +The default suspend mutation handler is a no-op, and the default suspension status handler always reports `Suspended` with reason "Service deleted on suspend". + +Override with `WithCustomSuspendDeletionDecision` if you want to keep the Service during suspension: + +```go +resource, err := service.NewBuilder(base). + WithCustomSuspendDeletionDecision(func(_ *corev1.Service) bool { + return false // keep the Service during suspension + }). + Build() +``` + +## Data Extraction + +Use `WithDataExtractor` to read values from the reconciled Service, such as the assigned ClusterIP or LoadBalancer ingress: + +```go +var assignedIP string + +resource, err := service.NewBuilder(base). + WithDataExtractor(func(svc corev1.Service) error { + assignedIP = svc.Spec.ClusterIP + return nil + }). + Build() +``` + +## Full Example: Feature-Composed Service + +```go +func BaseServiceMutation(version string) service.Mutation { + return service.Mutation{ + Name: "base-service", + Feature: feature.NewResourceFeature(version, nil), + Mutate: func(m *service.Mutator) error { + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{ + Name: "http", + Port: 80, + TargetPort: intstr.FromInt32(8080), + }) + return nil + }) + return nil + }, + } +} + +func MetricsPortMutation(version string, enabled bool) service.Mutation { + return service.Mutation{ + Name: "metrics-port", + Feature: feature.NewResourceFeature(version, nil).When(enabled), + Mutate: func(m *service.Mutator) error { + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{ + Name: "metrics", + Port: 9090, + TargetPort: intstr.FromInt32(9090), + }) + return nil + }) + return nil + }, + } +} + +resource, err := service.NewBuilder(base). + WithFieldApplicationFlavor(service.PreserveCurrentAnnotations). + WithMutation(BaseServiceMutation(owner.Spec.Version)). + WithMutation(MetricsPortMutation(owner.Spec.Version, owner.Spec.MetricsEnabled)). + Build() +``` + +When `MetricsEnabled` is true, the Service will expose both the HTTP port and the metrics port. When false, only the HTTP port is configured. Neither mutation needs to know about the other. + +## Guidance + +**`Feature: nil` applies unconditionally.** Omit `Feature` (leave it nil) for mutations that should always run. Use `feature.NewResourceFeature(version, constraints)` when version-based gating is needed, and chain `.When(bool)` for boolean conditions. + +**Register mutations in dependency order.** If mutation B relies on a port added by mutation A, register A first. + +**Use `EnsurePort` for idempotent port management.** The mutator tracks ports by name (or port number when unnamed), so repeated calls with the same name produce the same result. + +**Use `PreserveCurrentAnnotations` when cloud providers annotate Services.** Cloud load balancer controllers often add annotations to Services. This flavor prevents your operator from deleting those annotations each reconcile cycle. From d7390a6b34d8e37d22eae9cb903145b03470a7ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:44:28 +0000 Subject: [PATCH 04/28] Add Service primitive example Demonstrates base construction, feature-gated mutations (version labels, metrics port), annotation preservation, operational status tracking, suspension, and data extraction with a fake client. Co-Authored-By: Claude Opus 4.6 (1M context) --- examples/service-primitive/README.md | 31 +++++ examples/service-primitive/app/controller.go | 55 +++++++++ .../service-primitive/features/mutations.go | 46 +++++++ examples/service-primitive/main.go | 115 ++++++++++++++++++ .../service-primitive/resources/service.go | 63 ++++++++++ 5 files changed, 310 insertions(+) create mode 100644 examples/service-primitive/README.md create mode 100644 examples/service-primitive/app/controller.go create mode 100644 examples/service-primitive/features/mutations.go create mode 100644 examples/service-primitive/main.go create mode 100644 examples/service-primitive/resources/service.go diff --git a/examples/service-primitive/README.md b/examples/service-primitive/README.md new file mode 100644 index 00000000..63276b27 --- /dev/null +++ b/examples/service-primitive/README.md @@ -0,0 +1,31 @@ +# Service Primitive Example + +This example demonstrates the usage of the `service` primitive within the operator component framework. +It shows how to manage a Kubernetes Service as a component of a larger application, utilising features like: + +- **Base Construction**: Initializing a Service with basic metadata, selector, and ports. +- **Feature Mutations**: Applying version-gated or conditional changes (additional ports, labels) using the `Mutator`. +- **Field Flavors**: Preserving annotations that might be managed by external tools (e.g., cloud load balancer controllers). +- **Operational Status**: Tracking whether the Service is operational (relevant for LoadBalancer types). +- **Suspension**: Deleting the Service when the component is suspended. +- **Data Extraction**: Harvesting information (ClusterIP, ports) from the reconciled resource. + +## Directory Structure + +- `app/`: Defines the controller that uses the component framework. The `ExampleApp` CRD is shared from `examples/shared/app`. +- `features/`: Contains modular feature definitions: + - `mutations.go`: version labelling and conditional metrics port. +- `resources/`: Contains the central `NewServiceResource` factory that assembles all features using `service.Builder`. +- `main.go`: A standalone entry point that demonstrates multiple reconciliation cycles with a fake client. + +## Running the Example + +```bash +go run examples/service-primitive/main.go +``` + +This will: +1. Initialize a fake Kubernetes client. +2. Create an `ExampleApp` owner object. +3. Reconcile through four spec variations, printing the Service ports after each cycle. +4. Print the resulting status conditions. diff --git a/examples/service-primitive/app/controller.go b/examples/service-primitive/app/controller.go new file mode 100644 index 00000000..d903e715 --- /dev/null +++ b/examples/service-primitive/app/controller.go @@ -0,0 +1,55 @@ +// Package app provides a sample controller using the service primitive. +package app + +import ( + "context" + + sharedapp "github.com/sourcehawk/operator-component-framework/examples/shared/app" + "github.com/sourcehawk/operator-component-framework/pkg/component" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ExampleController reconciles an ExampleApp object using the component framework. +type ExampleController struct { + client.Client + Scheme *runtime.Scheme + Recorder record.EventRecorder + Metrics component.Recorder + + // NewServiceResource is a factory function to create the service resource. + // This allows us to inject the resource construction logic. + NewServiceResource func(*sharedapp.ExampleApp) (component.Resource, error) +} + +// Reconcile performs the reconciliation for a single ExampleApp. +func (r *ExampleController) Reconcile(ctx context.Context, owner *sharedapp.ExampleApp) error { + // 1. Build the service resource for this owner. + svcResource, err := r.NewServiceResource(owner) + if err != nil { + return err + } + + // 2. Build the component that manages the service. + comp, err := component.NewComponentBuilder(). + WithName("example-app"). + WithConditionType("AppReady"). + WithResource(svcResource, component.ResourceOptions{}). + Suspend(owner.Spec.Suspended). + Build() + if err != nil { + return err + } + + // 3. Execute the component reconciliation. + resCtx := component.ReconcileContext{ + Client: r.Client, + Scheme: r.Scheme, + Recorder: r.Recorder, + Metrics: r.Metrics, + Owner: owner, + } + + return comp.Reconcile(ctx, resCtx) +} diff --git a/examples/service-primitive/features/mutations.go b/examples/service-primitive/features/mutations.go new file mode 100644 index 00000000..e7de2640 --- /dev/null +++ b/examples/service-primitive/features/mutations.go @@ -0,0 +1,46 @@ +// Package features provides sample mutations for the service primitive example. +package features + +import ( + "github.com/sourcehawk/operator-component-framework/pkg/feature" + "github.com/sourcehawk/operator-component-framework/pkg/mutation/editors" + "github.com/sourcehawk/operator-component-framework/pkg/primitives/service" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/intstr" +) + +// VersionLabelMutation sets the app.kubernetes.io/version label on the Service. +// It is always enabled. +func VersionLabelMutation(version string) service.Mutation { + return service.Mutation{ + Name: "version-label", + Feature: feature.NewResourceFeature(version, nil), + Mutate: func(m *service.Mutator) error { + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("app.kubernetes.io/version", version) + return nil + }) + return nil + }, + } +} + +// MetricsPortMutation adds a metrics port to the Service when metrics are enabled. +func MetricsPortMutation(version string, enableMetrics bool) service.Mutation { + return service.Mutation{ + Name: "metrics-port", + Feature: feature.NewResourceFeature(version, nil).When(enableMetrics), + Mutate: func(m *service.Mutator) error { + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{ + Name: "metrics", + Port: 9090, + TargetPort: intstr.FromInt32(9090), + Protocol: corev1.ProtocolTCP, + }) + return nil + }) + return nil + }, + } +} diff --git a/examples/service-primitive/main.go b/examples/service-primitive/main.go new file mode 100644 index 00000000..68c01114 --- /dev/null +++ b/examples/service-primitive/main.go @@ -0,0 +1,115 @@ +// Package main is the entry point for the service primitive example. +package main + +import ( + "context" + "fmt" + "os" + + ocm "github.com/sourcehawk/go-crd-condition-metrics/pkg/crd-condition-metrics" + "github.com/sourcehawk/operator-component-framework/examples/service-primitive/app" + "github.com/sourcehawk/operator-component-framework/examples/service-primitive/resources" + sharedapp "github.com/sourcehawk/operator-component-framework/examples/shared/app" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func main() { + // 1. Setup scheme and fake client. + scheme := runtime.NewScheme() + if err := sharedapp.AddToScheme(scheme); err != nil { + fmt.Fprintf(os.Stderr, "failed to add to scheme: %v\n", err) + os.Exit(1) + } + if err := corev1.AddToScheme(scheme); err != nil { + fmt.Fprintf(os.Stderr, "failed to add core/v1 to scheme: %v\n", err) + os.Exit(1) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(&sharedapp.ExampleApp{}). + Build() + + // 2. Create an example Owner object. + owner := &sharedapp.ExampleApp{ + Spec: sharedapp.ExampleAppSpec{ + Version: "1.2.3", + EnableMetrics: true, + Suspended: false, + }, + } + owner.Name = "my-example-app" + owner.Namespace = "default" + + if err := fakeClient.Create(context.Background(), owner); err != nil { + fmt.Fprintf(os.Stderr, "failed to create owner: %v\n", err) + os.Exit(1) + } + + // 3. Initialize the controller. + gauge := ocm.NewOperatorConditionsGauge("example") + controller := &app.ExampleController{ + Client: fakeClient, + Scheme: scheme, + Recorder: record.NewFakeRecorder(100), + Metrics: &ocm.ConditionMetricRecorder{ + Controller: "example-controller", + OperatorConditionsGauge: gauge, + }, + NewServiceResource: resources.NewServiceResource, + } + + // 4. Run reconciliation with multiple spec versions to demonstrate + // how mutations compose service configuration. + specs := []sharedapp.ExampleAppSpec{ + { + Version: "1.2.3", + EnableMetrics: true, + Suspended: false, + }, + { + Version: "1.2.4", // Version upgrade + EnableMetrics: true, + Suspended: false, + }, + { + Version: "1.2.4", + EnableMetrics: false, // Disable metrics + Suspended: false, + }, + { + Version: "1.2.4", + EnableMetrics: false, + Suspended: true, // Suspend the app + }, + } + + ctx := context.Background() + + for i, spec := range specs { + fmt.Printf("\n--- Step %d: Version=%s, Metrics=%v, Suspended=%v ---\n", + i+1, spec.Version, spec.EnableMetrics, spec.Suspended) + + owner.Spec = spec + if err := fakeClient.Update(ctx, owner); err != nil { + fmt.Fprintf(os.Stderr, "failed to update owner: %v\n", err) + os.Exit(1) + } + + fmt.Println("Running reconciliation...") + if err := controller.Reconcile(ctx, owner); err != nil { + fmt.Fprintf(os.Stderr, "reconciliation failed: %v\n", err) + os.Exit(1) + } + + for _, cond := range owner.Status.Conditions { + fmt.Printf("Condition: %s, Status: %s, Reason: %s\n", + cond.Type, cond.Status, cond.Reason) + } + } + + fmt.Println("\nReconciliation sequence completed successfully!") +} diff --git a/examples/service-primitive/resources/service.go b/examples/service-primitive/resources/service.go new file mode 100644 index 00000000..db7f4f82 --- /dev/null +++ b/examples/service-primitive/resources/service.go @@ -0,0 +1,63 @@ +// Package resources provides resource implementations for the service primitive example. +package resources + +import ( + "fmt" + + "github.com/sourcehawk/operator-component-framework/examples/service-primitive/features" + sharedapp "github.com/sourcehawk/operator-component-framework/examples/shared/app" + "github.com/sourcehawk/operator-component-framework/pkg/component" + "github.com/sourcehawk/operator-component-framework/pkg/primitives/service" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" +) + +// NewServiceResource constructs a service primitive resource with all the features. +func NewServiceResource(owner *sharedapp.ExampleApp) (component.Resource, error) { + // 1. Create the base Service object. + base := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: owner.Name + "-service", + Namespace: owner.Namespace, + Labels: map[string]string{ + "app": owner.Name, + }, + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{ + "app": owner.Name, + }, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + TargetPort: intstr.FromInt32(8080), + Protocol: corev1.ProtocolTCP, + }, + }, + }, + } + + // 2. Initialize the service builder. + builder := service.NewBuilder(base) + + // 3. Register mutations in dependency order. + builder.WithMutation(features.VersionLabelMutation(owner.Spec.Version)) + builder.WithMutation(features.MetricsPortMutation(owner.Spec.Version, owner.Spec.EnableMetrics)) + + // 4. Preserve annotations added by external controllers (e.g., cloud LB controllers). + builder.WithFieldApplicationFlavor(service.PreserveCurrentAnnotations) + + // 5. Extract data from the reconciled Service. + builder.WithDataExtractor(func(svc corev1.Service) error { + fmt.Printf("Reconciled Service: %s (ClusterIP: %s)\n", svc.Name, svc.Spec.ClusterIP) + for _, port := range svc.Spec.Ports { + fmt.Printf(" Port: %s %d -> %s\n", port.Name, port.Port, port.TargetPort.String()) + } + return nil + }) + + // 6. Build the final resource. + return builder.Build() +} From 7d716e4360dd6832739a5463515b999367627a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:52:13 +0000 Subject: [PATCH 05/28] Fix lint issues in ServiceSpecEditor Use non-deprecated ServiceExternalTrafficPolicy type and rename unused test parameter. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/mutation/editors/servicespec.go | 2 +- pkg/mutation/editors/servicespec_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/mutation/editors/servicespec.go b/pkg/mutation/editors/servicespec.go index 5663cf5d..e8f6fbaf 100644 --- a/pkg/mutation/editors/servicespec.go +++ b/pkg/mutation/editors/servicespec.go @@ -88,7 +88,7 @@ func (e *ServiceSpecEditor) SetPublishNotReadyAddresses(v bool) { } // SetExternalTrafficPolicy sets the external traffic policy for the service. -func (e *ServiceSpecEditor) SetExternalTrafficPolicy(policy corev1.ServiceExternalTrafficPolicyType) { +func (e *ServiceSpecEditor) SetExternalTrafficPolicy(policy corev1.ServiceExternalTrafficPolicy) { e.spec.ExternalTrafficPolicy = policy } diff --git a/pkg/mutation/editors/servicespec_test.go b/pkg/mutation/editors/servicespec_test.go index 8f3f8691..3d380c88 100644 --- a/pkg/mutation/editors/servicespec_test.go +++ b/pkg/mutation/editors/servicespec_test.go @@ -119,7 +119,7 @@ func TestServiceSpecEditor(t *testing.T) { assert.Equal(t, "prod", spec.Selector["env"]) }) - t.Run("RemoveSelector no-op on nil selector", func(t *testing.T) { + t.Run("RemoveSelector no-op on nil selector", func(_ *testing.T) { spec := &corev1.ServiceSpec{} editor := NewServiceSpecEditor(spec) editor.RemoveSelector("missing") // should not panic From 2bad8c3d8fccf700c707bd351c8457a25421fe96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 13:27:05 +0000 Subject: [PATCH 06/28] Address PR review: fix LoadBalancer status check, add resource tests, fix doc terminology - Fix DefaultOperationalStatusHandler to verify ingress entries have an actual IP or Hostname before reporting Operational (not just non-empty slice) - Add resource_test.go covering Resource wrapper behaviors (Identity, Object, Mutate with ClusterIP preservation, ConvergingStatus, Suspend, ExtractData) - Fix inconsistent terminology in service.md (OperationPending -> Pending) - Add test case for ingress entries without IP/Hostname Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 2 +- pkg/primitives/service/handlers.go | 15 +- pkg/primitives/service/handlers_test.go | 25 ++- pkg/primitives/service/resource_test.go | 210 ++++++++++++++++++++++++ 4 files changed, 244 insertions(+), 8 deletions(-) create mode 100644 pkg/primitives/service/resource_test.go diff --git a/docs/primitives/service.md b/docs/primitives/service.md index b782deed..327820cf 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -235,7 +235,7 @@ The Service primitive implements the `Operational` concept to track whether the | Service Type | Behaviour | |----------------|---------------------------------------------------------------------------| -| `LoadBalancer` | Reports `OperationPending` until `Status.LoadBalancer.Ingress` has entries; then `Operational` | +| `LoadBalancer` | Reports `Pending` until `Status.LoadBalancer.Ingress` has entries with an IP or hostname; then `Operational` | | `ClusterIP` | Immediately `Operational` | | `NodePort` | Immediately `Operational` | | `ExternalName` | Immediately `Operational` | diff --git a/pkg/primitives/service/handlers.go b/pkg/primitives/service/handlers.go index 19e7feab..8a787037 100644 --- a/pkg/primitives/service/handlers.go +++ b/pkg/primitives/service/handlers.go @@ -21,15 +21,24 @@ func DefaultOperationalStatusHandler( _ concepts.ConvergingOperation, svc *corev1.Service, ) (concepts.OperationalStatusWithReason, error) { if svc.Spec.Type == corev1.ServiceTypeLoadBalancer { - if len(svc.Status.LoadBalancer.Ingress) == 0 { + hasIngressAddress := false + for _, ing := range svc.Status.LoadBalancer.Ingress { + if ing.IP != "" || ing.Hostname != "" { + hasIngressAddress = true + break + } + } + + if !hasIngressAddress { return concepts.OperationalStatusWithReason{ Status: concepts.OperationalStatusPending, - Reason: "Awaiting load balancer IP assignment", + Reason: "Awaiting load balancer IP/hostname assignment", }, nil } + return concepts.OperationalStatusWithReason{ Status: concepts.OperationalStatusOperational, - Reason: "Load balancer IP address assigned", + Reason: "Load balancer IP/hostname assigned", }, nil } diff --git a/pkg/primitives/service/handlers_test.go b/pkg/primitives/service/handlers_test.go index 6ed56829..1a50409d 100644 --- a/pkg/primitives/service/handlers_test.go +++ b/pkg/primitives/service/handlers_test.go @@ -63,7 +63,7 @@ func TestDefaultOperationalStatusHandler(t *testing.T) { Status: corev1.ServiceStatus{}, }, wantStatus: concepts.OperationalStatusPending, - wantReason: "Awaiting load balancer IP assignment", + wantReason: "Awaiting load balancer IP/hostname assignment", }, { name: "LoadBalancer pending (empty ingress slice)", @@ -78,7 +78,24 @@ func TestDefaultOperationalStatusHandler(t *testing.T) { }, }, wantStatus: concepts.OperationalStatusPending, - wantReason: "Awaiting load balancer IP assignment", + wantReason: "Awaiting load balancer IP/hostname assignment", + }, + { + name: "LoadBalancer pending (ingress entry without IP or hostname)", + svc: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + {}, + }, + }, + }, + }, + wantStatus: concepts.OperationalStatusPending, + wantReason: "Awaiting load balancer IP/hostname assignment", }, { name: "LoadBalancer operational (IP assigned)", @@ -95,7 +112,7 @@ func TestDefaultOperationalStatusHandler(t *testing.T) { }, }, wantStatus: concepts.OperationalStatusOperational, - wantReason: "Load balancer IP address assigned", + wantReason: "Load balancer IP/hostname assigned", }, { name: "LoadBalancer operational (hostname assigned)", @@ -112,7 +129,7 @@ func TestDefaultOperationalStatusHandler(t *testing.T) { }, }, wantStatus: concepts.OperationalStatusOperational, - wantReason: "Load balancer IP address assigned", + wantReason: "Load balancer IP/hostname assigned", }, } diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go new file mode 100644 index 00000000..cffacc62 --- /dev/null +++ b/pkg/primitives/service/resource_test.go @@ -0,0 +1,210 @@ +package service + +import ( + "errors" + "testing" + + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + "github.com/sourcehawk/operator-component-framework/pkg/feature" + "github.com/sourcehawk/operator-component-framework/pkg/mutation/editors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func newValidService() *corev1.Service { + return &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + }, + } +} + +func TestResource_Identity(t *testing.T) { + res, err := NewBuilder(newValidService()).Build() + require.NoError(t, err) + assert.Equal(t, "v1/Service/test-ns/test-svc", res.Identity()) +} + +func TestResource_Object(t *testing.T) { + svc := newValidService() + res, err := NewBuilder(svc).Build() + require.NoError(t, err) + + obj, err := res.Object() + require.NoError(t, err) + + got, ok := obj.(*corev1.Service) + require.True(t, ok) + assert.Equal(t, svc.Name, got.Name) + assert.Equal(t, svc.Namespace, got.Namespace) + + // Must be a deep copy. + got.Name = "changed" + assert.Equal(t, "test-svc", svc.Name) +} + +func TestResource_Mutate(t *testing.T) { + desired := newValidService() + res, err := NewBuilder(desired).Build() + require.NoError(t, err) + + current := &corev1.Service{} + require.NoError(t, res.Mutate(current)) + + assert.Equal(t, "test", current.Spec.Selector["app"]) + assert.Equal(t, int32(80), current.Spec.Ports[0].Port) +} + +func TestResource_Mutate_PreservesClusterIP(t *testing.T) { + desired := newValidService() + res, err := NewBuilder(desired).Build() + require.NoError(t, err) + + current := &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + ClusterIPs: []string{"10.0.0.1"}, + }, + } + require.NoError(t, res.Mutate(current)) + + assert.Equal(t, "10.0.0.1", current.Spec.ClusterIP) + assert.Equal(t, []string{"10.0.0.1"}, current.Spec.ClusterIPs) + assert.Equal(t, "test", current.Spec.Selector["app"]) +} + +func TestResource_Mutate_WithMutation(t *testing.T) { + desired := newValidService() + res, err := NewBuilder(desired). + WithMutation(Mutation{ + Name: "add-port", + Feature: feature.NewResourceFeature("v1", nil).When(true), + Mutate: func(m *Mutator) error { + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "metrics", Port: 9090}) + return nil + }) + return nil + }, + }). + Build() + require.NoError(t, err) + + current := &corev1.Service{} + require.NoError(t, res.Mutate(current)) + + assert.Equal(t, int32(80), current.Spec.Ports[0].Port) + require.Len(t, current.Spec.Ports, 2) + assert.Equal(t, "metrics", current.Spec.Ports[1].Name) +} + +func TestResource_Mutate_CustomFieldApplicator(t *testing.T) { + desired := newValidService() + + applicatorCalled := false + res, err := NewBuilder(desired). + WithCustomFieldApplicator(func(current, d *corev1.Service) error { + applicatorCalled = true + current.Spec.Ports = d.DeepCopy().Spec.Ports + return nil + }). + Build() + require.NoError(t, err) + + current := &corev1.Service{ + Spec: corev1.ServiceSpec{ + Selector: map[string]string{"external": "preserved"}, + }, + } + require.NoError(t, res.Mutate(current)) + + assert.True(t, applicatorCalled) + assert.Equal(t, int32(80), current.Spec.Ports[0].Port) + assert.Equal(t, "preserved", current.Spec.Selector["external"]) +} + +func TestResource_Mutate_CustomFieldApplicator_Error(t *testing.T) { + res, err := NewBuilder(newValidService()). + WithCustomFieldApplicator(func(_, _ *corev1.Service) error { + return errors.New("applicator error") + }). + Build() + require.NoError(t, err) + + err = res.Mutate(&corev1.Service{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "applicator error") +} + +func TestResource_ConvergingStatus(t *testing.T) { + svc := newValidService() + svc.Spec.Type = corev1.ServiceTypeLoadBalancer + + res, err := NewBuilder(svc).Build() + require.NoError(t, err) + + status, err := res.ConvergingStatus(concepts.ConvergingOperationNone) + require.NoError(t, err) + assert.Equal(t, concepts.OperationalStatusPending, status.Status) +} + +func TestResource_Suspend(t *testing.T) { + res, err := NewBuilder(newValidService()).Build() + require.NoError(t, err) + + require.NoError(t, res.Suspend()) +} + +func TestResource_DeleteOnSuspend(t *testing.T) { + res, err := NewBuilder(newValidService()).Build() + require.NoError(t, err) + + assert.True(t, res.DeleteOnSuspend()) +} + +func TestResource_SuspensionStatus(t *testing.T) { + res, err := NewBuilder(newValidService()).Build() + require.NoError(t, err) + + status, err := res.SuspensionStatus() + require.NoError(t, err) + assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) +} + +func TestResource_ExtractData(t *testing.T) { + svc := newValidService() + + var extracted string + res, err := NewBuilder(svc). + WithDataExtractor(func(s corev1.Service) error { + extracted = s.Spec.Selector["app"] + return nil + }). + Build() + require.NoError(t, err) + + require.NoError(t, res.ExtractData()) + assert.Equal(t, "test", extracted) +} + +func TestResource_ExtractData_Error(t *testing.T) { + res, err := NewBuilder(newValidService()). + WithDataExtractor(func(_ corev1.Service) error { + return errors.New("extract error") + }). + Build() + require.NoError(t, err) + + err = res.ExtractData() + require.Error(t, err) + assert.Contains(t, err.Error(), "extract error") +} From ee9c24bb398944cd4883ff11bd51b3903c6e0894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 21:54:06 +0000 Subject: [PATCH 07/28] add files --- pkg/primitives/service/resource.go | 9 +++-- pkg/primitives/service/resource_test.go | 54 +++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index 8e0f2ba0..153135ae 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -8,13 +8,16 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// DefaultFieldApplicator replaces current with a deep copy of desired, preserving -// the immutable spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns -// after creation. +// DefaultFieldApplicator replaces current with a deep copy of desired while +// preserving server-managed metadata (ResourceVersion, UID, Generation, etc.), +// shared-controller fields (OwnerReferences, Finalizers), and the immutable +// spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns after creation. func DefaultFieldApplicator(current, desired *corev1.Service) error { + original := current.DeepCopy() clusterIP := current.Spec.ClusterIP clusterIPs := current.Spec.ClusterIPs *current = *desired.DeepCopy() + generic.PreserveServerManagedFields(current, original) if clusterIP != "" { current.Spec.ClusterIP = clusterIP current.Spec.ClusterIPs = clusterIPs diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go index cffacc62..76d2a029 100644 --- a/pkg/primitives/service/resource_test.go +++ b/pkg/primitives/service/resource_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" ) func newValidService() *corev1.Service { @@ -82,6 +83,59 @@ func TestResource_Mutate_PreservesClusterIP(t *testing.T) { assert.Equal(t, "test", current.Spec.Selector["app"]) } +func TestDefaultFieldApplicator_PreservesServerManagedFields(t *testing.T) { + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + ResourceVersion: "12345", + UID: types.UID("abc-def"), + Generation: 3, + OwnerReferences: []metav1.OwnerReference{ + {APIVersion: "v1", Kind: "Pod", Name: "other-owner", UID: "other-uid"}, + }, + Finalizers: []string{"finalizer.example.com"}, + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + ClusterIPs: []string{"10.0.0.1"}, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + Labels: map[string]string{"app": "test"}, + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // Desired spec and labels are applied + assert.Equal(t, "test", current.Labels["app"]) + assert.Equal(t, "test", current.Spec.Selector["app"]) + assert.Equal(t, int32(80), current.Spec.Ports[0].Port) + + // Server-managed fields are preserved + assert.Equal(t, "12345", current.ResourceVersion) + assert.Equal(t, types.UID("abc-def"), current.UID) + assert.Equal(t, int64(3), current.Generation) + + // Shared-controller fields are preserved + assert.Len(t, current.OwnerReferences, 1) + assert.Equal(t, "other-owner", current.OwnerReferences[0].Name) + assert.Equal(t, []string{"finalizer.example.com"}, current.Finalizers) + + // Immutable ClusterIP fields are preserved + assert.Equal(t, "10.0.0.1", current.Spec.ClusterIP) + assert.Equal(t, []string{"10.0.0.1"}, current.Spec.ClusterIPs) +} + func TestResource_Mutate_WithMutation(t *testing.T) { desired := newValidService() res, err := NewBuilder(desired). From 80e8315ea94cc207157fb502ea5ae7e9982f2cf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 23:22:29 +0000 Subject: [PATCH 08/28] make service not deleted by default --- docs/primitives/service.md | 12 +++++++----- pkg/primitives/service/handlers.go | 15 ++++++++------- pkg/primitives/service/handlers_test.go | 4 ++-- pkg/primitives/service/resource.go | 11 +++++------ pkg/primitives/service/resource_test.go | 2 +- 5 files changed, 23 insertions(+), 21 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index 327820cf..50da9f54 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -7,7 +7,7 @@ The `service` primitive is the framework's built-in integration abstraction for | Capability | Detail | |-------------------------|-----------------------------------------------------------------------------------------------------| | **Operational tracking** | Monitors LoadBalancer ingress assignment; reports `Operational` or `Pending` | -| **Suspension** | Deletes the Service on suspend (no meaningful "scaled down" state) | +| **Suspension** | Unaffected by suspension by default; customizable via handlers to delete or mutate on suspend | | **Mutation pipeline** | Typed editors for metadata and service spec, with a raw escape hatch for free-form access | | **Flavors** | Preserves externally-managed fields (labels, annotations) | | **Data extraction** | Reads generated or updated values (ClusterIP, LoadBalancer ingress) after each sync cycle | @@ -254,20 +254,22 @@ resource, err := service.NewBuilder(base). ## Suspension -By default, Services are **deleted** when the parent component is suspended (`DefaultDeleteOnSuspendHandler` returns `true`). This is because Services have no meaningful "scaled down" state — they are either present and routing traffic, or absent. +By default, Services are **unaffected** by suspension — they remain in the cluster when the parent component is suspended. The default suspend mutation handler is a no-op, `DefaultDeleteOnSuspendHandler` returns `false`, and the default suspension status handler reports `Suspended` immediately (no work required). -The default suspend mutation handler is a no-op, and the default suspension status handler always reports `Suspended` with reason "Service deleted on suspend". +This is appropriate for most use cases because Services are stateless routing objects that are safe to leave in place. -Override with `WithCustomSuspendDeletionDecision` if you want to keep the Service during suspension: +Override with `WithCustomSuspendDeletionDecision` if you want to delete the Service on suspend: ```go resource, err := service.NewBuilder(base). WithCustomSuspendDeletionDecision(func(_ *corev1.Service) bool { - return false // keep the Service during suspension + return true // delete the Service during suspension }). Build() ``` +You can also combine `WithCustomSuspendMutation` and `WithCustomSuspendStatus` for more advanced suspension behaviour, such as modifying the Service before it is deleted or tracking external readiness before reporting suspended. + ## Data Extraction Use `WithDataExtractor` to read values from the reconciled Service, such as the assigned ClusterIP or LoadBalancer ingress: diff --git a/pkg/primitives/service/handlers.go b/pkg/primitives/service/handlers.go index 8a787037..0ec86434 100644 --- a/pkg/primitives/service/handlers.go +++ b/pkg/primitives/service/handlers.go @@ -51,19 +51,20 @@ func DefaultOperationalStatusHandler( // DefaultDeleteOnSuspendHandler provides the default decision of whether to delete the Service // when the parent component is suspended. // -// It always returns true because Services have no meaningful "scaled down" state and should -// be deleted when the component is suspended. +// It always returns false, leaving the Service untouched during suspension. Services are +// typically stateless routing objects that are safe to leave in place. Override with +// Builder.WithCustomSuspendDeletionDecision to delete the Service on suspend. // // This function is used as the default handler by the Resource if no custom handler is registered via // Builder.WithCustomSuspendDeletionDecision. It can be reused within custom handlers. func DefaultDeleteOnSuspendHandler(_ *corev1.Service) bool { - return true + return false } // DefaultSuspendMutationHandler provides the default mutation applied to a Service when the // component is suspended. // -// It is a no-op because Services are deleted on suspend rather than mutated. +// It is a no-op because the default behaviour leaves the Service unaffected by suspension. // // This function is used as the default handler by the Resource if no custom handler is registered via // Builder.WithCustomSuspendMutation. It can be reused within custom handlers. @@ -73,14 +74,14 @@ func DefaultSuspendMutationHandler(_ *Mutator) error { // DefaultSuspensionStatusHandler reports the progress of the suspension process. // -// It always reports Suspended because the Service is deleted on suspend. This handler -// is called before the deletion to confirm that the resource is ready to be removed. +// It always reports Suspended because the default behaviour is a no-op — the Service +// is left in place and requires no work to reach the suspended state. // // This function is used as the default handler by the Resource if no custom handler is registered via // Builder.WithCustomSuspendStatus. It can be reused within custom handlers. func DefaultSuspensionStatusHandler(_ *corev1.Service) (concepts.SuspensionStatusWithReason, error) { return concepts.SuspensionStatusWithReason{ Status: concepts.SuspensionStatusSuspended, - Reason: "Service deleted on suspend", + Reason: "Service unaffected by suspension", }, nil } diff --git a/pkg/primitives/service/handlers_test.go b/pkg/primitives/service/handlers_test.go index 1a50409d..a4b2f2c9 100644 --- a/pkg/primitives/service/handlers_test.go +++ b/pkg/primitives/service/handlers_test.go @@ -145,7 +145,7 @@ func TestDefaultOperationalStatusHandler(t *testing.T) { func TestDefaultDeleteOnSuspendHandler(t *testing.T) { svc := &corev1.Service{} - assert.True(t, DefaultDeleteOnSuspendHandler(svc)) + assert.False(t, DefaultDeleteOnSuspendHandler(svc)) } func TestDefaultSuspendMutationHandler(t *testing.T) { @@ -162,5 +162,5 @@ func TestDefaultSuspensionStatusHandler(t *testing.T) { got, err := DefaultSuspensionStatusHandler(svc) require.NoError(t, err) assert.Equal(t, concepts.SuspensionStatusSuspended, got.Status) - assert.Equal(t, "Service deleted on suspend", got.Reason) + assert.Equal(t, "Service unaffected by suspension", got.Reason) } diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index 153135ae..1d17126e 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -31,7 +31,7 @@ func DefaultFieldApplicator(current, desired *corev1.Service) error { // It implements the following component interfaces: // - component.Resource: for basic identity and mutation behaviour. // - component.Operational: for tracking whether the Service is operational. -// - component.Suspendable: for controlled deletion when the component is suspended. +// - component.Suspendable: for participating in the component suspension lifecycle. // - component.DataExtractable: for exporting values after successful reconciliation. type Resource struct { base *generic.IntegrationResource[*corev1.Service, *Mutator] @@ -76,16 +76,15 @@ func (r *Resource) ConvergingStatus(op concepts.ConvergingOperation) (concepts.O // DeleteOnSuspend determines whether the Service should be deleted from the // cluster when the parent component is suspended. // -// By default, it uses DefaultDeleteOnSuspendHandler, which returns true — Services -// are deleted on suspend because they have no meaningful "scaled down" state. +// By default, it uses DefaultDeleteOnSuspendHandler, which returns false — the +// Service is left in place during suspension. func (r *Resource) DeleteOnSuspend() bool { return r.base.DeleteOnSuspend() } // Suspend triggers the suspension of the Service. // -// The default handler is a no-op because Services are deleted on suspend -// rather than mutated. +// The default handler is a no-op — the Service is left unaffected by suspension. func (r *Resource) Suspend() error { return r.base.Suspend() } @@ -93,7 +92,7 @@ func (r *Resource) Suspend() error { // SuspensionStatus reports the progress of the suspension process. // // By default, it uses DefaultSuspensionStatusHandler, which always reports -// Suspended with a reason indicating the Service is deleted on suspend. +// Suspended because the default behaviour leaves the Service untouched. func (r *Resource) SuspensionStatus() (concepts.SuspensionStatusWithReason, error) { return r.base.SuspensionStatus() } diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go index 76d2a029..7e0c3b81 100644 --- a/pkg/primitives/service/resource_test.go +++ b/pkg/primitives/service/resource_test.go @@ -222,7 +222,7 @@ func TestResource_DeleteOnSuspend(t *testing.T) { res, err := NewBuilder(newValidService()).Build() require.NoError(t, err) - assert.True(t, res.DeleteOnSuspend()) + assert.False(t, res.DeleteOnSuspend()) } func TestResource_SuspensionStatus(t *testing.T) { From 132400889223504d6127285e82be28223223aeed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 02:57:03 +0000 Subject: [PATCH 09/28] Address Copilot review: preserve Status in field applicator, fix doc comments, improve port matching - Preserve server-populated Status (including LoadBalancer.Ingress) in DefaultFieldApplicator so ConvergingStatus correctly detects assigned ingress addresses. - Fix misleading doc comments on WithCustomSuspendStatus, WithCustomSuspendMutation, and WithCustomSuspendDeletionDecision to accurately reflect that Services are left in place by default. - Include Protocol in unnamed port matching in EnsurePort to avoid incorrect replacement when same port number is used with different protocols (e.g., TCP/UDP). - Update README suspension description to match actual default behavior. Co-Authored-By: Claude Opus 4.6 (1M context) --- examples/service-primitive/README.md | 2 +- pkg/mutation/editors/servicespec.go | 13 +++++++++++-- pkg/primitives/service/builder.go | 8 ++++---- pkg/primitives/service/resource.go | 7 +++++-- 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/examples/service-primitive/README.md b/examples/service-primitive/README.md index 63276b27..01239a93 100644 --- a/examples/service-primitive/README.md +++ b/examples/service-primitive/README.md @@ -7,7 +7,7 @@ It shows how to manage a Kubernetes Service as a component of a larger applicati - **Feature Mutations**: Applying version-gated or conditional changes (additional ports, labels) using the `Mutator`. - **Field Flavors**: Preserving annotations that might be managed by external tools (e.g., cloud load balancer controllers). - **Operational Status**: Tracking whether the Service is operational (relevant for LoadBalancer types). -- **Suspension**: Deleting the Service when the component is suspended. +- **Suspension**: Demonstrating that, by default, the Service remains present when the component is suspended (`DeleteOnSuspend=false`), and how to opt into deletion if desired. - **Data Extraction**: Harvesting information (ClusterIP, ports) from the reconciled resource. ## Directory Structure diff --git a/pkg/mutation/editors/servicespec.go b/pkg/mutation/editors/servicespec.go index e8f6fbaf..0a14f57c 100644 --- a/pkg/mutation/editors/servicespec.go +++ b/pkg/mutation/editors/servicespec.go @@ -27,14 +27,15 @@ func (e *ServiceSpecEditor) SetType(t corev1.ServiceType) { } // EnsurePort upserts a service port. If a port with the same Name exists (or the same -// Port number when Name is empty), it is replaced; otherwise the port is appended. +// Port number and Protocol when Name is empty), it is replaced; otherwise the port is appended. +// When matching unnamed ports, an empty Protocol is treated as TCP (the Kubernetes default). func (e *ServiceSpecEditor) EnsurePort(port corev1.ServicePort) { for i, existing := range e.spec.Ports { if port.Name != "" && existing.Name == port.Name { e.spec.Ports[i] = port return } - if port.Name == "" && existing.Port == port.Port { + if port.Name == "" && existing.Port == port.Port && normalizeProtocol(existing.Protocol) == normalizeProtocol(port.Protocol) { e.spec.Ports[i] = port return } @@ -42,6 +43,14 @@ func (e *ServiceSpecEditor) EnsurePort(port corev1.ServicePort) { e.spec.Ports = append(e.spec.Ports, port) } +// normalizeProtocol returns the effective protocol, treating empty as TCP (the Kubernetes default). +func normalizeProtocol(p corev1.Protocol) corev1.Protocol { + if p == "" { + return corev1.ProtocolTCP + } + return p +} + // RemovePort removes a service port by name. It is a no-op if the port does not exist. func (e *ServiceSpecEditor) RemovePort(name string) { for i, existing := range e.spec.Ports { diff --git a/pkg/primitives/service/builder.go b/pkg/primitives/service/builder.go index 8c412cfe..44a705de 100644 --- a/pkg/primitives/service/builder.go +++ b/pkg/primitives/service/builder.go @@ -111,7 +111,7 @@ func (b *Builder) WithCustomOperationalStatus( // WithCustomSuspendStatus overrides how the progress of suspension is reported. // // The default behavior uses DefaultSuspensionStatusHandler, which always reports -// Suspended because Services are deleted on suspend. +// Suspended because the default behaviour leaves the Service untouched. // // If you want to augment the default behavior, you can call DefaultSuspensionStatusHandler // within your custom handler. @@ -126,7 +126,7 @@ func (b *Builder) WithCustomSuspendStatus( // the component is suspended. // // The default behavior uses DefaultSuspendMutationHandler, which is a no-op -// because Services are deleted on suspend rather than mutated. +// because the default behaviour leaves the Service unaffected by suspension. // // If you want to augment the default behavior, you can call DefaultSuspendMutationHandler // within your custom handler. @@ -140,8 +140,8 @@ func (b *Builder) WithCustomSuspendMutation( // WithCustomSuspendDeletionDecision overrides the decision of whether to delete // the Service when the component is suspended. // -// The default behavior uses DefaultDeleteOnSuspendHandler, which returns true -// because Services have no meaningful "scaled down" state. +// The default behavior uses DefaultDeleteOnSuspendHandler, which returns false +// because Services are stateless routing objects that are safe to leave in place. // // If you want to augment the default behavior, you can call DefaultDeleteOnSuspendHandler // within your custom handler. diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index 1d17126e..54483929 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -10,14 +10,17 @@ import ( // DefaultFieldApplicator replaces current with a deep copy of desired while // preserving server-managed metadata (ResourceVersion, UID, Generation, etc.), -// shared-controller fields (OwnerReferences, Finalizers), and the immutable -// spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns after creation. +// shared-controller fields (OwnerReferences, Finalizers), the immutable +// spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns after creation, +// and the server-populated Status (including LoadBalancer.Ingress). func DefaultFieldApplicator(current, desired *corev1.Service) error { original := current.DeepCopy() clusterIP := current.Spec.ClusterIP clusterIPs := current.Spec.ClusterIPs + originalStatus := current.Status *current = *desired.DeepCopy() generic.PreserveServerManagedFields(current, original) + current.Status = originalStatus if clusterIP != "" { current.Spec.ClusterIP = clusterIP current.Spec.ClusterIPs = clusterIPs From 1954ebc77daf586fc7c9fa4bba2e75c7260180a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 16:08:16 +0000 Subject: [PATCH 10/28] Use generic PreserveStatus in service field applicator Replace manual status assignment with generic.PreserveStatus() to align with the deployment primitive pattern and the new shared helper. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 4 ++-- pkg/primitives/service/resource.go | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index 50da9f54..36f5c6c3 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -38,9 +38,9 @@ resource, err := service.NewBuilder(base). ## Default Field Application -`DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving the immutable `spec.clusterIP` and `spec.clusterIPs` fields that Kubernetes assigns after creation. +`DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), and the immutable `spec.clusterIP` and `spec.clusterIPs` fields that Kubernetes assigns after creation. -This prevents the operator from clearing the cluster-assigned IP on every reconciliation cycle while ensuring all other fields converge to the desired state. +This prevents the operator from clearing the cluster-assigned IP or load balancer ingress assignments on every reconciliation cycle while ensuring all other fields converge to the desired state. Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index 54483929..650d938b 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -17,10 +17,9 @@ func DefaultFieldApplicator(current, desired *corev1.Service) error { original := current.DeepCopy() clusterIP := current.Spec.ClusterIP clusterIPs := current.Spec.ClusterIPs - originalStatus := current.Status *current = *desired.DeepCopy() generic.PreserveServerManagedFields(current, original) - current.Status = originalStatus + generic.PreserveStatus(current, original) if clusterIP != "" { current.Spec.ClusterIP = clusterIP current.Spec.ClusterIPs = clusterIPs From 66564fb257dff1905fb3d526c2413b08c20c0182 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 16:42:42 +0000 Subject: [PATCH 11/28] Fix service mutator constructor to not call beginFeature Initialize plans slice inline and set active pointer directly, matching the fix already applied to deployment and configmap mutators. This prevents an empty feature from being created when the generic helper in mutator_helper.go calls beginFeature(). Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/service/mutator.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/primitives/service/mutator.go b/pkg/primitives/service/mutator.go index a39d5645..3103a9bf 100644 --- a/pkg/primitives/service/mutator.go +++ b/pkg/primitives/service/mutator.go @@ -33,8 +33,11 @@ type Mutator struct { // NewMutator creates a new Mutator for the given Service. func NewMutator(svc *corev1.Service) *Mutator { - m := &Mutator{svc: svc} - m.beginFeature() + m := &Mutator{ + svc: svc, + plans: []featurePlan{{}}, + } + m.active = &m.plans[0] return m } From bf28a1b267d4dacd5417822a8f55e667bc54172b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 17:02:07 +0000 Subject: [PATCH 12/28] Address PR review: export BeginFeature, preserve nodePorts, add tests - Export BeginFeature on FeatureMutator interface so cross-package mutators (service, deployment) can satisfy the interface and receive proper feature boundary calls from ApplyMutations. - Preserve auto-allocated nodePort values in DefaultFieldApplicator when the desired port's NodePort is 0, preventing churn on NodePort/LoadBalancer services. - Add TestDefaultFieldApplicator_PreservesStatus to guard against regressions clearing LoadBalancer.Ingress. - Add TestDefaultFieldApplicator_PreservesNodePorts for nodePort preservation. - Add TestResource_Mutate_FeatureOrdering to verify mutation ordering across multiple features. - Update EnsurePort docs to mention protocol matching semantics. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 2 +- pkg/primitives/service/mutator.go | 10 +- pkg/primitives/service/mutator_test.go | 4 +- pkg/primitives/service/resource.go | 39 ++++++++ pkg/primitives/service/resource_test.go | 121 ++++++++++++++++++++++++ 5 files changed, 170 insertions(+), 6 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index 36f5c6c3..924adb27 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -156,7 +156,7 @@ m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { #### Port Management -`EnsurePort` upserts a port: if a port with the same `Name` exists (or the same `Port` number when `Name` is empty), it is replaced; otherwise it is appended. `RemovePort` removes a port by name. +`EnsurePort` upserts a port: if a port with the same `Name` exists, it is replaced; otherwise, when `Name` is empty, the match is performed on the combination of `Port` and the effective `Protocol` (treating an empty protocol value as TCP). This means TCP and UDP ports with the same port number are considered distinct unless you explicitly set matching protocols. If no existing port matches, the new port is appended. `RemovePort` removes a port by name. ```go m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { diff --git a/pkg/primitives/service/mutator.go b/pkg/primitives/service/mutator.go index 3103a9bf..b583fe84 100644 --- a/pkg/primitives/service/mutator.go +++ b/pkg/primitives/service/mutator.go @@ -41,9 +41,13 @@ func NewMutator(svc *corev1.Service) *Mutator { return m } -// beginFeature starts a new feature planning scope. All subsequent mutation -// registrations will be grouped into this feature's plan. -func (m *Mutator) beginFeature() { +// BeginFeature starts a new feature planning scope. +// +// This method is intended for use by the generic feature/mutation framework to +// delineate feature boundaries. All subsequent mutation registrations will be +// grouped into a new feature plan and applied after any previously planned +// features. +func (m *Mutator) BeginFeature() { m.plans = append(m.plans, featurePlan{}) m.active = &m.plans[len(m.plans)-1] } diff --git a/pkg/primitives/service/mutator_test.go b/pkg/primitives/service/mutator_test.go index b7687432..1ba24f7d 100644 --- a/pkg/primitives/service/mutator_test.go +++ b/pkg/primitives/service/mutator_test.go @@ -145,7 +145,7 @@ func TestMutator_MultipleFeatures(t *testing.T) { e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) return nil }) - m.beginFeature() + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsurePort(corev1.ServicePort{Name: "https", Port: 443}) return nil @@ -162,7 +162,7 @@ func TestMutator_MultipleFeatures_LaterSeesEarlier(t *testing.T) { e.EnsureSelector("app", "myapp") return nil }) - m.beginFeature() + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { // Later feature should see the selector set by the earlier feature. e.EnsureSelector("env", "prod") diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index 650d938b..c0125b1a 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -24,9 +24,48 @@ func DefaultFieldApplicator(current, desired *corev1.Service) error { current.Spec.ClusterIP = clusterIP current.Spec.ClusterIPs = clusterIPs } + preserveNodePorts(current, original) return nil } +// preserveNodePorts restores auto-allocated nodePort values from the original +// object when the desired port's NodePort is 0. Ports are matched by Name, or +// by Port+Protocol (treating empty protocol as TCP) when unnamed. +func preserveNodePorts(current, original *corev1.Service) { + if len(original.Spec.Ports) == 0 { + return + } + + for i := range current.Spec.Ports { + if current.Spec.Ports[i].NodePort != 0 { + continue // explicitly set, don't override + } + for _, orig := range original.Spec.Ports { + if orig.NodePort == 0 { + continue + } + if matchPort(current.Spec.Ports[i], orig) { + current.Spec.Ports[i].NodePort = orig.NodePort + break + } + } + } +} + +func matchPort(a, b corev1.ServicePort) bool { + if a.Name != "" || b.Name != "" { + return a.Name == b.Name + } + return a.Port == b.Port && normalizeProtocol(a.Protocol) == normalizeProtocol(b.Protocol) +} + +func normalizeProtocol(p corev1.Protocol) corev1.Protocol { + if p == "" { + return corev1.ProtocolTCP + } + return p +} + // Resource is a high-level abstraction for managing a Kubernetes Service within // a controller's reconciliation loop. // diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go index 7e0c3b81..5f50a975 100644 --- a/pkg/primitives/service/resource_test.go +++ b/pkg/primitives/service/resource_test.go @@ -136,6 +136,87 @@ func TestDefaultFieldApplicator_PreservesServerManagedFields(t *testing.T) { assert.Equal(t, []string{"10.0.0.1"}, current.Spec.ClusterIPs) } +func TestDefaultFieldApplicator_PreservesStatus(t *testing.T) { + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + ClusterIP: "10.0.0.1", + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + {IP: "203.0.113.1", Hostname: "lb.example.com"}, + }, + }, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // Desired spec is applied + assert.Equal(t, "test", current.Spec.Selector["app"]) + + // Status is preserved (not zeroed by the deep copy of desired) + require.Len(t, current.Status.LoadBalancer.Ingress, 1) + assert.Equal(t, "203.0.113.1", current.Status.LoadBalancer.Ingress[0].IP) + assert.Equal(t, "lb.example.com", current.Status.LoadBalancer.Ingress[0].Hostname) +} + +func TestDefaultFieldApplicator_PreservesNodePorts(t *testing.T) { + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + ClusterIP: "10.0.0.1", + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, + {Name: "https", Port: 443, NodePort: 30443, Protocol: corev1.ProtocolTCP}, + }, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + {Name: "https", Port: 443}, + }, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // Auto-allocated nodePorts are preserved when desired nodePort is 0 + require.Len(t, current.Spec.Ports, 2) + assert.Equal(t, int32(30080), current.Spec.Ports[0].NodePort) + assert.Equal(t, int32(30443), current.Spec.Ports[1].NodePort) +} + func TestResource_Mutate_WithMutation(t *testing.T) { desired := newValidService() res, err := NewBuilder(desired). @@ -161,6 +242,46 @@ func TestResource_Mutate_WithMutation(t *testing.T) { assert.Equal(t, "metrics", current.Spec.Ports[1].Name) } +func TestResource_Mutate_FeatureOrdering(t *testing.T) { + desired := newValidService() + + var callOrder []string + observedFirstBeforeSecond := false + + res, err := NewBuilder(desired). + WithMutation(Mutation{ + Name: "first-mutation", + Feature: feature.NewResourceFeature("v1", nil).When(true), + Mutate: func(m *Mutator) error { + callOrder = append(callOrder, "first") + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "first", Port: 1000}) + return nil + }) + return nil + }, + }). + WithMutation(Mutation{ + Name: "second-mutation", + Feature: feature.NewResourceFeature("v1", nil).When(true), + Mutate: func(m *Mutator) error { + if len(callOrder) == 1 && callOrder[0] == "first" { + observedFirstBeforeSecond = true + } + callOrder = append(callOrder, "second") + return nil + }, + }). + Build() + require.NoError(t, err) + + current := &corev1.Service{} + require.NoError(t, res.Mutate(current)) + + assert.Equal(t, []string{"first", "second"}, callOrder) + assert.True(t, observedFirstBeforeSecond, "second mutation should observe effects of the first mutation") +} + func TestResource_Mutate_CustomFieldApplicator(t *testing.T) { desired := newValidService() From b7057e3e81a535da664ec984a2ca4861e8ed607a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 21:52:59 +0000 Subject: [PATCH 13/28] fix linter error --- pkg/primitives/service/resource_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go index 5f50a975..a8ec944f 100644 --- a/pkg/primitives/service/resource_test.go +++ b/pkg/primitives/service/resource_test.go @@ -264,7 +264,7 @@ func TestResource_Mutate_FeatureOrdering(t *testing.T) { WithMutation(Mutation{ Name: "second-mutation", Feature: feature.NewResourceFeature("v1", nil).When(true), - Mutate: func(m *Mutator) error { + Mutate: func(_ *Mutator) error { if len(callOrder) == 1 && callOrder[0] == "first" { observedFirstBeforeSecond = true } From d37ee5832869cc8f2298ab2f392c376f49802dea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 00:29:06 +0000 Subject: [PATCH 14/28] Format markdown files with prettier Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 103 +++++++++++++++++---------- examples/service-primitive/README.md | 16 +++-- 2 files changed, 75 insertions(+), 44 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index 924adb27..45afde15 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -1,16 +1,18 @@ # Service Primitive -The `service` primitive is the framework's built-in integration abstraction for managing Kubernetes `Service` resources. It integrates with the component lifecycle and provides a structured mutation API for managing ports, selectors, and service configuration. +The `service` primitive is the framework's built-in integration abstraction for managing Kubernetes `Service` resources. +It integrates with the component lifecycle and provides a structured mutation API for managing ports, selectors, and +service configuration. ## Capabilities -| Capability | Detail | -|-------------------------|-----------------------------------------------------------------------------------------------------| -| **Operational tracking** | Monitors LoadBalancer ingress assignment; reports `Operational` or `Pending` | -| **Suspension** | Unaffected by suspension by default; customizable via handlers to delete or mutate on suspend | -| **Mutation pipeline** | Typed editors for metadata and service spec, with a raw escape hatch for free-form access | -| **Flavors** | Preserves externally-managed fields (labels, annotations) | -| **Data extraction** | Reads generated or updated values (ClusterIP, LoadBalancer ingress) after each sync cycle | +| Capability | Detail | +| ------------------------ | --------------------------------------------------------------------------------------------- | +| **Operational tracking** | Monitors LoadBalancer ingress assignment; reports `Operational` or `Pending` | +| **Suspension** | Unaffected by suspension by default; customizable via handlers to delete or mutate on suspend | +| **Mutation pipeline** | Typed editors for metadata and service spec, with a raw escape hatch for free-form access | +| **Flavors** | Preserves externally-managed fields (labels, annotations) | +| **Data extraction** | Reads generated or updated values (ClusterIP, LoadBalancer ingress) after each sync cycle | ## Building a Service Primitive @@ -38,9 +40,12 @@ resource, err := service.NewBuilder(base). ## Default Field Application -`DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), and the immutable `spec.clusterIP` and `spec.clusterIPs` fields that Kubernetes assigns after creation. +`DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving +server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), and the immutable `spec.clusterIP` +and `spec.clusterIPs` fields that Kubernetes assigns after creation. -This prevents the operator from clearing the cluster-assigned IP or load balancer ingress assignments on every reconciliation cycle while ensuring all other fields converge to the desired state. +This prevents the operator from clearing the cluster-assigned IP or load balancer ingress assignments on every +reconciliation cycle while ensuring all other fields converge to the desired state. Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: @@ -56,9 +61,11 @@ resource, err := service.NewBuilder(base). ## Mutations -Mutations are the primary mechanism for modifying a `Service` beyond its baseline. Each mutation is a named function that receives a `*Mutator` and records edit intent through typed editors. +Mutations are the primary mechanism for modifying a `Service` beyond its baseline. Each mutation is a named function +that receives a `*Mutator` and records edit intent through typed editors. -The `Feature` field controls when a mutation applies. Leaving it nil applies the mutation unconditionally. A feature with no version constraints and no `When()` conditions is also always enabled: +The `Feature` field controls when a mutation applies. Leaving it nil applies the mutation unconditionally. A feature +with no version constraints and no `When()` conditions is also always enabled: ```go func MyFeatureMutation(version string) service.Mutation { @@ -73,7 +80,8 @@ func MyFeatureMutation(version string) service.Mutation { } ``` -Mutations are applied in the order they are registered with the builder. If one mutation depends on a change made by another, register the dependency first. +Mutations are applied in the order they are registered with the builder. If one mutation depends on a change made by +another, register the dependency first. ### Boolean-gated mutations @@ -124,14 +132,16 @@ All version constraints and `When()` conditions must be satisfied for a mutation ## Internal Mutation Ordering -Within a single mutation, edit operations are grouped into categories and applied in a fixed sequence regardless of the order they are recorded: +Within a single mutation, edit operations are grouped into categories and applied in a fixed sequence regardless of the +order they are recorded: -| Step | Category | What it affects | -|------|--------------------|--------------------------------------------| -| 1 | Metadata edits | Labels and annotations on the `Service` | -| 2 | ServiceSpec edits | Ports, selectors, type, traffic policies | +| Step | Category | What it affects | +| ---- | ----------------- | ---------------------------------------- | +| 1 | Metadata edits | Labels and annotations on the `Service` | +| 2 | ServiceSpec edits | Ports, selectors, type, traffic policies | -Within each category, edits are applied in their registration order. Later features observe the Service as modified by all previous features. +Within each category, edits are applied in their registration order. Later features observe the Service as modified by +all previous features. ## Editors @@ -139,7 +149,9 @@ Within each category, edits are applied in their registration order. Later featu Controls service-level settings via `m.EditServiceSpec`. -Available methods: `SetType`, `EnsurePort`, `RemovePort`, `SetSelector`, `EnsureSelector`, `RemoveSelector`, `SetSessionAffinity`, `SetSessionAffinityConfig`, `SetPublishNotReadyAddresses`, `SetExternalTrafficPolicy`, `SetInternalTrafficPolicy`, `SetLoadBalancerSourceRanges`, `SetExternalName`, `Raw`. +Available methods: `SetType`, `EnsurePort`, `RemovePort`, `SetSelector`, `EnsureSelector`, `RemoveSelector`, +`SetSessionAffinity`, `SetSessionAffinityConfig`, `SetPublishNotReadyAddresses`, `SetExternalTrafficPolicy`, +`SetInternalTrafficPolicy`, `SetLoadBalancerSourceRanges`, `SetExternalName`, `Raw`. ```go m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { @@ -156,7 +168,10 @@ m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { #### Port Management -`EnsurePort` upserts a port: if a port with the same `Name` exists, it is replaced; otherwise, when `Name` is empty, the match is performed on the combination of `Port` and the effective `Protocol` (treating an empty protocol value as TCP). This means TCP and UDP ports with the same port number are considered distinct unless you explicitly set matching protocols. If no existing port matches, the new port is appended. `RemovePort` removes a port by name. +`EnsurePort` upserts a port: if a port with the same `Name` exists, it is replaced; otherwise, when `Name` is empty, the +match is performed on the combination of `Port` and the effective `Protocol` (treating an empty protocol value as TCP). +This means TCP and UDP ports with the same port number are considered distinct unless you explicitly set matching +protocols. If no existing port matches, the new port is appended. `RemovePort` removes a port by name. ```go m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { @@ -168,7 +183,8 @@ m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { #### Selector Management -`SetSelector` replaces the entire selector map. `EnsureSelector` adds or updates a single key-value pair. `RemoveSelector` removes a single key. +`SetSelector` replaces the entire selector map. `EnsureSelector` adds or updates a single key-value pair. +`RemoveSelector` removes a single key. ```go m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { @@ -203,7 +219,8 @@ m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { ## Flavors -Flavors run after the baseline applicator and before mutations. They are used to preserve fields managed by external controllers or other tools. +Flavors run after the baseline applicator and before mutations. They are used to preserve fields managed by external +controllers or other tools. ### PreserveCurrentLabels @@ -217,7 +234,8 @@ resource, err := service.NewBuilder(base). ### PreserveCurrentAnnotations -Preserves annotations present on the live object but absent from the applied desired state. Applied annotations win on overlap. +Preserves annotations present on the live object but absent from the applied desired state. Applied annotations win on +overlap. ```go resource, err := service.NewBuilder(base). @@ -233,13 +251,13 @@ The Service primitive implements the `Operational` concept to track whether the ### DefaultOperationalStatusHandler -| Service Type | Behaviour | -|----------------|---------------------------------------------------------------------------| -| `LoadBalancer` | Reports `Pending` until `Status.LoadBalancer.Ingress` has entries with an IP or hostname; then `Operational` | -| `ClusterIP` | Immediately `Operational` | -| `NodePort` | Immediately `Operational` | -| `ExternalName` | Immediately `Operational` | -| Headless | Immediately `Operational` | +| Service Type | Behaviour | +| -------------- | ------------------------------------------------------------------------------------------------------------ | +| `LoadBalancer` | Reports `Pending` until `Status.LoadBalancer.Ingress` has entries with an IP or hostname; then `Operational` | +| `ClusterIP` | Immediately `Operational` | +| `NodePort` | Immediately `Operational` | +| `ExternalName` | Immediately `Operational` | +| Headless | Immediately `Operational` | Override with `WithCustomOperationalStatus` to add custom checks: @@ -254,7 +272,9 @@ resource, err := service.NewBuilder(base). ## Suspension -By default, Services are **unaffected** by suspension — they remain in the cluster when the parent component is suspended. The default suspend mutation handler is a no-op, `DefaultDeleteOnSuspendHandler` returns `false`, and the default suspension status handler reports `Suspended` immediately (no work required). +By default, Services are **unaffected** by suspension — they remain in the cluster when the parent component is +suspended. The default suspend mutation handler is a no-op, `DefaultDeleteOnSuspendHandler` returns `false`, and the +default suspension status handler reports `Suspended` immediately (no work required). This is appropriate for most use cases because Services are stateless routing objects that are safe to leave in place. @@ -268,11 +288,13 @@ resource, err := service.NewBuilder(base). Build() ``` -You can also combine `WithCustomSuspendMutation` and `WithCustomSuspendStatus` for more advanced suspension behaviour, such as modifying the Service before it is deleted or tracking external readiness before reporting suspended. +You can also combine `WithCustomSuspendMutation` and `WithCustomSuspendStatus` for more advanced suspension behaviour, +such as modifying the Service before it is deleted or tracking external readiness before reporting suspended. ## Data Extraction -Use `WithDataExtractor` to read values from the reconciled Service, such as the assigned ClusterIP or LoadBalancer ingress: +Use `WithDataExtractor` to read values from the reconciled Service, such as the assigned ClusterIP or LoadBalancer +ingress: ```go var assignedIP string @@ -331,14 +353,19 @@ resource, err := service.NewBuilder(base). Build() ``` -When `MetricsEnabled` is true, the Service will expose both the HTTP port and the metrics port. When false, only the HTTP port is configured. Neither mutation needs to know about the other. +When `MetricsEnabled` is true, the Service will expose both the HTTP port and the metrics port. When false, only the +HTTP port is configured. Neither mutation needs to know about the other. ## Guidance -**`Feature: nil` applies unconditionally.** Omit `Feature` (leave it nil) for mutations that should always run. Use `feature.NewResourceFeature(version, constraints)` when version-based gating is needed, and chain `.When(bool)` for boolean conditions. +**`Feature: nil` applies unconditionally.** Omit `Feature` (leave it nil) for mutations that should always run. Use +`feature.NewResourceFeature(version, constraints)` when version-based gating is needed, and chain `.When(bool)` for +boolean conditions. **Register mutations in dependency order.** If mutation B relies on a port added by mutation A, register A first. -**Use `EnsurePort` for idempotent port management.** The mutator tracks ports by name (or port number when unnamed), so repeated calls with the same name produce the same result. +**Use `EnsurePort` for idempotent port management.** The mutator tracks ports by name (or port number when unnamed), so +repeated calls with the same name produce the same result. -**Use `PreserveCurrentAnnotations` when cloud providers annotate Services.** Cloud load balancer controllers often add annotations to Services. This flavor prevents your operator from deleting those annotations each reconcile cycle. +**Use `PreserveCurrentAnnotations` when cloud providers annotate Services.** Cloud load balancer controllers often add +annotations to Services. This flavor prevents your operator from deleting those annotations each reconcile cycle. diff --git a/examples/service-primitive/README.md b/examples/service-primitive/README.md index 01239a93..4f815490 100644 --- a/examples/service-primitive/README.md +++ b/examples/service-primitive/README.md @@ -1,20 +1,23 @@ # Service Primitive Example -This example demonstrates the usage of the `service` primitive within the operator component framework. -It shows how to manage a Kubernetes Service as a component of a larger application, utilising features like: +This example demonstrates the usage of the `service` primitive within the operator component framework. It shows how to +manage a Kubernetes Service as a component of a larger application, utilising features like: - **Base Construction**: Initializing a Service with basic metadata, selector, and ports. - **Feature Mutations**: Applying version-gated or conditional changes (additional ports, labels) using the `Mutator`. -- **Field Flavors**: Preserving annotations that might be managed by external tools (e.g., cloud load balancer controllers). +- **Field Flavors**: Preserving annotations that might be managed by external tools (e.g., cloud load balancer + controllers). - **Operational Status**: Tracking whether the Service is operational (relevant for LoadBalancer types). -- **Suspension**: Demonstrating that, by default, the Service remains present when the component is suspended (`DeleteOnSuspend=false`), and how to opt into deletion if desired. +- **Suspension**: Demonstrating that, by default, the Service remains present when the component is suspended + (`DeleteOnSuspend=false`), and how to opt into deletion if desired. - **Data Extraction**: Harvesting information (ClusterIP, ports) from the reconciled resource. ## Directory Structure -- `app/`: Defines the controller that uses the component framework. The `ExampleApp` CRD is shared from `examples/shared/app`. +- `app/`: Defines the controller that uses the component framework. The `ExampleApp` CRD is shared from + `examples/shared/app`. - `features/`: Contains modular feature definitions: - - `mutations.go`: version labelling and conditional metrics port. + - `mutations.go`: version labelling and conditional metrics port. - `resources/`: Contains the central `NewServiceResource` factory that assembles all features using `service.Builder`. - `main.go`: A standalone entry point that demonstrates multiple reconciliation cycles with a fake client. @@ -25,6 +28,7 @@ go run examples/service-primitive/main.go ``` This will: + 1. Initialize a fake Kubernetes client. 2. Create an `ExampleApp` owner object. 3. Reconcile through four spec variations, printing the Service ports after each cycle. From 65ac2917ca7f5ded6c3b1c1e838e255dea2c4b74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 00:33:39 +0000 Subject: [PATCH 15/28] Address Copilot review feedback for service primitive - Fix matchPort to require both ports have names before name-only matching, preventing missed matches when one port is named and the other is not - Rename misleading "headless service" test case to "service with default type" - Add service primitive to built-in primitives table in docs/primitives.md - Clarify assertion message in feature ordering test Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives.md | 5 +++-- pkg/primitives/service/handlers_test.go | 2 +- pkg/primitives/service/resource.go | 2 +- pkg/primitives/service/resource_test.go | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/primitives.md b/docs/primitives.md index dc2d4c2a..68b324de 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -153,8 +153,9 @@ have been applied. This means a single mutation can safely add a container and t | Primitive | Category | Documentation | | --------------------------- | -------- | ----------------------------------------- | -| `pkg/primitives/deployment` | Workload | [deployment.md](primitives/deployment.md) | -| `pkg/primitives/configmap` | Static | [configmap.md](primitives/configmap.md) | +| `pkg/primitives/deployment` | Workload | [deployment.md](primitives/deployment.md) | +| `pkg/primitives/configmap` | Static | [configmap.md](primitives/configmap.md) | +| `pkg/primitives/service` | Integration | [service.md](primitives/service.md) | ## Usage Examples diff --git a/pkg/primitives/service/handlers_test.go b/pkg/primitives/service/handlers_test.go index a4b2f2c9..16d734a7 100644 --- a/pkg/primitives/service/handlers_test.go +++ b/pkg/primitives/service/handlers_test.go @@ -47,7 +47,7 @@ func TestDefaultOperationalStatusHandler(t *testing.T) { wantReason: "Service is operational", }, { - name: "headless service (empty type) is immediately operational", + name: "service with default type (empty spec) is immediately operational", svc: &corev1.Service{ Spec: corev1.ServiceSpec{}, }, diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index c0125b1a..f690aa0e 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -53,7 +53,7 @@ func preserveNodePorts(current, original *corev1.Service) { } func matchPort(a, b corev1.ServicePort) bool { - if a.Name != "" || b.Name != "" { + if a.Name != "" && b.Name != "" { return a.Name == b.Name } return a.Port == b.Port && normalizeProtocol(a.Protocol) == normalizeProtocol(b.Protocol) diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go index a8ec944f..b1ac69bc 100644 --- a/pkg/primitives/service/resource_test.go +++ b/pkg/primitives/service/resource_test.go @@ -279,7 +279,7 @@ func TestResource_Mutate_FeatureOrdering(t *testing.T) { require.NoError(t, res.Mutate(current)) assert.Equal(t, []string{"first", "second"}, callOrder) - assert.True(t, observedFirstBeforeSecond, "second mutation should observe effects of the first mutation") + assert.True(t, observedFirstBeforeSecond, "second mutation function should execute after the first") } func TestResource_Mutate_CustomFieldApplicator(t *testing.T) { From a77a53bde39d3832f1a2692253ca374baabe9d70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 00:48:07 +0000 Subject: [PATCH 16/28] Fix markdown table formatting in primitives.md Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/primitives.md b/docs/primitives.md index 68b324de..9c149659 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -151,11 +151,11 @@ have been applied. This means a single mutation can safely add a container and t ## Built-in Primitives -| Primitive | Category | Documentation | -| --------------------------- | -------- | ----------------------------------------- | -| `pkg/primitives/deployment` | Workload | [deployment.md](primitives/deployment.md) | -| `pkg/primitives/configmap` | Static | [configmap.md](primitives/configmap.md) | -| `pkg/primitives/service` | Integration | [service.md](primitives/service.md) | +| Primitive | Category | Documentation | +| --------------------------- | ----------- | ----------------------------------------- | +| `pkg/primitives/deployment` | Workload | [deployment.md](primitives/deployment.md) | +| `pkg/primitives/configmap` | Static | [configmap.md](primitives/configmap.md) | +| `pkg/primitives/service` | Integration | [service.md](primitives/service.md) | ## Usage Examples From a43bdc354f505a5492a57a32d0b72ff8da6f4d72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 01:09:05 +0000 Subject: [PATCH 17/28] Guard NodePort preservation by desired Service type Only preserve auto-allocated NodePorts when the desired Service type is NodePort or LoadBalancer. Previously, preserveNodePorts ran unconditionally, which could re-introduce NodePort values on ClusterIP or ExternalName services and cause API rejections. Also fix the preserveNodePorts doc comment to accurately describe the port matching semantics (Name match requires both ports to have a non-empty Name). Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/service/resource.go | 14 +++- pkg/primitives/service/resource_test.go | 106 ++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index f690aa0e..c6a27ba8 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -24,13 +24,21 @@ func DefaultFieldApplicator(current, desired *corev1.Service) error { current.Spec.ClusterIP = clusterIP current.Spec.ClusterIPs = clusterIPs } - preserveNodePorts(current, original) + // Only preserve nodePorts when the resulting Service type supports them. + effectiveType := current.Spec.Type + if effectiveType == "" { + effectiveType = corev1.ServiceTypeClusterIP + } + if effectiveType == corev1.ServiceTypeNodePort || effectiveType == corev1.ServiceTypeLoadBalancer { + preserveNodePorts(current, original) + } return nil } // preserveNodePorts restores auto-allocated nodePort values from the original -// object when the desired port's NodePort is 0. Ports are matched by Name, or -// by Port+Protocol (treating empty protocol as TCP) when unnamed. +// object when the desired port's NodePort is 0. Ports are matched by Name when +// both ports have a non-empty Name; otherwise they are matched by Port+Protocol +// (treating empty protocol as TCP). func preserveNodePorts(current, original *corev1.Service) { if len(original.Spec.Ports) == 0 { return diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go index b1ac69bc..bb619345 100644 --- a/pkg/primitives/service/resource_test.go +++ b/pkg/primitives/service/resource_test.go @@ -217,6 +217,112 @@ func TestDefaultFieldApplicator_PreservesNodePorts(t *testing.T) { assert.Equal(t, int32(30443), current.Spec.Ports[1].NodePort) } +func TestDefaultFieldApplicator_DoesNotPreserveNodePortsForClusterIP(t *testing.T) { + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + ClusterIP: "10.0.0.1", + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, + }, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // NodePort should NOT be preserved when switching to ClusterIP + assert.Equal(t, int32(0), current.Spec.Ports[0].NodePort) + assert.Equal(t, corev1.ServiceTypeClusterIP, current.Spec.Type) +} + +func TestDefaultFieldApplicator_DoesNotPreserveNodePortsForEmptyType(t *testing.T) { + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + ClusterIP: "10.0.0.1", + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, + }, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + // Empty type defaults to ClusterIP + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // NodePort should NOT be preserved when desired type is empty (defaults to ClusterIP) + assert.Equal(t, int32(0), current.Spec.Ports[0].NodePort) +} + +func TestDefaultFieldApplicator_PreservesNodePortsForLoadBalancer(t *testing.T) { + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + ClusterIP: "10.0.0.1", + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, + }, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // NodePort SHOULD be preserved for LoadBalancer type + assert.Equal(t, int32(30080), current.Spec.Ports[0].NodePort) +} + func TestResource_Mutate_WithMutation(t *testing.T) { desired := newValidService() res, err := NewBuilder(desired). From 9a1b9d28f482f60c9015081c37451a776daa0ada Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 01:24:31 +0000 Subject: [PATCH 18/28] Document NodePort preservation in DefaultFieldApplicator Update the doc comment on DefaultFieldApplicator and the "Default Field Application" section of service.md to describe the auto-allocated NodePort preservation behavior and its conditions, matching the actual implementation. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 13 ++++++++----- pkg/primitives/service/resource.go | 6 +++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index 45afde15..906a3b00 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -41,11 +41,14 @@ resource, err := service.NewBuilder(base). ## Default Field Application `DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving -server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), and the immutable `spec.clusterIP` -and `spec.clusterIPs` fields that Kubernetes assigns after creation. - -This prevents the operator from clearing the cluster-assigned IP or load balancer ingress assignments on every -reconciliation cycle while ensuring all other fields converge to the desired state. +server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), the immutable `spec.clusterIP` +and `spec.clusterIPs` fields that Kubernetes assigns after creation, and auto-allocated NodePort values for +`NodePort` and `LoadBalancer` Services. + +This prevents the operator from clearing the cluster-assigned IP, load balancer ingress assignments, or +automatically-assigned NodePort numbers on every reconciliation cycle while ensuring all other fields converge to +the desired state. Explicitly specified `spec.ports[].nodePort` values in the desired object are still applied, so +fixed NodePort assignments can be managed by the operator when needed. Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index c6a27ba8..f30dbc74 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -12,7 +12,11 @@ import ( // preserving server-managed metadata (ResourceVersion, UID, Generation, etc.), // shared-controller fields (OwnerReferences, Finalizers), the immutable // spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns after creation, -// and the server-populated Status (including LoadBalancer.Ingress). +// the server-populated Status (including LoadBalancer.Ingress), and auto-allocated +// NodePort values for NodePort and LoadBalancer Services. NodePorts are only +// preserved when the resulting Service type is NodePort or LoadBalancer and the +// desired port's NodePort is 0 (unset); explicitly specified NodePort values in +// the desired object are always applied. func DefaultFieldApplicator(current, desired *corev1.Service) error { original := current.DeepCopy() clusterIP := current.Spec.ClusterIP From 097408e1ba60c3a2a0fba428511d8e8bd0ada6f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 01:30:47 +0000 Subject: [PATCH 19/28] Fix Prettier formatting in service.md Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index 906a3b00..b08a0550 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -41,14 +41,14 @@ resource, err := service.NewBuilder(base). ## Default Field Application `DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving -server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), the immutable `spec.clusterIP` -and `spec.clusterIPs` fields that Kubernetes assigns after creation, and auto-allocated NodePort values for -`NodePort` and `LoadBalancer` Services. +server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), the immutable `spec.clusterIP` and +`spec.clusterIPs` fields that Kubernetes assigns after creation, and auto-allocated NodePort values for `NodePort` and +`LoadBalancer` Services. This prevents the operator from clearing the cluster-assigned IP, load balancer ingress assignments, or -automatically-assigned NodePort numbers on every reconciliation cycle while ensuring all other fields converge to -the desired state. Explicitly specified `spec.ports[].nodePort` values in the desired object are still applied, so -fixed NodePort assignments can be managed by the operator when needed. +automatically-assigned NodePort numbers on every reconciliation cycle while ensuring all other fields converge to the +desired state. Explicitly specified `spec.ports[].nodePort` values in the desired object are still applied, so fixed +NodePort assignments can be managed by the operator when needed. Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: From 8c7c90bfe65391fa0747d2d684819ec25a922ead Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 01:37:55 +0000 Subject: [PATCH 20/28] Restore auto-allocated NodePorts after mutation edits Mutator.Apply now snapshots the Service before applying edits and restores auto-allocated NodePort values for NodePort/LoadBalancer Services afterward. This prevents EnsurePort (which replaces the entire ServicePort) from clearing NodePorts that were preserved by DefaultFieldApplicator, avoiding perpetual updates and nodePort reallocation. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/service/mutator.go | 18 ++++++ pkg/primitives/service/mutator_test.go | 86 ++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) diff --git a/pkg/primitives/service/mutator.go b/pkg/primitives/service/mutator.go index b583fe84..f34788c6 100644 --- a/pkg/primitives/service/mutator.go +++ b/pkg/primitives/service/mutator.go @@ -86,7 +86,16 @@ func (m *Mutator) EditServiceSpec(edit func(*editors.ServiceSpecEditor) error) { // // Features are applied in the order they were registered. Later features observe // the Service as modified by all previous features. +// +// After all edits are applied, auto-allocated NodePort values that were present +// before mutations are restored for NodePort and LoadBalancer Services when the +// resulting port's NodePort is 0 (unset). This prevents mutations that replace +// ports via EnsurePort from unintentionally clearing server-assigned NodePorts. func (m *Mutator) Apply() error { + // Snapshot ports before mutations so we can restore auto-allocated + // NodePorts that mutations may inadvertently clear. + snapshot := m.svc.DeepCopy() + for _, plan := range m.plans { // 1. Metadata edits if len(plan.metadataEdits) > 0 { @@ -109,5 +118,14 @@ func (m *Mutator) Apply() error { } } + // Restore auto-allocated NodePorts for types that support them. + effectiveType := m.svc.Spec.Type + if effectiveType == "" { + effectiveType = corev1.ServiceTypeClusterIP + } + if effectiveType == corev1.ServiceTypeNodePort || effectiveType == corev1.ServiceTypeLoadBalancer { + preserveNodePorts(m.svc, snapshot) + } + return nil } diff --git a/pkg/primitives/service/mutator_test.go b/pkg/primitives/service/mutator_test.go index 1ba24f7d..381eb276 100644 --- a/pkg/primitives/service/mutator_test.go +++ b/pkg/primitives/service/mutator_test.go @@ -174,6 +174,92 @@ func TestMutator_MultipleFeatures_LaterSeesEarlier(t *testing.T) { assert.Equal(t, "prod", svc.Spec.Selector["env"]) } +// --- NodePort preservation across mutations --- + +func TestMutator_Apply_PreservesNodePortsAfterEnsurePort(t *testing.T) { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "test-svc", Namespace: "default"}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080}, + {Name: "https", Port: 443, NodePort: 30443}, + }, + }, + } + m := NewMutator(svc) + // EnsurePort replaces the entire ServicePort, which zeroes NodePort. + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + return nil + }) + require.NoError(t, m.Apply()) + + assert.Equal(t, int32(30080), svc.Spec.Ports[0].NodePort, "auto-allocated NodePort should be restored") + assert.Equal(t, int32(30443), svc.Spec.Ports[1].NodePort, "untouched port should keep its NodePort") +} + +func TestMutator_Apply_PreservesNodePortsForLoadBalancer(t *testing.T) { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "test-svc", Namespace: "default"}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 31080}, + }, + }, + } + m := NewMutator(svc) + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + return nil + }) + require.NoError(t, m.Apply()) + + assert.Equal(t, int32(31080), svc.Spec.Ports[0].NodePort) +} + +func TestMutator_Apply_ExplicitNodePortNotOverridden(t *testing.T) { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "test-svc", Namespace: "default"}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080}, + }, + }, + } + m := NewMutator(svc) + // Explicitly set a different NodePort — should not be reverted to the snapshot. + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80, NodePort: 32000}) + return nil + }) + require.NoError(t, m.Apply()) + + assert.Equal(t, int32(32000), svc.Spec.Ports[0].NodePort, "explicitly set NodePort should not be overridden") +} + +func TestMutator_Apply_NoNodePortPreservationForClusterIP(t *testing.T) { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "test-svc", Namespace: "default"}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080}, + }, + }, + } + m := NewMutator(svc) + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + return nil + }) + require.NoError(t, m.Apply()) + + assert.Equal(t, int32(0), svc.Spec.Ports[0].NodePort, "ClusterIP services should not preserve NodePorts") +} + // --- ObjectMutator interface --- func TestMutator_ImplementsObjectMutator(_ *testing.T) { From 841eb73a7690eb189ac37c967f7e879279189c39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 02:14:42 +0000 Subject: [PATCH 21/28] Preserve HealthCheckNodePort and IPFamilies in DefaultFieldApplicator The applicator now restores server-defaulted IPFamilies, IPFamilyPolicy, and the server-allocated HealthCheckNodePort from the original object when the desired object leaves them unset. This prevents update rejections and perpetual reconcile diffs on clusters where the API server populates these fields. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 15 +-- pkg/primitives/service/resource.go | 26 ++++- pkg/primitives/service/resource_test.go | 146 ++++++++++++++++++++++++ 3 files changed, 175 insertions(+), 12 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index b08a0550..78df2aab 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -42,13 +42,14 @@ resource, err := service.NewBuilder(base). `DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), the immutable `spec.clusterIP` and -`spec.clusterIPs` fields that Kubernetes assigns after creation, and auto-allocated NodePort values for `NodePort` and -`LoadBalancer` Services. - -This prevents the operator from clearing the cluster-assigned IP, load balancer ingress assignments, or -automatically-assigned NodePort numbers on every reconciliation cycle while ensuring all other fields converge to the -desired state. Explicitly specified `spec.ports[].nodePort` values in the desired object are still applied, so fixed -NodePort assignments can be managed by the operator when needed. +`spec.clusterIPs` fields that Kubernetes assigns after creation, server-defaulted IP family configuration +(`spec.ipFamilies` and `spec.ipFamilyPolicy`), the server-allocated `spec.healthCheckNodePort`, and auto-allocated +NodePort values for `NodePort` and `LoadBalancer` Services. + +This prevents the operator from clearing the cluster-assigned IP, load balancer ingress assignments, IP family +defaults, health check node ports, or automatically-assigned NodePort numbers on every reconciliation cycle while +ensuring all other fields converge to the desired state. Explicitly specified values in the desired object always take +precedence, so these fields can still be managed by the operator when needed. Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index f30dbc74..8b603437 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -12,11 +12,13 @@ import ( // preserving server-managed metadata (ResourceVersion, UID, Generation, etc.), // shared-controller fields (OwnerReferences, Finalizers), the immutable // spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns after creation, -// the server-populated Status (including LoadBalancer.Ingress), and auto-allocated -// NodePort values for NodePort and LoadBalancer Services. NodePorts are only -// preserved when the resulting Service type is NodePort or LoadBalancer and the -// desired port's NodePort is 0 (unset); explicitly specified NodePort values in -// the desired object are always applied. +// the server-populated Status (including LoadBalancer.Ingress), server-defaulted +// IP family configuration (spec.ipFamilies, spec.ipFamilyPolicy), the +// server-allocated spec.healthCheckNodePort, and auto-allocated NodePort values +// for NodePort and LoadBalancer Services. NodePorts are only preserved when the +// resulting Service type is NodePort or LoadBalancer and the desired port's +// NodePort is 0 (unset); explicitly specified NodePort values in the desired +// object are always applied. func DefaultFieldApplicator(current, desired *corev1.Service) error { original := current.DeepCopy() clusterIP := current.Spec.ClusterIP @@ -28,6 +30,20 @@ func DefaultFieldApplicator(current, desired *corev1.Service) error { current.Spec.ClusterIP = clusterIP current.Spec.ClusterIPs = clusterIPs } + // Preserve server-defaulted IP family configuration when the desired + // object leaves them unset, avoiding perpetual reconcile diffs. + if len(current.Spec.IPFamilies) == 0 && len(original.Spec.IPFamilies) > 0 { + current.Spec.IPFamilies = original.Spec.IPFamilies + } + if current.Spec.IPFamilyPolicy == nil && original.Spec.IPFamilyPolicy != nil { + current.Spec.IPFamilyPolicy = original.Spec.IPFamilyPolicy + } + // Preserve the server-allocated HealthCheckNodePort when the desired + // object does not explicitly set one. The API server assigns this field + // for LoadBalancer Services with externalTrafficPolicy: Local. + if current.Spec.HealthCheckNodePort == 0 && original.Spec.HealthCheckNodePort != 0 { + current.Spec.HealthCheckNodePort = original.Spec.HealthCheckNodePort + } // Only preserve nodePorts when the resulting Service type supports them. effectiveType := current.Spec.Type if effectiveType == "" { diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go index bb619345..88d9be5a 100644 --- a/pkg/primitives/service/resource_test.go +++ b/pkg/primitives/service/resource_test.go @@ -323,6 +323,152 @@ func TestDefaultFieldApplicator_PreservesNodePortsForLoadBalancer(t *testing.T) assert.Equal(t, int32(30080), current.Spec.Ports[0].NodePort) } +func TestDefaultFieldApplicator_PreservesIPFamilyFields(t *testing.T) { + singleStack := corev1.IPFamilyPolicySingleStack + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + IPFamilies: []corev1.IPFamily{corev1.IPv4Protocol}, + IPFamilyPolicy: &singleStack, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // Server-defaulted IP family fields are preserved + assert.Equal(t, []corev1.IPFamily{corev1.IPv4Protocol}, current.Spec.IPFamilies) + require.NotNil(t, current.Spec.IPFamilyPolicy) + assert.Equal(t, corev1.IPFamilyPolicySingleStack, *current.Spec.IPFamilyPolicy) +} + +func TestDefaultFieldApplicator_ExplicitIPFamilyOverridesPreservation(t *testing.T) { + singleStack := corev1.IPFamilyPolicySingleStack + preferDualStack := corev1.IPFamilyPolicyPreferDualStack + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + IPFamilies: []corev1.IPFamily{corev1.IPv4Protocol}, + IPFamilyPolicy: &singleStack, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, + IPFamilies: []corev1.IPFamily{corev1.IPv4Protocol, corev1.IPv6Protocol}, + IPFamilyPolicy: &preferDualStack, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // Explicitly set IP family fields in desired take precedence + assert.Equal(t, []corev1.IPFamily{corev1.IPv4Protocol, corev1.IPv6Protocol}, current.Spec.IPFamilies) + require.NotNil(t, current.Spec.IPFamilyPolicy) + assert.Equal(t, corev1.IPFamilyPolicyPreferDualStack, *current.Spec.IPFamilyPolicy) +} + +func TestDefaultFieldApplicator_PreservesHealthCheckNodePort(t *testing.T) { + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + ClusterIP: "10.0.0.1", + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + HealthCheckNodePort: 31234, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080}, + }, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // Server-allocated HealthCheckNodePort is preserved + assert.Equal(t, int32(31234), current.Spec.HealthCheckNodePort) +} + +func TestDefaultFieldApplicator_ExplicitHealthCheckNodePortOverridesPreservation(t *testing.T) { + current := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + ClusterIP: "10.0.0.1", + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + HealthCheckNodePort: 31234, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, NodePort: 30080}, + }, + }, + } + desired := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Namespace: "test-ns", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + HealthCheckNodePort: 32000, + Selector: map[string]string{"app": "test"}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80}, + }, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // Explicitly set HealthCheckNodePort in desired takes precedence + assert.Equal(t, int32(32000), current.Spec.HealthCheckNodePort) +} + func TestResource_Mutate_WithMutation(t *testing.T) { desired := newValidService() res, err := NewBuilder(desired). From f4d3db1d9f09bb5a69d42d6e599780cb1ae93ed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 11:45:15 +0000 Subject: [PATCH 22/28] Fix Prettier formatting in service.md Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index 78df2aab..c5362755 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -46,10 +46,10 @@ server-managed metadata, the Status subresource (including `LoadBalancer.Ingress (`spec.ipFamilies` and `spec.ipFamilyPolicy`), the server-allocated `spec.healthCheckNodePort`, and auto-allocated NodePort values for `NodePort` and `LoadBalancer` Services. -This prevents the operator from clearing the cluster-assigned IP, load balancer ingress assignments, IP family -defaults, health check node ports, or automatically-assigned NodePort numbers on every reconciliation cycle while -ensuring all other fields converge to the desired state. Explicitly specified values in the desired object always take -precedence, so these fields can still be managed by the operator when needed. +This prevents the operator from clearing the cluster-assigned IP, load balancer ingress assignments, IP family defaults, +health check node ports, or automatically-assigned NodePort numbers on every reconciliation cycle while ensuring all +other fields converge to the desired state. Explicitly specified values in the desired object always take precedence, so +these fields can still be managed by the operator when needed. Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: From 4ea2ab07680bd55895c62bd6d4098b92065f928d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 17:55:28 +0000 Subject: [PATCH 23/28] Fix service Mutator to not initialize a feature plan on construction Align with configmap and deployment mutators: NewMutator no longer creates an initial feature plan. BeginFeature must be called before registering mutations. Updated all tests to call BeginFeature and added constructor/plan invariant tests. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/service/mutator.go | 8 ++- pkg/primitives/service/mutator_test.go | 74 ++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/pkg/primitives/service/mutator.go b/pkg/primitives/service/mutator.go index f34788c6..722da976 100644 --- a/pkg/primitives/service/mutator.go +++ b/pkg/primitives/service/mutator.go @@ -32,13 +32,11 @@ type Mutator struct { } // NewMutator creates a new Mutator for the given Service. +// BeginFeature must be called before registering any mutations. func NewMutator(svc *corev1.Service) *Mutator { - m := &Mutator{ - svc: svc, - plans: []featurePlan{{}}, + return &Mutator{ + svc: svc, } - m.active = &m.plans[0] - return m } // BeginFeature starts a new feature planning scope. diff --git a/pkg/primitives/service/mutator_test.go b/pkg/primitives/service/mutator_test.go index 381eb276..349da87a 100644 --- a/pkg/primitives/service/mutator_test.go +++ b/pkg/primitives/service/mutator_test.go @@ -25,6 +25,7 @@ func newTestService() *corev1.Service { func TestMutator_EditObjectMetadata(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { e.EnsureLabel("app", "myapp") return nil @@ -36,6 +37,7 @@ func TestMutator_EditObjectMetadata(t *testing.T) { func TestMutator_EditObjectMetadata_Nil(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditObjectMetadata(nil) assert.NoError(t, m.Apply()) } @@ -43,6 +45,7 @@ func TestMutator_EditObjectMetadata_Nil(t *testing.T) { func TestMutator_EditObjectMetadata_Error(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditObjectMetadata(func(_ *editors.ObjectMetaEditor) error { return errors.New("metadata error") }) @@ -56,6 +59,7 @@ func TestMutator_EditObjectMetadata_Error(t *testing.T) { func TestMutator_EditServiceSpec(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.SetType(corev1.ServiceTypeNodePort) return nil @@ -67,6 +71,7 @@ func TestMutator_EditServiceSpec(t *testing.T) { func TestMutator_EditServiceSpec_Nil(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(nil) assert.NoError(t, m.Apply()) } @@ -74,6 +79,7 @@ func TestMutator_EditServiceSpec_Nil(t *testing.T) { func TestMutator_EditServiceSpec_Error(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(_ *editors.ServiceSpecEditor) error { return errors.New("spec error") }) @@ -85,6 +91,7 @@ func TestMutator_EditServiceSpec_Error(t *testing.T) { func TestMutator_EditServiceSpec_Ports(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) e.EnsurePort(corev1.ServicePort{Name: "https", Port: 443}) @@ -97,6 +104,7 @@ func TestMutator_EditServiceSpec_Ports(t *testing.T) { func TestMutator_EditServiceSpec_Selector(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsureSelector("app", "myapp") return nil @@ -108,6 +116,7 @@ func TestMutator_EditServiceSpec_Selector(t *testing.T) { func TestMutator_EditServiceSpec_RawAccess(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.Raw().ExternalName = "external.example.com" return nil @@ -122,6 +131,7 @@ func TestMutator_OperationOrder(t *testing.T) { // Within a feature: metadata edits run before service spec edits. svc := newTestService() m := NewMutator(svc) + m.BeginFeature() // Register in reverse logical order to confirm Apply() enforces category ordering. m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) @@ -141,6 +151,7 @@ func TestMutator_OperationOrder(t *testing.T) { func TestMutator_MultipleFeatures(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) return nil @@ -158,6 +169,7 @@ func TestMutator_MultipleFeatures(t *testing.T) { func TestMutator_MultipleFeatures_LaterSeesEarlier(t *testing.T) { svc := newTestService() m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsureSelector("app", "myapp") return nil @@ -188,6 +200,7 @@ func TestMutator_Apply_PreservesNodePortsAfterEnsurePort(t *testing.T) { }, } m := NewMutator(svc) + m.BeginFeature() // EnsurePort replaces the entire ServicePort, which zeroes NodePort. m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) @@ -210,6 +223,7 @@ func TestMutator_Apply_PreservesNodePortsForLoadBalancer(t *testing.T) { }, } m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) return nil @@ -230,6 +244,7 @@ func TestMutator_Apply_ExplicitNodePortNotOverridden(t *testing.T) { }, } m := NewMutator(svc) + m.BeginFeature() // Explicitly set a different NodePort — should not be reverted to the snapshot. m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80, NodePort: 32000}) @@ -251,6 +266,7 @@ func TestMutator_Apply_NoNodePortPreservationForClusterIP(t *testing.T) { }, } m := NewMutator(svc) + m.BeginFeature() m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) return nil @@ -260,6 +276,64 @@ func TestMutator_Apply_NoNodePortPreservationForClusterIP(t *testing.T) { assert.Equal(t, int32(0), svc.Spec.Ports[0].NodePort, "ClusterIP services should not preserve NodePorts") } +// --- Constructor and feature plan invariants --- + +func TestNewMutator_InitializesNoPlan(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + + assert.Empty(t, m.plans, "NewMutator must not create any plans") + assert.Nil(t, m.active, "active plan must not be set") +} + +func TestBeginFeature_AddsExactlyOnePlan(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + + m.BeginFeature() + require.Len(t, m.plans, 1, "BeginFeature must add exactly one plan") + assert.Equal(t, &m.plans[0], m.active, "active must point to the new plan") + + m.BeginFeature() + require.Len(t, m.plans, 2) + assert.Equal(t, &m.plans[1], m.active) +} + +func TestBeginFeature_IsolatesFeaturePlans(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + + // Record a mutation in the first feature plan + m.BeginFeature() + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + return nil + }) + + // Start a new feature and record a different mutation + m.BeginFeature() + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "https", Port: 443}) + return nil + }) + + assert.Len(t, m.plans[0].serviceSpecEdits, 1, "first plan should have one spec edit") + assert.Len(t, m.plans[1].serviceSpecEdits, 1, "second plan should have one spec edit") +} + +func TestMutator_SingleFeature_PlanCount(t *testing.T) { + svc := newTestService() + m := NewMutator(svc) + m.BeginFeature() + m.EditServiceSpec(func(e *editors.ServiceSpecEditor) error { + e.EnsurePort(corev1.ServicePort{Name: "http", Port: 80}) + return nil + }) + + require.NoError(t, m.Apply()) + assert.Len(t, svc.Spec.Ports, 1) +} + // --- ObjectMutator interface --- func TestMutator_ImplementsObjectMutator(_ *testing.T) { From e46b4bf9026c1238a4eaa9d728786bdd4173ab4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 18:11:01 +0000 Subject: [PATCH 24/28] Fix field name in service.md example to match ExampleApp spec The doc example used `owner.Spec.MetricsEnabled` but the shared ExampleApp type defines the field as `EnableMetrics`. Updated the snippet and prose to use the correct field name. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index c5362755..f3863a39 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -353,11 +353,11 @@ func MetricsPortMutation(version string, enabled bool) service.Mutation { resource, err := service.NewBuilder(base). WithFieldApplicationFlavor(service.PreserveCurrentAnnotations). WithMutation(BaseServiceMutation(owner.Spec.Version)). - WithMutation(MetricsPortMutation(owner.Spec.Version, owner.Spec.MetricsEnabled)). + WithMutation(MetricsPortMutation(owner.Spec.Version, owner.Spec.EnableMetrics)). Build() ``` -When `MetricsEnabled` is true, the Service will expose both the HTTP port and the metrics port. When false, only the +When `EnableMetrics` is true, the Service will expose both the HTTP port and the metrics port. When false, only the HTTP port is configured. Neither mutation needs to know about the other. ## Guidance From 340788f01590a757040b2b290b072c0af07ab3f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 18:16:44 +0000 Subject: [PATCH 25/28] format --- docs/primitives/service.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/primitives/service.md b/docs/primitives/service.md index f3863a39..70b0ff89 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -357,8 +357,8 @@ resource, err := service.NewBuilder(base). Build() ``` -When `EnableMetrics` is true, the Service will expose both the HTTP port and the metrics port. When false, only the -HTTP port is configured. Neither mutation needs to know about the other. +When `EnableMetrics` is true, the Service will expose both the HTTP port and the metrics port. When false, only the HTTP +port is configured. Neither mutation needs to know about the other. ## Guidance From f1f6721d3a53ca5e61ced27d5d3250e47e4294b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 18:25:35 +0000 Subject: [PATCH 26/28] Gate healthCheckNodePort preservation on LB+Local and restrict unnamed port matching - Only preserve server-allocated healthCheckNodePort when the desired Service is type LoadBalancer with externalTrafficPolicy: Local. This prevents carrying forward stale values when the Service changes type. - Restrict EnsurePort unnamed port matching to only match existing ports that are also unnamed, preventing accidental name erasure. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/mutation/editors/servicespec.go | 7 ++++--- pkg/primitives/service/resource.go | 25 +++++++++++++++++-------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/pkg/mutation/editors/servicespec.go b/pkg/mutation/editors/servicespec.go index 0a14f57c..33740130 100644 --- a/pkg/mutation/editors/servicespec.go +++ b/pkg/mutation/editors/servicespec.go @@ -27,15 +27,16 @@ func (e *ServiceSpecEditor) SetType(t corev1.ServiceType) { } // EnsurePort upserts a service port. If a port with the same Name exists (or the same -// Port number and Protocol when Name is empty), it is replaced; otherwise the port is appended. -// When matching unnamed ports, an empty Protocol is treated as TCP (the Kubernetes default). +// Port number and Protocol when both ports are unnamed), it is replaced; otherwise the port +// is appended. When matching unnamed ports, only existing unnamed ports are considered, and +// an empty Protocol is treated as TCP (the Kubernetes default). func (e *ServiceSpecEditor) EnsurePort(port corev1.ServicePort) { for i, existing := range e.spec.Ports { if port.Name != "" && existing.Name == port.Name { e.spec.Ports[i] = port return } - if port.Name == "" && existing.Port == port.Port && normalizeProtocol(existing.Protocol) == normalizeProtocol(port.Protocol) { + if port.Name == "" && existing.Name == "" && existing.Port == port.Port && normalizeProtocol(existing.Protocol) == normalizeProtocol(port.Protocol) { e.spec.Ports[i] = port return } diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index 8b603437..0d3d4425 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -14,7 +14,8 @@ import ( // spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns after creation, // the server-populated Status (including LoadBalancer.Ingress), server-defaulted // IP family configuration (spec.ipFamilies, spec.ipFamilyPolicy), the -// server-allocated spec.healthCheckNodePort, and auto-allocated NodePort values +// server-allocated spec.healthCheckNodePort (only when the desired Service is a +// LoadBalancer with externalTrafficPolicy: Local), and auto-allocated NodePort values // for NodePort and LoadBalancer Services. NodePorts are only preserved when the // resulting Service type is NodePort or LoadBalancer and the desired port's // NodePort is 0 (unset); explicitly specified NodePort values in the desired @@ -38,17 +39,25 @@ func DefaultFieldApplicator(current, desired *corev1.Service) error { if current.Spec.IPFamilyPolicy == nil && original.Spec.IPFamilyPolicy != nil { current.Spec.IPFamilyPolicy = original.Spec.IPFamilyPolicy } - // Preserve the server-allocated HealthCheckNodePort when the desired - // object does not explicitly set one. The API server assigns this field - // for LoadBalancer Services with externalTrafficPolicy: Local. - if current.Spec.HealthCheckNodePort == 0 && original.Spec.HealthCheckNodePort != 0 { - current.Spec.HealthCheckNodePort = original.Spec.HealthCheckNodePort - } - // Only preserve nodePorts when the resulting Service type supports them. + // Determine the effective Service type for fields that are only meaningful + // on specific Service kinds. When type is empty, the API server defaults it + // to ClusterIP. effectiveType := current.Spec.Type if effectiveType == "" { effectiveType = corev1.ServiceTypeClusterIP } + // Preserve the server-allocated HealthCheckNodePort only when the desired + // configuration represents a LoadBalancer Service with + // externalTrafficPolicy: Local and the desired spec leaves the field unset. + // In all other cases, leaving the field as-is (typically 0) clears any + // previously allocated value when the Service moves away from that shape. + if effectiveType == corev1.ServiceTypeLoadBalancer && + current.Spec.ExternalTrafficPolicy == corev1.ServiceExternalTrafficPolicyLocal && + current.Spec.HealthCheckNodePort == 0 && + original.Spec.HealthCheckNodePort != 0 { + current.Spec.HealthCheckNodePort = original.Spec.HealthCheckNodePort + } + // Only preserve nodePorts when the resulting Service type supports them. if effectiveType == corev1.ServiceTypeNodePort || effectiveType == corev1.ServiceTypeLoadBalancer { preserveNodePorts(current, original) } From b23101366d115c7c67e3721130f0b1c2416dcd0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 18:33:16 +0000 Subject: [PATCH 27/28] Add service-primitive example to run-examples Makefile target Co-Authored-By: Claude Opus 4.6 (1M context) --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 29424227..c083519a 100644 --- a/Makefile +++ b/Makefile @@ -116,6 +116,7 @@ run-examples: ## Run all examples to verify they execute without error. go run ./examples/deployment-primitive/. go run ./examples/configmap-primitive/. go run ./examples/custom-resource-implementation/. + go run ./examples/service-primitive/. # go-install-tool will 'go install' any package with custom target and name of binary, if it doesn't exist From 7b3620a1c306360c50f285e46d75923b10b48574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 25 Mar 2026 14:10:19 +0000 Subject: [PATCH 28/28] Remove field applicators and flavors from service primitive for SSA migration The framework now uses Server-Side Apply instead of ctrl.CreateOrUpdate, eliminating the need for field applicators to merge desired state onto current state. This removes DefaultFieldApplicator, WithCustomFieldApplicator, WithFieldApplicationFlavor, the flavors.go file, and updates all tests to use the Object()-then-Mutate() pattern. Documentation is updated to remove SSA and field applicator sections. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/service.md | 59 --- .../service-primitive/resources/service.go | 7 +- pkg/primitives/service/builder.go | 30 -- pkg/primitives/service/builder_test.go | 38 -- pkg/primitives/service/flavors.go | 25 - pkg/primitives/service/flavors_test.go | 125 ----- pkg/primitives/service/resource.go | 64 +-- pkg/primitives/service/resource_test.go | 468 +----------------- 8 files changed, 19 insertions(+), 797 deletions(-) delete mode 100644 pkg/primitives/service/flavors.go delete mode 100644 pkg/primitives/service/flavors_test.go diff --git a/docs/primitives/service.md b/docs/primitives/service.md index 70b0ff89..f831a605 100644 --- a/docs/primitives/service.md +++ b/docs/primitives/service.md @@ -11,7 +11,6 @@ service configuration. | **Operational tracking** | Monitors LoadBalancer ingress assignment; reports `Operational` or `Pending` | | **Suspension** | Unaffected by suspension by default; customizable via handlers to delete or mutate on suspend | | **Mutation pipeline** | Typed editors for metadata and service spec, with a raw escape hatch for free-form access | -| **Flavors** | Preserves externally-managed fields (labels, annotations) | | **Data extraction** | Reads generated or updated values (ClusterIP, LoadBalancer ingress) after each sync cycle | ## Building a Service Primitive @@ -33,36 +32,10 @@ base := &corev1.Service{ } resource, err := service.NewBuilder(base). - WithFieldApplicationFlavor(service.PreserveCurrentAnnotations). WithMutation(MyFeatureMutation(owner.Spec.Version)). Build() ``` -## Default Field Application - -`DefaultFieldApplicator` replaces the current Service with a deep copy of the desired object while preserving -server-managed metadata, the Status subresource (including `LoadBalancer.Ingress`), the immutable `spec.clusterIP` and -`spec.clusterIPs` fields that Kubernetes assigns after creation, server-defaulted IP family configuration -(`spec.ipFamilies` and `spec.ipFamilyPolicy`), the server-allocated `spec.healthCheckNodePort`, and auto-allocated -NodePort values for `NodePort` and `LoadBalancer` Services. - -This prevents the operator from clearing the cluster-assigned IP, load balancer ingress assignments, IP family defaults, -health check node ports, or automatically-assigned NodePort numbers on every reconciliation cycle while ensuring all -other fields converge to the desired state. Explicitly specified values in the desired object always take precedence, so -these fields can still be managed by the operator when needed. - -Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: - -```go -resource, err := service.NewBuilder(base). - WithCustomFieldApplicator(func(current, desired *corev1.Service) error { - // Only synchronise owned fields; leave other fields untouched. - current.Spec.Ports = desired.DeepCopy().Spec.Ports - return nil - }). - Build() -``` - ## Mutations Mutations are the primary mechanism for modifying a `Service` beyond its baseline. Each mutation is a named function @@ -221,34 +194,6 @@ m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { }) ``` -## Flavors - -Flavors run after the baseline applicator and before mutations. They are used to preserve fields managed by external -controllers or other tools. - -### PreserveCurrentLabels - -Preserves labels present on the live object but absent from the applied desired state. Applied labels win on overlap. - -```go -resource, err := service.NewBuilder(base). - WithFieldApplicationFlavor(service.PreserveCurrentLabels). - Build() -``` - -### PreserveCurrentAnnotations - -Preserves annotations present on the live object but absent from the applied desired state. Applied annotations win on -overlap. - -```go -resource, err := service.NewBuilder(base). - WithFieldApplicationFlavor(service.PreserveCurrentAnnotations). - Build() -``` - -Multiple flavors can be registered and run in registration order. - ## Operational Status The Service primitive implements the `Operational` concept to track whether the Service is ready to accept traffic. @@ -351,7 +296,6 @@ func MetricsPortMutation(version string, enabled bool) service.Mutation { } resource, err := service.NewBuilder(base). - WithFieldApplicationFlavor(service.PreserveCurrentAnnotations). WithMutation(BaseServiceMutation(owner.Spec.Version)). WithMutation(MetricsPortMutation(owner.Spec.Version, owner.Spec.EnableMetrics)). Build() @@ -370,6 +314,3 @@ boolean conditions. **Use `EnsurePort` for idempotent port management.** The mutator tracks ports by name (or port number when unnamed), so repeated calls with the same name produce the same result. - -**Use `PreserveCurrentAnnotations` when cloud providers annotate Services.** Cloud load balancer controllers often add -annotations to Services. This flavor prevents your operator from deleting those annotations each reconcile cycle. diff --git a/examples/service-primitive/resources/service.go b/examples/service-primitive/resources/service.go index db7f4f82..2f4b0f0c 100644 --- a/examples/service-primitive/resources/service.go +++ b/examples/service-primitive/resources/service.go @@ -46,10 +46,7 @@ func NewServiceResource(owner *sharedapp.ExampleApp) (component.Resource, error) builder.WithMutation(features.VersionLabelMutation(owner.Spec.Version)) builder.WithMutation(features.MetricsPortMutation(owner.Spec.Version, owner.Spec.EnableMetrics)) - // 4. Preserve annotations added by external controllers (e.g., cloud LB controllers). - builder.WithFieldApplicationFlavor(service.PreserveCurrentAnnotations) - - // 5. Extract data from the reconciled Service. + // 4. Extract data from the reconciled Service. builder.WithDataExtractor(func(svc corev1.Service) error { fmt.Printf("Reconciled Service: %s (ClusterIP: %s)\n", svc.Name, svc.Spec.ClusterIP) for _, port := range svc.Spec.Ports { @@ -58,6 +55,6 @@ func NewServiceResource(owner *sharedapp.ExampleApp) (component.Resource, error) return nil }) - // 6. Build the final resource. + // 5. Build the final resource. return builder.Build() } diff --git a/pkg/primitives/service/builder.go b/pkg/primitives/service/builder.go index 44a705de..79166fd5 100644 --- a/pkg/primitives/service/builder.go +++ b/pkg/primitives/service/builder.go @@ -34,7 +34,6 @@ func NewBuilder(svc *corev1.Service) *Builder { base := generic.NewIntegrationBuilder[*corev1.Service, *Mutator]( svc, identityFunc, - DefaultFieldApplicator, NewMutator, ) @@ -63,35 +62,6 @@ func (b *Builder) WithMutation(m Mutation) *Builder { return b } -// WithCustomFieldApplicator sets a custom strategy for applying the desired -// state to the existing Service in the cluster. -// -// The default applicator (DefaultFieldApplicator) replaces the current object -// with a deep copy of the desired object while preserving Kubernetes-assigned -// immutable fields (spec.clusterIP and spec.clusterIPs). -// -// The applicator receives the current object from the API server and the desired -// object from the Resource, and is responsible for merging the desired changes -// into the current object. -func (b *Builder) WithCustomFieldApplicator( - applicator func(current, desired *corev1.Service) error, -) *Builder { - b.base.WithCustomFieldApplicator(applicator) - return b -} - -// WithFieldApplicationFlavor registers a post-baseline field application flavor. -// -// Flavors run after the baseline applicator (default or custom) in registration -// order. They are typically used to preserve fields from the live cluster object -// that should not be overwritten by the desired state. -// -// A nil flavor is ignored. -func (b *Builder) WithFieldApplicationFlavor(flavor FieldApplicationFlavor) *Builder { - b.base.WithFieldApplicationFlavor(generic.FieldApplicationFlavor[*corev1.Service](flavor)) - return b -} - // WithCustomOperationalStatus overrides the default logic for determining if the // Service is operational. // diff --git a/pkg/primitives/service/builder_test.go b/pkg/primitives/service/builder_test.go index c22cd186..641d436d 100644 --- a/pkg/primitives/service/builder_test.go +++ b/pkg/primitives/service/builder_test.go @@ -92,44 +92,6 @@ func TestBuilder(t *testing.T) { assert.Equal(t, "test-mutation", res.base.Mutations[0].Name) }) - t.Run("WithCustomFieldApplicator", func(t *testing.T) { - t.Parallel() - svc := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - } - applied := false - applicator := func(_ *corev1.Service, _ *corev1.Service) error { - applied = true - return nil - } - res, err := NewBuilder(svc). - WithCustomFieldApplicator(applicator). - Build() - require.NoError(t, err) - require.NotNil(t, res.base.CustomFieldApplicator) - _ = res.base.CustomFieldApplicator(nil, nil) - assert.True(t, applied) - }) - - t.Run("WithFieldApplicationFlavor", func(t *testing.T) { - t.Parallel() - svc := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - } - res, err := NewBuilder(svc). - WithFieldApplicationFlavor(PreserveCurrentLabels). - WithFieldApplicationFlavor(nil). // nil must be ignored - Build() - require.NoError(t, err) - assert.Len(t, res.base.FieldFlavors, 1) - }) - t.Run("WithCustomOperationalStatus", func(t *testing.T) { t.Parallel() svc := &corev1.Service{ diff --git a/pkg/primitives/service/flavors.go b/pkg/primitives/service/flavors.go deleted file mode 100644 index ca97903e..00000000 --- a/pkg/primitives/service/flavors.go +++ /dev/null @@ -1,25 +0,0 @@ -package service - -import ( - "github.com/sourcehawk/operator-component-framework/pkg/flavors" - corev1 "k8s.io/api/core/v1" -) - -// FieldApplicationFlavor defines a function signature for applying flavors to a -// Service resource. A flavor is called after the baseline field applicator has -// run and can be used to preserve or merge fields from the live cluster object. -type FieldApplicationFlavor flavors.FieldApplicationFlavor[*corev1.Service] - -// PreserveCurrentLabels ensures that any labels present on the current live -// Service but missing from the applied (desired) object are preserved. -// If a label exists in both, the applied value wins. -func PreserveCurrentLabels(applied, current, desired *corev1.Service) error { - return flavors.PreserveCurrentLabels[*corev1.Service]()(applied, current, desired) -} - -// PreserveCurrentAnnotations ensures that any annotations present on the current -// live Service but missing from the applied (desired) object are preserved. -// If an annotation exists in both, the applied value wins. -func PreserveCurrentAnnotations(applied, current, desired *corev1.Service) error { - return flavors.PreserveCurrentAnnotations[*corev1.Service]()(applied, current, desired) -} diff --git a/pkg/primitives/service/flavors_test.go b/pkg/primitives/service/flavors_test.go deleted file mode 100644 index bc19f816..00000000 --- a/pkg/primitives/service/flavors_test.go +++ /dev/null @@ -1,125 +0,0 @@ -package service - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -func TestPreserveCurrentLabels(t *testing.T) { - t.Run("adds missing labels from current", func(t *testing.T) { - applied := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"keep": "applied"}}} - current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"extra": "current"}}} - - require.NoError(t, PreserveCurrentLabels(applied, current, nil)) - assert.Equal(t, "applied", applied.Labels["keep"]) - assert.Equal(t, "current", applied.Labels["extra"]) - }) - - t.Run("applied value wins on overlap", func(t *testing.T) { - applied := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "applied"}}} - current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "current"}}} - - require.NoError(t, PreserveCurrentLabels(applied, current, nil)) - assert.Equal(t, "applied", applied.Labels["key"]) - }) - - t.Run("handles nil applied labels", func(t *testing.T) { - applied := &corev1.Service{} - current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"extra": "current"}}} - - require.NoError(t, PreserveCurrentLabels(applied, current, nil)) - assert.Equal(t, "current", applied.Labels["extra"]) - }) -} - -func TestPreserveCurrentAnnotations(t *testing.T) { - t.Run("adds missing annotations from current", func(t *testing.T) { - applied := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"keep": "applied"}}} - current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"extra": "current"}}} - - require.NoError(t, PreserveCurrentAnnotations(applied, current, nil)) - assert.Equal(t, "applied", applied.Annotations["keep"]) - assert.Equal(t, "current", applied.Annotations["extra"]) - }) - - t.Run("applied value wins on overlap", func(t *testing.T) { - applied := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"key": "applied"}}} - current := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"key": "current"}}} - - require.NoError(t, PreserveCurrentAnnotations(applied, current, nil)) - assert.Equal(t, "applied", applied.Annotations["key"]) - }) -} - -func TestFlavors_Integration(t *testing.T) { - // Flavors run after baseline applicator via Mutate, in registration order. - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "default", - Labels: map[string]string{"app": "desired"}, - }, - Spec: corev1.ServiceSpec{ - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80}, - }, - }, - } - - t.Run("PreserveCurrentLabels via Mutate", func(t *testing.T) { - res, err := NewBuilder(desired). - WithFieldApplicationFlavor(PreserveCurrentLabels). - Build() - require.NoError(t, err) - - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"external": "keep", "app": "old"}, - }, - } - require.NoError(t, res.Mutate(current)) - - assert.Equal(t, "desired", current.Labels["app"]) - assert.Equal(t, "keep", current.Labels["external"]) - }) - - t.Run("flavors run in registration order", func(t *testing.T) { - var order []string - flavor1 := func(_, _, _ *corev1.Service) error { - order = append(order, "flavor1") - return nil - } - flavor2 := func(_, _, _ *corev1.Service) error { - order = append(order, "flavor2") - return nil - } - - res, err := NewBuilder(desired). - WithFieldApplicationFlavor(flavor1). - WithFieldApplicationFlavor(flavor2). - Build() - require.NoError(t, err) - - require.NoError(t, res.Mutate(&corev1.Service{})) - assert.Equal(t, []string{"flavor1", "flavor2"}, order) - }) - - t.Run("flavor error is returned", func(t *testing.T) { - flavorErr := errors.New("flavor boom") - res, err := NewBuilder(desired). - WithFieldApplicationFlavor(func(_, _, _ *corev1.Service) error { - return flavorErr - }). - Build() - require.NoError(t, err) - - err = res.Mutate(&corev1.Service{}) - require.Error(t, err) - assert.True(t, errors.Is(err, flavorErr)) - }) -} diff --git a/pkg/primitives/service/resource.go b/pkg/primitives/service/resource.go index 0d3d4425..82d17d20 100644 --- a/pkg/primitives/service/resource.go +++ b/pkg/primitives/service/resource.go @@ -8,62 +8,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// DefaultFieldApplicator replaces current with a deep copy of desired while -// preserving server-managed metadata (ResourceVersion, UID, Generation, etc.), -// shared-controller fields (OwnerReferences, Finalizers), the immutable -// spec.clusterIP and spec.clusterIPs fields that Kubernetes assigns after creation, -// the server-populated Status (including LoadBalancer.Ingress), server-defaulted -// IP family configuration (spec.ipFamilies, spec.ipFamilyPolicy), the -// server-allocated spec.healthCheckNodePort (only when the desired Service is a -// LoadBalancer with externalTrafficPolicy: Local), and auto-allocated NodePort values -// for NodePort and LoadBalancer Services. NodePorts are only preserved when the -// resulting Service type is NodePort or LoadBalancer and the desired port's -// NodePort is 0 (unset); explicitly specified NodePort values in the desired -// object are always applied. -func DefaultFieldApplicator(current, desired *corev1.Service) error { - original := current.DeepCopy() - clusterIP := current.Spec.ClusterIP - clusterIPs := current.Spec.ClusterIPs - *current = *desired.DeepCopy() - generic.PreserveServerManagedFields(current, original) - generic.PreserveStatus(current, original) - if clusterIP != "" { - current.Spec.ClusterIP = clusterIP - current.Spec.ClusterIPs = clusterIPs - } - // Preserve server-defaulted IP family configuration when the desired - // object leaves them unset, avoiding perpetual reconcile diffs. - if len(current.Spec.IPFamilies) == 0 && len(original.Spec.IPFamilies) > 0 { - current.Spec.IPFamilies = original.Spec.IPFamilies - } - if current.Spec.IPFamilyPolicy == nil && original.Spec.IPFamilyPolicy != nil { - current.Spec.IPFamilyPolicy = original.Spec.IPFamilyPolicy - } - // Determine the effective Service type for fields that are only meaningful - // on specific Service kinds. When type is empty, the API server defaults it - // to ClusterIP. - effectiveType := current.Spec.Type - if effectiveType == "" { - effectiveType = corev1.ServiceTypeClusterIP - } - // Preserve the server-allocated HealthCheckNodePort only when the desired - // configuration represents a LoadBalancer Service with - // externalTrafficPolicy: Local and the desired spec leaves the field unset. - // In all other cases, leaving the field as-is (typically 0) clears any - // previously allocated value when the Service moves away from that shape. - if effectiveType == corev1.ServiceTypeLoadBalancer && - current.Spec.ExternalTrafficPolicy == corev1.ServiceExternalTrafficPolicyLocal && - current.Spec.HealthCheckNodePort == 0 && - original.Spec.HealthCheckNodePort != 0 { - current.Spec.HealthCheckNodePort = original.Spec.HealthCheckNodePort - } - // Only preserve nodePorts when the resulting Service type supports them. - if effectiveType == corev1.ServiceTypeNodePort || effectiveType == corev1.ServiceTypeLoadBalancer { - preserveNodePorts(current, original) - } - return nil -} - // preserveNodePorts restores auto-allocated nodePort values from the original // object when the desired port's NodePort is 0. Ports are matched by Name when // both ports have a non-empty Name; otherwise they are matched by Port+Protocol @@ -132,12 +76,8 @@ func (r *Resource) Object() (client.Object, error) { // Mutate transforms the current state of a Kubernetes Service into the desired state. // // The mutation process follows this order: -// 1. Field application: the current object is updated to reflect the desired base state, -// using either DefaultFieldApplicator or a custom applicator if one is configured. -// The default applicator preserves spec.clusterIP and spec.clusterIPs. -// 2. Field application flavors: any registered flavors are applied in registration order. -// 3. Feature mutations: all registered feature-gated mutations are applied in order. -// 4. Suspension: if the resource is in a suspending state, the suspension mutation is applied. +// 1. Feature mutations: all registered feature-gated mutations are applied in order. +// 2. Suspension: if the resource is in a suspending state, the suspension mutation is applied. func (r *Resource) Mutate(current client.Object) error { return r.base.Mutate(current) } diff --git a/pkg/primitives/service/resource_test.go b/pkg/primitives/service/resource_test.go index 88d9be5a..10ef3668 100644 --- a/pkg/primitives/service/resource_test.go +++ b/pkg/primitives/service/resource_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" ) func newValidService() *corev1.Service { @@ -58,415 +57,13 @@ func TestResource_Mutate(t *testing.T) { res, err := NewBuilder(desired).Build() require.NoError(t, err) - current := &corev1.Service{} - require.NoError(t, res.Mutate(current)) - - assert.Equal(t, "test", current.Spec.Selector["app"]) - assert.Equal(t, int32(80), current.Spec.Ports[0].Port) -} - -func TestResource_Mutate_PreservesClusterIP(t *testing.T) { - desired := newValidService() - res, err := NewBuilder(desired).Build() - require.NoError(t, err) - - current := &corev1.Service{ - Spec: corev1.ServiceSpec{ - ClusterIP: "10.0.0.1", - ClusterIPs: []string{"10.0.0.1"}, - }, - } - require.NoError(t, res.Mutate(current)) - - assert.Equal(t, "10.0.0.1", current.Spec.ClusterIP) - assert.Equal(t, []string{"10.0.0.1"}, current.Spec.ClusterIPs) - assert.Equal(t, "test", current.Spec.Selector["app"]) -} - -func TestDefaultFieldApplicator_PreservesServerManagedFields(t *testing.T) { - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - ResourceVersion: "12345", - UID: types.UID("abc-def"), - Generation: 3, - OwnerReferences: []metav1.OwnerReference{ - {APIVersion: "v1", Kind: "Pod", Name: "other-owner", UID: "other-uid"}, - }, - Finalizers: []string{"finalizer.example.com"}, - }, - Spec: corev1.ServiceSpec{ - ClusterIP: "10.0.0.1", - ClusterIPs: []string{"10.0.0.1"}, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - Labels: map[string]string{"app": "test"}, - }, - Spec: corev1.ServiceSpec{ - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // Desired spec and labels are applied - assert.Equal(t, "test", current.Labels["app"]) - assert.Equal(t, "test", current.Spec.Selector["app"]) - assert.Equal(t, int32(80), current.Spec.Ports[0].Port) - - // Server-managed fields are preserved - assert.Equal(t, "12345", current.ResourceVersion) - assert.Equal(t, types.UID("abc-def"), current.UID) - assert.Equal(t, int64(3), current.Generation) - - // Shared-controller fields are preserved - assert.Len(t, current.OwnerReferences, 1) - assert.Equal(t, "other-owner", current.OwnerReferences[0].Name) - assert.Equal(t, []string{"finalizer.example.com"}, current.Finalizers) - - // Immutable ClusterIP fields are preserved - assert.Equal(t, "10.0.0.1", current.Spec.ClusterIP) - assert.Equal(t, []string{"10.0.0.1"}, current.Spec.ClusterIPs) -} - -func TestDefaultFieldApplicator_PreservesStatus(t *testing.T) { - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeLoadBalancer, - ClusterIP: "10.0.0.1", - }, - Status: corev1.ServiceStatus{ - LoadBalancer: corev1.LoadBalancerStatus{ - Ingress: []corev1.LoadBalancerIngress{ - {IP: "203.0.113.1", Hostname: "lb.example.com"}, - }, - }, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeLoadBalancer, - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // Desired spec is applied - assert.Equal(t, "test", current.Spec.Selector["app"]) - - // Status is preserved (not zeroed by the deep copy of desired) - require.Len(t, current.Status.LoadBalancer.Ingress, 1) - assert.Equal(t, "203.0.113.1", current.Status.LoadBalancer.Ingress[0].IP) - assert.Equal(t, "lb.example.com", current.Status.LoadBalancer.Ingress[0].Hostname) -} - -func TestDefaultFieldApplicator_PreservesNodePorts(t *testing.T) { - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeNodePort, - ClusterIP: "10.0.0.1", - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, - {Name: "https", Port: 443, NodePort: 30443, Protocol: corev1.ProtocolTCP}, - }, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeNodePort, - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80}, - {Name: "https", Port: 443}, - }, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // Auto-allocated nodePorts are preserved when desired nodePort is 0 - require.Len(t, current.Spec.Ports, 2) - assert.Equal(t, int32(30080), current.Spec.Ports[0].NodePort) - assert.Equal(t, int32(30443), current.Spec.Ports[1].NodePort) -} - -func TestDefaultFieldApplicator_DoesNotPreserveNodePortsForClusterIP(t *testing.T) { - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeNodePort, - ClusterIP: "10.0.0.1", - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, - }, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeClusterIP, - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80}, - }, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // NodePort should NOT be preserved when switching to ClusterIP - assert.Equal(t, int32(0), current.Spec.Ports[0].NodePort) - assert.Equal(t, corev1.ServiceTypeClusterIP, current.Spec.Type) -} - -func TestDefaultFieldApplicator_DoesNotPreserveNodePortsForEmptyType(t *testing.T) { - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeNodePort, - ClusterIP: "10.0.0.1", - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, - }, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - // Empty type defaults to ClusterIP - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80}, - }, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // NodePort should NOT be preserved when desired type is empty (defaults to ClusterIP) - assert.Equal(t, int32(0), current.Spec.Ports[0].NodePort) -} - -func TestDefaultFieldApplicator_PreservesNodePortsForLoadBalancer(t *testing.T) { - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeLoadBalancer, - ClusterIP: "10.0.0.1", - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, - }, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeLoadBalancer, - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80}, - }, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // NodePort SHOULD be preserved for LoadBalancer type - assert.Equal(t, int32(30080), current.Spec.Ports[0].NodePort) -} - -func TestDefaultFieldApplicator_PreservesIPFamilyFields(t *testing.T) { - singleStack := corev1.IPFamilyPolicySingleStack - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - ClusterIP: "10.0.0.1", - IPFamilies: []corev1.IPFamily{corev1.IPv4Protocol}, - IPFamilyPolicy: &singleStack, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // Server-defaulted IP family fields are preserved - assert.Equal(t, []corev1.IPFamily{corev1.IPv4Protocol}, current.Spec.IPFamilies) - require.NotNil(t, current.Spec.IPFamilyPolicy) - assert.Equal(t, corev1.IPFamilyPolicySingleStack, *current.Spec.IPFamilyPolicy) -} - -func TestDefaultFieldApplicator_ExplicitIPFamilyOverridesPreservation(t *testing.T) { - singleStack := corev1.IPFamilyPolicySingleStack - preferDualStack := corev1.IPFamilyPolicyPreferDualStack - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - ClusterIP: "10.0.0.1", - IPFamilies: []corev1.IPFamily{corev1.IPv4Protocol}, - IPFamilyPolicy: &singleStack, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, - IPFamilies: []corev1.IPFamily{corev1.IPv4Protocol, corev1.IPv6Protocol}, - IPFamilyPolicy: &preferDualStack, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // Explicitly set IP family fields in desired take precedence - assert.Equal(t, []corev1.IPFamily{corev1.IPv4Protocol, corev1.IPv6Protocol}, current.Spec.IPFamilies) - require.NotNil(t, current.Spec.IPFamilyPolicy) - assert.Equal(t, corev1.IPFamilyPolicyPreferDualStack, *current.Spec.IPFamilyPolicy) -} - -func TestDefaultFieldApplicator_PreservesHealthCheckNodePort(t *testing.T) { - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeLoadBalancer, - ClusterIP: "10.0.0.1", - ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, - HealthCheckNodePort: 31234, - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80, NodePort: 30080}, - }, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeLoadBalancer, - ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80}, - }, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // Server-allocated HealthCheckNodePort is preserved - assert.Equal(t, int32(31234), current.Spec.HealthCheckNodePort) -} - -func TestDefaultFieldApplicator_ExplicitHealthCheckNodePortOverridesPreservation(t *testing.T) { - current := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeLoadBalancer, - ClusterIP: "10.0.0.1", - ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, - HealthCheckNodePort: 31234, - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80, NodePort: 30080}, - }, - }, - } - desired := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-svc", - Namespace: "test-ns", - }, - Spec: corev1.ServiceSpec{ - Type: corev1.ServiceTypeLoadBalancer, - ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, - HealthCheckNodePort: 32000, - Selector: map[string]string{"app": "test"}, - Ports: []corev1.ServicePort{ - {Name: "http", Port: 80}, - }, - }, - } - - err := DefaultFieldApplicator(current, desired) + obj, err := res.Object() require.NoError(t, err) + require.NoError(t, res.Mutate(obj)) - // Explicitly set HealthCheckNodePort in desired takes precedence - assert.Equal(t, int32(32000), current.Spec.HealthCheckNodePort) + got := obj.(*corev1.Service) + assert.Equal(t, "test", got.Spec.Selector["app"]) + assert.Equal(t, int32(80), got.Spec.Ports[0].Port) } func TestResource_Mutate_WithMutation(t *testing.T) { @@ -486,12 +83,14 @@ func TestResource_Mutate_WithMutation(t *testing.T) { Build() require.NoError(t, err) - current := &corev1.Service{} - require.NoError(t, res.Mutate(current)) + obj, err := res.Object() + require.NoError(t, err) + require.NoError(t, res.Mutate(obj)) - assert.Equal(t, int32(80), current.Spec.Ports[0].Port) - require.Len(t, current.Spec.Ports, 2) - assert.Equal(t, "metrics", current.Spec.Ports[1].Name) + got := obj.(*corev1.Service) + assert.Equal(t, int32(80), got.Spec.Ports[0].Port) + require.Len(t, got.Spec.Ports, 2) + assert.Equal(t, "metrics", got.Spec.Ports[1].Name) } func TestResource_Mutate_FeatureOrdering(t *testing.T) { @@ -527,51 +126,14 @@ func TestResource_Mutate_FeatureOrdering(t *testing.T) { Build() require.NoError(t, err) - current := &corev1.Service{} - require.NoError(t, res.Mutate(current)) + obj, err := res.Object() + require.NoError(t, err) + require.NoError(t, res.Mutate(obj)) assert.Equal(t, []string{"first", "second"}, callOrder) assert.True(t, observedFirstBeforeSecond, "second mutation function should execute after the first") } -func TestResource_Mutate_CustomFieldApplicator(t *testing.T) { - desired := newValidService() - - applicatorCalled := false - res, err := NewBuilder(desired). - WithCustomFieldApplicator(func(current, d *corev1.Service) error { - applicatorCalled = true - current.Spec.Ports = d.DeepCopy().Spec.Ports - return nil - }). - Build() - require.NoError(t, err) - - current := &corev1.Service{ - Spec: corev1.ServiceSpec{ - Selector: map[string]string{"external": "preserved"}, - }, - } - require.NoError(t, res.Mutate(current)) - - assert.True(t, applicatorCalled) - assert.Equal(t, int32(80), current.Spec.Ports[0].Port) - assert.Equal(t, "preserved", current.Spec.Selector["external"]) -} - -func TestResource_Mutate_CustomFieldApplicator_Error(t *testing.T) { - res, err := NewBuilder(newValidService()). - WithCustomFieldApplicator(func(_, _ *corev1.Service) error { - return errors.New("applicator error") - }). - Build() - require.NoError(t, err) - - err = res.Mutate(&corev1.Service{}) - require.Error(t, err) - assert.Contains(t, err.Error(), "applicator error") -} - func TestResource_ConvergingStatus(t *testing.T) { svc := newValidService() svc.Spec.Type = corev1.ServiceTypeLoadBalancer