diff --git a/.goreleaser.yml b/.goreleaser.yml index e2807ca41..fc7a51c7d 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -45,6 +45,7 @@ dockers: use: buildx build_flag_templates: - "--platform=linux/amd64" + - "--provenance=false" - image_templates: - "{{ .Env.OPCON_IMAGE_REPO }}:{{ .Env.IMAGE_TAG }}-arm64" dockerfile: Dockerfile.operator-controller @@ -53,6 +54,7 @@ dockers: use: buildx build_flag_templates: - "--platform=linux/arm64" + - "--provenance=false" - image_templates: - "{{ .Env.OPCON_IMAGE_REPO }}:{{ .Env.IMAGE_TAG }}-ppc64le" dockerfile: Dockerfile.operator-controller @@ -61,6 +63,7 @@ dockers: use: buildx build_flag_templates: - "--platform=linux/ppc64le" + - "--provenance=false" - image_templates: - "{{ .Env.OPCON_IMAGE_REPO }}:{{ .Env.IMAGE_TAG }}-s390x" dockerfile: Dockerfile.operator-controller @@ -69,6 +72,7 @@ dockers: use: buildx build_flag_templates: - "--platform=linux/s390x" + - "--provenance=false" - image_templates: - "{{ .Env.CATD_IMAGE_REPO }}:{{ .Env.IMAGE_TAG }}-amd64" dockerfile: Dockerfile.catalogd @@ -77,6 +81,7 @@ dockers: use: buildx build_flag_templates: - "--platform=linux/amd64" + - "--provenance=false" - image_templates: - "{{ .Env.CATD_IMAGE_REPO }}:{{ .Env.IMAGE_TAG }}-arm64" dockerfile: Dockerfile.catalogd @@ -85,6 +90,7 @@ dockers: use: buildx build_flag_templates: - "--platform=linux/arm64" + - "--provenance=false" - image_templates: - "{{ .Env.CATD_IMAGE_REPO }}:{{ .Env.IMAGE_TAG }}-ppc64le" dockerfile: Dockerfile.catalogd @@ -93,6 +99,7 @@ dockers: use: buildx build_flag_templates: - "--platform=linux/ppc64le" + - "--provenance=false" - image_templates: - "{{ .Env.CATD_IMAGE_REPO }}:{{ .Env.IMAGE_TAG }}-s390x" dockerfile: Dockerfile.catalogd @@ -101,6 +108,7 @@ dockers: use: buildx build_flag_templates: - "--platform=linux/s390x" + - "--provenance=false" docker_manifests: - name_template: "{{ .Env.OPCON_IMAGE_REPO }}:{{ .Env.IMAGE_TAG }}" image_templates: diff --git a/api/v1/clusterextensionrevision_types.go b/api/v1/clusterextensionrevision_types.go index f3416bf25..389e4a5b3 100644 --- a/api/v1/clusterextensionrevision_types.go +++ b/api/v1/clusterextensionrevision_types.go @@ -105,6 +105,19 @@ type ClusterExtensionRevisionSpec struct { // +optional // ProgressDeadlineMinutes int32 `json:"progressDeadlineMinutes,omitempty"` + + // collisionProtection specifies the default collision protection strategy for all objects + // in this revision. Individual phases or objects can override this value. + // + // When set, this value is used as the default for any phase or object that does not + // explicitly specify its own collisionProtection. + // + // The resolution order is: object > phase > spec + // + // +required + // +kubebuilder:validation:Enum=Prevent;IfNoController;None + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="collisionProtection is immutable" + CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } // ClusterExtensionRevisionLifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -144,6 +157,19 @@ type ClusterExtensionRevisionPhase struct { // +required // +kubebuilder:validation:MaxItems=50 Objects []ClusterExtensionRevisionObject `json:"objects"` + + // collisionProtection specifies the default collision protection strategy for all objects + // in this phase. Individual objects can override this value. + // + // When set, this value is used as the default for any object in this phase that does not + // explicitly specify its own collisionProtection. + // + // When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + // explicitly specify its own collisionProtection. + // + // +optional + // +kubebuilder:validation:Enum=Prevent;IfNoController;None + CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } // ClusterExtensionRevisionObject represents a Kubernetes object to be applied as part @@ -174,7 +200,9 @@ type ClusterExtensionRevisionObject struct { // Use this setting with extreme caution as it may cause multiple controllers to fight over // the same resource, resulting in increased load on the API server and etcd. // - // +required + // When omitted, the value is inherited from the phase, then spec. + // + // +optional // +kubebuilder:validation:Enum=Prevent;IfNoController;None CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } diff --git a/api/v1/clusterextensionrevision_types_test.go b/api/v1/clusterextensionrevision_types_test.go index a6519ec04..75a1a6cc3 100644 --- a/api/v1/clusterextensionrevision_types_test.go +++ b/api/v1/clusterextensionrevision_types_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func TestClusterExtensionRevisionImmutability(t *testing.T) { @@ -21,8 +22,9 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { }{ "revision is immutable": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, }, updateFunc: func(cer *ClusterExtensionRevision) { cer.Spec.Revision = 2 @@ -30,9 +32,10 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { }, "phases may be initially empty": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, - Phases: []ClusterExtensionRevisionPhase{}, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Phases: []ClusterExtensionRevisionPhase{}, }, updateFunc: func(cer *ClusterExtensionRevision) { cer.Spec.Phases = []ClusterExtensionRevisionPhase{ @@ -46,8 +49,9 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { }, "phases may be initially unset": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, }, updateFunc: func(cer *ClusterExtensionRevision) { cer.Spec.Phases = []ClusterExtensionRevisionPhase{ @@ -61,8 +65,9 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { }, "phases are immutable if not empty": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, Phases: []ClusterExtensionRevisionPhase{ { Name: "foo", @@ -79,6 +84,16 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { } }, }, + "spec collisionProtection is immutable": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + }, + updateFunc: func(cer *ClusterExtensionRevision) { + cer.Spec.CollisionProtection = CollisionProtectionNone + }, + }, } { t.Run(name, func(t *testing.T) { cer := &ClusterExtensionRevision{ @@ -124,8 +139,9 @@ func TestClusterExtensionRevisionValidity(t *testing.T) { }, "revision must be positive": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, }, valid: true, }, @@ -192,6 +208,70 @@ func TestClusterExtensionRevisionValidity(t *testing.T) { }, valid: false, }, + "spec collisionProtection accepts Prevent": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + }, + valid: true, + }, + "spec collisionProtection accepts IfNoController": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionIfNoController, + }, + valid: true, + }, + "spec collisionProtection accepts None": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionNone, + }, + valid: true, + }, + "spec collisionProtection is required": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + }, + valid: false, + }, + "spec collisionProtection rejects invalid values": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtection("Invalid"), + }, + valid: false, + }, + "spec collisionProtection must be set": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + }, + valid: false, + }, + "object collisionProtection is optional": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Phases: []ClusterExtensionRevisionPhase{ + { + Name: "deploy", + Objects: []ClusterExtensionRevisionObject{ + { + Object: configMap(), + }, + }, + }, + }, + }, + valid: true, + }, } { t.Run(name, func(t *testing.T) { cer := &ClusterExtensionRevision{ @@ -211,3 +291,15 @@ func TestClusterExtensionRevisionValidity(t *testing.T) { }) } } + +func configMap() unstructured.Unstructured { + return unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", + }, + }, + } +} diff --git a/api/v1/validation_test.go b/api/v1/validation_test.go index 16c9447a5..c2f857493 100644 --- a/api/v1/validation_test.go +++ b/api/v1/validation_test.go @@ -36,6 +36,7 @@ func TestValidate(t *testing.T) { } defaultRevisionSpec := func(s *ClusterExtensionRevisionSpec) *ClusterExtensionRevisionSpec { s.Revision = 1 + s.CollisionProtection = CollisionProtectionPrevent return s } c := newClient(t) diff --git a/commitchecker.yaml b/commitchecker.yaml index 113dd347a..f7168dd90 100644 --- a/commitchecker.yaml +++ b/commitchecker.yaml @@ -1,4 +1,4 @@ -expectedMergeBase: fb28936f0227ad129151ce04f53598cc08e1b96e +expectedMergeBase: 1ef820f0ca56126586fca2dc7a422c71edd7deef upstreamBranch: main upstreamOrg: operator-framework upstreamRepo: operator-controller diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml index 2b431eab9..5ac459dea 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml @@ -56,6 +56,23 @@ spec: spec: description: spec defines the desired state of the ClusterExtensionRevision. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this revision. Individual phases or objects can override this value. + + When set, this value is used as the default for any phase or object that does not + explicitly specify its own collisionProtection. + + The resolution order is: object > phase > spec + enum: + - Prevent + - IfNoController + - None + type: string + x-kubernetes-validations: + - message: collisionProtection is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -102,6 +119,21 @@ spec: ClusterExtensionRevisionPhase represents a group of objects that are applied together. The phase is considered complete only after all objects pass their status probes. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this phase. Individual objects can override this value. + + When set, this value is used as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + + When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + enum: + - Prevent + - IfNoController + - None + type: string name: description: |- name is a required identifier for this phase. @@ -149,6 +181,8 @@ spec: owned by another controller. Use this setting with extreme caution as it may cause multiple controllers to fight over the same resource, resulting in increased load on the API server and etcd. + + When omitted, the value is inherited from the phase, then spec. enum: - Prevent - IfNoController @@ -163,7 +197,6 @@ spec: x-kubernetes-embedded-resource: true x-kubernetes-preserve-unknown-fields: true required: - - collisionProtection - object type: object maxItems: 50 @@ -205,6 +238,7 @@ spec: - message: revision is immutable rule: self == oldSelf required: + - collisionProtection - lifecycleState - revision type: object diff --git a/helm/olmv1/templates/rbac/role-olmv1-system-metrics-monitor-role.yml b/helm/olmv1/templates/rbac/role-olmv1-system-metrics-monitor-role.yml index 0a452d6b9..0cf8ee17a 100644 --- a/helm/olmv1/templates/rbac/role-olmv1-system-metrics-monitor-role.yml +++ b/helm/olmv1/templates/rbac/role-olmv1-system-metrics-monitor-role.yml @@ -21,5 +21,13 @@ rules: - get - list - watch + - apiGroups: + - discovery.k8s.io + resources: + - endpointslices + verbs: + - get + - list + - watch {{- end -}} {{- end -}} diff --git a/helm/olmv1/templates/servicemonitor-olmv1-system-metrics-monitor.yml b/helm/olmv1/templates/servicemonitor-olmv1-system-metrics-monitor.yml index a5bb357c3..ab8c43822 100644 --- a/helm/olmv1/templates/servicemonitor-olmv1-system-metrics-monitor.yml +++ b/helm/olmv1/templates/servicemonitor-olmv1-system-metrics-monitor.yml @@ -29,5 +29,6 @@ spec: selector: matchLabels: app.kubernetes.io/name: {{ include "olmv1.label.name" . }} + serviceDiscoveryRole: EndpointSlice {{- end -}} {{- end -}} diff --git a/internal/catalogd/controllers/core/clustercatalog_controller.go b/internal/catalogd/controllers/core/clustercatalog_controller.go index 3d7fd935c..fedfe500f 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller.go @@ -40,6 +40,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/catalogd/storage" + errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" k8sutil "github.com/operator-framework/operator-controller/internal/shared/util/k8s" ) @@ -336,7 +337,7 @@ func updateStatusProgressing(status *ocv1.ClusterCatalogStatus, generation int64 if err != nil { progressingCond.Status = metav1.ConditionTrue progressingCond.Reason = ocv1.ReasonRetrying - progressingCond.Message = err.Error() + progressingCond.Message = errorutil.SanitizeNetworkError(err) } if errors.Is(err, reconcile.TerminalError(nil)) { diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 04e59ca95..cd3579a4e 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -75,8 +75,7 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( sanitizedUnstructured(ctx, &obj) objs = append(objs, ocv1.ClusterExtensionRevisionObject{ - Object: obj, - CollisionProtection: ocv1.CollisionProtectionNone, // allow to adopt objects from previous release + Object: obj, }) } @@ -88,6 +87,7 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( }) rev.Name = fmt.Sprintf("%s-1", ext.Name) rev.Spec.Revision = 1 + rev.Spec.CollisionProtection = ocv1.CollisionProtectionNone // allow to adopt objects from previous release return rev, nil } @@ -147,11 +147,12 @@ func (r *SimpleRevisionGenerator) GenerateRevision( sanitizedUnstructured(ctx, &unstr) objs = append(objs, ocv1.ClusterExtensionRevisionObject{ - Object: unstr, - CollisionProtection: ocv1.CollisionProtectionPrevent, + Object: unstr, }) } - return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil + rev := r.buildClusterExtensionRevision(objs, ext, revisionAnnotations) + rev.Spec.CollisionProtection = ocv1.CollisionProtectionPrevent + return rev, nil } // sanitizedUnstructured takes an unstructured obj, removes status if present, and returns a sanitized copy containing only the allowed metadata entries set below. diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 887dee4a9..854f9517f 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -114,8 +114,9 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ - LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + CollisionProtection: ocv1.CollisionProtectionNone, + Revision: 1, Phases: []ocv1.ClusterExtensionRevisionPhase{ { Name: "deploy", @@ -132,7 +133,6 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) }, }, }, - CollisionProtection: ocv1.CollisionProtectionNone, }, { Object: unstructured.Unstructured{ @@ -146,7 +146,6 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) }, }, }, - CollisionProtection: ocv1.CollisionProtectionNone, }, }, }, @@ -215,6 +214,8 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { }, rev.Labels) t.Log("by checking the revision number is 0") require.Equal(t, int64(0), rev.Spec.Revision) + t.Log("by checking the spec-level collisionProtection is set") + require.Equal(t, ocv1.CollisionProtectionPrevent, rev.Spec.CollisionProtection) t.Log("by checking the rendered objects are present in the correct phases") require.Equal(t, []ocv1.ClusterExtensionRevisionPhase{ { @@ -231,7 +232,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { "spec": map[string]interface{}{}, }, }, - CollisionProtection: ocv1.CollisionProtectionPrevent, }, { Object: unstructured.Unstructured{ @@ -260,7 +260,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { }, }, }, - CollisionProtection: ocv1.CollisionProtectionPrevent, }, }, }, diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 138bcd8eb..eff03e9d9 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -464,10 +464,10 @@ func (c *ClusterExtensionRevisionReconciler) toBoxcutterRevision(ctx context.Con objLabels[labels.OwnerNameKey] = rev.Labels[labels.OwnerNameKey] obj.SetLabels(objLabels) - switch specObj.CollisionProtection { + switch cp := EffectiveCollisionProtection(rev.Spec.CollisionProtection, specPhase.CollisionProtection, specObj.CollisionProtection); cp { case ocv1.CollisionProtectionIfNoController, ocv1.CollisionProtectionNone: opts = append(opts, boxcutter.WithObjectReconcileOptions( - obj, boxcutter.WithCollisionProtection(specObj.CollisionProtection))) + obj, boxcutter.WithCollisionProtection(cp))) } phase.Objects = append(phase.Objects, *obj) @@ -477,6 +477,18 @@ func (c *ClusterExtensionRevisionReconciler) toBoxcutterRevision(ctx context.Con return r, opts, nil } +// EffectiveCollisionProtection resolves the collision protection value using +// the inheritance hierarchy: object > phase > spec > default ("Prevent"). +func EffectiveCollisionProtection(cp ...ocv1.CollisionProtection) ocv1.CollisionProtection { + ecp := ocv1.CollisionProtectionPrevent + for _, c := range cp { + if c != "" { + ecp = c + } + } + return ecp +} + var ( deploymentProbe = &probing.GroupKindSelector{ GroupKind: schema.GroupKind{Group: appsv1.GroupName, Kind: "Deployment"}, diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go index f456976f4..4426e0181 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go @@ -1266,6 +1266,60 @@ func (m *mockTrackingCache) Free(ctx context.Context, user client.Object) error return nil } +func Test_effectiveCollisionProtection(t *testing.T) { + for _, tc := range []struct { + name string + specCP ocv1.CollisionProtection + phaseCP ocv1.CollisionProtection + objectCP ocv1.CollisionProtection + expected ocv1.CollisionProtection + }{ + { + name: "all empty defaults to Prevent", + expected: ocv1.CollisionProtectionPrevent, + }, + { + name: "spec only", + specCP: ocv1.CollisionProtectionNone, + expected: ocv1.CollisionProtectionNone, + }, + { + name: "phase overrides spec", + specCP: ocv1.CollisionProtectionPrevent, + phaseCP: ocv1.CollisionProtectionIfNoController, + expected: ocv1.CollisionProtectionIfNoController, + }, + { + name: "object overrides phase", + specCP: ocv1.CollisionProtectionPrevent, + phaseCP: ocv1.CollisionProtectionIfNoController, + objectCP: ocv1.CollisionProtectionNone, + expected: ocv1.CollisionProtectionNone, + }, + { + name: "object overrides spec", + specCP: ocv1.CollisionProtectionNone, + objectCP: ocv1.CollisionProtectionPrevent, + expected: ocv1.CollisionProtectionPrevent, + }, + { + name: "phase only", + phaseCP: ocv1.CollisionProtectionNone, + expected: ocv1.CollisionProtectionNone, + }, + { + name: "object only", + objectCP: ocv1.CollisionProtectionIfNoController, + expected: ocv1.CollisionProtectionIfNoController, + }, + } { + t.Run(tc.name, func(t *testing.T) { + result := controllers.EffectiveCollisionProtection(tc.specCP, tc.phaseCP, tc.objectCP) + require.Equal(t, tc.expected, result) + }) + } +} + func Test_ClusterExtensionRevisionReconciler_getScopedClient_Errors(t *testing.T) { testScheme := newScheme(t) diff --git a/internal/operator-controller/controllers/common_controller.go b/internal/operator-controller/controllers/common_controller.go index cb6834c6b..e1991e4d1 100644 --- a/internal/operator-controller/controllers/common_controller.go +++ b/internal/operator-controller/controllers/common_controller.go @@ -157,7 +157,7 @@ func setStatusProgressing(ext *ocv1.ClusterExtension, err error) { if err != nil { progressingCond.Reason = ocv1.ReasonRetrying // Unwrap TerminalError to avoid "terminal error:" prefix in message - progressingCond.Message = errorutil.UnwrapTerminal(err).Error() + progressingCond.Message = errorutil.SanitizeNetworkError(errorutil.UnwrapTerminal(err)) } if errors.Is(err, reconcile.TerminalError(nil)) { diff --git a/internal/operator-controller/controllers/common_controller_test.go b/internal/operator-controller/controllers/common_controller_test.go index 3cc757543..2055a2b89 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -3,6 +3,7 @@ package controllers import ( "errors" "fmt" + "net" "strings" "testing" @@ -68,6 +69,50 @@ func TestSetStatusProgressing(t *testing.T) { Message: "missing required field", }, }, + { + name: "non-nil ClusterExtension, non-terminal network error with source port, Progressing condition message has source port stripped", + err: fmt.Errorf("source catalog content: %w", &net.OpError{ + Op: "read", + Net: "tcp", + Source: &net.TCPAddr{ + IP: net.ParseIP("10.0.0.1"), + Port: 52341, + }, + Addr: &net.TCPAddr{ + IP: net.ParseIP("192.168.1.100"), + Port: 443, + }, + Err: fmt.Errorf("connect: connection refused"), + }), + clusterExtension: &ocv1.ClusterExtension{}, + expected: metav1.Condition{ + Type: ocv1.TypeProgressing, + Status: metav1.ConditionTrue, + Reason: ocv1.ReasonRetrying, + Message: "source catalog content: connect: connection refused", + }, + }, + { + name: "non-nil ClusterExtension, non-terminal DNS error, Progressing condition message is sanitized", + err: fmt.Errorf("source catalog content: %w", &net.OpError{ + Op: "dial", + Net: "tcp", + Err: &net.DNSError{ + IsTemporary: true, + IsTimeout: true, + Server: "10.96.0.10:53", + Name: "registry.example.com", + Err: "read udp 10.244.0.8:46753->10.96.0.10:53: i/o timeout", + }, + }), + clusterExtension: &ocv1.ClusterExtension{}, + expected: metav1.Condition{ + Type: ocv1.TypeProgressing, + Status: metav1.ConditionTrue, + Reason: ocv1.ReasonRetrying, + Message: "source catalog content: dial tcp: lookup registry.example.com on 10.96.0.10:53: i/o timeout", + }, + }, } { t.Run(tc.name, func(t *testing.T) { setStatusProgressing(tc.clusterExtension, tc.err) diff --git a/internal/shared/util/error/sanitize.go b/internal/shared/util/error/sanitize.go new file mode 100644 index 000000000..dc1cd7ee0 --- /dev/null +++ b/internal/shared/util/error/sanitize.go @@ -0,0 +1,26 @@ +package error + +import ( + "regexp" +) + +var ephemeralNetworkErrorPattern = regexp.MustCompile(`(read|write) (tcp|udp) ((?:[0-9]{1,3}(?:\.[0-9]{1,3}){3}|\[[0-9a-fA-F:]+\])(?::\d+)?)->((?:[0-9]{1,3}(?:\.[0-9]{1,3}){3}|\[[0-9a-fA-F:]+\])(?::\d+)?)(: )?`) + +// SanitizeNetworkError returns a stable, deterministic error message for network +// errors by stripping ephemeral details (such as source and destination addresses +// and ports) from low-level socket operations. This ensures consistent error +// messages across retries, preventing unnecessary status condition updates when +// the only change is an ephemeral source port. +// +// The function uses a regex to remove substrings matching the pattern +// "read/write tcp/udp ->: " (with IPv4 or IPv6 addresses), which +// commonly appear inside [net.DNSError] Err fields (e.g., +// "read udp 10.244.0.8:46753->10.96.0.10:53: i/o timeout"). +// +// Returns "" for nil errors, or the sanitized error string otherwise. +func SanitizeNetworkError(err error) string { + if err == nil { + return "" + } + return ephemeralNetworkErrorPattern.ReplaceAllString(err.Error(), "") +} diff --git a/internal/shared/util/error/sanitize_test.go b/internal/shared/util/error/sanitize_test.go new file mode 100644 index 000000000..c7ef92d95 --- /dev/null +++ b/internal/shared/util/error/sanitize_test.go @@ -0,0 +1,235 @@ +package error + +import ( + "fmt" + "net" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSanitizeNetworkError(t *testing.T) { + for _, tc := range []struct { + name string + err error + expected string + }{ + { + name: "nil error returns empty string", + err: nil, + expected: "", + }, + { + name: "non-network error returns original message", + err: fmt.Errorf("some random error"), + expected: "some random error", + }, + { + name: "strips read udp address pattern from DNS inner error", + err: &net.OpError{ + Op: "dial", + Net: "tcp", + Err: &net.DNSError{ + IsNotFound: false, + IsTemporary: true, + IsTimeout: true, + Server: "10.96.0.10:53", + Name: "docker-registry.operator-controller.svc", + Err: "read udp 10.244.0.8:46753->10.96.0.10:53: i/o timeout", + }, + }, + expected: "dial tcp: lookup docker-registry.operator-controller.svc on 10.96.0.10:53: i/o timeout", + }, + { + name: "strips read udp pattern from wrapped error preserving outer context", + err: fmt.Errorf("source catalog content: error creating image source: %w", &net.OpError{ + Op: "dial", + Net: "tcp", + Err: &net.DNSError{ + IsNotFound: false, + IsTemporary: true, + IsTimeout: true, + Server: "10.96.0.10:53", + Name: "docker-registry.operator-controller.svc", + Err: "read udp 10.244.0.8:46753->10.96.0.10:53: i/o timeout", + }, + }), + expected: "source catalog content: error creating image source: dial tcp: lookup docker-registry.operator-controller.svc on 10.96.0.10:53: i/o timeout", + }, + { + name: "net.DNSError with IsNotFound unchanged", + err: &net.DNSError{ + IsNotFound: true, + Server: "10.96.0.10:53", + Name: "registry.example.com", + Err: "no such host", + }, + expected: "lookup registry.example.com on 10.96.0.10:53: no such host", + }, + { + name: "net.DNSError without server unchanged", + err: &net.DNSError{ + IsNotFound: true, + Name: "registry.example.com", + Err: "no such host", + }, + expected: "lookup registry.example.com: no such host", + }, + { + name: "net.DNSError without read/write pattern unchanged", + err: &net.DNSError{ + Server: "10.96.0.10:53", + Name: "registry.example.com", + Err: "server misbehaving", + }, + expected: "lookup registry.example.com on 10.96.0.10:53: server misbehaving", + }, + { + name: "wrapped net.DNSError without read/write pattern preserves wrapping", + err: fmt.Errorf("source catalog content: %w", &net.DNSError{ + Server: "10.96.0.10:53", + Name: "registry.example.com", + Err: "server misbehaving", + }), + expected: "source catalog content: lookup registry.example.com on 10.96.0.10:53: server misbehaving", + }, + { + name: "net.DNSError with both IsTimeout and IsNotFound unchanged", + err: &net.DNSError{ + IsTimeout: true, + IsNotFound: true, + Server: "10.96.0.10:53", + Name: "registry.example.com", + Err: "i/o timeout", + }, + expected: "lookup registry.example.com on 10.96.0.10:53: i/o timeout", + }, + { + name: "net.DNSError without server unchanged", + err: &net.DNSError{ + IsTimeout: true, + Name: "registry.example.com", + Err: "i/o timeout", + }, + expected: "lookup registry.example.com: i/o timeout", + }, + { + name: "net.OpError dial with source and addr unchanged", + err: &net.OpError{ + Op: "dial", + Net: "tcp", + Source: &net.TCPAddr{ + IP: net.ParseIP("10.0.0.1"), + Port: 52341, + }, + Addr: &net.TCPAddr{ + IP: net.ParseIP("192.168.1.100"), + Port: 443, + }, + Err: fmt.Errorf("connect: connection refused"), + }, + expected: "dial tcp 10.0.0.1:52341->192.168.1.100:443: connect: connection refused", + }, + { + name: "net.OpError dial without source unchanged", + err: &net.OpError{ + Op: "dial", + Net: "tcp", + Addr: &net.TCPAddr{ + IP: net.ParseIP("192.168.1.100"), + Port: 443, + }, + Err: fmt.Errorf("connect: connection refused"), + }, + expected: "dial tcp 192.168.1.100:443: connect: connection refused", + }, + { + name: "wrapped net.OpError dial preserves full error string", + err: fmt.Errorf("source catalog content: error creating image source: %w", &net.OpError{ + Op: "dial", + Net: "tcp", + Source: &net.TCPAddr{ + IP: net.ParseIP("10.0.0.1"), + Port: 52341, + }, + Addr: &net.TCPAddr{ + IP: net.ParseIP("192.168.1.100"), + Port: 443, + }, + Err: fmt.Errorf("connect: connection refused"), + }), + expected: "source catalog content: error creating image source: dial tcp 10.0.0.1:52341->192.168.1.100:443: connect: connection refused", + }, + { + name: "strips read tcp pattern", + err: fmt.Errorf("read tcp 10.0.0.1:52341->192.168.1.100:443: connection reset by peer"), + expected: "connection reset by peer", + }, + { + name: "strips write tcp pattern", + err: fmt.Errorf("write tcp 10.0.0.1:52341->192.168.1.100:443: broken pipe"), + expected: "broken pipe", + }, + { + name: "strips read udp pattern with IPv6 addresses", + err: fmt.Errorf("read udp [::1]:52341->[fd00::1]:53: i/o timeout"), + expected: "i/o timeout", + }, + } { + t.Run(tc.name, func(t *testing.T) { + result := SanitizeNetworkError(tc.err) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestSanitizeNetworkErrorConsistency(t *testing.T) { + // Simulate DNS errors with different ephemeral ports in the read udp pattern + makeError := func(sourcePort int) error { + return &net.OpError{ + Op: "dial", + Net: "tcp", + Err: &net.DNSError{ + IsTemporary: true, + IsTimeout: true, + Server: "10.96.0.10:53", + Name: "docker-registry.operator-controller.svc", + Err: fmt.Sprintf("read udp 10.244.0.8:%d->10.96.0.10:53: i/o timeout", sourcePort), + }, + } + } + + // Different source ports should produce the same sanitized message + msg1 := SanitizeNetworkError(makeError(46753)) + msg2 := SanitizeNetworkError(makeError(51234)) + msg3 := SanitizeNetworkError(makeError(60000)) + + assert.Equal(t, msg1, msg2, "sanitized messages with different source ports should be equal") + assert.Equal(t, msg2, msg3, "sanitized messages with different source ports should be equal") + assert.Equal(t, "dial tcp: lookup docker-registry.operator-controller.svc on 10.96.0.10:53: i/o timeout", msg1) +} + +func TestSanitizeNetworkErrorDNSConsistency(t *testing.T) { + // Simulate DNS errors with different ephemeral ports wrapped in context + makeError := func(sourcePort int) error { + return fmt.Errorf("source catalog content: error creating image source: %w", &net.OpError{ + Op: "dial", + Net: "tcp", + Err: &net.DNSError{ + IsTemporary: true, + IsTimeout: true, + Server: "10.96.0.10:53", + Name: "registry.example.com", + Err: fmt.Sprintf("read udp 10.244.0.8:%d->10.96.0.10:53: i/o timeout", sourcePort), + }, + }) + } + + msg1 := SanitizeNetworkError(makeError(46753)) + msg2 := SanitizeNetworkError(makeError(51234)) + msg3 := SanitizeNetworkError(makeError(60000)) + + assert.Equal(t, msg1, msg2, "sanitized DNS messages with different source ports should be equal") + assert.Equal(t, msg2, msg3, "sanitized DNS messages with different source ports should be equal") + assert.Equal(t, "source catalog content: error creating image source: dial tcp: lookup registry.example.com on 10.96.0.10:53: i/o timeout", msg1) +} diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index c6e370cda..da5bda705 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -668,6 +668,23 @@ spec: spec: description: spec defines the desired state of the ClusterExtensionRevision. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this revision. Individual phases or objects can override this value. + + When set, this value is used as the default for any phase or object that does not + explicitly specify its own collisionProtection. + + The resolution order is: object > phase > spec + enum: + - Prevent + - IfNoController + - None + type: string + x-kubernetes-validations: + - message: collisionProtection is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -714,6 +731,21 @@ spec: ClusterExtensionRevisionPhase represents a group of objects that are applied together. The phase is considered complete only after all objects pass their status probes. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this phase. Individual objects can override this value. + + When set, this value is used as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + + When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + enum: + - Prevent + - IfNoController + - None + type: string name: description: |- name is a required identifier for this phase. @@ -761,6 +793,8 @@ spec: owned by another controller. Use this setting with extreme caution as it may cause multiple controllers to fight over the same resource, resulting in increased load on the API server and etcd. + + When omitted, the value is inherited from the phase, then spec. enum: - Prevent - IfNoController @@ -775,7 +809,6 @@ spec: x-kubernetes-embedded-resource: true x-kubernetes-preserve-unknown-fields: true required: - - collisionProtection - object type: object maxItems: 50 @@ -817,6 +850,7 @@ spec: - message: revision is immutable rule: self == oldSelf required: + - collisionProtection - lifecycleState - revision type: object diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 46ca67c91..be1138655 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -629,6 +629,23 @@ spec: spec: description: spec defines the desired state of the ClusterExtensionRevision. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this revision. Individual phases or objects can override this value. + + When set, this value is used as the default for any phase or object that does not + explicitly specify its own collisionProtection. + + The resolution order is: object > phase > spec + enum: + - Prevent + - IfNoController + - None + type: string + x-kubernetes-validations: + - message: collisionProtection is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -675,6 +692,21 @@ spec: ClusterExtensionRevisionPhase represents a group of objects that are applied together. The phase is considered complete only after all objects pass their status probes. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this phase. Individual objects can override this value. + + When set, this value is used as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + + When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + enum: + - Prevent + - IfNoController + - None + type: string name: description: |- name is a required identifier for this phase. @@ -722,6 +754,8 @@ spec: owned by another controller. Use this setting with extreme caution as it may cause multiple controllers to fight over the same resource, resulting in increased load on the API server and etcd. + + When omitted, the value is inherited from the phase, then spec. enum: - Prevent - IfNoController @@ -736,7 +770,6 @@ spec: x-kubernetes-embedded-resource: true x-kubernetes-preserve-unknown-fields: true required: - - collisionProtection - object type: object maxItems: 50 @@ -778,6 +811,7 @@ spec: - message: revision is immutable rule: self == oldSelf required: + - collisionProtection - lifecycleState - revision type: object diff --git a/openshift/catalogd/manifests-experimental.yaml b/openshift/catalogd/manifests-experimental.yaml index 6d35355b3..d46d31157 100644 --- a/openshift/catalogd/manifests-experimental.yaml +++ b/openshift/catalogd/manifests-experimental.yaml @@ -693,6 +693,14 @@ rules: - get - list - watch + - apiGroups: + - discovery.k8s.io + resources: + - endpointslices + verbs: + - get + - list + - watch --- # Source: olmv1/templates/openshift/rolebinding-openshift-config-manager-rolebinding.yml apiVersion: rbac.authorization.k8s.io/v1 @@ -1084,3 +1092,4 @@ spec: selector: matchLabels: app.kubernetes.io/name: catalogd + serviceDiscoveryRole: EndpointSlice diff --git a/openshift/catalogd/manifests.yaml b/openshift/catalogd/manifests.yaml index 124406c76..57dfeb91f 100644 --- a/openshift/catalogd/manifests.yaml +++ b/openshift/catalogd/manifests.yaml @@ -693,6 +693,14 @@ rules: - get - list - watch + - apiGroups: + - discovery.k8s.io + resources: + - endpointslices + verbs: + - get + - list + - watch --- # Source: olmv1/templates/openshift/rolebinding-openshift-config-manager-rolebinding.yml apiVersion: rbac.authorization.k8s.io/v1 @@ -1083,3 +1091,4 @@ spec: selector: matchLabels: app.kubernetes.io/name: catalogd + serviceDiscoveryRole: EndpointSlice diff --git a/openshift/operator-controller/manifests-experimental.yaml b/openshift/operator-controller/manifests-experimental.yaml index 5e5ce11d6..c5ee983db 100644 --- a/openshift/operator-controller/manifests-experimental.yaml +++ b/openshift/operator-controller/manifests-experimental.yaml @@ -1021,6 +1021,14 @@ rules: - get - list - watch + - apiGroups: + - discovery.k8s.io + resources: + - endpointslices + verbs: + - get + - list + - watch --- # Source: olmv1/templates/rbac/role-olmv1-system-operator-controller-manager-role.yml apiVersion: rbac.authorization.k8s.io/v1 @@ -1366,3 +1374,4 @@ spec: selector: matchLabels: app.kubernetes.io/name: operator-controller + serviceDiscoveryRole: EndpointSlice diff --git a/openshift/operator-controller/manifests.yaml b/openshift/operator-controller/manifests.yaml index 015d42889..ec6be7cc2 100644 --- a/openshift/operator-controller/manifests.yaml +++ b/openshift/operator-controller/manifests.yaml @@ -929,6 +929,14 @@ rules: - get - list - watch + - apiGroups: + - discovery.k8s.io + resources: + - endpointslices + verbs: + - get + - list + - watch --- # Source: olmv1/templates/rbac/role-olmv1-system-operator-controller-manager-role.yml apiVersion: rbac.authorization.k8s.io/v1 @@ -1268,3 +1276,4 @@ spec: selector: matchLabels: app.kubernetes.io/name: operator-controller + serviceDiscoveryRole: EndpointSlice diff --git a/openshift/tests-extension/vendor/github.com/operator-framework/operator-controller/api/v1/clusterextensionrevision_types.go b/openshift/tests-extension/vendor/github.com/operator-framework/operator-controller/api/v1/clusterextensionrevision_types.go index f3416bf25..389e4a5b3 100644 --- a/openshift/tests-extension/vendor/github.com/operator-framework/operator-controller/api/v1/clusterextensionrevision_types.go +++ b/openshift/tests-extension/vendor/github.com/operator-framework/operator-controller/api/v1/clusterextensionrevision_types.go @@ -105,6 +105,19 @@ type ClusterExtensionRevisionSpec struct { // +optional // ProgressDeadlineMinutes int32 `json:"progressDeadlineMinutes,omitempty"` + + // collisionProtection specifies the default collision protection strategy for all objects + // in this revision. Individual phases or objects can override this value. + // + // When set, this value is used as the default for any phase or object that does not + // explicitly specify its own collisionProtection. + // + // The resolution order is: object > phase > spec + // + // +required + // +kubebuilder:validation:Enum=Prevent;IfNoController;None + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="collisionProtection is immutable" + CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } // ClusterExtensionRevisionLifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -144,6 +157,19 @@ type ClusterExtensionRevisionPhase struct { // +required // +kubebuilder:validation:MaxItems=50 Objects []ClusterExtensionRevisionObject `json:"objects"` + + // collisionProtection specifies the default collision protection strategy for all objects + // in this phase. Individual objects can override this value. + // + // When set, this value is used as the default for any object in this phase that does not + // explicitly specify its own collisionProtection. + // + // When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + // explicitly specify its own collisionProtection. + // + // +optional + // +kubebuilder:validation:Enum=Prevent;IfNoController;None + CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } // ClusterExtensionRevisionObject represents a Kubernetes object to be applied as part @@ -174,7 +200,9 @@ type ClusterExtensionRevisionObject struct { // Use this setting with extreme caution as it may cause multiple controllers to fight over // the same resource, resulting in increased load on the API server and etcd. // - // +required + // When omitted, the value is inherited from the phase, then spec. + // + // +optional // +kubebuilder:validation:Enum=Prevent;IfNoController;None CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` }