From f2c37608789e634a26d137589dc85be726f34991 Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Tue, 28 Nov 2023 13:56:04 +0100 Subject: [PATCH 01/11] Allow empty backend w/o defaulting --- api/operator/v1alpha1/eventing_types.go | 3 +- .../operator.kyma-project.io_eventings.yaml | 9 --- docs/user/02-configuration.md | 77 ++++++++++--------- 3 files changed, 41 insertions(+), 48 deletions(-) diff --git a/api/operator/v1alpha1/eventing_types.go b/api/operator/v1alpha1/eventing_types.go index c7b21be0..34b4bc8f 100644 --- a/api/operator/v1alpha1/eventing_types.go +++ b/api/operator/v1alpha1/eventing_types.go @@ -98,9 +98,8 @@ type EventingStatus struct { // EventingSpec defines the desired state of Eventing. type EventingSpec struct { // Backend defines the active backend used by Eventing. - // +kubebuilder:default:={type:"NATS", config:{natsStreamStorageType:"File", natsStreamReplicas:3, natsStreamMaxSize:"700Mi", natsMaxMsgsPerTopic:1000000}} // +kubebuilder:validation:XValidation:rule=" (self.type != 'EventMesh') || ((self.type == 'EventMesh') && (self.config.eventMeshSecret != ''))", message="secret cannot be empty if EventMesh backend is used" - Backend Backend `json:"backend"` + Backend Backend `json:"backend,omitempty"` // Publisher defines the configurations for eventing-publisher-proxy. // +kubebuilder:default:={replicas:{min:2,max:2}, resources:{limits:{cpu:"500m",memory:"512Mi"}, requests:{cpu:"40m",memory:"256Mi"}}} diff --git a/config/crd/bases/operator.kyma-project.io_eventings.yaml b/config/crd/bases/operator.kyma-project.io_eventings.yaml index 67b7fc49..8c774444 100644 --- a/config/crd/bases/operator.kyma-project.io_eventings.yaml +++ b/config/crd/bases/operator.kyma-project.io_eventings.yaml @@ -75,13 +75,6 @@ spec: description: Annotations allows to add annotations to resources. type: object backend: - default: - config: - natsMaxMsgsPerTopic: 1000000 - natsStreamMaxSize: 700Mi - natsStreamReplicas: 3 - natsStreamStorageType: File - type: NATS description: Backend defines the active backend used by Eventing. properties: config: @@ -261,8 +254,6 @@ spec: type: object type: object type: object - required: - - backend type: object status: description: EventingStatus defines the observed state of Eventing diff --git a/docs/user/02-configuration.md b/docs/user/02-configuration.md index 65a72b6d..a90a6cd3 100644 --- a/docs/user/02-configuration.md +++ b/docs/user/02-configuration.md @@ -28,46 +28,49 @@ Use the following sample CRs as guidance. Each can be applied immediately when y **Spec:** -| Parameter | Type | Description | -|----------------------------------------------------------|-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| **annotations** | map\[string\]string | Annotations allows to add annotations to resources. | -| **backend** (required) | object | Backend defines the active backend used by Eventing. | -| **backend.​config** | object | Config defines configuration for eventing backend. | -| **backend.​config.​domain** | string | Domain defines the cluster public domain used to configure the EventMesh Subscriptions and their corresponding ApiRules. | -| **backend.​config.​eventMeshSecret** | string | EventMeshSecret defines the namespaced name of K8s Secret containing EventMesh credentials. The format of name is "namespace/name". | -| **backend.​config.​eventTypePrefix** | string | | -| **backend.​config.​natsMaxMsgsPerTopic** | integer | NATSMaxMsgsPerTopic limits how many messages in the NATS stream to retain per subject. | -| **backend.​config.​natsStreamMaxSize** | \{integer or string\} | NATSStreamMaxSize defines the maximum storage size for stream data. | -| **backend.​config.​natsStreamReplicas** | integer | NATSStreamReplicas defines the number of replicas for stream. | -| **backend.​config.​natsStreamStorageType** | string | NATSStreamStorageType defines the storage type for stream data. | -| **backend.​type** (required) | string | Type defines which backend to use. The value is either `EventMesh`, or `NATS`. | -| **labels** | map\[string\]string | Labels allows to add Labels to resources. | -| **logging** | object | Logging defines the log level for eventing-manager. | -| **logging.​logLevel** | string | LogLevel defines the log level. | -| **publisher** | object | Publisher defines the configurations for eventing-publisher-proxy. | -| **publisher.​replicas** | object | Replicas defines the scaling min/max for eventing-publisher-proxy. | -| **publisher.​replicas.​max** | integer | Max defines maximum number of replicas. | -| **publisher.​replicas.​min** | integer | Min defines minimum number of replicas. | -| **publisher.​resources** | object | Resources defines resources for eventing-publisher-proxy. | -| **publisher.​resources.​claims** | \[\]object | Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container. This is an alpha field and requires enabling the DynamicResourceAllocation feature gate. This field is immutable. It can only be set for containers. | -| **publisher.​resources.​claims.​name** (required) | string | Name must match the name of one entry in pod.spec.resourceClaims of the Pod where this field is used. It makes that resource available inside a container. | -| **publisher.​resources.​limits** | map\[string\]\{integer or string\} | Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | +| Parameter | Type | Description | +| ---- | ----------- | ---- | +| **annotations** | map\[string\]string | Annotations allows to add annotations to resources. | +| **backend** | object | Backend defines the active backend used by Eventing. | +| **backend.​config** | object | Config defines configuration for eventing backend. | +| **backend.​config.​domain** | string | Domain defines the cluster public domain used to configure the EventMesh Subscriptions and their corresponding ApiRules. | +| **backend.​config.​eventMeshSecret** | string | EventMeshSecret defines the namespaced name of K8s Secret containing EventMesh credentials. The format of name is "namespace/name". | +| **backend.​config.​eventTypePrefix** | string | | +| **backend.​config.​natsMaxMsgsPerTopic** | integer | NATSMaxMsgsPerTopic limits how many messages in the NATS stream to retain per subject. | +| **backend.​config.​natsStreamMaxSize** | \{integer or string\} | NATSStreamMaxSize defines the maximum storage size for stream data. | +| **backend.​config.​natsStreamReplicas** | integer | NATSStreamReplicas defines the number of replicas for stream. | +| **backend.​config.​natsStreamStorageType** | string | NATSStreamStorageType defines the storage type for stream data. | +| **backend.​type** (required) | string | Type defines which backend to use. The value is either `EventMesh`, or `NATS`. | +| **labels** | map\[string\]string | Labels allows to add Labels to resources. | +| **logging** | object | Logging defines the log level for eventing-manager. | +| **logging.​logLevel** | string | LogLevel defines the log level. | +| **publisher** | object | Publisher defines the configurations for eventing-publisher-proxy. | +| **publisher.​replicas** | object | Replicas defines the scaling min/max for eventing-publisher-proxy. | +| **publisher.​replicas.​max** | integer | Max defines maximum number of replicas. | +| **publisher.​replicas.​min** | integer | Min defines minimum number of replicas. | +| **publisher.​resources** | object | Resources defines resources for eventing-publisher-proxy. | +| **publisher.​resources.​claims** | \[\]object | Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container. + This is an alpha field and requires enabling the DynamicResourceAllocation feature gate. + This field is immutable. It can only be set for containers. | +| **publisher.​resources.​claims.​name** (required) | string | Name must match the name of one entry in pod.spec.resourceClaims of the Pod where this field is used. It makes that resource available inside a container. | +| **publisher.​resources.​limits** | map\[string\]\{integer or string\} | Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | | **publisher.​resources.​requests** | map\[string\]\{integer or string\} | Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | **Status:** -| Parameter | Type | Description | -|------------------------------------------------------|------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| **activeBackend** (required) | string | | -| **conditions** | \[\]object | Condition contains details for one aspect of the current state of this API Resource. --- This struct is intended for direct use as an array at the field path .status.conditions. For example, type FooStatus struct{ // Represents the observations of a foo's current state. // Known .status.conditions.type are: "Available", "Progressing", and "Degraded" // +patchMergeKey=type // +patchStrategy=merge // +listType=map // +listMapKey=type Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` | -| // other fields } | | | -| **conditions.​lastTransitionTime** (required) | string | lastTransitionTime is the last time the condition transitioned from one status to another. This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. | -| **conditions.​message** (required) | string | message is a human readable message indicating details about the transition. This may be an empty string. | -| **conditions.​observedGeneration** | integer | observedGeneration represents the .metadata.generation that the condition was set based upon. For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date with respect to the current state of the instance. | -| **conditions.​reason** (required) | string | reason contains a programmatic identifier indicating the reason for the condition's last transition. Producers of specific condition types may define expected values and meanings for this field, and whether the values are considered a guaranteed API. The value should be a CamelCase string. This field may not be empty. | -| **conditions.​status** (required) | string | status of the condition, one of `True`, `False`, `Unknown`. | -| **conditions.​type** (required) | string | type of condition in CamelCase or in foo.example.com/CamelCase. --- Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be useful (see .node.status.conditions), the ability to deconflict is important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) | -| **specHash** (required) | integer | | -| **state** (required) | string | | +| Parameter | Type | Description | +| ---- | ----------- | ---- | +| **activeBackend** (required) | string | | +| **conditions** | \[\]object | Condition contains details for one aspect of the current state of this API Resource. --- This struct is intended for direct use as an array at the field path .status.conditions. For example, + type FooStatus struct{ // Represents the observations of a foo's current state. // Known .status.conditions.type are: "Available", "Progressing", and "Degraded" // +patchMergeKey=type // +patchStrategy=merge // +listType=map // +listMapKey=type Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` + // other fields } | +| **conditions.​lastTransitionTime** (required) | string | lastTransitionTime is the last time the condition transitioned from one status to another. This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. | +| **conditions.​message** (required) | string | message is a human readable message indicating details about the transition. This may be an empty string. | +| **conditions.​observedGeneration** | integer | observedGeneration represents the .metadata.generation that the condition was set based upon. For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date with respect to the current state of the instance. | +| **conditions.​reason** (required) | string | reason contains a programmatic identifier indicating the reason for the condition's last transition. Producers of specific condition types may define expected values and meanings for this field, and whether the values are considered a guaranteed API. The value should be a CamelCase string. This field may not be empty. | +| **conditions.​status** (required) | string | status of the condition, one of True, False, Unknown. | +| **conditions.​type** (required) | string | type of condition in CamelCase or in foo.example.com/CamelCase. --- Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be useful (see .node.status.conditions), the ability to deconflict is important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) | +| **specHash** (required) | integer | | +| **state** (required) | string | | \ No newline at end of file From 1da00f922e54b913dc836de762aa4f73f6de33e4 Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Thu, 30 Nov 2023 09:08:09 +0100 Subject: [PATCH 02/11] Set warning state for empty backend Make state warning and BackendAvailable condition false for empty backend config --- api/operator/v1alpha1/eventing_types.go | 10 ++++++---- api/operator/v1alpha1/eventing_types_test.go | 8 ++++---- api/operator/v1alpha1/status.go | 11 +++++----- .../v1alpha1/zz_generated.deepcopy.go | 6 +++++- hack/e2e/common/fixtures/fixtures.go | 4 ++-- .../operator/eventing/controller.go | 9 +++++++++ .../controller/integration_test.go | 20 +++++++++++++++++-- .../controller/operator/eventing/status.go | 13 +++++++++++- pkg/env/nats_config_test.go | 2 +- test/matchers/matchers.go | 13 ++++++++++-- test/utils/options.go | 13 +++++++++--- test/utils/utils.go | 2 +- 12 files changed, 84 insertions(+), 27 deletions(-) diff --git a/api/operator/v1alpha1/eventing_types.go b/api/operator/v1alpha1/eventing_types.go index 34b4bc8f..5b3277a4 100644 --- a/api/operator/v1alpha1/eventing_types.go +++ b/api/operator/v1alpha1/eventing_types.go @@ -33,7 +33,7 @@ const ( StateProcessing string = "Processing" StateWarning string = "Warning" - ConditionNATSAvailable ConditionType = "NATSAvailable" + ConditionBackendAvailable ConditionType = "BackendAvailable" ConditionPublisherProxyReady ConditionType = "PublisherProxyReady" ConditionWebhookReady ConditionType = "WebhookReady" ConditionSubscriptionManagerReady ConditionType = "SubscriptionManagerReady" @@ -48,8 +48,9 @@ const ( ConditionReasonDeployed ConditionReason = "Deployed" ConditionReasonDeployedFailed ConditionReason = "DeployFailed" ConditionReasonDeploymentStatusSyncFailed ConditionReason = "DeploymentStatusSyncFailed" - ConditionReasonNATSAvailable ConditionReason = "Available" - ConditionReasonNATSNotAvailable ConditionReason = "NotAvailable" + ConditionReasonNATSAvailable ConditionReason = "NATSAvailable" + ConditionReasonNATSNotAvailable ConditionReason = "NATSUnavailable" + ConditionReasonBackendNotSpecified ConditionReason = "BackendNotSpecified" ConditionReasonForbidden ConditionReason = "Forbidden" ConditionReasonWebhookFailed ConditionReason = "WebhookFailed" ConditionReasonWebhookReady ConditionReason = "Ready" @@ -63,6 +64,7 @@ const ( ConditionPublisherProxyProcessingMessage = "Eventing publisher proxy deployment is in progress" ConditionSubscriptionManagerReadyMessage = "Subscription manager is ready" ConditionSubscriptionManagerStoppedMessage = "Subscription manager is stopped" + ConditionBackendNotSpecifiedMessage = "Backend config is not provided. Please specify a backend." // subscription manager reasons. ConditionReasonEventMeshSubManagerReady ConditionReason = "EventMeshSubscriptionManagerReady" @@ -99,7 +101,7 @@ type EventingStatus struct { type EventingSpec struct { // Backend defines the active backend used by Eventing. // +kubebuilder:validation:XValidation:rule=" (self.type != 'EventMesh') || ((self.type == 'EventMesh') && (self.config.eventMeshSecret != ''))", message="secret cannot be empty if EventMesh backend is used" - Backend Backend `json:"backend,omitempty"` + Backend *Backend `json:"backend,omitempty"` // Publisher defines the configurations for eventing-publisher-proxy. // +kubebuilder:default:={replicas:{min:2,max:2}, resources:{limits:{cpu:"500m",memory:"512Mi"}, requests:{cpu:"40m",memory:"256Mi"}}} diff --git a/api/operator/v1alpha1/eventing_types_test.go b/api/operator/v1alpha1/eventing_types_test.go index a53e2e0f..8cbc3b79 100644 --- a/api/operator/v1alpha1/eventing_types_test.go +++ b/api/operator/v1alpha1/eventing_types_test.go @@ -19,7 +19,7 @@ func TestSyncStatusActiveBackend(t *testing.T) { name: "it should set ActiveBackend to NATS", givenEventing: &Eventing{ Spec: EventingSpec{ - Backend: Backend{Type: NatsBackendType}, + Backend: &Backend{Type: NatsBackendType}, }, Status: EventingStatus{}, }, @@ -29,7 +29,7 @@ func TestSyncStatusActiveBackend(t *testing.T) { name: "it should set ActiveBackend to EventMesh", givenEventing: &Eventing{ Spec: EventingSpec{ - Backend: Backend{Type: EventMeshBackendType}, + Backend: &Backend{Type: EventMeshBackendType}, }, Status: EventingStatus{}, }, @@ -65,7 +65,7 @@ func TestIsSpecBackendTypeChanged(t *testing.T) { name: "it should return false if backend is not changed", givenEventing: &Eventing{ Spec: EventingSpec{ - Backend: Backend{Type: NatsBackendType}, + Backend: &Backend{Type: NatsBackendType}, }, Status: EventingStatus{ ActiveBackend: NatsBackendType, @@ -77,7 +77,7 @@ func TestIsSpecBackendTypeChanged(t *testing.T) { name: "it should return true if backend is changed", givenEventing: &Eventing{ Spec: EventingSpec{ - Backend: Backend{Type: NatsBackendType}, + Backend: &Backend{Type: NatsBackendType}, }, Status: EventingStatus{ ActiveBackend: EventMeshBackendType, diff --git a/api/operator/v1alpha1/status.go b/api/operator/v1alpha1/status.go index f79fda59..9d707a58 100644 --- a/api/operator/v1alpha1/status.go +++ b/api/operator/v1alpha1/status.go @@ -8,11 +8,10 @@ import ( kmetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func (es *EventingStatus) UpdateConditionNATSAvailable(status kmetav1.ConditionStatus, reason ConditionReason, - message string, -) { +func (es *EventingStatus) UpdateConditionBackendAvailable(status kmetav1.ConditionStatus, reason ConditionReason, + message string) { condition := kmetav1.Condition{ - Type: string(ConditionNATSAvailable), + Type: string(ConditionBackendAvailable), Status: status, LastTransitionTime: kmetav1.Now(), Reason: string(reason), @@ -80,7 +79,7 @@ func (es *EventingStatus) SetSubscriptionManagerReadyConditionToTrue() { func (es *EventingStatus) SetStateReady() { es.State = StateReady - es.UpdateConditionNATSAvailable(kmetav1.ConditionTrue, ConditionReasonNATSAvailable, ConditionNATSAvailableMessage) + es.UpdateConditionBackendAvailable(kmetav1.ConditionTrue, ConditionReasonNATSAvailable, ConditionNATSAvailableMessage) es.UpdateConditionPublisherProxyReady(kmetav1.ConditionTrue, ConditionReasonDeployed, ConditionPublisherProxyReadyMessage) } @@ -89,7 +88,7 @@ func (ns *EventingStatus) SetStateWarning() { } func (es *EventingStatus) SetNATSAvailableConditionToTrue() { - es.UpdateConditionNATSAvailable(kmetav1.ConditionTrue, ConditionReasonNATSAvailable, ConditionNATSAvailableMessage) + es.UpdateConditionBackendAvailable(kmetav1.ConditionTrue, ConditionReasonNATSAvailable, ConditionNATSAvailableMessage) } func (es *EventingStatus) SetSubscriptionManagerReadyConditionToFalse(reason ConditionReason, message string) { diff --git a/api/operator/v1alpha1/zz_generated.deepcopy.go b/api/operator/v1alpha1/zz_generated.deepcopy.go index 28204a61..1a8cdd8f 100644 --- a/api/operator/v1alpha1/zz_generated.deepcopy.go +++ b/api/operator/v1alpha1/zz_generated.deepcopy.go @@ -120,7 +120,11 @@ func (in *EventingList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *EventingSpec) DeepCopyInto(out *EventingSpec) { *out = *in - in.Backend.DeepCopyInto(&out.Backend) + if in.Backend != nil { + in, out := &in.Backend, &out.Backend + *out = new(Backend) + (*in).DeepCopyInto(*out) + } in.Publisher.DeepCopyInto(&out.Publisher) out.Logging = in.Logging if in.Annotations != nil { diff --git a/hack/e2e/common/fixtures/fixtures.go b/hack/e2e/common/fixtures/fixtures.go index 01de7c2b..4db73194 100644 --- a/hack/e2e/common/fixtures/fixtures.go +++ b/hack/e2e/common/fixtures/fixtures.go @@ -63,7 +63,7 @@ func EventingNATSCR() *operatorv1alpha1.Eventing { Namespace: NamespaceName, }, Spec: operatorv1alpha1.EventingSpec{ - Backend: operatorv1alpha1.Backend{ + Backend: &operatorv1alpha1.Backend{ Type: "NATS", Config: operatorv1alpha1.BackendConfig{ NATSStreamStorageType: "File", @@ -88,7 +88,7 @@ func EventingEventMeshCR() *operatorv1alpha1.Eventing { Namespace: NamespaceName, }, Spec: operatorv1alpha1.EventingSpec{ - Backend: operatorv1alpha1.Backend{ + Backend: &operatorv1alpha1.Backend{ Type: "EventMesh", Config: operatorv1alpha1.BackendConfig{ EventMeshSecret: fmt.Sprintf("%s/%s", EventMeshSecretNamespace, EventMeshSecretName), diff --git a/internal/controller/operator/eventing/controller.go b/internal/controller/operator/eventing/controller.go index e9efdc71..ce7de177 100644 --- a/internal/controller/operator/eventing/controller.go +++ b/internal/controller/operator/eventing/controller.go @@ -379,6 +379,12 @@ func (r *Reconciler) handleEventingDeletion(ctx context.Context, eventing *opera return kctrl.Result{}, nil } + if eventing.Spec.Backend == nil { + // backend config can only be empty during creation and nothing is created if it is missing. + // Hence, eventing can be safely removed. + return r.removeFinalizer(ctx, eventing) + } + // check if subscription resources exist exists, err := r.eventingManager.SubscriptionExists(ctx) if err != nil { @@ -441,6 +447,9 @@ func (r *Reconciler) handleEventingReconcile(ctx context.Context, // set state processing if not set yet r.InitStateProcessing(eventing) + if eventing.Spec.Backend == nil { + return ctrl.Result{Requeue: true}, r.syncStatusForEmptyBackend(ctx, eventing, log) + } // sync webhooks CABundle. if err := r.reconcileWebhooksWithCABundle(ctx); err != nil { diff --git a/internal/controller/operator/eventing/integrationtests/controller/integration_test.go b/internal/controller/operator/eventing/integrationtests/controller/integration_test.go index 422b9eb5..9946d0a5 100644 --- a/internal/controller/operator/eventing/integrationtests/controller/integration_test.go +++ b/internal/controller/operator/eventing/integrationtests/controller/integration_test.go @@ -152,6 +152,22 @@ func Test_CreateEventingCR_NATS(t *testing.T) { matchers.HaveFinalizer(), ), }, + { + name: "Eventing CR should have warning state when backend config is empty", + givenEventing: utils.NewEventingCR( + utils.WithEventingEmptyBackend(), + utils.WithEventingPublisherData(2, 2, "199m", "99Mi", "399m", "199Mi"), + ), + givenNATS: natstestutils.NewNATSCR( + natstestutils.WithNATSCRDefaults(), + ), + wantMatches: gomega.And( + matchers.HaveStatusWarning(), + matchers.HaveBackendNotAvailableConditionWith(eventingv1alpha1.ConditionBackendNotSpecifiedMessage, + eventingv1alpha1.ConditionReasonBackendNotSpecified), + matchers.HaveFinalizer(), + ), + }, } for _, tc := range testCases { @@ -202,13 +218,13 @@ func Test_CreateEventingCR_NATS(t *testing.T) { // then // check Eventing CR status. testEnvironment.GetEventingAssert(g, tc.givenEventing).Should(tc.wantMatches) - if tc.givenDeploymentReady { + if tc.givenDeploymentReady && tc.givenEventing.Spec.Backend != nil { // check if EPP deployment, HPA resources created and values are reflected including owner reference. ensureEPPDeploymentAndHPAResources(t, tc.givenEventing, testEnvironment) // TODO: ensure NATS Backend config is reflected. Done as subscription controller is implemented. } - if tc.wantEnsureK8sObjects { + if tc.wantEnsureK8sObjects && tc.givenEventing.Spec.Backend != nil { // check if EPP resources exists. ensureK8sResources(t, tc.givenEventing, testEnvironment) // check if webhook configurations are updated with correct CABundle. diff --git a/internal/controller/operator/eventing/status.go b/internal/controller/operator/eventing/status.go index 63562514..f5674794 100644 --- a/internal/controller/operator/eventing/status.go +++ b/internal/controller/operator/eventing/status.go @@ -30,12 +30,23 @@ func (r *Reconciler) syncStatusWithNATSErr(ctx context.Context, ) error { // Set error state in status eventing.Status.SetStateError() - eventing.Status.UpdateConditionNATSAvailable(kmetav1.ConditionFalse, operatorv1alpha1.ConditionReasonNATSNotAvailable, + eventing.Status.UpdateConditionBackendAvailable(kmetav1.ConditionFalse, operatorv1alpha1.ConditionReasonNATSNotAvailable, err.Error()) return errors.Join(err, r.syncEventingStatus(ctx, eventing, log)) } +func (r *Reconciler) syncStatusForEmptyBackend(ctx context.Context, + eventing *operatorv1alpha1.Eventing, log *zap.SugaredLogger) error { + // Set error state in status + eventing.Status.SetStateWarning() + eventing.Status.UpdateConditionBackendAvailable( + kmetav1.ConditionFalse, + operatorv1alpha1.ConditionReasonBackendNotSpecified, + operatorv1alpha1.ConditionBackendNotSpecifiedMessage) + return r.syncEventingStatus(ctx, eventing, log) +} + // syncStatusWithPublisherProxyErr updates Publisher Proxy condition and sets an error state. // Returns the relevant error. func (r *Reconciler) syncStatusWithPublisherProxyErr(ctx context.Context, diff --git a/pkg/env/nats_config_test.go b/pkg/env/nats_config_test.go index 1a6df157..9072e955 100644 --- a/pkg/env/nats_config_test.go +++ b/pkg/env/nats_config_test.go @@ -43,7 +43,7 @@ func Test_GetNewNATSConfig(t *testing.T) { UID: "1234-5678-1234-5678", }, Spec: v1alpha1.EventingSpec{ - Backend: v1alpha1.Backend{ + Backend: &v1alpha1.Backend{ Type: v1alpha1.NatsBackendType, Config: v1alpha1.BackendConfig{ EventTypePrefix: "sap.kyma.custom", diff --git a/test/matchers/matchers.go b/test/matchers/matchers.go index f57c6aaa..948865ae 100644 --- a/test/matchers/matchers.go +++ b/test/matchers/matchers.go @@ -90,16 +90,25 @@ func HavePublisherProxyConditionForbiddenWithMsg(msg string) gomegatypes.GomegaM func HaveNATSAvailableCondition() gomegatypes.GomegaMatcher { return HaveCondition(kmetav1.Condition{ - Type: string(v1alpha1.ConditionNATSAvailable), + Type: string(v1alpha1.ConditionBackendAvailable), Status: kmetav1.ConditionTrue, Reason: string(v1alpha1.ConditionReasonNATSAvailable), Message: v1alpha1.ConditionNATSAvailableMessage, }) } +func HaveBackendNotAvailableConditionWith(message string, reason v1alpha1.ConditionReason) gomegatypes.GomegaMatcher { + return HaveCondition(kmetav1.Condition{ + Type: string(v1alpha1.ConditionBackendAvailable), + Status: kmetav1.ConditionFalse, + Reason: string(reason), + Message: message, + }) +} + func HaveNATSNotAvailableConditionWith(message string) gomegatypes.GomegaMatcher { return HaveCondition(kmetav1.Condition{ - Type: string(v1alpha1.ConditionNATSAvailable), + Type: string(v1alpha1.ConditionBackendAvailable), Status: kmetav1.ConditionFalse, Reason: string(v1alpha1.ConditionReasonNATSNotAvailable), Message: message, diff --git a/test/utils/options.go b/test/utils/options.go index 9ba703fe..be8ae7fb 100644 --- a/test/utils/options.go +++ b/test/utils/options.go @@ -12,7 +12,7 @@ import ( func WithEventingCRMinimal() EventingOption { return func(e *v1alpha1.Eventing) error { e.Spec = v1alpha1.EventingSpec{ - Backend: v1alpha1.Backend{ + Backend: &v1alpha1.Backend{ Type: v1alpha1.NatsBackendType, }, } @@ -20,6 +20,13 @@ func WithEventingCRMinimal() EventingOption { } } +func WithEventingEmptyBackend() EventingOption { + return func(e *v1alpha1.Eventing) error { + e.Spec = v1alpha1.EventingSpec{} + return nil + } +} + func WithEventingCRName(name string) EventingOption { return func(e *v1alpha1.Eventing) error { e.Name = name @@ -78,7 +85,7 @@ func WithEventingPublisherData(minReplicas, maxReplicas int, requestCPU, request func WithEventingInvalidBackend() EventingOption { return func(e *v1alpha1.Eventing) error { e.Spec = v1alpha1.EventingSpec{ - Backend: v1alpha1.Backend{ + Backend: &v1alpha1.Backend{ Type: "invalid", }, } @@ -109,7 +116,7 @@ func WithEventingLogLevel(logLevel string) EventingOption { func WithEventMeshBackend(eventMeshSecretName string) EventingOption { return func(e *v1alpha1.Eventing) error { - e.Spec.Backend = v1alpha1.Backend{ + e.Spec.Backend = &v1alpha1.Backend{ Type: v1alpha1.EventMeshBackendType, Config: v1alpha1.BackendConfig{ EventMeshSecret: e.Namespace + "/" + eventMeshSecretName, diff --git a/test/utils/utils.go b/test/utils/utils.go index 53e0411a..2e4a804f 100644 --- a/test/utils/utils.go +++ b/test/utils/utils.go @@ -120,7 +120,7 @@ func NewEventingCR(opts ...EventingOption) *v1alpha1.Eventing { UID: "1234-5678-1234-5678", }, Spec: v1alpha1.EventingSpec{ - Backend: v1alpha1.Backend{ + Backend: &v1alpha1.Backend{ Type: v1alpha1.NatsBackendType, }, }, From 17ad7803aa5eb38fc408ce67ee17dd32b5407723 Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Mon, 4 Dec 2023 10:05:48 +0100 Subject: [PATCH 03/11] Set Warning if NATS module is missing --- .../operator/eventing/controller.go | 13 ++++++++++--- .../controller/integration_test.go | 4 ++-- .../controller/operator/eventing/status.go | 19 ++++++++++++------- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/internal/controller/operator/eventing/controller.go b/internal/controller/operator/eventing/controller.go index ce7de177..8ac0384b 100644 --- a/internal/controller/operator/eventing/controller.go +++ b/internal/controller/operator/eventing/controller.go @@ -448,7 +448,10 @@ func (r *Reconciler) handleEventingReconcile(ctx context.Context, // set state processing if not set yet r.InitStateProcessing(eventing) if eventing.Spec.Backend == nil { - return ctrl.Result{Requeue: true}, r.syncStatusForEmptyBackend(ctx, eventing, log) + return kctrl.Result{Requeue: true}, r.syncStatusForEmptyBackend(ctx, + operatorv1alpha1.ConditionReasonBackendNotSpecified, + operatorv1alpha1.ConditionBackendNotSpecifiedMessage, + eventing, log) } // sync webhooks CABundle. @@ -523,9 +526,13 @@ func (r *Reconciler) reconcileNATSBackend(ctx context.Context, eventing *operato // into CrashLoopBackOff. log.Infof("NATS module not enabled, deleting publisher proxy resources") delErr := r.eventingManager.DeletePublisherProxyResources(ctx, eventing) + if delErr != nil { + return kctrl.Result{}, delErr + } // update the Eventing CR status. notFoundErr := fmt.Errorf("NATS module has to be installed: %v", err) - return kctrl.Result{}, errors.Join(r.syncStatusWithNATSErr(ctx, eventing, notFoundErr, log), delErr) + return kctrl.Result{}, r.syncStatusWithNATSState(ctx, operatorv1alpha1.StateWarning, eventing, + notFoundErr, log) } return kctrl.Result{}, err } @@ -537,7 +544,7 @@ func (r *Reconciler) reconcileNATSBackend(ctx context.Context, eventing *operato // check nats CR if it exists and is in natsAvailable state err = r.checkNATSAvailability(ctx, eventing) if err != nil { - return kctrl.Result{}, r.syncStatusWithNATSErr(ctx, eventing, err, log) + return kctrl.Result{}, r.syncStatusWithNATSState(ctx, operatorv1alpha1.StateWarning, eventing, err, log) } // set NATSAvailable condition to true and update status diff --git a/internal/controller/operator/eventing/integrationtests/controller/integration_test.go b/internal/controller/operator/eventing/integrationtests/controller/integration_test.go index 9946d0a5..d577b29b 100644 --- a/internal/controller/operator/eventing/integrationtests/controller/integration_test.go +++ b/internal/controller/operator/eventing/integrationtests/controller/integration_test.go @@ -87,7 +87,7 @@ func Test_CreateEventingCR_NATS(t *testing.T) { ), givenNATSReady: false, wantMatches: gomega.And( - matchers.HaveStatusError(), + matchers.HaveStatusWarning(), matchers.HaveNATSNotAvailableCondition(), matchers.HaveFinalizer(), ), @@ -146,7 +146,7 @@ func Test_CreateEventingCR_NATS(t *testing.T) { ), givenNATSCRDMissing: true, wantMatches: gomega.And( - matchers.HaveStatusError(), + matchers.HaveStatusWarning(), matchers.HaveNATSNotAvailableConditionWith("NATS module has to be installed: "+ "customresourcedefinitions.apiextensions.k8s.io \"nats.operator.kyma-project.io\" not found"), matchers.HaveFinalizer(), diff --git a/internal/controller/operator/eventing/status.go b/internal/controller/operator/eventing/status.go index f5674794..63985e04 100644 --- a/internal/controller/operator/eventing/status.go +++ b/internal/controller/operator/eventing/status.go @@ -26,24 +26,29 @@ func (es *Reconciler) InitStateProcessing(eventing *operatorv1alpha1.Eventing) { // syncStatusWithNATSErr syncs Eventing status and sets an error state. // Returns the relevant error. func (r *Reconciler) syncStatusWithNATSErr(ctx context.Context, - eventing *operatorv1alpha1.Eventing, err error, log *zap.SugaredLogger, -) error { + eventing *operatorv1alpha1.Eventing, err error, log *zap.SugaredLogger) error { + return r.syncStatusWithNATSState(ctx, operatorv1alpha1.StateError, eventing, err, log) +} + +func (r *Reconciler) syncStatusWithNATSState(ctx context.Context, state string, + eventing *operatorv1alpha1.Eventing, err error, log *zap.SugaredLogger) error { // Set error state in status - eventing.Status.SetStateError() - eventing.Status.UpdateConditionBackendAvailable(kmetav1.ConditionFalse, operatorv1alpha1.ConditionReasonNATSNotAvailable, + eventing.Status.State = state + eventing.Status.UpdateConditionBackendAvailable(kmetav1.ConditionFalse, + operatorv1alpha1.ConditionReasonNATSNotAvailable, err.Error()) return errors.Join(err, r.syncEventingStatus(ctx, eventing, log)) } -func (r *Reconciler) syncStatusForEmptyBackend(ctx context.Context, - eventing *operatorv1alpha1.Eventing, log *zap.SugaredLogger) error { +func (r *Reconciler) syncStatusForEmptyBackend(ctx context.Context, reason operatorv1alpha1.ConditionReason, + message string, eventing *operatorv1alpha1.Eventing, log *zap.SugaredLogger) error { // Set error state in status eventing.Status.SetStateWarning() eventing.Status.UpdateConditionBackendAvailable( kmetav1.ConditionFalse, operatorv1alpha1.ConditionReasonBackendNotSpecified, - operatorv1alpha1.ConditionBackendNotSpecifiedMessage) + message) return r.syncEventingStatus(ctx, eventing, log) } From 32cce57a1118cb40ef23558ba43b8c9fc8dbb235 Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Mon, 4 Dec 2023 15:24:31 +0100 Subject: [PATCH 04/11] Set Warning state if EventMes secret missing * Set Warning state if EventMes secret missing * Fix failing tests --- .../operator/eventing/controller.go | 4 +- .../controller/operator/eventing/eventmesh.go | 9 ++- .../operator/eventing/eventmesh_test.go | 12 ++-- .../controller/integration_test.go | 57 +++++++++++++++---- .../nats_disabled/integration_test.go | 2 +- 5 files changed, 64 insertions(+), 20 deletions(-) diff --git a/internal/controller/operator/eventing/controller.go b/internal/controller/operator/eventing/controller.go index 8ac0384b..5be13478 100644 --- a/internal/controller/operator/eventing/controller.go +++ b/internal/controller/operator/eventing/controller.go @@ -544,7 +544,7 @@ func (r *Reconciler) reconcileNATSBackend(ctx context.Context, eventing *operato // check nats CR if it exists and is in natsAvailable state err = r.checkNATSAvailability(ctx, eventing) if err != nil { - return kctrl.Result{}, r.syncStatusWithNATSState(ctx, operatorv1alpha1.StateWarning, eventing, err, log) + return kctrl.Result{}, r.syncStatusWithNATSErr(ctx, eventing, err, log) } // set NATSAvailable condition to true and update status @@ -613,7 +613,7 @@ func (r *Reconciler) reconcileEventMeshBackend(ctx context.Context, eventing *op } // Start the EventMesh subscription controller - err = r.reconcileEventMeshSubManager(ctx, eventing) + err = r.reconcileEventMeshSubManager(ctx, eventing, log) if err != nil { return kctrl.Result{}, r.syncStatusWithSubscriptionManagerErr(ctx, eventing, err, log) } diff --git a/internal/controller/operator/eventing/eventmesh.go b/internal/controller/operator/eventing/eventmesh.go index cfc37cbc..e36c9466 100644 --- a/internal/controller/operator/eventing/eventmesh.go +++ b/internal/controller/operator/eventing/eventmesh.go @@ -36,7 +36,10 @@ type oauth2Credentials struct { certsURL []byte } -func (r *Reconciler) reconcileEventMeshSubManager(ctx context.Context, eventing *v1alpha1.Eventing) error { +const EventMeshSecretMissingMessage = "the specified EventMesh secret is missing. Please provide the secret" + +func (r *Reconciler) reconcileEventMeshSubManager(ctx context.Context, eventing *v1alpha1.Eventing, + log *zap.SugaredLogger) error { // gets oauth2ClientID and secret and stops the EventMesh subscription manager if changed err := r.syncOauth2ClientIDAndSecret(ctx, eventing) if err != nil { @@ -45,6 +48,10 @@ func (r *Reconciler) reconcileEventMeshSubManager(ctx context.Context, eventing // retrieve secret to authenticate with EventMesh eventMeshSecret, err := r.kubeClient.GetSecret(ctx, eventing.Spec.Backend.Config.EventMeshSecret) if err != nil { + if kerrors.IsNotFound(err) { + return r.syncStatusWithNATSState(ctx, v1alpha1.StateWarning, eventing, + errors.New(EventMeshSecretMissingMessage), log) + } return errors.Errorf("failed to get EventMesh secret: %v", err) } // CreateOrUpdate deployment for publisher proxy secret diff --git a/internal/controller/operator/eventing/eventmesh_test.go b/internal/controller/operator/eventing/eventmesh_test.go index efa1e4b7..5fa81446 100644 --- a/internal/controller/operator/eventing/eventmesh_test.go +++ b/internal/controller/operator/eventing/eventmesh_test.go @@ -274,6 +274,8 @@ func Test_reconcileEventMeshSubManager(t *testing.T) { testEnv := NewMockedUnitTestEnvironment(t, givenEventing, givenOauthSecret) testEnv.Reconciler.backendConfig = *givenBackendConfig + logger := testEnv.Reconciler.logger.WithContext().Named(ControllerName) + // get mocks from test-case. givenEventMeshSubManagerMock := tc.givenEventMeshSubManagerMock() givenManagerFactoryMock := tc.givenManagerFactoryMock(givenEventMeshSubManagerMock) @@ -296,15 +298,15 @@ func Test_reconcileEventMeshSubManager(t *testing.T) { givenEventing.Status.BackendConfigHash = tc.givenHashBefore // when - err := testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing) + err := testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, logger) if err != nil && tc.givenShouldRetry { // This is to test the scenario where initialization of eventMeshSubManager was successful but // starting the eventMeshSubManager failed. So on next try it should again try to start the eventMeshSubManager. - err = testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing) + err = testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, logger) } if tc.givenUpdateTest { // Run reconcile again with newBackendConfig: - err = testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing) + err = testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, logger) require.NoError(t, err) } @@ -433,6 +435,8 @@ func Test_reconcileEventMeshSubManager_ReadClusterDomain(t *testing.T) { testEnv := NewMockedUnitTestEnvironment(t, tc.givenEventing, givenOauthSecret) testEnv.Reconciler.backendConfig = *givenBackendConfig + logger := testEnv.Reconciler.logger.WithContext().Named(ControllerName) + givenEventMeshSubManagerMock := tc.givenEventMeshSubManagerMock() givenManagerFactoryMock := tc.givenManagerFactoryMock(givenEventMeshSubManagerMock) givenEventingManagerMock := tc.givenEventingManagerMock() @@ -448,7 +452,7 @@ func Test_reconcileEventMeshSubManager_ReadClusterDomain(t *testing.T) { testEnv.Reconciler.eventMeshSubManager = nil // when - err := testEnv.Reconciler.reconcileEventMeshSubManager(ctx, tc.givenEventing) + err := testEnv.Reconciler.reconcileEventMeshSubManager(ctx, tc.givenEventing, logger) // then require.NoError(t, err) diff --git a/internal/controller/operator/eventing/integrationtests/controller/integration_test.go b/internal/controller/operator/eventing/integrationtests/controller/integration_test.go index d577b29b..9a5937c4 100644 --- a/internal/controller/operator/eventing/integrationtests/controller/integration_test.go +++ b/internal/controller/operator/eventing/integrationtests/controller/integration_test.go @@ -16,6 +16,7 @@ import ( kapiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" kmetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kclient "sigs.k8s.io/controller-runtime/pkg/client" eventingv1alpha2 "github.com/kyma-project/eventing-manager/api/eventing/v1alpha2" operatorv1alpha1 "github.com/kyma-project/eventing-manager/api/operator/v1alpha1" @@ -87,7 +88,7 @@ func Test_CreateEventingCR_NATS(t *testing.T) { ), givenNATSReady: false, wantMatches: gomega.And( - matchers.HaveStatusWarning(), + matchers.HaveStatusError(), matchers.HaveNATSNotAvailableCondition(), matchers.HaveFinalizer(), ), @@ -163,8 +164,8 @@ func Test_CreateEventingCR_NATS(t *testing.T) { ), wantMatches: gomega.And( matchers.HaveStatusWarning(), - matchers.HaveBackendNotAvailableConditionWith(eventingv1alpha1.ConditionBackendNotSpecifiedMessage, - eventingv1alpha1.ConditionReasonBackendNotSpecified), + matchers.HaveBackendNotAvailableConditionWith(operatorv1alpha1.ConditionBackendNotSpecifiedMessage, + operatorv1alpha1.ConditionReasonBackendNotSpecified), matchers.HaveFinalizer(), ), }, @@ -590,12 +591,13 @@ func Test_WatcherEventingCRK8sObjects(t *testing.T) { func Test_CreateEventingCR_EventMesh(t *testing.T) { testCases := []struct { - name string - givenEventing *operatorv1alpha1.Eventing - givenDeploymentReady bool - shouldFailSubManager bool - wantMatches gomegatypes.GomegaMatcher - wantEnsureK8sObjects bool + name string + givenEventing *operatorv1alpha1.Eventing + givenDeploymentReady bool + shouldFailSubManager bool + shouldEventMeshSecretNotFound bool + wantMatches gomegatypes.GomegaMatcher + wantEnsureK8sObjects bool }{ { name: "Eventing CR should have error state when subscription manager is not ready", @@ -607,11 +609,26 @@ func Test_CreateEventingCR_EventMesh(t *testing.T) { wantMatches: gomega.And( matchers.HaveStatusError(), matchers.HaveEventMeshSubManagerNotReadyCondition( - "failed to get EventMesh secret: Secret \"test-secret-name1\" not found"), + "failed to sync Publisher Proxy secret: unexpected error"), matchers.HaveFinalizer(), ), shouldFailSubManager: true, }, + { + name: "Eventing CR should have warning state when EventMesh secret is missing", + givenEventing: utils.NewEventingCR( + utils.WithEventMeshBackend("test-secret-name2"), + utils.WithEventingPublisherData(1, 1, "199m", "99Mi", "399m", "199Mi"), + utils.WithEventingEventTypePrefix("test-prefix"), + ), + wantMatches: gomega.And( + matchers.HaveStatusWarning(), + matchers.HaveEventMeshSubManagerNotReadyCondition( + eventingcontroller.EventMeshSecretMissingMessage), + matchers.HaveFinalizer(), + ), + shouldEventMeshSecretNotFound: true, + }, { name: "Eventing CR should have ready state when all deployment replicas are ready", givenEventing: utils.NewEventingCR( @@ -664,18 +681,30 @@ func Test_CreateEventingCR_EventMesh(t *testing.T) { // create eventing-webhook-auth secret. testEnvironment.EnsureOAuthSecretCreated(t, tc.givenEventing) - if !tc.shouldFailSubManager { + if !tc.shouldEventMeshSecretNotFound { // create EventMesh secret. testEnvironment.EnsureEventMeshSecretCreated(t, tc.givenEventing) } + originalKubeClient := testEnvironment.KubeClient + if tc.shouldFailSubManager { + mockedKubeClient := &MockKubeClient{ + Client: originalKubeClient, + } + testEnvironment.KubeClient = mockedKubeClient + testEnvironment.Reconciler.SetKubeClient(mockedKubeClient) + } + // when // create Eventing CR. testEnvironment.EnsureK8sResourceCreated(t, tc.givenEventing) defer func() { + testEnvironment.KubeClient = originalKubeClient + testEnvironment.Reconciler.SetKubeClient(originalKubeClient) + testEnvironment.EnsureEventingResourceDeletion(t, tc.givenEventing.Name, givenNamespace) - if !*testEnvironment.EnvTestInstance.UseExistingCluster && !tc.shouldFailSubManager { + if !*testEnvironment.EnvTestInstance.UseExistingCluster && !tc.shouldFailSubManager && !tc.shouldEventMeshSecretNotFound { testEnvironment.EnsureDeploymentDeletion(t, eventing.GetPublisherDeploymentName(*tc.givenEventing), givenNamespace) } testEnvironment.EnsureNamespaceDeleted(t, givenNamespace) @@ -1077,3 +1106,7 @@ func (mkc *MockKubeClient) GetCRD(ctx context.Context, name string) (*kapiextens } return nil, notFoundError } + +func (mkc *MockKubeClient) PatchApply(ctx context.Context, object kclient.Object) error { + return fmt.Errorf("unexpected error") +} diff --git a/internal/controller/operator/eventing/integrationtests/nats_disabled/integration_test.go b/internal/controller/operator/eventing/integrationtests/nats_disabled/integration_test.go index 11a9052c..40c8f746 100644 --- a/internal/controller/operator/eventing/integrationtests/nats_disabled/integration_test.go +++ b/internal/controller/operator/eventing/integrationtests/nats_disabled/integration_test.go @@ -117,7 +117,7 @@ func Test_DeletionOfPublisherResourcesWhenNATSNotEnabled(t *testing.T) { // then // wait until Eventing CR status is Error. testEnvironment.GetEventingAssert(g, givenEventing).Should(gomega.And( - matchers.HaveStatusError(), + matchers.HaveStatusWarning(), matchers.HaveNATSNotAvailableConditionWith("NATS module has to be installed: customresourcedefinitions.apiextensions.k8s.io \"nats.operator.kyma-project.io\" not found"), )) From f655f2ccea528aa2b2fec601f3c021063c66d3d6 Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Wed, 6 Dec 2023 07:29:16 +0100 Subject: [PATCH 05/11] Fix linting errors --- .golangci.yaml | 6 ++---- api/operator/v1alpha1/status.go | 3 ++- api/operator/v1alpha1/zz_generated.deepcopy.go | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 1de839f0..4f247381 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -34,17 +34,14 @@ linters: - goerr113 - gomnd - gosec - - inamedparam - ireturn - maintidx - noctx - nolintlint - nonamedreturns - paralleltest - - perfsprint - prealloc - stylecheck - - testifylint - testpackage - thelper - tparallel @@ -53,7 +50,6 @@ linters: - varnamelen - wrapcheck - godot - - tagalign - whitespace linters-settings: @@ -137,6 +133,8 @@ linters-settings: alias: kctrl - pkg: sigs.k8s.io/controller-runtime/pkg/log alias: kctrllog + - pkg: sigs.k8s.io/controller-runtime/pkg/client + alias: kclient - pkg: k8s.io/api/autoscaling/v1 alias: kautoscalingv1 - pkg: k8s.io/api/autoscaling/v2 diff --git a/api/operator/v1alpha1/status.go b/api/operator/v1alpha1/status.go index 9d707a58..bff8971d 100644 --- a/api/operator/v1alpha1/status.go +++ b/api/operator/v1alpha1/status.go @@ -9,7 +9,8 @@ import ( ) func (es *EventingStatus) UpdateConditionBackendAvailable(status kmetav1.ConditionStatus, reason ConditionReason, - message string) { + message string, +) { condition := kmetav1.Condition{ Type: string(ConditionBackendAvailable), Status: status, diff --git a/api/operator/v1alpha1/zz_generated.deepcopy.go b/api/operator/v1alpha1/zz_generated.deepcopy.go index 1a8cdd8f..a7bd0a7a 100644 --- a/api/operator/v1alpha1/zz_generated.deepcopy.go +++ b/api/operator/v1alpha1/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ limitations under the License. package v1alpha1 import ( - "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" ) From 6eace18610423f032aa2f9e9562ebb5f5c8efa6d Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Wed, 6 Dec 2023 14:25:43 +0100 Subject: [PATCH 06/11] Forbid deleting backend configuration It is not allowed to delete existing backend config, but evneting CR can be created with empty backend config. --- api/operator/v1alpha1/eventing_types.go | 3 ++- .../bases/operator.kyma-project.io_eventings.yaml | 14 +++++--------- internal/controller/operator/eventing/eventmesh.go | 3 ++- internal/controller/operator/eventing/status.go | 9 ++++++--- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/api/operator/v1alpha1/eventing_types.go b/api/operator/v1alpha1/eventing_types.go index 5b3277a4..5bead265 100644 --- a/api/operator/v1alpha1/eventing_types.go +++ b/api/operator/v1alpha1/eventing_types.go @@ -84,7 +84,8 @@ type Eventing struct { kmetav1.TypeMeta `json:",inline"` kmetav1.ObjectMeta `json:"metadata,omitempty"` - // +kubebuilder:default:={backend:{type:"NATS", config:{natsStreamStorageType:"File", natsStreamReplicas:3, natsStreamMaxSize:"700Mi", natsMaxMsgsPerTopic:1000000}}, logging:{logLevel:Info}, publisher:{replicas:{min:2,max:2}, resources:{limits:{cpu:"500m",memory:"512Mi"}, requests:{cpu:"40m",memory:"256Mi"}}}} + // +kubebuilder:default:={logging:{logLevel:Info}, publisher:{replicas:{min:2,max:2}, resources:{limits:{cpu:"500m",memory:"512Mi"}, requests:{cpu:"40m",memory:"256Mi"}}}} + // +kubebuilder:validation:XValidation:rule="!(oldSelf!=null && has(oldSelf.backend)) || has(self.backend)", message="backend config cannot be deleted" Spec EventingSpec `json:"spec,omitempty"` Status EventingStatus `json:"status,omitempty"` } diff --git a/config/crd/bases/operator.kyma-project.io_eventings.yaml b/config/crd/bases/operator.kyma-project.io_eventings.yaml index 8c774444..3513c2cc 100644 --- a/config/crd/bases/operator.kyma-project.io_eventings.yaml +++ b/config/crd/bases/operator.kyma-project.io_eventings.yaml @@ -47,13 +47,6 @@ spec: type: object spec: default: - backend: - config: - natsMaxMsgsPerTopic: 1000000 - natsStreamMaxSize: 700Mi - natsStreamReplicas: 3 - natsStreamStorageType: File - type: NATS logging: logLevel: Info publisher: @@ -67,7 +60,7 @@ spec: requests: cpu: 40m memory: 256Mi - description: EventingSpec defines the desired state of Eventing + description: EventingSpec defines the desired state of Eventing. properties: annotations: additionalProperties: @@ -255,8 +248,11 @@ spec: type: object type: object type: object + x-kubernetes-validations: + - message: backend config cannot be deleted + rule: '!(oldSelf!=null && has(oldSelf.backend)) || has(self.backend)' status: - description: EventingStatus defines the observed state of Eventing + description: EventingStatus defines the observed state of Eventing. properties: activeBackend: type: string diff --git a/internal/controller/operator/eventing/eventmesh.go b/internal/controller/operator/eventing/eventmesh.go index e36c9466..00b6e525 100644 --- a/internal/controller/operator/eventing/eventmesh.go +++ b/internal/controller/operator/eventing/eventmesh.go @@ -39,7 +39,8 @@ type oauth2Credentials struct { const EventMeshSecretMissingMessage = "the specified EventMesh secret is missing. Please provide the secret" func (r *Reconciler) reconcileEventMeshSubManager(ctx context.Context, eventing *v1alpha1.Eventing, - log *zap.SugaredLogger) error { + log *zap.SugaredLogger, +) error { // gets oauth2ClientID and secret and stops the EventMesh subscription manager if changed err := r.syncOauth2ClientIDAndSecret(ctx, eventing) if err != nil { diff --git a/internal/controller/operator/eventing/status.go b/internal/controller/operator/eventing/status.go index 63985e04..97c87926 100644 --- a/internal/controller/operator/eventing/status.go +++ b/internal/controller/operator/eventing/status.go @@ -26,12 +26,14 @@ func (es *Reconciler) InitStateProcessing(eventing *operatorv1alpha1.Eventing) { // syncStatusWithNATSErr syncs Eventing status and sets an error state. // Returns the relevant error. func (r *Reconciler) syncStatusWithNATSErr(ctx context.Context, - eventing *operatorv1alpha1.Eventing, err error, log *zap.SugaredLogger) error { + eventing *operatorv1alpha1.Eventing, err error, log *zap.SugaredLogger, +) error { return r.syncStatusWithNATSState(ctx, operatorv1alpha1.StateError, eventing, err, log) } func (r *Reconciler) syncStatusWithNATSState(ctx context.Context, state string, - eventing *operatorv1alpha1.Eventing, err error, log *zap.SugaredLogger) error { + eventing *operatorv1alpha1.Eventing, err error, log *zap.SugaredLogger, +) error { // Set error state in status eventing.Status.State = state eventing.Status.UpdateConditionBackendAvailable(kmetav1.ConditionFalse, @@ -42,7 +44,8 @@ func (r *Reconciler) syncStatusWithNATSState(ctx context.Context, state string, } func (r *Reconciler) syncStatusForEmptyBackend(ctx context.Context, reason operatorv1alpha1.ConditionReason, - message string, eventing *operatorv1alpha1.Eventing, log *zap.SugaredLogger) error { + message string, eventing *operatorv1alpha1.Eventing, log *zap.SugaredLogger, +) error { // Set error state in status eventing.Status.SetStateWarning() eventing.Status.UpdateConditionBackendAvailable( From 3c103651c97856a67166fc146b8e115e2dd87132 Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Wed, 6 Dec 2023 14:35:18 +0100 Subject: [PATCH 07/11] Revert golangci-lint configuration --- .golangci.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.golangci.yaml b/.golangci.yaml index 4f247381..ce5309a1 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -34,14 +34,17 @@ linters: - goerr113 - gomnd - gosec + - inamedparam - ireturn - maintidx - noctx - nolintlint - nonamedreturns - paralleltest + - perfsprint - prealloc - stylecheck + - testifylint - testpackage - thelper - tparallel @@ -50,6 +53,7 @@ linters: - varnamelen - wrapcheck - godot + - tagalign - whitespace linters-settings: From 73de8c3a43bb48310656bf4a70d58dac70ae124a Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Wed, 6 Dec 2023 15:53:49 +0100 Subject: [PATCH 08/11] Implement int tests Create integration tests for backend config deletion validation --- .../validation/integration_test.go | 103 +++++++++++++++++- test/utils/integration/integration.go | 4 + 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/internal/controller/operator/eventing/integrationtests/validation/integration_test.go b/internal/controller/operator/eventing/integrationtests/validation/integration_test.go index 71d1ba61..6cbedbe0 100644 --- a/internal/controller/operator/eventing/integrationtests/validation/integration_test.go +++ b/internal/controller/operator/eventing/integrationtests/validation/integration_test.go @@ -686,6 +686,20 @@ func Test_Validate_CreateEventing(t *testing.T) { }, }, }, + { + name: `validation of spec.backend is empty`, + givenUnstructuredEventing: unstructured.Unstructured{ + Object: map[string]any{ + kind: kindEventing, + apiVersion: apiVersionEventing, + metadata: map[string]any{ + name: test.GetRandK8sName(7), + namespace: test.GetRandK8sName(7), + }, + spec: map[string]any{}, + }, + }, + }, } for _, tc := range testCases { @@ -714,6 +728,93 @@ func Test_Validate_CreateEventing(t *testing.T) { } } +// Test_Validate_UpdateEventing updates an eventing CR with correct and purposefully incorrect values, and compares +// the error that was caused by this against a wantErrMsg to test the eventing CR validation rules. +func Test_Validate_UpdateEventing(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + givenOriginalUnstructuredEventing unstructured.Unstructured + givenTargetUnstructuredEventing unstructured.Unstructured + wantErrMsg string + }{ + { + name: `validation of spec.backend deletion, which is not allowed`, + givenOriginalUnstructuredEventing: unstructured.Unstructured{ + Object: map[string]any{ + kind: kindEventing, + apiVersion: apiVersionEventing, + metadata: map[string]any{ + name: "test-name-777", + namespace: "test-namespace-777", + }, + spec: map[string]any{ + backend: map[string]any{ + backendType: typeNats, + }, + publisher: map[string]any{ + replicas: map[string]any{ + min: 2, + max: 3, + }, + }, + }, + }, + }, + givenTargetUnstructuredEventing: unstructured.Unstructured{ + Object: map[string]any{ + kind: kindEventing, + apiVersion: apiVersionEventing, + metadata: map[string]any{ + name: "test-name-777", + namespace: "test-namespace-777", + }, + spec: map[string]any{ + publisher: map[string]any{ + replicas: map[string]any{ + min: 2, + max: 3, + }, + }, + }, + }, + }, + wantErrMsg: "backend config cannot be deleted", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // given + testEnvironment.EnsureNamespaceCreation(t, tc.givenOriginalUnstructuredEventing.GetNamespace()) + + // when + err := testEnvironment.CreateUnstructuredK8sResource(&tc.givenOriginalUnstructuredEventing) + require.NoError(t, err, "Expected error message to be empty but got error instead.") + + tc.givenTargetUnstructuredEventing.SetResourceVersion(tc.givenOriginalUnstructuredEventing.GetResourceVersion()) + + err = testEnvironment.UpdateUnstructuredK8sResource(&tc.givenTargetUnstructuredEventing) + + // then + if tc.wantErrMsg == noError { + require.NoError(t, err, "Expected error message to be empty but got error instead."+ + " Check the validation rule of the eventing CR.") + } else { + require.Error(t, err, fmt.Sprintf("Expected the following error message: \n \" %s \" \n"+ + " but got no error. Check the validation rules of the eventing CR.", tc.wantErrMsg)) + + require.Contains(t, err.Error(), tc.wantErrMsg, "Expected a specific error message"+ + " but messages do not match. Check the validation rules of the eventing CR.") + } + }) + } +} + // Test_Validate_CreateEventing creates an eventing CR with correct and purposefully incorrect values, and compares // the error that was caused by this against a wantErrMsg to test the eventing CR validation rules. func Test_Validate_Defaulting(t *testing.T) { @@ -737,7 +838,6 @@ func Test_Validate_Defaulting(t *testing.T) { }, }, wantMatches: gomega.And( - testmatchers.HaveBackendTypeNats(defaultBackendConfig()), testmatchers.HavePublisher(defaultPublisher()), testmatchers.HavePublisherResources(defaultPublisherResources()), testmatchers.HaveLogging(defaultLogging()), @@ -757,7 +857,6 @@ func Test_Validate_Defaulting(t *testing.T) { }, }, wantMatches: gomega.And( - testmatchers.HaveBackendTypeNats(defaultBackendConfig()), testmatchers.HavePublisher(defaultPublisher()), testmatchers.HavePublisherResources(defaultPublisherResources()), testmatchers.HaveLogging(defaultLogging()), diff --git a/test/utils/integration/integration.go b/test/utils/integration/integration.go index f2fa5aad..1b7ee2e1 100644 --- a/test/utils/integration/integration.go +++ b/test/utils/integration/integration.go @@ -1144,6 +1144,10 @@ func (env TestEnvironment) CreateUnstructuredK8sResource(obj *unstructured.Unstr return env.k8sClient.Create(env.Context, obj) } +func (env TestEnvironment) UpdateUnstructuredK8sResource(obj *unstructured.Unstructured) error { + return env.k8sClient.Update(env.Context, obj) +} + func (env TestEnvironment) EnsureK8sUnStructResourceCreated(t *testing.T, obj *unstructured.Unstructured) { require.NoError(t, env.k8sClient.Create(env.Context, obj)) } From 3f26c8644c7ca8c28fa4f5485d1f89cd65b6aacc Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Thu, 7 Dec 2023 07:05:09 +0100 Subject: [PATCH 09/11] Revert the documentation Auto generation of doc is breaking the documentation --- docs/user/02-configuration.md | 77 +++++++++++++++++------------------ 1 file changed, 37 insertions(+), 40 deletions(-) diff --git a/docs/user/02-configuration.md b/docs/user/02-configuration.md index a90a6cd3..f88b228f 100644 --- a/docs/user/02-configuration.md +++ b/docs/user/02-configuration.md @@ -28,49 +28,46 @@ Use the following sample CRs as guidance. Each can be applied immediately when y **Spec:** -| Parameter | Type | Description | -| ---- | ----------- | ---- | -| **annotations** | map\[string\]string | Annotations allows to add annotations to resources. | -| **backend** | object | Backend defines the active backend used by Eventing. | -| **backend.​config** | object | Config defines configuration for eventing backend. | -| **backend.​config.​domain** | string | Domain defines the cluster public domain used to configure the EventMesh Subscriptions and their corresponding ApiRules. | -| **backend.​config.​eventMeshSecret** | string | EventMeshSecret defines the namespaced name of K8s Secret containing EventMesh credentials. The format of name is "namespace/name". | -| **backend.​config.​eventTypePrefix** | string | | -| **backend.​config.​natsMaxMsgsPerTopic** | integer | NATSMaxMsgsPerTopic limits how many messages in the NATS stream to retain per subject. | -| **backend.​config.​natsStreamMaxSize** | \{integer or string\} | NATSStreamMaxSize defines the maximum storage size for stream data. | -| **backend.​config.​natsStreamReplicas** | integer | NATSStreamReplicas defines the number of replicas for stream. | -| **backend.​config.​natsStreamStorageType** | string | NATSStreamStorageType defines the storage type for stream data. | -| **backend.​type** (required) | string | Type defines which backend to use. The value is either `EventMesh`, or `NATS`. | -| **labels** | map\[string\]string | Labels allows to add Labels to resources. | -| **logging** | object | Logging defines the log level for eventing-manager. | -| **logging.​logLevel** | string | LogLevel defines the log level. | -| **publisher** | object | Publisher defines the configurations for eventing-publisher-proxy. | -| **publisher.​replicas** | object | Replicas defines the scaling min/max for eventing-publisher-proxy. | -| **publisher.​replicas.​max** | integer | Max defines maximum number of replicas. | -| **publisher.​replicas.​min** | integer | Min defines minimum number of replicas. | -| **publisher.​resources** | object | Resources defines resources for eventing-publisher-proxy. | -| **publisher.​resources.​claims** | \[\]object | Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container. - This is an alpha field and requires enabling the DynamicResourceAllocation feature gate. - This field is immutable. It can only be set for containers. | -| **publisher.​resources.​claims.​name** (required) | string | Name must match the name of one entry in pod.spec.resourceClaims of the Pod where this field is used. It makes that resource available inside a container. | -| **publisher.​resources.​limits** | map\[string\]\{integer or string\} | Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | +| Parameter | Type | Description | +|----------------------------------------------------------|-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| **annotations** | map\[string\]string | Annotations allows to add annotations to resources. | +| **backend** | object | Backend defines the active backend used by Eventing. | +| **backend.​config** | object | Config defines configuration for eventing backend. | +| **backend.​config.​domain** | string | Domain defines the cluster public domain used to configure the EventMesh Subscriptions and their corresponding ApiRules. | +| **backend.​config.​eventMeshSecret** | string | EventMeshSecret defines the namespaced name of K8s Secret containing EventMesh credentials. The format of name is "namespace/name". | +| **backend.​config.​eventTypePrefix** | string | | +| **backend.​config.​natsMaxMsgsPerTopic** | integer | NATSMaxMsgsPerTopic limits how many messages in the NATS stream to retain per subject. | +| **backend.​config.​natsStreamMaxSize** | \{integer or string\} | NATSStreamMaxSize defines the maximum storage size for stream data. | +| **backend.​config.​natsStreamReplicas** | integer | NATSStreamReplicas defines the number of replicas for stream. | +| **backend.​config.​natsStreamStorageType** | string | NATSStreamStorageType defines the storage type for stream data. | +| **backend.​type** (required) | string | Type defines which backend to use. The value is either `EventMesh`, or `NATS`. | +| **labels** | map\[string\]string | Labels allows to add Labels to resources. | +| **logging** | object | Logging defines the log level for eventing-manager. | +| **logging.​logLevel** | string | LogLevel defines the log level. | +| **publisher** | object | Publisher defines the configurations for eventing-publisher-proxy. | +| **publisher.​replicas** | object | Replicas defines the scaling min/max for eventing-publisher-proxy. | +| **publisher.​replicas.​max** | integer | Max defines maximum number of replicas. | +| **publisher.​replicas.​min** | integer | Min defines minimum number of replicas. | +| **publisher.​resources** | object | Resources defines resources for eventing-publisher-proxy. | +| **publisher.​resources.​claims** | \[\]object | Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container. This is an alpha field and requires enabling the DynamicResourceAllocation feature gate. This field is immutable. It can only be set for containers. | +| **publisher.​resources.​claims.​name** (required) | string | Name must match the name of one entry in pod.spec.resourceClaims of the Pod where this field is used. It makes that resource available inside a container. | +| **publisher.​resources.​limits** | map\[string\]\{integer or string\} | Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | | **publisher.​resources.​requests** | map\[string\]\{integer or string\} | Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | **Status:** -| Parameter | Type | Description | -| ---- | ----------- | ---- | -| **activeBackend** (required) | string | | -| **conditions** | \[\]object | Condition contains details for one aspect of the current state of this API Resource. --- This struct is intended for direct use as an array at the field path .status.conditions. For example, - type FooStatus struct{ // Represents the observations of a foo's current state. // Known .status.conditions.type are: "Available", "Progressing", and "Degraded" // +patchMergeKey=type // +patchStrategy=merge // +listType=map // +listMapKey=type Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` - // other fields } | -| **conditions.​lastTransitionTime** (required) | string | lastTransitionTime is the last time the condition transitioned from one status to another. This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. | -| **conditions.​message** (required) | string | message is a human readable message indicating details about the transition. This may be an empty string. | -| **conditions.​observedGeneration** | integer | observedGeneration represents the .metadata.generation that the condition was set based upon. For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date with respect to the current state of the instance. | -| **conditions.​reason** (required) | string | reason contains a programmatic identifier indicating the reason for the condition's last transition. Producers of specific condition types may define expected values and meanings for this field, and whether the values are considered a guaranteed API. The value should be a CamelCase string. This field may not be empty. | -| **conditions.​status** (required) | string | status of the condition, one of True, False, Unknown. | -| **conditions.​type** (required) | string | type of condition in CamelCase or in foo.example.com/CamelCase. --- Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be useful (see .node.status.conditions), the ability to deconflict is important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) | -| **specHash** (required) | integer | | -| **state** (required) | string | | +| Parameter | Type | Description | +|------------------------------------------------------|------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| **activeBackend** (required) | string | | +| **conditions** | \[\]object | Condition contains details for one aspect of the current state of this API Resource. --- This struct is intended for direct use as an array at the field path .status.conditions. For example, type FooStatus struct{ // Represents the observations of a foo's current state. // Known .status.conditions.type are: "Available", "Progressing", and "Degraded" // +patchMergeKey=type // +patchStrategy=merge // +listType=map // +listMapKey=type Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` | +| // other fields } | | | +| **conditions.​lastTransitionTime** (required) | string | lastTransitionTime is the last time the condition transitioned from one status to another. This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. | +| **conditions.​message** (required) | string | message is a human readable message indicating details about the transition. This may be an empty string. | +| **conditions.​observedGeneration** | integer | observedGeneration represents the .metadata.generation that the condition was set based upon. For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date with respect to the current state of the instance. | +| **conditions.​reason** (required) | string | reason contains a programmatic identifier indicating the reason for the condition's last transition. Producers of specific condition types may define expected values and meanings for this field, and whether the values are considered a guaranteed API. The value should be a CamelCase string. This field may not be empty. | +| **conditions.​status** (required) | string | status of the condition, one of `True`, `False`, `Unknown`. | +| **conditions.​type** (required) | string | type of condition in CamelCase or in foo.example.com/CamelCase. --- Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be useful (see .node.status.conditions), the ability to deconflict is important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) | +| **specHash** (required) | integer | | +| **state** (required) | string | | \ No newline at end of file From d10505255c9e9d6221f9ff564a9dea2f96fb113a Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Thu, 7 Dec 2023 10:04:24 +0100 Subject: [PATCH 10/11] Move Retrieving EventMesh Secret --- .../operator/eventing/controller.go | 13 +++++- .../controller/operator/eventing/eventmesh.go | 11 +---- .../operator/eventing/eventmesh_test.go | 40 ++++--------------- .../controller/operator/eventing/status.go | 12 ++++++ 4 files changed, 33 insertions(+), 43 deletions(-) diff --git a/internal/controller/operator/eventing/controller.go b/internal/controller/operator/eventing/controller.go index 5be13478..85494795 100644 --- a/internal/controller/operator/eventing/controller.go +++ b/internal/controller/operator/eventing/controller.go @@ -612,8 +612,19 @@ func (r *Reconciler) reconcileEventMeshBackend(ctx context.Context, eventing *op return kctrl.Result{}, r.syncStatusWithSubscriptionManagerErr(ctx, eventing, apiRuleMissingErr, log) } + // retrieve secret used to authenticate with EventMesh + eventMeshSecret, err := r.kubeClient.GetSecret(ctx, eventing.Spec.Backend.Config.EventMeshSecret) + if err != nil { + if kerrors.IsNotFound(err) { + return kctrl.Result{}, r.syncSubManagerStatusWithNATSState(ctx, operatorv1alpha1.StateWarning, eventing, + fmt.Errorf(EventMeshSecretMissingMessage), log) + } + return kctrl.Result{}, r.syncStatusWithSubscriptionManagerErr(ctx, eventing, + fmt.Errorf("failed to get EventMesh secret: %v", err), log) + } + // Start the EventMesh subscription controller - err = r.reconcileEventMeshSubManager(ctx, eventing, log) + err = r.reconcileEventMeshSubManager(ctx, eventing, eventMeshSecret, log) if err != nil { return kctrl.Result{}, r.syncStatusWithSubscriptionManagerErr(ctx, eventing, err, log) } diff --git a/internal/controller/operator/eventing/eventmesh.go b/internal/controller/operator/eventing/eventmesh.go index 00b6e525..249f362e 100644 --- a/internal/controller/operator/eventing/eventmesh.go +++ b/internal/controller/operator/eventing/eventmesh.go @@ -39,22 +39,13 @@ type oauth2Credentials struct { const EventMeshSecretMissingMessage = "the specified EventMesh secret is missing. Please provide the secret" func (r *Reconciler) reconcileEventMeshSubManager(ctx context.Context, eventing *v1alpha1.Eventing, - log *zap.SugaredLogger, + eventMeshSecret *kcorev1.Secret, log *zap.SugaredLogger, ) error { // gets oauth2ClientID and secret and stops the EventMesh subscription manager if changed err := r.syncOauth2ClientIDAndSecret(ctx, eventing) if err != nil { return errors.Errorf("failed to sync OAuth secret: %v", err) } - // retrieve secret to authenticate with EventMesh - eventMeshSecret, err := r.kubeClient.GetSecret(ctx, eventing.Spec.Backend.Config.EventMeshSecret) - if err != nil { - if kerrors.IsNotFound(err) { - return r.syncStatusWithNATSState(ctx, v1alpha1.StateWarning, eventing, - errors.New(EventMeshSecretMissingMessage), log) - } - return errors.Errorf("failed to get EventMesh secret: %v", err) - } // CreateOrUpdate deployment for publisher proxy secret secretForPublisher, err := r.SyncPublisherProxySecret(ctx, eventMeshSecret) if err != nil { diff --git a/internal/controller/operator/eventing/eventmesh_test.go b/internal/controller/operator/eventing/eventmesh_test.go index 5fa81446..4d017295 100644 --- a/internal/controller/operator/eventing/eventmesh_test.go +++ b/internal/controller/operator/eventing/eventmesh_test.go @@ -92,30 +92,6 @@ func Test_reconcileEventMeshSubManager(t *testing.T) { wantError: errors.New("failed to sync OAuth secret"), wantHashAfter: int64(0), }, - { - name: "it should do nothing because failed syncing EventMesh secret", - givenIsEventMeshSubManagerStarted: true, - givenHashBefore: int64(0), - givenEventMeshSubManagerMock: func() *submgrmanagermocks.Manager { - eventMeshSubManagerMock := new(submgrmanagermocks.Manager) - eventMeshSubManagerMock.On("Stop", mock.Anything).Return(nil).Once() - return eventMeshSubManagerMock - }, - givenEventingManagerMock: func() *eventingmocks.Manager { - return nil - }, - givenManagerFactoryMock: func(_ *submgrmanagermocks.Manager) *submgrmocks.ManagerFactory { - return nil - }, - givenKubeClientMock: func() k8s.Client { - mockKubeClient := new(k8smocks.Client) - mockKubeClient.On("GetSecret", ctx, mock.Anything, mock.Anything).Return(nil, - errors.New("failed getting secret")).Once() - return mockKubeClient - }, - wantError: errors.New("failed to get EventMesh secret"), - wantHashAfter: int64(0), - }, { name: "it should do nothing because failed sync Publisher Proxy secret", givenIsEventMeshSubManagerStarted: true, @@ -298,15 +274,18 @@ func Test_reconcileEventMeshSubManager(t *testing.T) { givenEventing.Status.BackendConfigHash = tc.givenHashBefore // when - err := testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, logger) + + eventMeshSecret := utils.NewEventMeshSecret("eventing-backend", givenEventing.Namespace) + + err := testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, eventMeshSecret, logger) if err != nil && tc.givenShouldRetry { // This is to test the scenario where initialization of eventMeshSubManager was successful but // starting the eventMeshSubManager failed. So on next try it should again try to start the eventMeshSubManager. - err = testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, logger) + err = testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, eventMeshSecret, logger) } if tc.givenUpdateTest { // Run reconcile again with newBackendConfig: - err = testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, logger) + err = testEnv.Reconciler.reconcileEventMeshSubManager(ctx, givenEventing, eventMeshSecret, logger) require.NoError(t, err) } @@ -388,8 +367,6 @@ func Test_reconcileEventMeshSubManager_ReadClusterDomain(t *testing.T) { givenKubeClientMock: func() (k8s.Client, *k8smocks.Client) { mockKubeClient := new(k8smocks.Client) mockKubeClient.On("PatchApply", ctx, mock.Anything).Return(nil).Once() - mockKubeClient.On("GetSecret", ctx, mock.Anything, mock.Anything).Return( - utils.NewEventMeshSecret("test-secret", namespace), nil).Once() return mockKubeClient, mockKubeClient }, }, @@ -421,8 +398,6 @@ func Test_reconcileEventMeshSubManager_ReadClusterDomain(t *testing.T) { mockKubeClient := new(k8smocks.Client) mockKubeClient.On("GetConfigMap", ctx, mock.Anything, mock.Anything).Return(givenConfigMap, nil).Once() mockKubeClient.On("PatchApply", ctx, mock.Anything).Return(nil).Once() - mockKubeClient.On("GetSecret", ctx, mock.Anything, mock.Anything).Return( - utils.NewEventMeshSecret("test-secret", namespace), nil).Once() return mockKubeClient, mockKubeClient }, }, @@ -452,7 +427,8 @@ func Test_reconcileEventMeshSubManager_ReadClusterDomain(t *testing.T) { testEnv.Reconciler.eventMeshSubManager = nil // when - err := testEnv.Reconciler.reconcileEventMeshSubManager(ctx, tc.givenEventing, logger) + eventMeshSecret := utils.NewEventMeshSecret("test-secret", tc.givenEventing.Namespace) + err := testEnv.Reconciler.reconcileEventMeshSubManager(ctx, tc.givenEventing, eventMeshSecret, logger) // then require.NoError(t, err) diff --git a/internal/controller/operator/eventing/status.go b/internal/controller/operator/eventing/status.go index 97c87926..a11be7fc 100644 --- a/internal/controller/operator/eventing/status.go +++ b/internal/controller/operator/eventing/status.go @@ -85,6 +85,18 @@ func (r *Reconciler) syncStatusWithSubscriptionManagerErr(ctx context.Context, operatorv1alpha1.ConditionReasonEventMeshSubManagerFailed, eventing, err, log) } +func (r *Reconciler) syncSubManagerStatusWithNATSState(ctx context.Context, state string, + eventing *operatorv1alpha1.Eventing, err error, log *zap.SugaredLogger, +) error { + // Set error state in status + eventing.Status.State = state + eventing.Status.UpdateConditionSubscriptionManagerReady(kmetav1.ConditionFalse, + operatorv1alpha1.ConditionReasonEventMeshSubManagerFailed, + err.Error()) + + return errors.Join(err, r.syncEventingStatus(ctx, eventing, log)) +} + func (r *Reconciler) syncStatusWithSubscriptionManagerErrWithReason(ctx context.Context, reason operatorv1alpha1.ConditionReason, eventing *operatorv1alpha1.Eventing, From 5eb1206284d4221a3a9e29824b52b305bd418c83 Mon Sep 17 00:00:00 2001 From: Mansur Uralov Date: Thu, 7 Dec 2023 10:41:52 +0100 Subject: [PATCH 11/11] Improve for review comments --- .golangci.yaml | 2 +- internal/controller/operator/eventing/eventmesh.go | 2 +- .../eventing/integrationtests/controller/integration_test.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index ce5309a1..ee4bad9b 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -138,7 +138,7 @@ linters-settings: - pkg: sigs.k8s.io/controller-runtime/pkg/log alias: kctrllog - pkg: sigs.k8s.io/controller-runtime/pkg/client - alias: kclient + alias: kctrlclient - pkg: k8s.io/api/autoscaling/v1 alias: kautoscalingv1 - pkg: k8s.io/api/autoscaling/v2 diff --git a/internal/controller/operator/eventing/eventmesh.go b/internal/controller/operator/eventing/eventmesh.go index 249f362e..cd1a092f 100644 --- a/internal/controller/operator/eventing/eventmesh.go +++ b/internal/controller/operator/eventing/eventmesh.go @@ -36,7 +36,7 @@ type oauth2Credentials struct { certsURL []byte } -const EventMeshSecretMissingMessage = "the specified EventMesh secret is missing. Please provide the secret" +const EventMeshSecretMissingMessage = "The specified EventMesh secret is not found. Please provide an existing secret." func (r *Reconciler) reconcileEventMeshSubManager(ctx context.Context, eventing *v1alpha1.Eventing, eventMeshSecret *kcorev1.Secret, log *zap.SugaredLogger, diff --git a/internal/controller/operator/eventing/integrationtests/controller/integration_test.go b/internal/controller/operator/eventing/integrationtests/controller/integration_test.go index 9a5937c4..eeeeb894 100644 --- a/internal/controller/operator/eventing/integrationtests/controller/integration_test.go +++ b/internal/controller/operator/eventing/integrationtests/controller/integration_test.go @@ -16,7 +16,7 @@ import ( kapiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" kmetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - kclient "sigs.k8s.io/controller-runtime/pkg/client" + kctrlclient "sigs.k8s.io/controller-runtime/pkg/client" eventingv1alpha2 "github.com/kyma-project/eventing-manager/api/eventing/v1alpha2" operatorv1alpha1 "github.com/kyma-project/eventing-manager/api/operator/v1alpha1" @@ -1107,6 +1107,6 @@ func (mkc *MockKubeClient) GetCRD(ctx context.Context, name string) (*kapiextens return nil, notFoundError } -func (mkc *MockKubeClient) PatchApply(ctx context.Context, object kclient.Object) error { +func (mkc *MockKubeClient) PatchApply(ctx context.Context, object kctrlclient.Object) error { return fmt.Errorf("unexpected error") }