From 38a7db27ac2d79a287d40ca65c9f665cbd5cf8d0 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 2 Dec 2024 17:37:07 +0100 Subject: [PATCH 01/35] Add account, consumer and stream controller stubs to be implemented Controllers and tests are based on files generated by operator-sdk. Adds a minimal test suite for the controllers with a etcd test env and a test nats jetStream server to test against. --- internal/controller/consumer_controller.go | 10 ++++++---- internal/controller/stream_controller.go | 3 +-- internal/controller/suite_test.go | 12 +++--------- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/internal/controller/consumer_controller.go b/internal/controller/consumer_controller.go index 6dbe7ba8..af1566bf 100644 --- a/internal/controller/consumer_controller.go +++ b/internal/controller/consumer_controller.go @@ -19,12 +19,13 @@ package controller import ( "context" "github.com/nats-io/nats.go/jetstream" - "k8s.io/klog/v2" - jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + + jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" ) // ConsumerReconciler reconciles a Consumer object @@ -41,8 +42,9 @@ type ConsumerReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile func (r *ConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - log := klog.FromContext(ctx) - log.Info("reconcile", "namespace", req.Namespace, "name", req.Name) + _ = log.FromContext(ctx) + + // TODO(user): your logic here return ctrl.Result{}, nil } diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 4469b7f0..517ce675 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -42,8 +42,7 @@ type StreamReconciler struct { // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := klog.FromContext(ctx) - log.Info("reconcile", "namespace", req.Namespace, "name", req.Name) - // TODO(user): your logic here + log.Info("reconcile %s", "namespace", req.Namespace, "name", req.Name) return ctrl.Result{}, nil } diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 2dac16e2..cf57430f 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -20,7 +20,6 @@ import ( "fmt" "github.com/nats-io/nats-server/v2/server" natsserver "github.com/nats-io/nats-server/v2/test" - "github.com/nats-io/nats.go/jetstream" "os" "path/filepath" "runtime" @@ -37,6 +36,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" + //+kubebuilder:scaffold:imports ) // These tests use Ginkgo (BDD-style Go testing framework). Refer to @@ -46,7 +46,6 @@ var cfg *rest.Config var k8sClient client.Client var testEnv *envtest.Environment var testServer *server.Server -var jsClient jetstream.JetStream func TestControllers(t *testing.T) { RegisterFailHandler(Fail) @@ -80,19 +79,14 @@ var _ = BeforeSuite(func() { err = jetstreamnatsiov1beta2.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) + //+kubebuilder:scaffold:scheme + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) Expect(err).NotTo(HaveOccurred()) Expect(k8sClient).NotTo(BeNil()) By("bootstrapping the test server") testServer = CreateTestServer() - jsClient, err = CreateJetStreamClient( - &NatsConfig{ServerURL: testServer.ClientURL()}, - true, - ) - Expect(err).NotTo(HaveOccurred()) - Expect(jsClient).NotTo(BeNil()) - }) var _ = AfterSuite(func() { From 52c5d9fa973ef7c96e3aaf6db02cb298695ef7c4 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 2 Dec 2024 17:52:37 +0100 Subject: [PATCH 02/35] Add logs to Reconcile functions --- internal/controller/account_controller.go | 2 +- internal/controller/consumer_controller.go | 10 ++++------ internal/controller/stream_controller.go | 1 + 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/controller/account_controller.go b/internal/controller/account_controller.go index a5686437..085c2d84 100644 --- a/internal/controller/account_controller.go +++ b/internal/controller/account_controller.go @@ -42,7 +42,7 @@ type AccountReconciler struct { // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := klog.FromContext(ctx) - log.Info("reconcile", "namespace", req.Namespace, "name", req.Name) + log.Info("reconcile %s", "namespace", req.Namespace, "name", req.Name) // TODO(user): your logic here diff --git a/internal/controller/consumer_controller.go b/internal/controller/consumer_controller.go index af1566bf..b91eaed0 100644 --- a/internal/controller/consumer_controller.go +++ b/internal/controller/consumer_controller.go @@ -19,13 +19,12 @@ package controller import ( "context" "github.com/nats-io/nats.go/jetstream" + "k8s.io/klog/v2" + jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/log" - - jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" ) // ConsumerReconciler reconciles a Consumer object @@ -42,9 +41,8 @@ type ConsumerReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile func (r *ConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - _ = log.FromContext(ctx) - - // TODO(user): your logic here + log := klog.FromContext(ctx) + log.Info("reconcile %s", "namespace", req.Namespace, "name", req.Name) return ctrl.Result{}, nil } diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 517ce675..ea41127d 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -43,6 +43,7 @@ type StreamReconciler struct { func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := klog.FromContext(ctx) log.Info("reconcile %s", "namespace", req.Namespace, "name", req.Name) + // TODO(user): your logic here return ctrl.Result{}, nil } From 25f3fa07015ffb54aad713a151902ff3bcf33404 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 2 Dec 2024 18:38:56 +0100 Subject: [PATCH 03/35] Add jsClient to test suit variables --- internal/controller/suite_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index cf57430f..2dac16e2 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -20,6 +20,7 @@ import ( "fmt" "github.com/nats-io/nats-server/v2/server" natsserver "github.com/nats-io/nats-server/v2/test" + "github.com/nats-io/nats.go/jetstream" "os" "path/filepath" "runtime" @@ -36,7 +37,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" - //+kubebuilder:scaffold:imports ) // These tests use Ginkgo (BDD-style Go testing framework). Refer to @@ -46,6 +46,7 @@ var cfg *rest.Config var k8sClient client.Client var testEnv *envtest.Environment var testServer *server.Server +var jsClient jetstream.JetStream func TestControllers(t *testing.T) { RegisterFailHandler(Fail) @@ -79,14 +80,19 @@ var _ = BeforeSuite(func() { err = jetstreamnatsiov1beta2.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) - //+kubebuilder:scaffold:scheme - k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) Expect(err).NotTo(HaveOccurred()) Expect(k8sClient).NotTo(BeNil()) By("bootstrapping the test server") testServer = CreateTestServer() + jsClient, err = CreateJetStreamClient( + &NatsConfig{ServerURL: testServer.ClientURL()}, + true, + ) + Expect(err).NotTo(HaveOccurred()) + Expect(jsClient).NotTo(BeNil()) + }) var _ = AfterSuite(func() { From 60a28f5a168e36ebada84c6da34a5839b765a596 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 2 Dec 2024 18:40:23 +0100 Subject: [PATCH 04/35] Remove format from log string --- internal/controller/account_controller.go | 2 +- internal/controller/consumer_controller.go | 2 +- internal/controller/stream_controller.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/controller/account_controller.go b/internal/controller/account_controller.go index 085c2d84..a5686437 100644 --- a/internal/controller/account_controller.go +++ b/internal/controller/account_controller.go @@ -42,7 +42,7 @@ type AccountReconciler struct { // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := klog.FromContext(ctx) - log.Info("reconcile %s", "namespace", req.Namespace, "name", req.Name) + log.Info("reconcile", "namespace", req.Namespace, "name", req.Name) // TODO(user): your logic here diff --git a/internal/controller/consumer_controller.go b/internal/controller/consumer_controller.go index b91eaed0..6dbe7ba8 100644 --- a/internal/controller/consumer_controller.go +++ b/internal/controller/consumer_controller.go @@ -42,7 +42,7 @@ type ConsumerReconciler struct { // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile func (r *ConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := klog.FromContext(ctx) - log.Info("reconcile %s", "namespace", req.Namespace, "name", req.Name) + log.Info("reconcile", "namespace", req.Namespace, "name", req.Name) return ctrl.Result{}, nil } diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index ea41127d..4469b7f0 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -42,7 +42,7 @@ type StreamReconciler struct { // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := klog.FromContext(ctx) - log.Info("reconcile %s", "namespace", req.Namespace, "name", req.Name) + log.Info("reconcile", "namespace", req.Namespace, "name", req.Name) // TODO(user): your logic here return ctrl.Result{}, nil From bffc435d17688eef9bdbfb9bfee9750fe56ae983 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 3 Dec 2024 13:50:22 +0100 Subject: [PATCH 05/35] Make upsertCondition public to be used in new controllers --- controllers/jetstream/consumer.go | 4 ++-- controllers/jetstream/controller.go | 2 +- controllers/jetstream/controller_test.go | 6 +++--- controllers/jetstream/stream.go | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/controllers/jetstream/consumer.go b/controllers/jetstream/consumer.go index 52f3f532..8dba6a89 100644 --- a/controllers/jetstream/consumer.go +++ b/controllers/jetstream/consumer.go @@ -462,7 +462,7 @@ func setConsumerOK(ctx context.Context, s *apis.Consumer, i typed.ConsumerInterf sc := s.DeepCopy() sc.Status.ObservedGeneration = s.Generation - sc.Status.Conditions = upsertCondition(sc.Status.Conditions, apis.Condition{ + sc.Status.Conditions = UpsertCondition(sc.Status.Conditions, apis.Condition{ Type: readyCondType, Status: k8sapi.ConditionTrue, LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), @@ -490,7 +490,7 @@ func setConsumerErrored(ctx context.Context, s *apis.Consumer, sif typed.Consume } sc := s.DeepCopy() - sc.Status.Conditions = upsertCondition(sc.Status.Conditions, apis.Condition{ + sc.Status.Conditions = UpsertCondition(sc.Status.Conditions, apis.Condition{ Type: readyCondType, Status: k8sapi.ConditionFalse, LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), diff --git a/controllers/jetstream/controller.go b/controllers/jetstream/controller.go index f4100ca2..8c009537 100644 --- a/controllers/jetstream/controller.go +++ b/controllers/jetstream/controller.go @@ -495,7 +495,7 @@ func processQueueNext(q workqueue.RateLimitingInterface, jmsClient jsmClientFunc q.Forget(item) } -func upsertCondition(cs []apis.Condition, next apis.Condition) []apis.Condition { +func UpsertCondition(cs []apis.Condition, next apis.Condition) []apis.Condition { for i := 0; i < len(cs); i++ { if cs[i].Type != next.Type { continue diff --git a/controllers/jetstream/controller_test.go b/controllers/jetstream/controller_test.go index 68f8f7fd..aae4d9b6 100644 --- a/controllers/jetstream/controller_test.go +++ b/controllers/jetstream/controller_test.go @@ -186,7 +186,7 @@ func TestUpsertCondition(t *testing.T) { var cs []apis.Condition - cs = upsertCondition(cs, apis.Condition{ + cs = UpsertCondition(cs, apis.Condition{ Type: readyCondType, Status: k8sapis.ConditionTrue, LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), @@ -202,7 +202,7 @@ func TestUpsertCondition(t *testing.T) { t.Fatalf("got=%s; want=%s", got, want) } - cs = upsertCondition(cs, apis.Condition{ + cs = UpsertCondition(cs, apis.Condition{ Type: readyCondType, Status: k8sapis.ConditionFalse, LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), @@ -218,7 +218,7 @@ func TestUpsertCondition(t *testing.T) { t.Fatalf("got=%s; want=%s", got, want) } - cs = upsertCondition(cs, apis.Condition{ + cs = UpsertCondition(cs, apis.Condition{ Type: "Foo", Status: k8sapis.ConditionTrue, LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), diff --git a/controllers/jetstream/stream.go b/controllers/jetstream/stream.go index 7dc2ec89..a340b88f 100644 --- a/controllers/jetstream/stream.go +++ b/controllers/jetstream/stream.go @@ -571,7 +571,7 @@ func setStreamErrored(ctx context.Context, s *apis.Stream, sif typed.StreamInter } sc := s.DeepCopy() - sc.Status.Conditions = upsertCondition(sc.Status.Conditions, apis.Condition{ + sc.Status.Conditions = UpsertCondition(sc.Status.Conditions, apis.Condition{ Type: readyCondType, Status: k8sapi.ConditionFalse, LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), @@ -600,7 +600,7 @@ func setStreamOK(ctx context.Context, s *apis.Stream, i typed.StreamInterface) ( sc := s.DeepCopy() sc.Status.ObservedGeneration = s.Generation - sc.Status.Conditions = upsertCondition(sc.Status.Conditions, apis.Condition{ + sc.Status.Conditions = UpsertCondition(sc.Status.Conditions, apis.Condition{ Type: readyCondType, Status: k8sapi.ConditionTrue, LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), From ac845087dbed5c3be9fb8b7483d9784210983624 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 4 Dec 2024 16:57:12 +0100 Subject: [PATCH 06/35] Implement basic cases for stream reconciliation See TODOs on what still needs to be implemented. --- internal/controller/stream_controller.go | 367 +++++++++++++++- internal/controller/stream_controller_test.go | 395 +++++++++++++++++- internal/controller/types.go | 6 + 3 files changed, 740 insertions(+), 28 deletions(-) create mode 100644 internal/controller/types.go diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 4469b7f0..705e678c 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -18,13 +18,20 @@ package controller import ( "context" + "fmt" + js "github.com/nats-io/nack/controllers/jetstream" + api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" "github.com/nats-io/nats.go/jetstream" + v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - - jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "time" ) // StreamReconciler reconciles a Stream object @@ -41,16 +48,364 @@ type StreamReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - log := klog.FromContext(ctx) - log.Info("reconcile", "namespace", req.Namespace, "name", req.Name) - // TODO(user): your logic here + log := klog.FromContext(ctx). + WithName("StreamReconciler"). + WithValues("namespace", req.Namespace, "name", req.Name) + + log.Info("reconciling") + + // TODO honour r.Config.ReadOnly and r.Config.Namespace + + // Fetch stream resource + stream := &api.Stream{} + if err := r.Get(ctx, req.NamespacedName, stream); err != nil { + if apierrors.IsNotFound(err) { + log.Info("stream resource not found. Ignoring since object must be deleted") + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) + } + + // Add finalizer + if !controllerutil.ContainsFinalizer(stream, streamFinalizer) { + log.Info("Adding stream finalizer") + if ok := controllerutil.AddFinalizer(stream, streamFinalizer); !ok { + log.Error(nil, "Failed to add finalizer to stream resource") + return ctrl.Result{Requeue: true}, nil + } + + if err := r.Update(ctx, stream); err != nil { + return ctrl.Result{}, fmt.Errorf("update stream resource to add finalizer: %w", err) + } + + // re-fetch stream + if err := r.Get(ctx, req.NamespacedName, stream); err != nil { + if apierrors.IsNotFound(err) { + log.Info("stream resource not found. Ignoring since object must be deleted") + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + } + } + + // Update Status to unknown when no status is set + if stream.Status.Conditions == nil || len(stream.Status.Conditions) == 0 { + updated, err := r.updateReadyCondition(ctx, stream, v1.ConditionUnknown, "Reconciling", "Starting reconciliation") + if err != nil { + return ctrl.Result{}, fmt.Errorf("set condition unknown: %w", err) + } + if updated { + // re-fetch stream + if err := r.Get(ctx, req.NamespacedName, stream); err != nil { + if apierrors.IsNotFound(err) { + log.Info("stream resource not found. Ignoring since object must be deleted") + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + } + } + + } + + // TODO Check if marked for deletion and delete + // TODO honour stream.Spec.PreventDelete and + + // Create or Update the stream based on the spec + + // Map spec to stream targetConfig + targetConfig, err := mapSpecToConfig(&stream.Spec) + if err != nil { + return ctrl.Result{}, fmt.Errorf("map spec to stream targetConfig: %w", err) + } + + // TODO Handle Account, Nkey, Servers and TLS from spec. + // TODO Get specific client when there is connection config in the spec. + var sm jetstream.StreamManager + sm = r.JetStream + + // TODO honour stream.Spec.PreventUpdate + _, err = sm.CreateOrUpdateStream(ctx, targetConfig) + if err != nil { + _, conditionErr := r.updateReadyCondition(ctx, stream, v1.ConditionFalse, "Errored", fmt.Sprintf("create or update stream: %s", err.Error())) + if conditionErr != nil { + log.Error(conditionErr, "failed to update ready condition to CreationFailed") + } + return ctrl.Result{}, fmt.Errorf("create or update stream: %w", err) + } + + // TODO update the generation in the status + + _, err = r.updateReadyCondition(ctx, stream, v1.ConditionTrue, "Reconciling", "Stream successfully created or updated") + if err != nil { + return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + } return ctrl.Result{}, nil } +// updateReadyCondition sets the status, reason and message. +// Then updates the resource. +// Returns true if the resource was updated by this call. +func (r *StreamReconciler) updateReadyCondition(ctx context.Context, stream *api.Stream, status v1.ConditionStatus, reason string, message string) (bool, error) { + + newCondition := api.Condition{ + Type: readyCondType, + Status: status, + Reason: reason, + Message: message, + LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), + } + + // Mapping to and from the metav1.Condition allows using meta.SetStatusCondition, + // which properly handles updating conditions. + // The mapping happens transparently to the caller. + + cond := mapConditionToMeta(newCondition) + var conditions []metav1.Condition + for _, condition := range stream.Status.Conditions { + conditions = append(conditions, mapConditionToMeta(condition)) + } + + changed := meta.SetStatusCondition(&conditions, cond) + if !changed { + return false, nil + } + + stream.Status.Conditions = []api.Condition{} + for _, condition := range conditions { + stream.Status.Conditions = append(stream.Status.Conditions, mapConditionFromMeta(condition)) + } + + stream.Status.Conditions = js.UpsertCondition(stream.Status.Conditions, newCondition) + + if err := r.Status().Update(ctx, stream); err != nil { + return false, fmt.Errorf("update stream status: %w", err) + } + return true, nil +} + +// mapConditionToMeta transforms a condition according the jetstream v1beta2 spec to a metav1 condition. +func mapConditionToMeta(condition api.Condition) metav1.Condition { + + status := metav1.ConditionUnknown + switch condition.Status { + case v1.ConditionTrue: + status = metav1.ConditionTrue + case v1.ConditionFalse: + status = metav1.ConditionFalse + } + + lastTransitionTime, err := time.Parse(time.RFC3339Nano, condition.LastTransitionTime) + if err != nil { + lastTransitionTime = time.Time{} + } + + return metav1.Condition{ + Type: condition.Type, + Status: status, + LastTransitionTime: metav1.NewTime(lastTransitionTime), + Reason: condition.Reason, + Message: condition.Message, + } +} + +// mapConditionFromMeta transforms a condition according the metav1 spec to a jetstream v1beta2 condition +func mapConditionFromMeta(condition metav1.Condition) api.Condition { + + status := v1.ConditionUnknown + switch condition.Status { + case metav1.ConditionTrue: + status = v1.ConditionTrue + case metav1.ConditionFalse: + status = v1.ConditionFalse + } + + lastTransitionTime := condition.LastTransitionTime.Format(time.RFC3339Nano) + + return api.Condition{ + Type: condition.Type, + Status: status, + LastTransitionTime: lastTransitionTime, + Reason: condition.Reason, + Message: condition.Message, + } +} + +// mapSpecToConfig creates a jetstream.StreamConfig matching the given stream resource spec +func mapSpecToConfig(spec *api.StreamSpec) (jetstream.StreamConfig, error) { + + // Set directly mapped fields + config := jetstream.StreamConfig{ + Name: spec.Name, + Description: spec.Description, + Subjects: spec.Subjects, + MaxConsumers: spec.MaxConsumers, + MaxMsgs: int64(spec.MaxMsgs), + MaxBytes: int64(spec.MaxBytes), + DiscardNewPerSubject: spec.DiscardPerSubject, + MaxMsgsPerSubject: int64(spec.MaxMsgsPerSubject), + MaxMsgSize: int32(spec.MaxMsgSize), + Replicas: spec.Replicas, + NoAck: spec.NoAck, + DenyDelete: spec.DenyDelete, + DenyPurge: spec.DenyPurge, + AllowRollup: spec.AllowRollup, + FirstSeq: spec.FirstSequence, + AllowDirect: spec.AllowDirect, + // Explicitly set not (yet) mapped fields + Sealed: false, + MirrorDirect: false, + ConsumerLimits: jetstream.StreamConsumerLimits{}, + } + + // Set not directly mapped fields + + // retention + if spec.Retention != "" { + // Wrap string in " to be properly unmarshalled as json string + err := config.Retention.UnmarshalJSON(asJsonString(spec.Retention)) + if err != nil { + return jetstream.StreamConfig{}, fmt.Errorf("invalid retention policy: %w", err) + } + } + + // discard + if spec.Discard != "" { + err := config.Discard.UnmarshalJSON(asJsonString(spec.Discard)) + if err != nil { + return jetstream.StreamConfig{}, fmt.Errorf("invalid retention policy: %w", err) + } + } + + // maxAge + if spec.MaxAge != "" { + d, err := time.ParseDuration(spec.MaxAge) + if err != nil { + return jetstream.StreamConfig{}, fmt.Errorf("parse max age: %w", err) + } + config.MaxAge = d + } + // storage + if spec.Storage != "" { + err := config.Storage.UnmarshalJSON(asJsonString(spec.Storage)) + if err != nil { + return jetstream.StreamConfig{}, fmt.Errorf("invalid storage: %w", err) + } + } + + // duplicates + if spec.DuplicateWindow != "" { + d, err := time.ParseDuration(spec.DuplicateWindow) + if err != nil { + return jetstream.StreamConfig{}, fmt.Errorf("parse duplicate window: %w", err) + } + config.Duplicates = d + } + + // placement + if spec.Placement != nil { + config.Placement = &jetstream.Placement{ + Cluster: spec.Placement.Cluster, + Tags: spec.Placement.Tags, + } + } + + // mirror + if spec.Mirror != nil { + ss, err := mapStreamSource(spec.Mirror) + if err != nil { + return jetstream.StreamConfig{}, fmt.Errorf("map mirror stream soruce: %w", err) + } + config.Mirror = ss + } + + // sources + if spec.Sources != nil { + config.Sources = []*jetstream.StreamSource{} + for _, source := range spec.Sources { + s, err := mapStreamSource(source) + if err != nil { + return jetstream.StreamConfig{}, fmt.Errorf("map stream soruce: %w", err) + } + config.Sources = append(config.Sources, s) + } + } + + // compression + if spec.Compression != "" { + err := config.Compression.UnmarshalJSON(asJsonString(spec.Compression)) + if err != nil { + return jetstream.StreamConfig{}, fmt.Errorf("invalid compression: %w", err) + } + } + + // subjectTransform + if spec.SubjectTransform != nil { + config.SubjectTransform = &jetstream.SubjectTransformConfig{ + Source: spec.SubjectTransform.Source, + Destination: spec.SubjectTransform.Dest, + } + } + + // rePublish + if spec.Republish != nil { + config.RePublish = &jetstream.RePublish{ + Source: spec.Republish.Source, + Destination: spec.Republish.Destination, + HeadersOnly: spec.Republish.HeadersOnly, + } + } + + // metadata + if spec.Metadata != nil { + config.Metadata = spec.Metadata + } + + return config, nil +} + +func mapStreamSource(ss *api.StreamSource) (*jetstream.StreamSource, error) { + jss := &jetstream.StreamSource{ + Name: ss.Name, + FilterSubject: ss.FilterSubject, + } + + if ss.OptStartSeq > 0 { + jss.OptStartSeq = uint64(ss.OptStartSeq) + } + if ss.OptStartTime != "" { + t, err := time.Parse(time.RFC3339, ss.OptStartTime) + if err != nil { + return nil, fmt.Errorf("parse opt start time: %w", err) + } + jss.OptStartTime = &t + } + + if ss.ExternalAPIPrefix != "" || ss.ExternalDeliverPrefix != "" { + jss.External = &jetstream.ExternalStream{ + APIPrefix: ss.ExternalAPIPrefix, + DeliverPrefix: ss.ExternalDeliverPrefix, + } + } + + for _, transform := range ss.SubjectTransforms { + jss.SubjectTransforms = append(jss.SubjectTransforms, jetstream.SubjectTransformConfig{ + Source: transform.Source, + Destination: transform.Dest, + }) + } + + return jss, nil +} + +// asJsonString returns the given string wrapped in " and converted to []byte. +func asJsonString(v string) []byte { + return []byte("\"" + v + "\"") +} + // SetupWithManager sets up the controller with the Manager. func (r *StreamReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). - For(&jetstreamnatsiov1beta2.Stream{}). + For(&api.Stream{}). Complete(r) } diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index ddd0e1c8..01700578 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -17,74 +17,425 @@ limitations under the License. package controller import ( + "github.com/nats-io/nats.go/jetstream" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/api/errors" + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "testing" + "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" + api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" ) var _ = Describe("Stream Controller", func() { - Context("When reconciling a resource", func() { - const resourceName = "test-resource" + When("reconciling a resource", func() { + const resourceName = "test-stream" typeNamespacedName := types.NamespacedName{ Name: resourceName, Namespace: "default", // TODO(user):Modify as needed } - stream := &jetstreamnatsiov1beta2.Stream{} + stream := &api.Stream{} BeforeEach(func(ctx SpecContext) { By("creating the custom resource for the Kind Stream") err := k8sClient.Get(ctx, typeNamespacedName, stream) - if err != nil && errors.IsNotFound(err) { - resource := &jetstreamnatsiov1beta2.Stream{ + if err != nil && k8serrors.IsNotFound(err) { + resource := &api.Stream{ ObjectMeta: metav1.ObjectMeta{ Name: resourceName, Namespace: "default", }, - Spec: jetstreamnatsiov1beta2.StreamSpec{ - Name: "test-stream", - Replicas: 1, - Discard: "old", - Storage: "file", - Retention: "workqueue", + Spec: api.StreamSpec{ + Name: "test-stream", + Replicas: 1, + Subjects: []string{"tests.*"}, + Description: "test stream", + Retention: "workqueue", + Discard: "old", + Storage: "file", }, - - // TODO(user): Specify other spec details if needed. } Expect(k8sClient.Create(ctx, resource)).To(Succeed()) } }) AfterEach(func(ctx SpecContext) { - // TODO(user): Cleanup logic after each test, like removing the resource instance. - resource := &jetstreamnatsiov1beta2.Stream{} + // Remove test stream resource + resource := &api.Stream{} err := k8sClient.Get(ctx, typeNamespacedName, resource) Expect(err).NotTo(HaveOccurred()) By("Cleanup the specific resource instance Stream") Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + + By("Deleting the stream") + err = jsClient.DeleteStream(ctx, resource.Spec.Name) + if err != nil { + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + } }) - It("should successfully reconcile the resource", func(ctx SpecContext) { + It("should successfully create the stream", func(ctx SpecContext) { By("Reconciling the created resource") controllerReconciler := &StreamReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Config: &Config{}, + JetStream: jsClient, } _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ NamespacedName: typeNamespacedName, }) Expect(err).NotTo(HaveOccurred()) - // TODO(user): Add more specific assertions depending on your controller's reconciliation logic. - // Example: If you expect a certain status condition after reconciliation, verify it here. + + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + // Fetch resource + err = k8sClient.Get(ctx, typeNamespacedName, stream) + Expect(err).NotTo(HaveOccurred()) + + By("Checking if the ready state was updated") + readyCondition := api.Condition{ + Type: readyCondType, + Status: v1.ConditionTrue, + Reason: "Reconciling", + Message: "Stream successfully created or updated", + } + gotCondition := stream.Status.Conditions[0] + // Unset LastTransitionTime to be equal for assertion + gotCondition.LastTransitionTime = "" + Expect(gotCondition).To(Equal(readyCondition)) + + By("Checking if the finalizer was set") + Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) + + By("Checking if the stream was created") + natsStream, err := jsClient.Stream(ctx, "test-stream") + Expect(err).NotTo(HaveOccurred()) + streamInfo, err := natsStream.Info(ctx) + Expect(err).NotTo(HaveOccurred()) + Expect(streamInfo.Config.Name).To(Equal("test-stream")) + Expect(streamInfo.Created).To(BeTemporally("~", time.Now(), time.Second)) + }) + + It("should successfully update the stream and update the status", func(ctx SpecContext) { + + By("Creating the stream with empty subjects and description") + _, err := jsClient.CreateStream(ctx, jetstream.StreamConfig{ + Name: "test-stream", + Replicas: 1, + Retention: jetstream.WorkQueuePolicy, + Discard: jetstream.DiscardOld, + Storage: jetstream.FileStorage, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Reconciling the created resource") + controllerReconciler := &StreamReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Config: &Config{}, + JetStream: jsClient, + } + + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + // Fetch resource + err = k8sClient.Get(ctx, typeNamespacedName, stream) + Expect(err).NotTo(HaveOccurred()) + + By("Checking if the ready state was updated") + readyCondition := api.Condition{ + Type: readyCondType, + Status: v1.ConditionTrue, + Reason: "Reconciling", + Message: "Stream successfully created or updated", + } + gotCondition := stream.Status.Conditions[0] + // Unset LastTransitionTime to be equal for assertion + gotCondition.LastTransitionTime = "" + Expect(gotCondition).To(Equal(readyCondition)) + + By("Checking if the stream was updated") + natsStream, err := jsClient.Stream(ctx, "test-stream") + Expect(err).NotTo(HaveOccurred()) + + streamInfo, err := natsStream.Info(ctx) + Expect(err).NotTo(HaveOccurred()) + Expect(streamInfo.Config.Description).To(Equal("test stream")) + Expect(streamInfo.Config.Subjects).To(Equal([]string{"tests.*"})) + }) + + It("should not fail on not existing resource", func(ctx SpecContext) { + By("Reconciling the created resource") + controllerReconciler := &StreamReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Config: &Config{}, + JetStream: jsClient, + } + + result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "fake", + Name: "not-existing", + }, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(ctrl.Result{})) + }) + + It("should set an error state, register finalizer and re-queue when the nats server is not available", func(ctx SpecContext) { + By("Reconciling the created resource") + + // Setup client for not running server + sv := CreateTestServer() + js, err := CreateJetStreamClient(&NatsConfig{ + ServerURL: sv.ClientURL(), + }, true) + Expect(err).NotTo(HaveOccurred()) + sv.Shutdown() + // Is there an easier way to create a failing js client? + + controllerReconciler := &StreamReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Config: &Config{}, + JetStream: js, + } + + result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(result).To(Equal(ctrl.Result{})) + Expect(err).To(HaveOccurred()) // Will be re-queued with back-off + + // Fetch resource + err = k8sClient.Get(ctx, typeNamespacedName, stream) + Expect(err).NotTo(HaveOccurred()) + + By("Checking if the status was updated") + Expect(stream.Status.Conditions).To(HaveLen(1)) + + cond := stream.Status.Conditions[0] + Expect(cond.Type).To(Equal(readyCondType)) + Expect(cond.Status).To(Equal(v1.ConditionFalse)) + Expect(cond.Reason).To(Equal("Errored")) + Expect(cond.Message).To(Equal("create or update stream: context deadline exceeded")) + Expect(cond.LastTransitionTime).NotTo(BeEmpty()) + + By("Checking if the finalizer was set") + Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) + }) + + PIt("should delete stream marked for deletion", func(ctx SpecContext) { + }) }) }) + +func Test_mapSpecToConfig(t *testing.T) { + + date := time.Date(2024, 12, 03, 16, 55, 5, 0, time.UTC) + dateString := date.Format(time.RFC3339) + + tests := []struct { + name string + spec *api.StreamSpec + want jetstream.StreamConfig + wantErr bool + }{ + { + name: "emtpy spec", + spec: &api.StreamSpec{}, + want: jetstream.StreamConfig{}, + wantErr: false, + }, + { + name: "full spec", + spec: &api.StreamSpec{ + Account: "", + AllowDirect: true, + AllowRollup: true, + Creds: "", + DenyDelete: true, + DenyPurge: true, + Description: "stream description", + DiscardPerSubject: true, + PreventDelete: false, + PreventUpdate: false, + Discard: "new", + DuplicateWindow: "5s", + MaxAge: "30s", + MaxBytes: -1, + MaxConsumers: -1, + MaxMsgs: -1, + MaxMsgSize: -1, + MaxMsgsPerSubject: 10, + Mirror: &api.StreamSource{ + Name: "mirror", + OptStartSeq: 5, + OptStartTime: dateString, + FilterSubject: "orders", + ExternalAPIPrefix: "api", + ExternalDeliverPrefix: "deliver", + SubjectTransforms: []*api.SubjectTransform{{ + Source: "transform-source", + Dest: "transform-dest", + }}, + }, + Name: "stream-name", + Nkey: "", + NoAck: true, + Placement: &api.StreamPlacement{ + Cluster: "test-cluster", + Tags: []string{"tag"}, + }, + Replicas: 3, + Republish: &api.RePublish{ + Source: "re-publish-source", + Destination: "re-publish-dest", + HeadersOnly: true, + }, + SubjectTransform: &api.SubjectTransform{ + Source: "transform-source", + Dest: "transform-dest", + }, + FirstSequence: 42, + Compression: "s2", + Metadata: map[string]string{ + "meta": "data", + }, + Retention: "interest", + Servers: nil, + Sources: []*api.StreamSource{{ + Name: "source", + OptStartSeq: 5, + OptStartTime: dateString, + FilterSubject: "orders", + ExternalAPIPrefix: "api", + ExternalDeliverPrefix: "deliver", + SubjectTransforms: []*api.SubjectTransform{{ + Source: "transform-source", + Dest: "transform-dest", + }}, + }}, + Storage: "file", + Subjects: []string{"orders.*"}, + TLS: api.TLS{}, + }, + want: jetstream.StreamConfig{ + Name: "stream-name", + Description: "stream description", + Subjects: []string{"orders.*"}, + Retention: jetstream.InterestPolicy, + MaxConsumers: -1, + MaxMsgs: -1, + MaxBytes: -1, + Discard: jetstream.DiscardNew, + DiscardNewPerSubject: true, + MaxAge: time.Second * 30, + MaxMsgsPerSubject: 10, + MaxMsgSize: -1, + Storage: jetstream.FileStorage, + Replicas: 3, + NoAck: true, + Duplicates: time.Second * 5, + Placement: &jetstream.Placement{ + Cluster: "test-cluster", + Tags: []string{"tag"}, + }, + Mirror: &jetstream.StreamSource{ + Name: "mirror", + OptStartSeq: 5, + OptStartTime: &date, + FilterSubject: "orders", + SubjectTransforms: []jetstream.SubjectTransformConfig{{ + Source: "transform-source", + Destination: "transform-dest", + }}, + External: &jetstream.ExternalStream{ + APIPrefix: "api", + DeliverPrefix: "deliver", + }, + Domain: "", + }, + Sources: []*jetstream.StreamSource{{ + Name: "source", + OptStartSeq: 5, + OptStartTime: &date, + FilterSubject: "orders", + SubjectTransforms: []jetstream.SubjectTransformConfig{{ + Source: "transform-source", + Destination: "transform-dest", + }}, + External: &jetstream.ExternalStream{ + APIPrefix: "api", + DeliverPrefix: "deliver", + }, + Domain: "", + }}, + Sealed: false, + DenyDelete: true, + DenyPurge: true, + AllowRollup: true, + Compression: jetstream.S2Compression, + FirstSeq: 42, + SubjectTransform: &jetstream.SubjectTransformConfig{ + Source: "transform-source", + Destination: "transform-dest", + }, + RePublish: &jetstream.RePublish{ + Source: "re-publish-source", + Destination: "re-publish-dest", + HeadersOnly: true, + }, + AllowDirect: true, + MirrorDirect: false, + ConsumerLimits: jetstream.StreamConsumerLimits{}, + Metadata: map[string]string{ + "meta": "data", + }, + Template: "", + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert := assert.New(t) + got, err := mapSpecToConfig(tt.spec) + if (err != nil) != tt.wantErr { + t.Errorf("mapSpecToConfig() error = %v, wantErr %v", err, tt.wantErr) + return + } + + // Compare nested structs + assert.EqualValues(tt.want, got) + //if !reflect.DeepEqual(got, tt.want) { + // t.Errorf("mapSpecToConfig() \ngot = %v\nwant = %v", got, tt.want) + //} + }) + } +} diff --git a/internal/controller/types.go b/internal/controller/types.go new file mode 100644 index 00000000..f48d3c4b --- /dev/null +++ b/internal/controller/types.go @@ -0,0 +1,6 @@ +package controller + +const ( + readyCondType = "Ready" + streamFinalizer = "stream.nats.io/finalizer" +) From d041edc3cf44aeb3eff467a2a72d64aa09deffe7 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 4 Dec 2024 20:53:16 +0100 Subject: [PATCH 07/35] refactor to use shared base controller --- internal/controller/account_controller.go | 8 +- .../controller/account_controller_test.go | 3 +- internal/controller/client.go | 17 +- internal/controller/consumer_controller.go | 8 +- .../controller/consumer_controller_test.go | 3 +- internal/controller/jetstream_controller.go | 123 +++++++++++++++ .../controller/jetstream_controller_test.go | 138 ++++++++++++++++ internal/controller/register.go | 20 +-- internal/controller/stream_controller.go | 149 ++++-------------- internal/controller/stream_controller_test.go | 26 +-- internal/controller/suite_test.go | 9 +- 11 files changed, 325 insertions(+), 179 deletions(-) create mode 100644 internal/controller/jetstream_controller.go create mode 100644 internal/controller/jetstream_controller_test.go diff --git a/internal/controller/account_controller.go b/internal/controller/account_controller.go index a5686437..72418667 100644 --- a/internal/controller/account_controller.go +++ b/internal/controller/account_controller.go @@ -18,21 +18,15 @@ package controller import ( "context" - "github.com/nats-io/nats.go/jetstream" "k8s.io/klog/v2" jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" - "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" ) // AccountReconciler reconciles a Account object type AccountReconciler struct { - client.Client - Scheme *runtime.Scheme - Config *Config - JetStream jetstream.JetStream + JetStreamController } // Reconcile is part of the main kubernetes reconciliation loop which aims to diff --git a/internal/controller/account_controller_test.go b/internal/controller/account_controller_test.go index aa897363..46debade 100644 --- a/internal/controller/account_controller_test.go +++ b/internal/controller/account_controller_test.go @@ -69,8 +69,7 @@ var _ = Describe("Account Controller", func() { It("should successfully reconcile the resource", func() { By("Reconciling the created resource") controllerReconciler := &AccountReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), + baseController, } _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ diff --git a/internal/controller/client.go b/internal/controller/client.go index 8c81994e..1423c9d8 100644 --- a/internal/controller/client.go +++ b/internal/controller/client.go @@ -54,11 +54,18 @@ func (o *NatsConfig) buildOptions() ([]nats.Option, error) { return opts, nil } -func CreateJetStreamClient(cfg *NatsConfig, pedantic bool) (jetstream.JetStream, error) { +type Closable interface { + Close() +} + +// CreateJetStreamClient creates new Jetstream client with a connection based on the given NatsConfig. +// Returns a jetstream.Jetstream client and the Closable of the underlying connection. +// Close should be called when the client is no longer used. +func CreateJetStreamClient(cfg *NatsConfig, pedantic bool) (jetstream.JetStream, Closable, error) { opts, err := cfg.buildOptions() if err != nil { - return nil, fmt.Errorf("nats options: %w", err) + return nil, nil, fmt.Errorf("nats options: %w", err) } // Set pedantic option @@ -74,12 +81,12 @@ func CreateJetStreamClient(cfg *NatsConfig, pedantic bool) (jetstream.JetStream, nc, err := nats.Connect(cfg.ServerURL, opts...) if err != nil { - return nil, fmt.Errorf("nats connect: %w", err) + return nil, nil, fmt.Errorf("nats connect: %w", err) } js, err := jetstream.New(nc) if err != nil { - return nil, fmt.Errorf("new jetstream: %w", err) + return nil, nil, fmt.Errorf("new jetstream: %w", err) } - return js, nil + return js, nc, nil } diff --git a/internal/controller/consumer_controller.go b/internal/controller/consumer_controller.go index 6dbe7ba8..22905065 100644 --- a/internal/controller/consumer_controller.go +++ b/internal/controller/consumer_controller.go @@ -18,21 +18,15 @@ package controller import ( "context" - "github.com/nats-io/nats.go/jetstream" "k8s.io/klog/v2" jetstreamnatsiov1beta2 "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" - "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" ) // ConsumerReconciler reconciles a Consumer object type ConsumerReconciler struct { - client.Client - Scheme *runtime.Scheme - Config *Config - JetStream jetstream.JetStream + JetStreamController } // Reconcile is part of the main kubernetes reconciliation loop which aims to diff --git a/internal/controller/consumer_controller_test.go b/internal/controller/consumer_controller_test.go index 1ffce13e..6ce92bcc 100644 --- a/internal/controller/consumer_controller_test.go +++ b/internal/controller/consumer_controller_test.go @@ -75,8 +75,7 @@ var _ = Describe("Consumer Controller", func() { It("should successfully reconcile the resource", func() { By("Reconciling the created resource") controllerReconciler := &ConsumerReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), + baseController, } _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go new file mode 100644 index 00000000..76f0e063 --- /dev/null +++ b/internal/controller/jetstream_controller.go @@ -0,0 +1,123 @@ +package controller + +import ( + "errors" + "fmt" + js "github.com/nats-io/nack/controllers/jetstream" + api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" + "github.com/nats-io/nats.go/jetstream" + v1 "k8s.io/api/core/v1" + + "sigs.k8s.io/controller-runtime/pkg/client" + "time" +) + +type connectionOptions struct { + Account string `json:"account"` + Creds string `json:"creds"` + Nkey string `json:"nkey"` + Servers []string `json:"servers"` + TLS api.TLS `json:"tls"` +} + +func (o connectionOptions) empty() bool { + return o.Account == "" && + o.Creds == "" && + o.Nkey == "" && + len(o.Servers) == 0 && + o.TLS.ClientCert == "" && + o.TLS.ClientKey == "" && + len(o.TLS.RootCAs) == 0 +} + +type JetStreamController interface { + client.Client + + // ReadOnly returns true when this controller + readOnly() bool + Namespace() string + WithJetStreamClient(*connectionOptions, func(js jetstream.JetStream) error) error +} + +func NewJSController(k8sClient client.Client, natsConfig *NatsConfig, controllerConfig *Config) (JetStreamController, error) { + + var baseClient jetstream.JetStream + if natsConfig.ServerURL != "" { + var err error + baseClient, _, err = CreateJetStreamClient(natsConfig, true) + if err != nil { + return nil, fmt.Errorf("create base js k8sClient: %w", err) + } + } + + return &jsController{ + Client: k8sClient, + config: natsConfig, + controllerConfig: controllerConfig, + baseClient: baseClient, + }, nil +} + +type jsController struct { + client.Client + config *NatsConfig + controllerConfig *Config + baseClient jetstream.JetStream +} + +func (c *jsController) readOnly() bool { + return c.controllerConfig.ReadOnly +} + +func (c *jsController) Namespace() string { + return c.controllerConfig.Namespace +} + +func (c *jsController) WithJetStreamClient(opts *connectionOptions, op func(js jetstream.JetStream) error) error { + // TODO Handle Account, Nkey, Servers and TLS from spec. + // TODO Get specific client when there is connection config in the spec. + + if c.baseClient == nil { + return errors.New("no client available") + } + + if opts == nil || opts.empty() { + return op(c.baseClient) + } + + // TODO Build single use client + return errors.New("Not Implemented") +} + +// updateReadyCondition returns the conditions with an added or updated ready condition +func updateReadyCondition(conditions []api.Condition, status v1.ConditionStatus, reason string, message string) []api.Condition { + + var currentStatus v1.ConditionStatus + var lastTransitionTime string + for _, condition := range conditions { + if condition.Type == readyCondType { + currentStatus = condition.Status + lastTransitionTime = condition.LastTransitionTime + break + } + } + + // Set transition time to now, when no previous ready condition or the status changed + if lastTransitionTime == "" || currentStatus != status { + lastTransitionTime = time.Now().UTC().Format(time.RFC3339Nano) + } + + newCondition := api.Condition{ + Type: readyCondType, + Status: status, + Reason: reason, + Message: message, + LastTransitionTime: lastTransitionTime, + } + if conditions == nil { + return []api.Condition{newCondition} + } else { + return js.UpsertCondition(conditions, newCondition) + } + +} diff --git a/internal/controller/jetstream_controller_test.go b/internal/controller/jetstream_controller_test.go new file mode 100644 index 00000000..d7176d04 --- /dev/null +++ b/internal/controller/jetstream_controller_test.go @@ -0,0 +1,138 @@ +package controller + +import ( + api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + "testing" + "time" +) + +func Test_updateReadyCondition(t *testing.T) { + + pastTransition := time.Now().UTC().Add(-time.Hour).Format(time.RFC3339Nano) + updatedTransition := "now" + + otherCondition := api.Condition{ + Type: "other", + Status: v1.ConditionFalse, + Reason: "Reason", + Message: "Message", + LastTransitionTime: pastTransition, + } + + type args struct { + conditions []api.Condition + status v1.ConditionStatus + reason string + message string + } + tests := []struct { + name string + args args + want []api.Condition + }{ + { + name: "new ready condition", + args: args{ + conditions: nil, + status: v1.ConditionTrue, + reason: "Test", + message: "Test Message", + }, + want: []api.Condition{ + { + Type: readyCondType, + Status: v1.ConditionTrue, + Reason: "Test", + Message: "Test Message", + LastTransitionTime: updatedTransition, + }, + }, + }, + { + name: "update ready condition", + args: args{ + conditions: []api.Condition{ + otherCondition, + { + Type: readyCondType, + Status: v1.ConditionFalse, + Reason: "Test", + Message: "Test Message", + LastTransitionTime: pastTransition, + }, + }, + status: v1.ConditionTrue, + reason: "New Reason", + message: "New Message", + }, + want: []api.Condition{ + otherCondition, + { + Type: readyCondType, + Status: v1.ConditionTrue, + Reason: "New Reason", + Message: "New Message", + LastTransitionTime: updatedTransition, + }, + }, + }, + { + name: "should not update transition time when status is not changed", + args: args{ + conditions: []api.Condition{ + otherCondition, + { + Type: readyCondType, + Status: v1.ConditionTrue, + Reason: "Test", + Message: "Test Message", + LastTransitionTime: pastTransition, + }, + }, + status: v1.ConditionTrue, + reason: "New Reason", + message: "New Message", + }, + want: []api.Condition{ + otherCondition, + { + Type: readyCondType, + Status: v1.ConditionTrue, + Reason: "New Reason", + Message: "New Message", + LastTransitionTime: pastTransition, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert := assert.New(t) + + got := updateReadyCondition(tt.args.conditions, tt.args.status, tt.args.reason, tt.args.message) + + assert.Len(got, len(tt.want)) + for i, want := range tt.want { + actual := got[i] + + assert.Equal(actual.Type, want.Type) + assert.Equal(actual.Status, want.Status) + assert.Equal(actual.Reason, want.Reason) + assert.Equal(actual.Message, want.Message) + + // Assert transition time was updated + if want.LastTransitionTime == updatedTransition { + actualTransitionTime, err := time.Parse(time.RFC3339Nano, actual.LastTransitionTime) + assert.NoError(err) + assert.WithinDuration(actualTransitionTime, time.Now(), 5*time.Second) + } + // Assert transition time was not updated + if want.LastTransitionTime == pastTransition { + assert.Equal(pastTransition, actual.LastTransitionTime) + } + } + }) + } +} diff --git a/internal/controller/register.go b/internal/controller/register.go index 611ff157..5fa21b5a 100644 --- a/internal/controller/register.go +++ b/internal/controller/register.go @@ -20,36 +20,26 @@ type Config struct { // controllerCfg defines behaviour of the registered controllers. func RegisterAll(mgr ctrl.Manager, clientConfig *NatsConfig, config *Config) error { - // Controllers need access to a nats client in pedantic mode - js, err := CreateJetStreamClient(clientConfig, true) + baseController, err := NewJSController(mgr.GetClient(), clientConfig, config) if err != nil { - return fmt.Errorf("create jetstream client: %w", err) + return fmt.Errorf("create base jetstream controller: %w", err) } // Register controllers if err := (&AccountReconciler{ - Client: mgr.GetClient(), - Config: config, - Scheme: mgr.GetScheme(), - JetStream: js, + baseController, }).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create account controller: %w", err) } if err := (&ConsumerReconciler{ - Client: mgr.GetClient(), - Config: config, - Scheme: mgr.GetScheme(), - JetStream: js, + baseController, }).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create consumer controller: %w", err) } if err := (&StreamReconciler{ - Client: mgr.GetClient(), - Config: config, - Scheme: mgr.GetScheme(), - JetStream: js, + JetStreamController: baseController, }).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create stream controller: %w", err) } diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 705e678c..8fca07a2 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -19,27 +19,19 @@ package controller import ( "context" "fmt" - js "github.com/nats-io/nack/controllers/jetstream" api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" "github.com/nats-io/nats.go/jetstream" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "time" ) // StreamReconciler reconciles a Stream object type StreamReconciler struct { - client.Client - Scheme *runtime.Scheme - Config *Config - JetStream jetstream.JetStream + JetStreamController } // Reconcile is part of the main kubernetes reconciliation loop which aims to @@ -90,21 +82,29 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Update Status to unknown when no status is set if stream.Status.Conditions == nil || len(stream.Status.Conditions) == 0 { - updated, err := r.updateReadyCondition(ctx, stream, v1.ConditionUnknown, "Reconciling", "Starting reconciliation") + stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionUnknown, "Reconciling", "Starting reconciliation") + err := r.Status().Update(ctx, stream) if err != nil { return ctrl.Result{}, fmt.Errorf("set condition unknown: %w", err) } - if updated { - // re-fetch stream - if err := r.Get(ctx, req.NamespacedName, stream); err != nil { - if apierrors.IsNotFound(err) { - log.Info("stream resource not found. Ignoring since object must be deleted") - return ctrl.Result{}, nil - } - return ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + + // re-fetch stream + if err := r.Get(ctx, req.NamespacedName, stream); err != nil { + if apierrors.IsNotFound(err) { + log.Info("stream resource not found. Ignoring since object must be deleted") + return ctrl.Result{}, nil } + return ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) } + } + // Connection options specific to this spec + specConnectionOptions := &connectionOptions{ + Account: stream.Spec.Account, + Creds: stream.Spec.Creds, + Nkey: stream.Spec.Nkey, + Servers: stream.Spec.Servers, + TLS: stream.Spec.TLS, } // TODO Check if marked for deletion and delete @@ -118,24 +118,29 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, fmt.Errorf("map spec to stream targetConfig: %w", err) } - // TODO Handle Account, Nkey, Servers and TLS from spec. - // TODO Get specific client when there is connection config in the spec. - var sm jetstream.StreamManager - sm = r.JetStream - // TODO honour stream.Spec.PreventUpdate - _, err = sm.CreateOrUpdateStream(ctx, targetConfig) + err = r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { + _, err = js.CreateOrUpdateStream(ctx, targetConfig) + return err + }) if err != nil { - _, conditionErr := r.updateReadyCondition(ctx, stream, v1.ConditionFalse, "Errored", fmt.Sprintf("create or update stream: %s", err.Error())) - if conditionErr != nil { - log.Error(conditionErr, "failed to update ready condition to CreationFailed") + msg := fmt.Sprintf("create or update stream: %s", err.Error()) + stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionFalse, "Errored", msg) + if err := r.Status().Update(ctx, stream); err != nil { + log.Error(err, "failed to update ready condition to Errored") } return ctrl.Result{}, fmt.Errorf("create or update stream: %w", err) } // TODO update the generation in the status - _, err = r.updateReadyCondition(ctx, stream, v1.ConditionTrue, "Reconciling", "Stream successfully created or updated") + stream.Status.Conditions = updateReadyCondition( + stream.Status.Conditions, + v1.ConditionTrue, + "Reconciling", + "Stream successfully created or updated", + ) + err = r.Status().Update(ctx, stream) if err != nil { return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) } @@ -143,94 +148,6 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } -// updateReadyCondition sets the status, reason and message. -// Then updates the resource. -// Returns true if the resource was updated by this call. -func (r *StreamReconciler) updateReadyCondition(ctx context.Context, stream *api.Stream, status v1.ConditionStatus, reason string, message string) (bool, error) { - - newCondition := api.Condition{ - Type: readyCondType, - Status: status, - Reason: reason, - Message: message, - LastTransitionTime: time.Now().UTC().Format(time.RFC3339Nano), - } - - // Mapping to and from the metav1.Condition allows using meta.SetStatusCondition, - // which properly handles updating conditions. - // The mapping happens transparently to the caller. - - cond := mapConditionToMeta(newCondition) - var conditions []metav1.Condition - for _, condition := range stream.Status.Conditions { - conditions = append(conditions, mapConditionToMeta(condition)) - } - - changed := meta.SetStatusCondition(&conditions, cond) - if !changed { - return false, nil - } - - stream.Status.Conditions = []api.Condition{} - for _, condition := range conditions { - stream.Status.Conditions = append(stream.Status.Conditions, mapConditionFromMeta(condition)) - } - - stream.Status.Conditions = js.UpsertCondition(stream.Status.Conditions, newCondition) - - if err := r.Status().Update(ctx, stream); err != nil { - return false, fmt.Errorf("update stream status: %w", err) - } - return true, nil -} - -// mapConditionToMeta transforms a condition according the jetstream v1beta2 spec to a metav1 condition. -func mapConditionToMeta(condition api.Condition) metav1.Condition { - - status := metav1.ConditionUnknown - switch condition.Status { - case v1.ConditionTrue: - status = metav1.ConditionTrue - case v1.ConditionFalse: - status = metav1.ConditionFalse - } - - lastTransitionTime, err := time.Parse(time.RFC3339Nano, condition.LastTransitionTime) - if err != nil { - lastTransitionTime = time.Time{} - } - - return metav1.Condition{ - Type: condition.Type, - Status: status, - LastTransitionTime: metav1.NewTime(lastTransitionTime), - Reason: condition.Reason, - Message: condition.Message, - } -} - -// mapConditionFromMeta transforms a condition according the metav1 spec to a jetstream v1beta2 condition -func mapConditionFromMeta(condition metav1.Condition) api.Condition { - - status := v1.ConditionUnknown - switch condition.Status { - case metav1.ConditionTrue: - status = v1.ConditionTrue - case metav1.ConditionFalse: - status = v1.ConditionFalse - } - - lastTransitionTime := condition.LastTransitionTime.Format(time.RFC3339Nano) - - return api.Condition{ - Type: condition.Type, - Status: status, - LastTransitionTime: lastTransitionTime, - Reason: condition.Reason, - Message: condition.Message, - } -} - // mapSpecToConfig creates a jetstream.StreamConfig matching the given stream resource spec func mapSpecToConfig(spec *api.StreamSpec) (jetstream.StreamConfig, error) { diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 01700578..1c943671 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -87,10 +87,7 @@ var _ = Describe("Stream Controller", func() { It("should successfully create the stream", func(ctx SpecContext) { By("Reconciling the created resource") controllerReconciler := &StreamReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Config: &Config{}, - JetStream: jsClient, + baseController, } _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ @@ -145,10 +142,7 @@ var _ = Describe("Stream Controller", func() { By("Reconciling the created resource") controllerReconciler := &StreamReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Config: &Config{}, - JetStream: jsClient, + baseController, } _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ @@ -190,10 +184,7 @@ var _ = Describe("Stream Controller", func() { It("should not fail on not existing resource", func(ctx SpecContext) { By("Reconciling the created resource") controllerReconciler := &StreamReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Config: &Config{}, - JetStream: jsClient, + baseController, } result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ @@ -211,18 +202,13 @@ var _ = Describe("Stream Controller", func() { // Setup client for not running server sv := CreateTestServer() - js, err := CreateJetStreamClient(&NatsConfig{ - ServerURL: sv.ClientURL(), - }, true) + // Is there an easier way to create a failing js client? + controller, err := NewJSController(k8sClient, &NatsConfig{ServerURL: sv.ClientURL()}, &Config{}) Expect(err).NotTo(HaveOccurred()) sv.Shutdown() - // Is there an easier way to create a failing js client? controllerReconciler := &StreamReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Config: &Config{}, - JetStream: js, + controller, } result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 2dac16e2..86f6906d 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -47,6 +47,7 @@ var k8sClient client.Client var testEnv *envtest.Environment var testServer *server.Server var jsClient jetstream.JetStream +var baseController JetStreamController func TestControllers(t *testing.T) { RegisterFailHandler(Fail) @@ -86,13 +87,11 @@ var _ = BeforeSuite(func() { By("bootstrapping the test server") testServer = CreateTestServer() - jsClient, err = CreateJetStreamClient( - &NatsConfig{ServerURL: testServer.ClientURL()}, - true, - ) Expect(err).NotTo(HaveOccurred()) - Expect(jsClient).NotTo(BeNil()) + testNatsConfig := &NatsConfig{ServerURL: testServer.ClientURL()} + baseController, err = NewJSController(k8sClient, testNatsConfig, &Config{}) + jsClient, _, err = CreateJetStreamClient(testNatsConfig, true) }) var _ = AfterSuite(func() { From 4d57584e187e14778672b017b046711b379aea86 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Thu, 5 Dec 2024 15:24:15 +0100 Subject: [PATCH 08/35] Support jetstream connection options in stream spec --- cmd/jetstream-controller/main.go | 2 +- internal/controller/client.go | 6 +-- internal/controller/jetstream_controller.go | 42 ++++++++++++++++--- internal/controller/stream_controller_test.go | 39 +++++++++++++++++ 4 files changed, 80 insertions(+), 9 deletions(-) diff --git a/cmd/jetstream-controller/main.go b/cmd/jetstream-controller/main.go index 1ec60c3d..23855c56 100644 --- a/cmd/jetstream-controller/main.go +++ b/cmd/jetstream-controller/main.go @@ -94,7 +94,7 @@ func run() error { Credentials: *creds, NKey: *nkey, ServerURL: *server, - CA: *ca, + CAs: []string{*ca}, Certificate: *cert, Key: *key, TLSFirst: *tlsfirst, diff --git a/internal/controller/client.go b/internal/controller/client.go index 1423c9d8..c20144b3 100644 --- a/internal/controller/client.go +++ b/internal/controller/client.go @@ -12,7 +12,7 @@ type NatsConfig struct { Credentials string NKey string ServerURL string - CA string + CAs []string Certificate string Key string TLSFirst bool @@ -46,8 +46,8 @@ func (o *NatsConfig) buildOptions() ([]nats.Option, error) { opts = append(opts, nats.ClientCert(o.Certificate, o.Key)) } - if o.CA != "" { - opts = append(opts, nats.RootCAs(o.CA)) + if o.CAs != nil && len(o.CAs) > 0 { + opts = append(opts, nats.RootCAs(o.CAs...)) } } diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go index 76f0e063..aac84feb 100644 --- a/internal/controller/jetstream_controller.go +++ b/internal/controller/jetstream_controller.go @@ -7,6 +7,7 @@ import ( api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" "github.com/nats-io/nats.go/jetstream" v1 "k8s.io/api/core/v1" + "strings" "sigs.k8s.io/controller-runtime/pkg/client" "time" @@ -74,19 +75,50 @@ func (c *jsController) Namespace() string { } func (c *jsController) WithJetStreamClient(opts *connectionOptions, op func(js jetstream.JetStream) error) error { - // TODO Handle Account, Nkey, Servers and TLS from spec. - // TODO Get specific client when there is connection config in the spec. - if c.baseClient == nil { return errors.New("no client available") } + // Use base client when no config is given if opts == nil || opts.empty() { return op(c.baseClient) } - // TODO Build single use client - return errors.New("Not Implemented") + // Build single use client + serverUrl := strings.Join(opts.Servers, ",") + + // TODO needs review: + // Here the config from spec takes precedence over base config on a value by value basis. + // This could lead to issues when an NKey is set in the spec config and Credentials are set in the base config. + // In NatsConfig.buildOptions, Credentials take precedence over the Nkey, + // which would lead to the nkey from spec to be ignored. + cfg := &NatsConfig{ + CRDConnect: false, + ClientName: c.config.ClientName, + Credentials: or(opts.Creds, c.config.Credentials), + NKey: or(opts.Nkey, c.config.NKey), + ServerURL: or(serverUrl, c.config.ServerURL), + CAs: *or(&opts.TLS.RootCAs, &c.config.CAs), + Certificate: or(opts.TLS.ClientCert, c.config.Certificate), + Key: or(opts.TLS.ClientKey, c.config.Key), + TLSFirst: false, + } + + client, closer, err := CreateJetStreamClient(cfg, true) + if err != nil { + return fmt.Errorf("create jetstream client: %w", err) + } + defer closer.Close() + + return op(client) +} + +// or returns the value if it is not the null value. Otherwise, the fallback value is returned +func or[T comparable](v T, fallback T) T { + if v == *new(T) { + return fallback + } + return v } // updateReadyCondition returns the conditions with an added or updated ready condition diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 1c943671..36fe98ca 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -65,6 +65,9 @@ var _ = Describe("Stream Controller", func() { }, } Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + // Re-fetch stream + Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) + } }) @@ -238,6 +241,42 @@ var _ = Describe("Stream Controller", func() { PIt("should delete stream marked for deletion", func(ctx SpecContext) { }) + + It("should update stream on different server as specified in spec", func(ctx SpecContext) { + By("Setting up the alternative server") + // Setup altClient for alternate server + altServer := CreateTestServer() + defer altServer.Shutdown() + + By("Setting the server in the stream spec") + stream.Spec.Servers = []string{altServer.ClientURL()} + Expect(k8sClient.Update(ctx, stream)).To(Succeed()) + + By("Reconciling the resource") + controllerReconciler := &StreamReconciler{ + baseController, + } + + result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(result).To(Equal(ctrl.Result{})) + Expect(err).NotTo(HaveOccurred()) + + By("Checking if the stream was created on the alternative server") + altClient, closer, err := CreateJetStreamClient(&NatsConfig{ServerURL: altServer.ClientURL()}, true) + defer closer.Close() + Expect(err).NotTo(HaveOccurred()) + + got, err := altClient.Stream(ctx, stream.Spec.Name) + Expect(err).NotTo(HaveOccurred()) + Expect(got.CachedInfo().Created).To(BeTemporally("~", time.Now(), time.Second)) + + By("Checking that the stream was NOT created on the alternative server") + got, err = jsClient.Stream(ctx, stream.Spec.Name) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + + }) }) }) From 814e243c9dd534ae7e274125cf85869df6c5bcf1 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Thu, 5 Dec 2024 16:48:27 +0100 Subject: [PATCH 09/35] implement stream deletion --- internal/controller/stream_controller.go | 52 ++++++++++++- internal/controller/stream_controller_test.go | 78 +++++++++++++------ 2 files changed, 104 insertions(+), 26 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 8fca07a2..b1a00621 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -18,6 +18,7 @@ package controller import ( "context" + "errors" "fmt" api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" "github.com/nats-io/nats.go/jetstream" @@ -107,8 +108,55 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr TLS: stream.Spec.TLS, } - // TODO Check if marked for deletion and delete - // TODO honour stream.Spec.PreventDelete and + // Delete if marked for deletion and has stream finalizer + markedForDeletion := stream.GetDeletionTimestamp() != nil + if markedForDeletion && controllerutil.ContainsFinalizer(stream, streamFinalizer) { + // Set status to not unknown + stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionUnknown, "Finalizing", "Performing finalizer operations.") + if err := r.Status().Update(ctx, stream); err != nil { + return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + } + + // Perform finalizer operations + // TODO honour stream.Spec.PreventDelete and readonly + // Remove stream + err := r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { + return js.DeleteStream(ctx, stream.Spec.Name) + }) + if err != nil { + return ctrl.Result{}, fmt.Errorf("delete stream during finalization: %w", err) + } + + // Re-Fetch resource + if err := r.Get(ctx, req.NamespacedName, stream); err != nil { + if apierrors.IsNotFound(err) { + log.Info("stream resource not found. Ignoring since object must be deleted") + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + } + + // Update status to not ready + stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionFalse, "Finalizing", "Performed finalizer operations.") + if err := r.Status().Update(ctx, stream); err != nil { + return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + } + + if err := r.Status().Update(ctx, stream); err != nil { + return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + } + + log.Info("Removing stream finalizer after performing finalizing operations") + if ok := controllerutil.RemoveFinalizer(stream, streamFinalizer); !ok { + return ctrl.Result{Requeue: true}, errors.New("failed to remove stream finalizer") + } + + if err := r.Update(ctx, stream); err != nil { + return ctrl.Result{}, fmt.Errorf("remove finalizer: %w", err) + } + + return ctrl.Result{}, nil + } // Create or Update the stream based on the spec diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 36fe98ca..7ad749a0 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -25,6 +25,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" "testing" "time" @@ -37,6 +38,7 @@ import ( var _ = Describe("Stream Controller", func() { When("reconciling a resource", func() { const resourceName = "test-stream" + const streamName = "orders" typeNamespacedName := types.NamespacedName{ Name: resourceName, @@ -55,7 +57,7 @@ var _ = Describe("Stream Controller", func() { Namespace: "default", }, Spec: api.StreamSpec{ - Name: "test-stream", + Name: streamName, Replicas: 1, Subjects: []string{"tests.*"}, Description: "test stream", @@ -72,16 +74,22 @@ var _ = Describe("Stream Controller", func() { }) AfterEach(func(ctx SpecContext) { - // Remove test stream resource + // Get and remove test stream resource if it exists resource := &api.Stream{} err := k8sClient.Get(ctx, typeNamespacedName, resource) - Expect(err).NotTo(HaveOccurred()) - - By("Cleanup the specific resource instance Stream") - Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + if err != nil { + Expect(err).To(MatchError(k8serrors.IsNotFound, "Is not found")) + } else { + By("Removing the finalizer") + controllerutil.RemoveFinalizer(resource, streamFinalizer) + Expect(k8sClient.Update(ctx, resource)).To(Succeed()) + + By("Cleanup the specific resource instance Stream") + Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + } By("Deleting the stream") - err = jsClient.DeleteStream(ctx, resource.Spec.Name) + err = jsClient.DeleteStream(ctx, streamName) if err != nil { Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) } @@ -98,11 +106,6 @@ var _ = Describe("Stream Controller", func() { }) Expect(err).NotTo(HaveOccurred()) - _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - Expect(err).NotTo(HaveOccurred()) - // Fetch resource err = k8sClient.Get(ctx, typeNamespacedName, stream) Expect(err).NotTo(HaveOccurred()) @@ -123,11 +126,11 @@ var _ = Describe("Stream Controller", func() { Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) By("Checking if the stream was created") - natsStream, err := jsClient.Stream(ctx, "test-stream") + natsStream, err := jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) streamInfo, err := natsStream.Info(ctx) Expect(err).NotTo(HaveOccurred()) - Expect(streamInfo.Config.Name).To(Equal("test-stream")) + Expect(streamInfo.Config.Name).To(Equal(streamName)) Expect(streamInfo.Created).To(BeTemporally("~", time.Now(), time.Second)) }) @@ -135,7 +138,7 @@ var _ = Describe("Stream Controller", func() { By("Creating the stream with empty subjects and description") _, err := jsClient.CreateStream(ctx, jetstream.StreamConfig{ - Name: "test-stream", + Name: streamName, Replicas: 1, Retention: jetstream.WorkQueuePolicy, Discard: jetstream.DiscardOld, @@ -153,11 +156,6 @@ var _ = Describe("Stream Controller", func() { }) Expect(err).NotTo(HaveOccurred()) - _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - Expect(err).NotTo(HaveOccurred()) - // Fetch resource err = k8sClient.Get(ctx, typeNamespacedName, stream) Expect(err).NotTo(HaveOccurred()) @@ -175,7 +173,7 @@ var _ = Describe("Stream Controller", func() { Expect(gotCondition).To(Equal(readyCondition)) By("Checking if the stream was updated") - natsStream, err := jsClient.Stream(ctx, "test-stream") + natsStream, err := jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) streamInfo, err := natsStream.Info(ctx) @@ -238,8 +236,40 @@ var _ = Describe("Stream Controller", func() { Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) }) - PIt("should delete stream marked for deletion", func(ctx SpecContext) { + It("should delete stream marked for deletion", func(ctx SpecContext) { + By("Reconciling the created resource once to ensure the finalizer is set and stream created") + controllerReconciler := &StreamReconciler{ + baseController, + } + + _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + // Stream exists + _, err = jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + + By("Marking the resource as deleted") + Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + + By("Reconciling the deleted resource") + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Checking if the resource was removed") + Eventually(func() error { + found := &api.Stream{} + return k8sClient.Get(ctx, typeNamespacedName, found) + }, 5*time.Second, time.Second).ShouldNot(Succeed()) + + By("Checking if the stream was deleted") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) }) It("should update stream on different server as specified in spec", func(ctx SpecContext) { @@ -268,12 +298,12 @@ var _ = Describe("Stream Controller", func() { defer closer.Close() Expect(err).NotTo(HaveOccurred()) - got, err := altClient.Stream(ctx, stream.Spec.Name) + got, err := altClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) Expect(got.CachedInfo().Created).To(BeTemporally("~", time.Now(), time.Second)) By("Checking that the stream was NOT created on the alternative server") - got, err = jsClient.Stream(ctx, stream.Spec.Name) + got, err = jsClient.Stream(ctx, streamName) Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) }) From fb431bc0d197e3fccb31ae9fea97d8bbc6a7a97c Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 9 Dec 2024 13:20:15 +0100 Subject: [PATCH 10/35] update observedGeneration of status --- internal/controller/stream_controller.go | 4 ++-- internal/controller/stream_controller_test.go | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index b1a00621..76b90f74 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -180,8 +180,8 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, fmt.Errorf("create or update stream: %w", err) } - // TODO update the generation in the status - + // update the observed generation and ready status + stream.Status.ObservedGeneration = stream.Generation stream.Status.Conditions = updateReadyCondition( stream.Status.Conditions, v1.ConditionTrue, diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 7ad749a0..725b34a1 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -122,6 +122,9 @@ var _ = Describe("Stream Controller", func() { gotCondition.LastTransitionTime = "" Expect(gotCondition).To(Equal(readyCondition)) + By("Checking if the observed generation matches") + Expect(stream.Status.ObservedGeneration).To(Equal(stream.Generation)) + By("Checking if the finalizer was set") Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) @@ -172,6 +175,9 @@ var _ = Describe("Stream Controller", func() { gotCondition.LastTransitionTime = "" Expect(gotCondition).To(Equal(readyCondition)) + By("Checking if the observed generation matches") + Expect(stream.Status.ObservedGeneration).To(Equal(stream.Generation)) + By("Checking if the stream was updated") natsStream, err := jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) @@ -232,6 +238,9 @@ var _ = Describe("Stream Controller", func() { Expect(cond.Message).To(Equal("create or update stream: context deadline exceeded")) Expect(cond.LastTransitionTime).NotTo(BeEmpty()) + By("Checking if the observed generation does not match") + Expect(stream.Status.ObservedGeneration).ToNot(Equal(stream.Generation)) + By("Checking if the finalizer was set") Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) }) From 1f6be77c109acfd5db8b67d990b40dcd820cb545 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 9 Dec 2024 13:42:56 +0100 Subject: [PATCH 11/35] check Spec.PreventDelete before stream deletion --- internal/controller/stream_controller.go | 19 +++++---- internal/controller/stream_controller_test.go | 42 +++++++++++++++++++ 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 76b90f74..2465b87b 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -117,14 +117,17 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) } - // Perform finalizer operations - // TODO honour stream.Spec.PreventDelete and readonly - // Remove stream - err := r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { - return js.DeleteStream(ctx, stream.Spec.Name) - }) - if err != nil { - return ctrl.Result{}, fmt.Errorf("delete stream during finalization: %w", err) + log.Info("performing finalizing operations") + if stream.Spec.PreventDelete { + log.Info("Spec.PreventDelete: skip delete stream during resource deletion.", "streamName", stream.Spec.Name) + } else { + // Remove stream + err := r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { + return js.DeleteStream(ctx, stream.Spec.Name) + }) + if err != nil { + return ctrl.Result{}, fmt.Errorf("delete stream during finalization: %w", err) + } } // Re-Fetch resource diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 725b34a1..3fd9fd77 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -281,6 +281,48 @@ var _ = Describe("Stream Controller", func() { Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) }) + When("PreventDelete is set", func() { + It("should not delete stream marked for deletion when ", func(ctx SpecContext) { + + By("Setting preventDelete on the resource") + stream.Spec.PreventDelete = true + Expect(k8sClient.Update(ctx, stream)).To(Succeed()) + + By("Reconciling the updated resource once") + controllerReconciler := &StreamReconciler{ + baseController, + } + + _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + // Stream exists + _, err = jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + + By("Marking the resource as deleted") + Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + + By("Reconciling the deleted resource") + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Checking if the resource was removed") + Eventually(func() error { + found := &api.Stream{} + return k8sClient.Get(ctx, typeNamespacedName, found) + }, 5*time.Second, time.Second).ShouldNot(Succeed()) + + By("Checking if the stream was *not* deleted") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + }) + }) + It("should update stream on different server as specified in spec", func(ctx SpecContext) { By("Setting up the alternative server") // Setup altClient for alternate server From 37f44fccbc4b047e22ffc310a98e8692143ba5d7 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 9 Dec 2024 18:09:40 +0100 Subject: [PATCH 12/35] remove base js client Use a single use client on every connection. This should be replaced by a client pool in the future. --- internal/controller/jetstream_controller.go | 71 ++++++++------------- 1 file changed, 27 insertions(+), 44 deletions(-) diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go index aac84feb..8189c6b9 100644 --- a/internal/controller/jetstream_controller.go +++ b/internal/controller/jetstream_controller.go @@ -21,16 +21,6 @@ type connectionOptions struct { TLS api.TLS `json:"tls"` } -func (o connectionOptions) empty() bool { - return o.Account == "" && - o.Creds == "" && - o.Nkey == "" && - len(o.Servers) == 0 && - o.TLS.ClientCert == "" && - o.TLS.ClientKey == "" && - len(o.TLS.RootCAs) == 0 -} - type JetStreamController interface { client.Client @@ -42,20 +32,10 @@ type JetStreamController interface { func NewJSController(k8sClient client.Client, natsConfig *NatsConfig, controllerConfig *Config) (JetStreamController, error) { - var baseClient jetstream.JetStream - if natsConfig.ServerURL != "" { - var err error - baseClient, _, err = CreateJetStreamClient(natsConfig, true) - if err != nil { - return nil, fmt.Errorf("create base js k8sClient: %w", err) - } - } - return &jsController{ Client: k8sClient, config: natsConfig, controllerConfig: controllerConfig, - baseClient: baseClient, }, nil } @@ -63,7 +43,6 @@ type jsController struct { client.Client config *NatsConfig controllerConfig *Config - baseClient jetstream.JetStream } func (c *jsController) readOnly() bool { @@ -75,33 +54,37 @@ func (c *jsController) Namespace() string { } func (c *jsController) WithJetStreamClient(opts *connectionOptions, op func(js jetstream.JetStream) error) error { - if c.baseClient == nil { - return errors.New("no client available") - } - - // Use base client when no config is given - if opts == nil || opts.empty() { - return op(c.baseClient) - } - // Build single use client + // TODO(future feature): Use client-pool serverUrl := strings.Join(opts.Servers, ",") - // TODO needs review: - // Here the config from spec takes precedence over base config on a value by value basis. - // This could lead to issues when an NKey is set in the spec config and Credentials are set in the base config. - // In NatsConfig.buildOptions, Credentials take precedence over the Nkey, - // which would lead to the nkey from spec to be ignored. + // Build nats config from opts and controller base config. + // Takes opts values if present. cfg := &NatsConfig{ - CRDConnect: false, - ClientName: c.config.ClientName, - Credentials: or(opts.Creds, c.config.Credentials), - NKey: or(opts.Nkey, c.config.NKey), - ServerURL: or(serverUrl, c.config.ServerURL), - CAs: *or(&opts.TLS.RootCAs, &c.config.CAs), - Certificate: or(opts.TLS.ClientCert, c.config.Certificate), - Key: or(opts.TLS.ClientKey, c.config.Key), - TLSFirst: false, + CRDConnect: false, + ClientName: c.config.ClientName, + ServerURL: or(serverUrl, c.config.ServerURL), + TLSFirst: c.config.TLSFirst, // TODO(review): should this value depend on any opts? There is no TLSFirst in the spec + } + + // Authentication either from opts or base config + if opts.Creds != "" || opts.Nkey != "" { + cfg.Credentials = opts.Creds + cfg.NKey = opts.Nkey + } else { + cfg.Credentials = c.config.Credentials + cfg.NKey = c.config.NKey + } + + // Cert config either from opts or base config + if len(opts.TLS.RootCAs) > 0 || opts.TLS.ClientCert != "" || opts.TLS.ClientKey != "" { + cfg.CAs = opts.TLS.RootCAs + cfg.Certificate = opts.TLS.ClientCert + cfg.Key = opts.TLS.ClientKey + } else { + cfg.CAs = c.config.CAs + cfg.Certificate = c.config.Certificate + cfg.Key = c.config.Key } client, closer, err := CreateJetStreamClient(cfg, true) From 00c98e53cbbd4bc9361be59abc858bb445c0baeb Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 9 Dec 2024 18:10:09 +0100 Subject: [PATCH 13/35] move asJsonString to jetstream_controller --- internal/controller/jetstream_controller.go | 4 ++++ internal/controller/stream_controller.go | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go index 8189c6b9..0fa2d5ae 100644 --- a/internal/controller/jetstream_controller.go +++ b/internal/controller/jetstream_controller.go @@ -135,4 +135,8 @@ func updateReadyCondition(conditions []api.Condition, status v1.ConditionStatus, return js.UpsertCondition(conditions, newCondition) } +// asJsonString returns the given string wrapped in " and converted to []byte. +// Helper for mapping spec config to jetStream config using UnmarshalJSON. +func asJsonString(v string) []byte { + return []byte("\"" + v + "\"") } diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 2465b87b..b15398a2 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -366,11 +366,6 @@ func mapStreamSource(ss *api.StreamSource) (*jetstream.StreamSource, error) { return jss, nil } -// asJsonString returns the given string wrapped in " and converted to []byte. -func asJsonString(v string) []byte { - return []byte("\"" + v + "\"") -} - // SetupWithManager sets up the controller with the Manager. func (r *StreamReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). From 7e2006b51270ac96608bd0c793a87012dfc3c4de Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Mon, 9 Dec 2024 18:22:19 +0100 Subject: [PATCH 14/35] check namespace read only and prevent update mode --- internal/controller/jetstream_controller.go | 25 ++++++++++++++------- internal/controller/stream_controller.go | 23 +++++++++++++++---- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go index 0fa2d5ae..3b959fe9 100644 --- a/internal/controller/jetstream_controller.go +++ b/internal/controller/jetstream_controller.go @@ -1,7 +1,6 @@ package controller import ( - "errors" "fmt" js "github.com/nats-io/nack/controllers/jetstream" api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" @@ -24,10 +23,18 @@ type connectionOptions struct { type JetStreamController interface { client.Client - // ReadOnly returns true when this controller - readOnly() bool - Namespace() string - WithJetStreamClient(*connectionOptions, func(js jetstream.JetStream) error) error + // ReadOnly returns true when no changes should be made by the controller. + ReadOnly() bool + // ValidNamespace ok if the controllers namespace restriction allows the given namespace. + ValidNamespace(string string) bool + + // WithJetStreamClient provides a jetStream client to the given operation. + // The client uses the controllers connection configuration merged with opts. + // + // The given opts values take precedence over the controllers base configuration. + // + // Returns the error of the operation or errors during client setup. + WithJetStreamClient(opts *connectionOptions, op func(js jetstream.JetStream) error) error } func NewJSController(k8sClient client.Client, natsConfig *NatsConfig, controllerConfig *Config) (JetStreamController, error) { @@ -45,12 +52,13 @@ type jsController struct { controllerConfig *Config } -func (c *jsController) readOnly() bool { +func (c *jsController) ReadOnly() bool { return c.controllerConfig.ReadOnly } -func (c *jsController) Namespace() string { - return c.controllerConfig.Namespace +func (c *jsController) ValidNamespace(targetNamespace string) bool { + ns := c.controllerConfig.Namespace + return ns == "" || ns == targetNamespace } func (c *jsController) WithJetStreamClient(opts *connectionOptions, op func(js jetstream.JetStream) error) error { @@ -134,6 +142,7 @@ func updateReadyCondition(conditions []api.Condition, status v1.ConditionStatus, } else { return js.UpsertCondition(conditions, newCondition) } +} // asJsonString returns the given string wrapped in " and converted to []byte. // Helper for mapping spec config to jetStream config using UnmarshalJSON. diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index b15398a2..bbbf077d 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -47,7 +47,10 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr log.Info("reconciling") - // TODO honour r.Config.ReadOnly and r.Config.Namespace + if ok := r.ValidNamespace(req.Namespace); !ok { + log.Info("Controller restricted to namespace, skipping reconciliation") + return ctrl.Result{}, nil + } // Fetch stream resource stream := &api.Stream{} @@ -81,8 +84,9 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } } - // Update Status to unknown when no status is set + // Update ready status to unknown when no status is set if stream.Status.Conditions == nil || len(stream.Status.Conditions) == 0 { + log.Info("Setting initial ready condition to unknown") stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionUnknown, "Reconciling", "Starting reconciliation") err := r.Status().Update(ctx, stream) if err != nil { @@ -99,9 +103,14 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } } + if r.ReadOnly() { + log.Info("read-only enabled, skipping reconciliation") + return ctrl.Result{}, nil + } + // Connection options specific to this spec specConnectionOptions := &connectionOptions{ - Account: stream.Spec.Account, + Account: stream.Spec.Account, // TODO(review): Where does Spec.Account have to be considered? Creds: stream.Spec.Creds, Nkey: stream.Spec.Nkey, Servers: stream.Spec.Servers, @@ -110,6 +119,7 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Delete if marked for deletion and has stream finalizer markedForDeletion := stream.GetDeletionTimestamp() != nil + // TODO deletion is triggered multiple times?. Set additional status condition? if markedForDeletion && controllerutil.ContainsFinalizer(stream, streamFinalizer) { // Set status to not unknown stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionUnknown, "Finalizing", "Performing finalizer operations.") @@ -162,6 +172,10 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } // Create or Update the stream based on the spec + if stream.Spec.PreventUpdate { + log.Info("Spec.PreventUpdate: skip create/updating the stream during reconciliation.", "streamName", stream.Spec.Name) + return ctrl.Result{}, nil + } // Map spec to stream targetConfig targetConfig, err := mapSpecToConfig(&stream.Spec) @@ -169,7 +183,8 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, fmt.Errorf("map spec to stream targetConfig: %w", err) } - // TODO honour stream.Spec.PreventUpdate + // CreateOrUpdateStream is called on every reconciliation when the stream is not to be deleted. + // TODO Do we need to check if generation has changed or the config differs? err = r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { _, err = js.CreateOrUpdateStream(ctx, targetConfig) return err From 1b8b96db16012e6c9fdb8de5ec5376dde4b2f007 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 11:41:49 +0100 Subject: [PATCH 15/35] Update comments and log --- internal/controller/stream_controller.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index bbbf077d..eba477b3 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -38,8 +38,10 @@ type StreamReconciler struct { // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. // -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile +// It performs three main operations: +// - Initialize finalizer and ready condition if not present +// - Delete stream if it is marked for deletion. +// - Create or Update the stream func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := klog.FromContext(ctx). WithName("StreamReconciler"). @@ -129,7 +131,7 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr log.Info("performing finalizing operations") if stream.Spec.PreventDelete { - log.Info("Spec.PreventDelete: skip delete stream during resource deletion.", "streamName", stream.Spec.Name) + log.Info("skip delete stream during resource deletion.", "streamName", stream.Spec.Name) } else { // Remove stream err := r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { @@ -184,7 +186,7 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } // CreateOrUpdateStream is called on every reconciliation when the stream is not to be deleted. - // TODO Do we need to check if generation has changed or the config differs? + // TODO(future-feature): Do we need to check if generation has changed or the config differs? err = r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { _, err = js.CreateOrUpdateStream(ctx, targetConfig) return err From 8ddea8dce1aebdfd26a8470f00df80b227862a2a Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 11:42:24 +0100 Subject: [PATCH 16/35] Fix test docs and check precondition --- internal/controller/stream_controller_test.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 3fd9fd77..1e0f8b0f 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -208,8 +208,8 @@ var _ = Describe("Stream Controller", func() { By("Reconciling the created resource") // Setup client for not running server + // Use actual test server to ensure port not used by other service on test instance sv := CreateTestServer() - // Is there an easier way to create a failing js client? controller, err := NewJSController(k8sClient, &NatsConfig{ServerURL: sv.ClientURL()}, &Config{}) Expect(err).NotTo(HaveOccurred()) sv.Shutdown() @@ -235,7 +235,7 @@ var _ = Describe("Stream Controller", func() { Expect(cond.Type).To(Equal(readyCondType)) Expect(cond.Status).To(Equal(v1.ConditionFalse)) Expect(cond.Reason).To(Equal("Errored")) - Expect(cond.Message).To(Equal("create or update stream: context deadline exceeded")) + Expect(cond.Message).To(HavePrefix("create or update stream:")) Expect(cond.LastTransitionTime).NotTo(BeEmpty()) By("Checking if the observed generation does not match") @@ -282,7 +282,7 @@ var _ = Describe("Stream Controller", func() { }) When("PreventDelete is set", func() { - It("should not delete stream marked for deletion when ", func(ctx SpecContext) { + It("should not delete stream marked for deletion", func(ctx SpecContext) { By("Setting preventDelete on the resource") stream.Spec.PreventDelete = true @@ -333,6 +333,10 @@ var _ = Describe("Stream Controller", func() { stream.Spec.Servers = []string{altServer.ClientURL()} Expect(k8sClient.Update(ctx, stream)).To(Succeed()) + By("Checking precondition, that the stream does not yet exist") + got, err := jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + By("Reconciling the resource") controllerReconciler := &StreamReconciler{ baseController, @@ -349,12 +353,12 @@ var _ = Describe("Stream Controller", func() { defer closer.Close() Expect(err).NotTo(HaveOccurred()) - got, err := altClient.Stream(ctx, streamName) + got, err = altClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) Expect(got.CachedInfo().Created).To(BeTemporally("~", time.Now(), time.Second)) - By("Checking that the stream was NOT created on the alternative server") - got, err = jsClient.Stream(ctx, streamName) + By("Checking that the stream was NOT created on the original server") + _, err = jsClient.Stream(ctx, streamName) Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) }) From 5a5a7f551fa26e3c43a70c0ddd95340fe52ebfbf Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 12:43:57 +0100 Subject: [PATCH 17/35] Add preventUpdate test cases --- internal/controller/stream_controller_test.go | 70 +++++++++++++++++-- 1 file changed, 63 insertions(+), 7 deletions(-) diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 1e0f8b0f..cac8918f 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -45,6 +45,14 @@ var _ = Describe("Stream Controller", func() { Namespace: "default", // TODO(user):Modify as needed } + emptyStreamConfig := jetstream.StreamConfig{ + Name: streamName, + Replicas: 1, + Retention: jetstream.WorkQueuePolicy, + Discard: jetstream.DiscardOld, + Storage: jetstream.FileStorage, + } + stream := &api.Stream{} BeforeEach(func(ctx SpecContext) { @@ -140,13 +148,7 @@ var _ = Describe("Stream Controller", func() { It("should successfully update the stream and update the status", func(ctx SpecContext) { By("Creating the stream with empty subjects and description") - _, err := jsClient.CreateStream(ctx, jetstream.StreamConfig{ - Name: streamName, - Replicas: 1, - Retention: jetstream.WorkQueuePolicy, - Discard: jetstream.DiscardOld, - Storage: jetstream.FileStorage, - }) + _, err := jsClient.CreateStream(ctx, emptyStreamConfig) Expect(err).NotTo(HaveOccurred()) By("Reconciling the created resource") @@ -323,6 +325,60 @@ var _ = Describe("Stream Controller", func() { }) }) + When("PreventUpdate is set", func() { + BeforeEach(func(ctx SpecContext) { + By("Setting prevent update") + + stream.Spec.PreventUpdate = true + Expect(k8sClient.Update(ctx, stream)).To(Succeed()) + }) + + It("should not create stream", func(ctx SpecContext) { + By("Reconciling the updated resource") + controllerReconciler := &StreamReconciler{ + baseController, + } + _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Checking that the stream was not created") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + + By("Checking that the streams status was not updated") + err = k8sClient.Get(ctx, typeNamespacedName, stream) + Expect(err).NotTo(HaveOccurred()) + Expect(stream.Status.ObservedGeneration).To(BeNumerically("<", stream.Generation)) + }) + + It("should not update stream", func(ctx SpecContext) { + By("Creating the stream with empty subjects and description") + _, err := jsClient.CreateStream(ctx, emptyStreamConfig) + Expect(err).NotTo(HaveOccurred()) + + By("Reconciling the resource") + controllerReconciler := &StreamReconciler{ + baseController, + } + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Checking that the stream was not updated") + s, err := jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + Expect(s.CachedInfo().Config.Description).To(BeEmpty()) + + By("Checking that the streams generation was not updated") + err = k8sClient.Get(ctx, typeNamespacedName, stream) + Expect(err).NotTo(HaveOccurred()) + Expect(stream.Status.ObservedGeneration).To(BeNumerically("<", stream.Generation)) + }) + }) + It("should update stream on different server as specified in spec", func(ctx SpecContext) { By("Setting up the alternative server") // Setup altClient for alternate server From 6f909320e7599850304dd9b2a556e0a10e8ed12f Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 12:44:31 +0100 Subject: [PATCH 18/35] Add tests for read-only or namespace restricted mode --- internal/controller/stream_controller_test.go | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index cac8918f..8b43ee21 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -379,6 +379,80 @@ var _ = Describe("Stream Controller", func() { }) }) + DescribeTableSubtree("controller restricted to not perform any updates on resource", + func(getController func(SpecContext) StreamReconciler) { + var controller StreamReconciler + + BeforeEach(func(ctx SpecContext) { + controller = getController(ctx) + }) + + It("should not create stream", func(ctx SpecContext) { + By("reconciling the resource") + _, err := controller.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + + By("Checking that the stream was not created") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + }) + It("should not update stream", func(ctx SpecContext) { + By("creating the stream") + _, err := jsClient.CreateStream(ctx, emptyStreamConfig) + Expect(err).NotTo(HaveOccurred()) + + By("reconciling the resource") + _, err = controller.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + + By("Checking that the stream was not updated") + s, err := jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + Expect(s.CachedInfo().Config.Description).To(BeEmpty()) + + By("Checking that the streams generation was not updated") + err = k8sClient.Get(ctx, typeNamespacedName, stream) + Expect(err).NotTo(HaveOccurred()) + Expect(stream.Status.ObservedGeneration).To(BeNumerically("<", stream.Generation)) + }) + It("should not delete stream", func(ctx SpecContext) { + + By("creating the stream") + _, err := jsClient.CreateStream(ctx, emptyStreamConfig) + Expect(err).NotTo(HaveOccurred()) + + By("Marking the resource as deleted") + Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + + By("reconciling the resource") + _, err = controller.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + + By("Checking if the resource was removed") + Eventually(func() error { + found := &api.Stream{} + return k8sClient.Get(ctx, typeNamespacedName, found) + }, 5*time.Second, time.Second).ShouldNot(Succeed()) + + By("Checking if the stream was *not* deleted") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + }) + + }, + // Test cases provide functions for providing a configured the controller and setup preconditions + Entry("read only", func(ctx SpecContext) StreamReconciler { + readOnly, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{ReadOnly: true}) + Expect(err).NotTo(HaveOccurred()) + return StreamReconciler{readOnly} + }), + Entry("namespaced", func(ctx SpecContext) StreamReconciler { + readOnly, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{Namespace: "some-namespace"}) + Expect(err).NotTo(HaveOccurred()) + return StreamReconciler{readOnly} + }), + ) + It("should update stream on different server as specified in spec", func(ctx SpecContext) { By("Setting up the alternative server") // Setup altClient for alternate server From 19c6bebeadfbd386d1b8d654ae65484423ff40c7 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 13:00:27 +0100 Subject: [PATCH 19/35] fix empty ca when no ca set Setting CAs: []string{*ca} resulted in []string{""} when no CA was set, leading to an error when creating clients. --- cmd/jetstream-controller/main.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cmd/jetstream-controller/main.go b/cmd/jetstream-controller/main.go index 23855c56..62638feb 100644 --- a/cmd/jetstream-controller/main.go +++ b/cmd/jetstream-controller/main.go @@ -88,18 +88,23 @@ func run() error { if *controlLoop { klog.Warning("Starting jetStream controller in experimental control loop mode") + natsCfg := &controller.NatsConfig{ CRDConnect: *crdConnect, ClientName: "jetstream-controller", Credentials: *creds, NKey: *nkey, ServerURL: *server, - CAs: []string{*ca}, + CAs: []string{}, Certificate: *cert, Key: *key, TLSFirst: *tlsfirst, } + if *ca != "" { + natsCfg.CAs = []string{*ca} + } + controllerCfg := &controller.Config{ ReadOnly: *readOnly, Namespace: *namespace, From 9418bcd9f7a6f3d32a289b6049196f11b6f3c80a Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 13:00:40 +0100 Subject: [PATCH 20/35] simplify error message --- internal/controller/stream_controller.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index eba477b3..ae4e3944 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -192,12 +192,12 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return err }) if err != nil { - msg := fmt.Sprintf("create or update stream: %s", err.Error()) - stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionFalse, "Errored", msg) + err = fmt.Errorf("create or update stream: %w", err) + stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionFalse, "Errored", err.Error()) if err := r.Status().Update(ctx, stream); err != nil { log.Error(err, "failed to update ready condition to Errored") } - return ctrl.Result{}, fmt.Errorf("create or update stream: %w", err) + return ctrl.Result{}, err } // update the observed generation and ready status From f7abcc68946c9e0723c130bcfb53247a3f1d3d20 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 13:33:15 +0100 Subject: [PATCH 21/35] fix error loop when the underlying stream was deleted --- internal/controller/stream_controller.go | 4 ++- internal/controller/stream_controller_test.go | 32 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index ae4e3944..3796b399 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -137,7 +137,9 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr err := r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { return js.DeleteStream(ctx, stream.Spec.Name) }) - if err != nil { + if errors.Is(err, jetstream.ErrStreamNotFound) { + log.Info("managed stream was already deleted") + } else if err != nil { return ctrl.Result{}, fmt.Errorf("delete stream during finalization: %w", err) } } diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 8b43ee21..e9d8091a 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -283,6 +283,38 @@ var _ = Describe("Stream Controller", func() { Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) }) + It("should succeed deleting stream resource where the underlying stream was already deleted", func(ctx SpecContext) { + + By("Reconciling the created resource once to ensure the finalizer is set and stream created") + controllerReconciler := &StreamReconciler{ + baseController, + } + + _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Deleting the managed stream") + err = jsClient.DeleteStream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + + By("Marking the resource as deleted") + Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + + By("Reconciling the deleted resource") + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Checking if the resource was removed") + Eventually(func() error { + found := &api.Stream{} + return k8sClient.Get(ctx, typeNamespacedName, found) + }, 5*time.Second, time.Second).ShouldNot(Succeed()) + }) + When("PreventDelete is set", func() { It("should not delete stream marked for deletion", func(ctx SpecContext) { From 84619b581f04f9547f144be15df86f53f395edf5 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 14:54:19 +0100 Subject: [PATCH 22/35] refactor each phase into separate method --- internal/controller/stream_controller.go | 146 +++++++++++++++++------ 1 file changed, 110 insertions(+), 36 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 3796b399..3471bfb8 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -20,8 +20,10 @@ import ( "context" "errors" "fmt" + "github.com/go-logr/logr" api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" "github.com/nats-io/nats.go/jetstream" + log "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/klog/v2" @@ -54,14 +56,60 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } + result, err := r.initializationPhase(ctx, &log, req) + if err != nil { + err = fmt.Errorf("initialization: %w", err) + if result == nil { + return ctrl.Result{}, err + } + return *result, err + } + if result != nil { + return *result, nil + } + + if r.ReadOnly() { + log.Info("read-only enabled, skipping reconciliation") + return ctrl.Result{}, nil + } + + result, err = r.deletionPhase(ctx, log, req) + if err != nil { + err = fmt.Errorf("deletion: %w", err) + if result == nil { + return ctrl.Result{}, err + } + return *result, err + } + if result != nil { + return *result, nil + } + + result, err = r.createUpdatePhase(ctx, log, req) + if err != nil { + err = fmt.Errorf("deletion: %w", err) + if result == nil { + return ctrl.Result{}, err + } + return *result, err + } + if result != nil { + return *result, nil + } + + return ctrl.Result{}, nil +} + +func (r *StreamReconciler) initializationPhase(ctx context.Context, log *logr.Logger, req ctrl.Request) (*ctrl.Result, error) { + // Fetch stream resource stream := &api.Stream{} if err := r.Get(ctx, req.NamespacedName, stream); err != nil { if apierrors.IsNotFound(err) { log.Info("stream resource not found. Ignoring since object must be deleted") - return ctrl.Result{}, nil + return &ctrl.Result{}, nil } - return ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) + return &ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) } // Add finalizer @@ -69,20 +117,20 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr log.Info("Adding stream finalizer") if ok := controllerutil.AddFinalizer(stream, streamFinalizer); !ok { log.Error(nil, "Failed to add finalizer to stream resource") - return ctrl.Result{Requeue: true}, nil + return &ctrl.Result{}, nil // TODO how to handle } if err := r.Update(ctx, stream); err != nil { - return ctrl.Result{}, fmt.Errorf("update stream resource to add finalizer: %w", err) + return &ctrl.Result{}, fmt.Errorf("update stream resource to add finalizer: %w", err) } // re-fetch stream if err := r.Get(ctx, req.NamespacedName, stream); err != nil { if apierrors.IsNotFound(err) { log.Info("stream resource not found. Ignoring since object must be deleted") - return ctrl.Result{}, nil + return &ctrl.Result{}, nil } - return ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + return &ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) } } @@ -92,31 +140,31 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionUnknown, "Reconciling", "Starting reconciliation") err := r.Status().Update(ctx, stream) if err != nil { - return ctrl.Result{}, fmt.Errorf("set condition unknown: %w", err) + return &ctrl.Result{}, fmt.Errorf("set condition unknown: %w", err) } // re-fetch stream if err := r.Get(ctx, req.NamespacedName, stream); err != nil { if apierrors.IsNotFound(err) { log.Info("stream resource not found. Ignoring since object must be deleted") - return ctrl.Result{}, nil + return &ctrl.Result{}, nil } - return ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + return &ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) } } - if r.ReadOnly() { - log.Info("read-only enabled, skipping reconciliation") - return ctrl.Result{}, nil - } + return nil, nil +} - // Connection options specific to this spec - specConnectionOptions := &connectionOptions{ - Account: stream.Spec.Account, // TODO(review): Where does Spec.Account have to be considered? - Creds: stream.Spec.Creds, - Nkey: stream.Spec.Nkey, - Servers: stream.Spec.Servers, - TLS: stream.Spec.TLS, +func (r *StreamReconciler) deletionPhase(ctx context.Context, logger logr.Logger, req ctrl.Request) (*ctrl.Result, error) { + // Fetch stream resource + stream := &api.Stream{} + if err := r.Get(ctx, req.NamespacedName, stream); err != nil { + if apierrors.IsNotFound(err) { + log.Info("stream resource not found. Ignoring since object must be deleted") + return &ctrl.Result{}, nil + } + return &ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) } // Delete if marked for deletion and has stream finalizer @@ -126,7 +174,7 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Set status to not unknown stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionUnknown, "Finalizing", "Performing finalizer operations.") if err := r.Status().Update(ctx, stream); err != nil { - return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + return &ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) } log.Info("performing finalizing operations") @@ -134,13 +182,13 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr log.Info("skip delete stream during resource deletion.", "streamName", stream.Spec.Name) } else { // Remove stream - err := r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { + err := r.WithJetStreamClient(mapToConnectionOptions(stream.Spec), func(js jetstream.JetStream) error { return js.DeleteStream(ctx, stream.Spec.Name) }) if errors.Is(err, jetstream.ErrStreamNotFound) { log.Info("managed stream was already deleted") } else if err != nil { - return ctrl.Result{}, fmt.Errorf("delete stream during finalization: %w", err) + return &ctrl.Result{}, fmt.Errorf("delete stream during finalization: %w", err) } } @@ -148,48 +196,64 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if err := r.Get(ctx, req.NamespacedName, stream); err != nil { if apierrors.IsNotFound(err) { log.Info("stream resource not found. Ignoring since object must be deleted") - return ctrl.Result{}, nil + return &ctrl.Result{}, nil } - return ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + return &ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) } // Update status to not ready stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionFalse, "Finalizing", "Performed finalizer operations.") if err := r.Status().Update(ctx, stream); err != nil { - return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + return &ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) } if err := r.Status().Update(ctx, stream); err != nil { - return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + return &ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) } log.Info("Removing stream finalizer after performing finalizing operations") if ok := controllerutil.RemoveFinalizer(stream, streamFinalizer); !ok { - return ctrl.Result{Requeue: true}, errors.New("failed to remove stream finalizer") + return &ctrl.Result{Requeue: true}, errors.New("failed to remove stream finalizer") } if err := r.Update(ctx, stream); err != nil { - return ctrl.Result{}, fmt.Errorf("remove finalizer: %w", err) + return &ctrl.Result{}, fmt.Errorf("remove finalizer: %w", err) } - return ctrl.Result{}, nil + return &ctrl.Result{}, nil + } + + return nil, nil +} + +func (r *StreamReconciler) createUpdatePhase(ctx context.Context, logger logr.Logger, req ctrl.Request) (*ctrl.Result, error) { + + // Fetch stream resource + stream := &api.Stream{} + if err := r.Get(ctx, req.NamespacedName, stream); err != nil { + if apierrors.IsNotFound(err) { + log.Info("stream resource not found. Ignoring since object must be deleted") + return &ctrl.Result{}, nil + } + return &ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) } // Create or Update the stream based on the spec if stream.Spec.PreventUpdate { log.Info("Spec.PreventUpdate: skip create/updating the stream during reconciliation.", "streamName", stream.Spec.Name) - return ctrl.Result{}, nil + return &ctrl.Result{}, nil } // Map spec to stream targetConfig targetConfig, err := mapSpecToConfig(&stream.Spec) if err != nil { - return ctrl.Result{}, fmt.Errorf("map spec to stream targetConfig: %w", err) + return &ctrl.Result{}, fmt.Errorf("map spec to stream targetConfig: %w", err) } // CreateOrUpdateStream is called on every reconciliation when the stream is not to be deleted. // TODO(future-feature): Do we need to check if generation has changed or the config differs? - err = r.WithJetStreamClient(specConnectionOptions, func(js jetstream.JetStream) error { + err = r.WithJetStreamClient(mapToConnectionOptions(stream.Spec), func(js jetstream.JetStream) error { + log.Info("create or update stream", "streamName", targetConfig.Name) _, err = js.CreateOrUpdateStream(ctx, targetConfig) return err }) @@ -199,7 +263,7 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if err := r.Status().Update(ctx, stream); err != nil { log.Error(err, "failed to update ready condition to Errored") } - return ctrl.Result{}, err + return &ctrl.Result{}, err } // update the observed generation and ready status @@ -212,10 +276,20 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr ) err = r.Status().Update(ctx, stream) if err != nil { - return ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + return &ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) } - return ctrl.Result{}, nil + return nil, nil +} + +func mapToConnectionOptions(spec api.StreamSpec) *connectionOptions { + return &connectionOptions{ + Account: spec.Account, // TODO(review): Where does Spec.Account have to be considered? + Creds: spec.Creds, + Nkey: spec.Nkey, + Servers: spec.Servers, + TLS: spec.TLS, + } } // mapSpecToConfig creates a jetstream.StreamConfig matching the given stream resource spec From 28cb7b0c1048e6c9feca093a26f75caba5e64d2f Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 10 Dec 2024 21:33:29 +0100 Subject: [PATCH 23/35] Fix errors during parallel reconciliation & Refactor tests - Trigger only on generation changes - Split initialization and create into separate calls to Reconcile --- internal/controller/helpers_test.go | 43 + internal/controller/stream_controller.go | 242 ++---- internal/controller/stream_controller_test.go | 747 +++++++++--------- internal/controller/suite_test.go | 17 - 4 files changed, 512 insertions(+), 537 deletions(-) create mode 100644 internal/controller/helpers_test.go diff --git a/internal/controller/helpers_test.go b/internal/controller/helpers_test.go new file mode 100644 index 00000000..8235f11c --- /dev/null +++ b/internal/controller/helpers_test.go @@ -0,0 +1,43 @@ +package controller + +import ( + api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" + "github.com/nats-io/nats-server/v2/server" + natsserver "github.com/nats-io/nats-server/v2/test" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + v1 "k8s.io/api/core/v1" + + "os" + "time" +) + +func assertReadyStateMatches(condition api.Condition, status v1.ConditionStatus, reason string, message string, transitionTime time.Time) { + GinkgoHelper() + + Expect(condition.Type).To(Equal(readyCondType)) + Expect(condition.Status).To(Equal(status)) + Expect(condition.Reason).To(Equal(reason)) + Expect(condition.Message).To(ContainSubstring(message)) + + // Assert valid transition time + t, err := time.Parse(time.RFC3339Nano, condition.LastTransitionTime) + Expect(err).NotTo(HaveOccurred()) + Expect(t).To(BeTemporally("~", transitionTime, time.Second)) +} + +func CreateTestServer() *server.Server { + opts := &natsserver.DefaultTestOptions + opts.JetStream = true + opts.Port = -1 + opts.Debug = true + + dir, err := os.MkdirTemp("", "nats-*") + Expect(err).NotTo(HaveOccurred()) + opts.StoreDir = dir + + ns := natsserver.RunServer(opts) + Expect(err).NotTo(HaveOccurred()) + + return ns +} diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 3471bfb8..9946c756 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -23,12 +23,12 @@ import ( "github.com/go-logr/logr" api "github.com/nats-io/nack/pkg/jetstream/apis/jetstream/v1beta2" "github.com/nats-io/nats.go/jetstream" - log "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/predicate" "time" ) @@ -45,213 +45,124 @@ type StreamReconciler struct { // - Delete stream if it is marked for deletion. // - Create or Update the stream func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - log := klog.FromContext(ctx). - WithName("StreamReconciler"). - WithValues("namespace", req.Namespace, "name", req.Name) - - log.Info("reconciling") + log := klog.FromContext(ctx) if ok := r.ValidNamespace(req.Namespace); !ok { - log.Info("Controller restricted to namespace, skipping reconciliation") + log.Info("Controller restricted to namespace, skipping reconciliation.") return ctrl.Result{}, nil } - result, err := r.initializationPhase(ctx, &log, req) - if err != nil { - err = fmt.Errorf("initialization: %w", err) - if result == nil { - return ctrl.Result{}, err - } - return *result, err - } - if result != nil { - return *result, nil - } - - if r.ReadOnly() { - log.Info("read-only enabled, skipping reconciliation") - return ctrl.Result{}, nil - } - - result, err = r.deletionPhase(ctx, log, req) - if err != nil { - err = fmt.Errorf("deletion: %w", err) - if result == nil { - return ctrl.Result{}, err - } - return *result, err - } - if result != nil { - return *result, nil - } - - result, err = r.createUpdatePhase(ctx, log, req) - if err != nil { - err = fmt.Errorf("deletion: %w", err) - if result == nil { - return ctrl.Result{}, err - } - return *result, err - } - if result != nil { - return *result, nil - } - - return ctrl.Result{}, nil -} - -func (r *StreamReconciler) initializationPhase(ctx context.Context, log *logr.Logger, req ctrl.Request) (*ctrl.Result, error) { - // Fetch stream resource stream := &api.Stream{} if err := r.Get(ctx, req.NamespacedName, stream); err != nil { if apierrors.IsNotFound(err) { - log.Info("stream resource not found. Ignoring since object must be deleted") - return &ctrl.Result{}, nil - } - return &ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) - } - - // Add finalizer - if !controllerutil.ContainsFinalizer(stream, streamFinalizer) { - log.Info("Adding stream finalizer") - if ok := controllerutil.AddFinalizer(stream, streamFinalizer); !ok { - log.Error(nil, "Failed to add finalizer to stream resource") - return &ctrl.Result{}, nil // TODO how to handle - } - - if err := r.Update(ctx, stream); err != nil { - return &ctrl.Result{}, fmt.Errorf("update stream resource to add finalizer: %w", err) - } - - // re-fetch stream - if err := r.Get(ctx, req.NamespacedName, stream); err != nil { - if apierrors.IsNotFound(err) { - log.Info("stream resource not found. Ignoring since object must be deleted") - return &ctrl.Result{}, nil - } - return &ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + log.Info("Stream resource not found. Ignoring since object must be deleted.") + return ctrl.Result{}, nil } + return ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) } // Update ready status to unknown when no status is set if stream.Status.Conditions == nil || len(stream.Status.Conditions) == 0 { - log.Info("Setting initial ready condition to unknown") + log.Info("Setting initial ready condition to unknown.") stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionUnknown, "Reconciling", "Starting reconciliation") err := r.Status().Update(ctx, stream) if err != nil { - return &ctrl.Result{}, fmt.Errorf("set condition unknown: %w", err) - } - - // re-fetch stream - if err := r.Get(ctx, req.NamespacedName, stream); err != nil { - if apierrors.IsNotFound(err) { - log.Info("stream resource not found. Ignoring since object must be deleted") - return &ctrl.Result{}, nil - } - return &ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) + return ctrl.Result{}, fmt.Errorf("set condition unknown: %w", err) } + return ctrl.Result{Requeue: true}, nil } - return nil, nil -} + // Add finalizer + if !controllerutil.ContainsFinalizer(stream, streamFinalizer) { + log.Info("Adding stream finalizer.") + if ok := controllerutil.AddFinalizer(stream, streamFinalizer); !ok { + return ctrl.Result{}, errors.New("failed to add finalizer to stream resource") + } -func (r *StreamReconciler) deletionPhase(ctx context.Context, logger logr.Logger, req ctrl.Request) (*ctrl.Result, error) { - // Fetch stream resource - stream := &api.Stream{} - if err := r.Get(ctx, req.NamespacedName, stream); err != nil { - if apierrors.IsNotFound(err) { - log.Info("stream resource not found. Ignoring since object must be deleted") - return &ctrl.Result{}, nil + if err := r.Update(ctx, stream); err != nil { + return ctrl.Result{}, fmt.Errorf("update stream resource to add finalizer: %w", err) } - return &ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) + return ctrl.Result{}, nil } - // Delete if marked for deletion and has stream finalizer + // Check Deletion markedForDeletion := stream.GetDeletionTimestamp() != nil - // TODO deletion is triggered multiple times?. Set additional status condition? - if markedForDeletion && controllerutil.ContainsFinalizer(stream, streamFinalizer) { - // Set status to not unknown - stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionUnknown, "Finalizing", "Performing finalizer operations.") - if err := r.Status().Update(ctx, stream); err != nil { - return &ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) - } - - log.Info("performing finalizing operations") - if stream.Spec.PreventDelete { - log.Info("skip delete stream during resource deletion.", "streamName", stream.Spec.Name) - } else { - // Remove stream - err := r.WithJetStreamClient(mapToConnectionOptions(stream.Spec), func(js jetstream.JetStream) error { - return js.DeleteStream(ctx, stream.Spec.Name) - }) - if errors.Is(err, jetstream.ErrStreamNotFound) { - log.Info("managed stream was already deleted") - } else if err != nil { - return &ctrl.Result{}, fmt.Errorf("delete stream during finalization: %w", err) + if markedForDeletion { + if controllerutil.ContainsFinalizer(stream, streamFinalizer) { + err := r.deleteStream(ctx, log, stream) + if err != nil { + return ctrl.Result{}, fmt.Errorf("delete stream: %w", err) } + } else { + log.Info("Stream marked for deletion and already finalized. Ignoring.") } - // Re-Fetch resource - if err := r.Get(ctx, req.NamespacedName, stream); err != nil { - if apierrors.IsNotFound(err) { - log.Info("stream resource not found. Ignoring since object must be deleted") - return &ctrl.Result{}, nil - } - return &ctrl.Result{}, fmt.Errorf("re-fetch stream resource: %w", err) - } + return ctrl.Result{}, nil + } - // Update status to not ready - stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionFalse, "Finalizing", "Performed finalizer operations.") - if err := r.Status().Update(ctx, stream); err != nil { - return &ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) - } + // Create or update stream + if err := r.createOrUpdate(ctx, log, stream); err != nil { + return ctrl.Result{}, fmt.Errorf("create or update: %s", err) + } + return ctrl.Result{}, nil +} - if err := r.Status().Update(ctx, stream); err != nil { - return &ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) - } +func (r *StreamReconciler) deleteStream(ctx context.Context, log logr.Logger, stream *api.Stream) error { - log.Info("Removing stream finalizer after performing finalizing operations") - if ok := controllerutil.RemoveFinalizer(stream, streamFinalizer); !ok { - return &ctrl.Result{Requeue: true}, errors.New("failed to remove stream finalizer") - } + // Set status to not false + stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionFalse, "Finalizing", "Performing finalizer operations.") + if err := r.Status().Update(ctx, stream); err != nil { + return fmt.Errorf("update ready condition: %w", err) + } - if err := r.Update(ctx, stream); err != nil { - return &ctrl.Result{}, fmt.Errorf("remove finalizer: %w", err) + if stream.Spec.PreventDelete { + log.Info("PreventDelete set: skipping stream deletion.", "streamName", stream.Spec.Name) + } else if r.ReadOnly() { + log.Info("Read-only mode: skipping stream deletion.", "streamName", stream.Spec.Name) + } else { + log.Info("Deleting stream.") + err := r.WithJetStreamClient(mapToConnectionOptions(stream.Spec), func(js jetstream.JetStream) error { + return js.DeleteStream(ctx, stream.Spec.Name) + }) + if errors.Is(err, jetstream.ErrStreamNotFound) { + log.Info("Managed stream was already deleted.") + } else if err != nil { + return fmt.Errorf("delete stream during finalization: %w", err) } + } - return &ctrl.Result{}, nil + log.Info("Stream deleted. Removing stream finalizer.") + if ok := controllerutil.RemoveFinalizer(stream, streamFinalizer); !ok { + return errors.New("failed to remove stream finalizer") } - return nil, nil -} + if err := r.Update(ctx, stream); err != nil { + return fmt.Errorf("remove finalizer: %w", err) + } -func (r *StreamReconciler) createUpdatePhase(ctx context.Context, logger logr.Logger, req ctrl.Request) (*ctrl.Result, error) { + return nil +} - // Fetch stream resource - stream := &api.Stream{} - if err := r.Get(ctx, req.NamespacedName, stream); err != nil { - if apierrors.IsNotFound(err) { - log.Info("stream resource not found. Ignoring since object must be deleted") - return &ctrl.Result{}, nil - } - return &ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) - } +func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, stream *api.Stream) error { // Create or Update the stream based on the spec if stream.Spec.PreventUpdate { - log.Info("Spec.PreventUpdate: skip create/updating the stream during reconciliation.", "streamName", stream.Spec.Name) - return &ctrl.Result{}, nil + log.Info("PreventUpdate is set: skipping stream creation/update.", "streamName", stream.Spec.Name) + return nil + } else if r.ReadOnly() { + log.Info("Read-only mode: skipping stream creation/update.", "streamName", stream.Spec.Name) + return nil } // Map spec to stream targetConfig targetConfig, err := mapSpecToConfig(&stream.Spec) if err != nil { - return &ctrl.Result{}, fmt.Errorf("map spec to stream targetConfig: %w", err) + return fmt.Errorf("map spec to stream targetConfig: %w", err) } // CreateOrUpdateStream is called on every reconciliation when the stream is not to be deleted. - // TODO(future-feature): Do we need to check if generation has changed or the config differs? + // TODO(future-feature): Do we need to check if config differs? err = r.WithJetStreamClient(mapToConnectionOptions(stream.Spec), func(js jetstream.JetStream) error { log.Info("create or update stream", "streamName", targetConfig.Name) _, err = js.CreateOrUpdateStream(ctx, targetConfig) @@ -261,9 +172,9 @@ func (r *StreamReconciler) createUpdatePhase(ctx context.Context, logger logr.Lo err = fmt.Errorf("create or update stream: %w", err) stream.Status.Conditions = updateReadyCondition(stream.Status.Conditions, v1.ConditionFalse, "Errored", err.Error()) if err := r.Status().Update(ctx, stream); err != nil { - log.Error(err, "failed to update ready condition to Errored") + log.Error(err, "Failed to update ready condition to Errored.") } - return &ctrl.Result{}, err + return err } // update the observed generation and ready status @@ -272,14 +183,14 @@ func (r *StreamReconciler) createUpdatePhase(ctx context.Context, logger logr.Lo stream.Status.Conditions, v1.ConditionTrue, "Reconciling", - "Stream successfully created or updated", + "Stream successfully created or updated.", ) err = r.Status().Update(ctx, stream) if err != nil { - return &ctrl.Result{}, fmt.Errorf("update ready condition: %w", err) + return fmt.Errorf("update ready condition: %w", err) } - return nil, nil + return nil } func mapToConnectionOptions(spec api.StreamSpec) *connectionOptions { @@ -463,5 +374,8 @@ func mapStreamSource(ss *api.StreamSource) (*jetstream.StreamSource, error) { func (r *StreamReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&api.Stream{}). + Owns(&api.Stream{}). + // Only trigger on generation changes + WithEventFilter(predicate.GenerationChangedPredicate{}). Complete(r) } diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index e9d8091a..26ca41a4 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -36,454 +36,493 @@ import ( ) var _ = Describe("Stream Controller", func() { - When("reconciling a resource", func() { - const resourceName = "test-stream" - const streamName = "orders" - typeNamespacedName := types.NamespacedName{ - Name: resourceName, - Namespace: "default", // TODO(user):Modify as needed - } + // The test stream resource + const resourceName = "test-stream" + const streamName = "orders" + typeNamespacedName := types.NamespacedName{ + Name: resourceName, + Namespace: "default", + } + stream := &api.Stream{} + + // The tested controller + var controller *StreamReconciler + + // Config to create minimal nats stream + emptyStreamConfig := jetstream.StreamConfig{ + Name: streamName, + Replicas: 1, + Retention: jetstream.WorkQueuePolicy, + Discard: jetstream.DiscardOld, + Storage: jetstream.FileStorage, + } - emptyStreamConfig := jetstream.StreamConfig{ - Name: streamName, - Replicas: 1, - Retention: jetstream.WorkQueuePolicy, - Discard: jetstream.DiscardOld, - Storage: jetstream.FileStorage, + BeforeEach(func(ctx SpecContext) { + By("creating a test stream resource") + err := k8sClient.Get(ctx, typeNamespacedName, stream) + if err != nil && k8serrors.IsNotFound(err) { + resource := &api.Stream{ + ObjectMeta: metav1.ObjectMeta{ + Name: resourceName, + Namespace: "default", + }, + Spec: api.StreamSpec{ + Name: streamName, + Replicas: 1, + Subjects: []string{"tests.*"}, + Description: "test stream", + Retention: "workqueue", + Discard: "old", + Storage: "file", + }, + } + Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + // Re-fetch stream + Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) } - stream := &api.Stream{} + By("Checking precondition: nats stream does not exist") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) - BeforeEach(func(ctx SpecContext) { - By("creating the custom resource for the Kind Stream") - err := k8sClient.Get(ctx, typeNamespacedName, stream) - if err != nil && k8serrors.IsNotFound(err) { - resource := &api.Stream{ - ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, - Namespace: "default", - }, - Spec: api.StreamSpec{ - Name: streamName, - Replicas: 1, - Subjects: []string{"tests.*"}, - Description: "test stream", - Retention: "workqueue", - Discard: "old", - Storage: "file", - }, - } - Expect(k8sClient.Create(ctx, resource)).To(Succeed()) - // Re-fetch stream - Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) - - } - }) + By("Setting up the tested controller") + controller = &StreamReconciler{ + baseController, + } + }) - AfterEach(func(ctx SpecContext) { - // Get and remove test stream resource if it exists - resource := &api.Stream{} - err := k8sClient.Get(ctx, typeNamespacedName, resource) - if err != nil { - Expect(err).To(MatchError(k8serrors.IsNotFound, "Is not found")) - } else { + AfterEach(func(ctx SpecContext) { + By("removing the test stream resource") + resource := &api.Stream{} + err := k8sClient.Get(ctx, typeNamespacedName, resource) + if err != nil { + Expect(err).To(MatchError(k8serrors.IsNotFound, "Is not found")) + } else { + if controllerutil.ContainsFinalizer(resource, streamFinalizer) { By("Removing the finalizer") controllerutil.RemoveFinalizer(resource, streamFinalizer) Expect(k8sClient.Update(ctx, resource)).To(Succeed()) - - By("Cleanup the specific resource instance Stream") - Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) } - By("Deleting the stream") - err = jsClient.DeleteStream(ctx, streamName) - if err != nil { - Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) - } - }) + By("Removing the stream resource") + Expect(k8sClient.Delete(ctx, resource)). + To(SatisfyAny( + Succeed(), + MatchError(k8serrors.IsNotFound, "is not found"), + )) + } - It("should successfully create the stream", func(ctx SpecContext) { - By("Reconciling the created resource") - controllerReconciler := &StreamReconciler{ - baseController, - } + By("deleting the nats stream") + Expect(jsClient.DeleteStream(ctx, streamName)). + To(SatisfyAny( + Succeed(), + MatchError(jetstream.ErrStreamNotFound), + )) + }) - _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - Expect(err).NotTo(HaveOccurred()) + When("reconciling a not existing resource", func() { + It("stop reconciliation", func(ctx SpecContext) { - // Fetch resource - err = k8sClient.Get(ctx, typeNamespacedName, stream) + By("Reconciling the created resource") + result, err := controller.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "fake", + Name: "not-existing", + }, + }) Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(ctrl.Result{})) + }) + }) - By("Checking if the ready state was updated") - readyCondition := api.Condition{ - Type: readyCondType, - Status: v1.ConditionTrue, - Reason: "Reconciling", - Message: "Stream successfully created or updated", - } - gotCondition := stream.Status.Conditions[0] - // Unset LastTransitionTime to be equal for assertion - gotCondition.LastTransitionTime = "" - Expect(gotCondition).To(Equal(readyCondition)) + When("reconciling a not initialized resource", func() { - By("Checking if the observed generation matches") - Expect(stream.Status.ObservedGeneration).To(Equal(stream.Generation)) + It("should initialize a new resource", func(ctx SpecContext) { - By("Checking if the finalizer was set") - Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) + By("re-queueing until it is initialized") + // Initialization can require multiple reconciliation loops + Eventually(func(ctx SpecContext) *api.Stream { + _, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + got := &api.Stream{} + Expect(k8sClient.Get(ctx, typeNamespacedName, got)).To(Succeed()) + return got + }).WithContext(ctx). + Should(SatisfyAll( + HaveField("Finalizers", HaveExactElements(streamFinalizer)), + HaveField("Status.Conditions", Not(BeEmpty())), + )) + + By("validating the ready condition") + // Fetch stream + Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) + Expect(stream.Status.Conditions).To(HaveLen(1)) - By("Checking if the stream was created") - natsStream, err := jsClient.Stream(ctx, streamName) - Expect(err).NotTo(HaveOccurred()) - streamInfo, err := natsStream.Info(ctx) - Expect(err).NotTo(HaveOccurred()) - Expect(streamInfo.Config.Name).To(Equal(streamName)) - Expect(streamInfo.Created).To(BeTemporally("~", time.Now(), time.Second)) + assertReadyStateMatches(stream.Status.Conditions[0], v1.ConditionUnknown, "Reconciling", "Starting reconciliation", time.Now()) }) - It("should successfully update the stream and update the status", func(ctx SpecContext) { + }) - By("Creating the stream with empty subjects and description") - _, err := jsClient.CreateStream(ctx, emptyStreamConfig) - Expect(err).NotTo(HaveOccurred()) + When("reconciling an initialized resource", func() { - By("Reconciling the created resource") - controllerReconciler := &StreamReconciler{ - baseController, - } + BeforeEach(func(ctx SpecContext) { + By("Initializing the stream resource") - _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) + By("Setting the finalizer") + Expect(controllerutil.AddFinalizer(stream, streamFinalizer)).To(BeTrue()) + Expect(k8sClient.Update(ctx, stream)).To(Succeed()) + + By("Setting an unknown ready state") + stream.Status.Conditions = []api.Condition{{ + Type: readyCondType, + Status: v1.ConditionUnknown, + Reason: "Test", + Message: "start condition", + LastTransitionTime: time.Now().Format(time.RFC3339Nano), + }} + Expect(k8sClient.Status().Update(ctx, stream)).To(Succeed()) + + }) + + It("should create a new stream", func(ctx SpecContext) { + + By("Running Reconcile") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) // Fetch resource - err = k8sClient.Get(ctx, typeNamespacedName, stream) - Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) By("Checking if the ready state was updated") - readyCondition := api.Condition{ - Type: readyCondType, - Status: v1.ConditionTrue, - Reason: "Reconciling", - Message: "Stream successfully created or updated", - } - gotCondition := stream.Status.Conditions[0] - // Unset LastTransitionTime to be equal for assertion - gotCondition.LastTransitionTime = "" - Expect(gotCondition).To(Equal(readyCondition)) + Expect(stream.Status.Conditions).To(HaveLen(1)) + assertReadyStateMatches(stream.Status.Conditions[0], v1.ConditionTrue, "Reconciling", "created or updated", time.Now()) By("Checking if the observed generation matches") Expect(stream.Status.ObservedGeneration).To(Equal(stream.Generation)) - By("Checking if the stream was updated") + By("Checking if the stream was created") natsStream, err := jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) - streamInfo, err := natsStream.Info(ctx) Expect(err).NotTo(HaveOccurred()) + Expect(streamInfo.Config.Name).To(Equal(streamName)) Expect(streamInfo.Config.Description).To(Equal("test stream")) - Expect(streamInfo.Config.Subjects).To(Equal([]string{"tests.*"})) + Expect(streamInfo.Created).To(BeTemporally("~", time.Now(), time.Second)) }) - It("should not fail on not existing resource", func(ctx SpecContext) { - By("Reconciling the created resource") - controllerReconciler := &StreamReconciler{ - baseController, - } + When("PreventUpdate is set", func() { - result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: "fake", - Name: "not-existing", - }, + BeforeEach(func(ctx SpecContext) { + By("Setting preventDelete on the resource") + stream.Spec.PreventUpdate = true + Expect(k8sClient.Update(ctx, stream)).To(Succeed()) }) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(ctrl.Result{})) - }) - - It("should set an error state, register finalizer and re-queue when the nats server is not available", func(ctx SpecContext) { - By("Reconciling the created resource") + It("should not create the stream", func(ctx SpecContext) { + By("Running Reconcile") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - // Setup client for not running server - // Use actual test server to ensure port not used by other service on test instance - sv := CreateTestServer() - controller, err := NewJSController(k8sClient, &NatsConfig{ServerURL: sv.ClientURL()}, &Config{}) - Expect(err).NotTo(HaveOccurred()) - sv.Shutdown() + By("checking that no stream was created") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + }) + It("should not update the stream", func(ctx SpecContext) { + By("creating the stream") + _, err := jsClient.CreateStream(ctx, emptyStreamConfig) + Expect(err).NotTo(HaveOccurred()) - controllerReconciler := &StreamReconciler{ - controller, - } + By("Running Reconcile") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, + By("checking that stream was not updated") + s, err := jsClient.Stream(ctx, streamName) + Expect(s.CachedInfo().Config.Description).To(BeEmpty()) }) - Expect(result).To(Equal(ctrl.Result{})) - Expect(err).To(HaveOccurred()) // Will be re-queued with back-off + }) - // Fetch resource - err = k8sClient.Get(ctx, typeNamespacedName, stream) - Expect(err).NotTo(HaveOccurred()) + When("read-only mode is enabled", func() { - By("Checking if the status was updated") - Expect(stream.Status.Conditions).To(HaveLen(1)) + BeforeEach(func(ctx SpecContext) { + By("Setting read only on the controller") + readOnly, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{ReadOnly: true}) + Expect(err).NotTo(HaveOccurred()) + controller = &StreamReconciler{ + JetStreamController: readOnly, + } + }) - cond := stream.Status.Conditions[0] - Expect(cond.Type).To(Equal(readyCondType)) - Expect(cond.Status).To(Equal(v1.ConditionFalse)) - Expect(cond.Reason).To(Equal("Errored")) - Expect(cond.Message).To(HavePrefix("create or update stream:")) - Expect(cond.LastTransitionTime).NotTo(BeEmpty()) + It("should not create the stream", func(ctx SpecContext) { + By("Running Reconcile") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - By("Checking if the observed generation does not match") - Expect(stream.Status.ObservedGeneration).ToNot(Equal(stream.Generation)) + By("checking that no stream was created") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + }) + It("should not update the stream", func(ctx SpecContext) { + By("creating the stream") + _, err := jsClient.CreateStream(ctx, emptyStreamConfig) + Expect(err).NotTo(HaveOccurred()) + + By("Running Reconcile") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - By("Checking if the finalizer was set") - Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) + By("checking that stream was not updated") + s, err := jsClient.Stream(ctx, streamName) + Expect(s.CachedInfo().Config.Description).To(BeEmpty()) + }) }) - It("should delete stream marked for deletion", func(ctx SpecContext) { + When("namespace restriction is enabled", func() { - By("Reconciling the created resource once to ensure the finalizer is set and stream created") - controllerReconciler := &StreamReconciler{ - baseController, - } - - _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, + BeforeEach(func(ctx SpecContext) { + namespaced, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{Namespace: "other-namespace"}) + Expect(err).NotTo(HaveOccurred()) + controller = &StreamReconciler{ + JetStreamController: namespaced, + } }) - Expect(err).NotTo(HaveOccurred()) - - // Stream exists - _, err = jsClient.Stream(ctx, streamName) - Expect(err).NotTo(HaveOccurred()) - By("Marking the resource as deleted") - Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + It("should not create the stream", func(ctx SpecContext) { + By("Running Reconcile") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - By("Reconciling the deleted resource") - _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, + By("checking that no stream was created") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) }) - Expect(err).NotTo(HaveOccurred()) + It("should not update the stream", func(ctx SpecContext) { + By("creating the stream") + _, err := jsClient.CreateStream(ctx, emptyStreamConfig) + Expect(err).NotTo(HaveOccurred()) - By("Checking if the resource was removed") - Eventually(func() error { - found := &api.Stream{} - return k8sClient.Get(ctx, typeNamespacedName, found) - }, 5*time.Second, time.Second).ShouldNot(Succeed()) + By("Running Reconcile") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - By("Checking if the stream was deleted") - _, err = jsClient.Stream(ctx, streamName) - Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + By("checking that stream was not updated") + s, err := jsClient.Stream(ctx, streamName) + Expect(s.CachedInfo().Config.Description).To(BeEmpty()) + }) }) - It("should succeed deleting stream resource where the underlying stream was already deleted", func(ctx SpecContext) { - - By("Reconciling the created resource once to ensure the finalizer is set and stream created") - controllerReconciler := &StreamReconciler{ - baseController, - } - - _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) + It("should update an existing stream", func(ctx SpecContext) { + By("Reconciling once to create the stream") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - By("Deleting the managed stream") - err = jsClient.DeleteStream(ctx, streamName) - Expect(err).NotTo(HaveOccurred()) + // Fetch resource + Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) + previousTransitionTime := stream.Status.Conditions[0].LastTransitionTime - By("Marking the resource as deleted") - Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + By("Updating the resource") + stream.Spec.Description = "new description" + Expect(k8sClient.Update(ctx, stream)).To(Succeed()) - By("Reconciling the deleted resource") - _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) + By("reconciling the updated resource") + result, err = controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - By("Checking if the resource was removed") - Eventually(func() error { - found := &api.Stream{} - return k8sClient.Get(ctx, typeNamespacedName, found) - }, 5*time.Second, time.Second).ShouldNot(Succeed()) - }) - - When("PreventDelete is set", func() { - It("should not delete stream marked for deletion", func(ctx SpecContext) { + // Fetch resource + Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) - By("Setting preventDelete on the resource") - stream.Spec.PreventDelete = true - Expect(k8sClient.Update(ctx, stream)).To(Succeed()) + By("Checking if the state transition time was not updated") + Expect(stream.Status.Conditions).To(HaveLen(1)) + Expect(stream.Status.Conditions[0].LastTransitionTime).To(Equal(previousTransitionTime)) - By("Reconciling the updated resource once") - controllerReconciler := &StreamReconciler{ - baseController, - } + By("Checking if the observed generation matches") + Expect(stream.Status.ObservedGeneration).To(Equal(stream.Generation)) - _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - Expect(err).NotTo(HaveOccurred()) + By("Checking if the stream was updated") + natsStream, err := jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) - // Stream exists - _, err = jsClient.Stream(ctx, streamName) - Expect(err).NotTo(HaveOccurred()) + streamInfo, err := natsStream.Info(ctx) + Expect(err).NotTo(HaveOccurred()) + Expect(streamInfo.Config.Description).To(Equal("new description")) + // Other fields unchanged + Expect(streamInfo.Config.Subjects).To(Equal([]string{"tests.*"})) + }) - By("Marking the resource as deleted") - Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + It("should set an error state when the nats server is not available", func(ctx SpecContext) { - By("Reconciling the deleted resource") - _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - Expect(err).NotTo(HaveOccurred()) + By("Setting up controller with unavailable nats server") + // Setup client for not running server + // Use actual test server to ensure port not used by other service on test instance + sv := CreateTestServer() + base, err := NewJSController(k8sClient, &NatsConfig{ServerURL: sv.ClientURL()}, &Config{}) + Expect(err).NotTo(HaveOccurred()) + sv.Shutdown() - By("Checking if the resource was removed") - Eventually(func() error { - found := &api.Stream{} - return k8sClient.Get(ctx, typeNamespacedName, found) - }, 5*time.Second, time.Second).ShouldNot(Succeed()) + controller := &StreamReconciler{ + base, + } - By("Checking if the stream was *not* deleted") - _, err = jsClient.Stream(ctx, streamName) - Expect(err).NotTo(HaveOccurred()) + By("reconciling resource") + result, err := controller.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, }) - }) + Expect(result).To(Equal(ctrl.Result{})) + Expect(err).To(HaveOccurred()) // Will be re-queued with back-off - When("PreventUpdate is set", func() { - BeforeEach(func(ctx SpecContext) { - By("Setting prevent update") + // Fetch resource + err = k8sClient.Get(ctx, typeNamespacedName, stream) + Expect(err).NotTo(HaveOccurred()) - stream.Spec.PreventUpdate = true - Expect(k8sClient.Update(ctx, stream)).To(Succeed()) - }) + By("Checking if the status was updated") + Expect(stream.Status.Conditions).To(HaveLen(1)) + assertReadyStateMatches( + stream.Status.Conditions[0], + v1.ConditionFalse, + "Errored", + "create or update stream:", + time.Now(), + ) - It("should not create stream", func(ctx SpecContext) { - By("Reconciling the updated resource") - controllerReconciler := &StreamReconciler{ - baseController, - } - _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - Expect(err).NotTo(HaveOccurred()) + By("Checking if the observed generation does not match") + Expect(stream.Status.ObservedGeneration).ToNot(Equal(stream.Generation)) + }) - By("Checking that the stream was not created") - _, err = jsClient.Stream(ctx, streamName) - Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + When("the resource is marked for deletion", func() { - By("Checking that the streams status was not updated") - err = k8sClient.Get(ctx, typeNamespacedName, stream) - Expect(err).NotTo(HaveOccurred()) - Expect(stream.Status.ObservedGeneration).To(BeNumerically("<", stream.Generation)) + BeforeEach(func(ctx SpecContext) { + By("Marking the resource for deletion") + Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) // re-fetch after update }) - It("should not update stream", func(ctx SpecContext) { - By("Creating the stream with empty subjects and description") - _, err := jsClient.CreateStream(ctx, emptyStreamConfig) - Expect(err).NotTo(HaveOccurred()) - - By("Reconciling the resource") - controllerReconciler := &StreamReconciler{ - baseController, - } - _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - Expect(err).NotTo(HaveOccurred()) - - By("Checking that the stream was not updated") - s, err := jsClient.Stream(ctx, streamName) + It("should succeed deleting a not existing stream", func(ctx SpecContext) { + By("Reconciling") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) - Expect(s.CachedInfo().Config.Description).To(BeEmpty()) + Expect(result.IsZero()).To(BeTrue()) - By("Checking that the streams generation was not updated") - err = k8sClient.Get(ctx, typeNamespacedName, stream) - Expect(err).NotTo(HaveOccurred()) - Expect(stream.Status.ObservedGeneration).To(BeNumerically("<", stream.Generation)) + By("Checking that the resource is deleted") + Eventually(k8sClient.Get). + WithArguments(ctx, typeNamespacedName, stream). + ShouldNot(Succeed()) }) - }) - - DescribeTableSubtree("controller restricted to not perform any updates on resource", - func(getController func(SpecContext) StreamReconciler) { - var controller StreamReconciler + When("the underlying stream exists", func() { BeforeEach(func(ctx SpecContext) { - controller = getController(ctx) - }) - - It("should not create stream", func(ctx SpecContext) { - By("reconciling the resource") - _, err := controller.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName}) - Expect(err).NotTo(HaveOccurred()) - - By("Checking that the stream was not created") - _, err = jsClient.Stream(ctx, streamName) - Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) - }) - It("should not update stream", func(ctx SpecContext) { - By("creating the stream") + By("Creating the stream on the nats server") _, err := jsClient.CreateStream(ctx, emptyStreamConfig) Expect(err).NotTo(HaveOccurred()) + }) - By("reconciling the resource") - _, err = controller.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName}) - Expect(err).NotTo(HaveOccurred()) - - By("Checking that the stream was not updated") - s, err := jsClient.Stream(ctx, streamName) - Expect(err).NotTo(HaveOccurred()) - Expect(s.CachedInfo().Config.Description).To(BeEmpty()) - - By("Checking that the streams generation was not updated") - err = k8sClient.Get(ctx, typeNamespacedName, stream) - Expect(err).NotTo(HaveOccurred()) - Expect(stream.Status.ObservedGeneration).To(BeNumerically("<", stream.Generation)) + AfterEach(func(ctx SpecContext) { + err := jsClient.DeleteStream(ctx, streamName) + if err != nil { + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) + } }) - It("should not delete stream", func(ctx SpecContext) { - By("creating the stream") - _, err := jsClient.CreateStream(ctx, emptyStreamConfig) + It("should delete the stream", func(ctx SpecContext) { + By("Reconciling") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) - By("Marking the resource as deleted") - Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) + By("Checking that the stream is deleted") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) - By("reconciling the resource") - _, err = controller.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName}) - Expect(err).NotTo(HaveOccurred()) + By("Checking that the resource is deleted") + Eventually(k8sClient.Get). + WithArguments(ctx, typeNamespacedName, stream). + ShouldNot(Succeed()) + }) - By("Checking if the resource was removed") - Eventually(func() error { - found := &api.Stream{} - return k8sClient.Get(ctx, typeNamespacedName, found) - }, 5*time.Second, time.Second).ShouldNot(Succeed()) + When("PreventDelete is set", func() { + BeforeEach(func(ctx SpecContext) { + By("Setting preventDelete on the resource") + stream.Spec.PreventDelete = true + Expect(k8sClient.Update(ctx, stream)).To(Succeed()) + }) + It("Should delete the resource and not delete the nats stream", func(ctx SpecContext) { + By("Reconciling") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) + + By("Checking that the stream is not deleted") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + + By("Checking that the resource is deleted") + Eventually(k8sClient.Get). + WithArguments(ctx, typeNamespacedName, stream). + ShouldNot(Succeed()) + }) + }) - By("Checking if the stream was *not* deleted") - _, err = jsClient.Stream(ctx, streamName) - Expect(err).NotTo(HaveOccurred()) + When("read only is set", func() { + BeforeEach(func(ctx SpecContext) { + By("Setting read only on the controller") + readOnly, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{ReadOnly: true}) + Expect(err).NotTo(HaveOccurred()) + controller = &StreamReconciler{ + JetStreamController: readOnly, + } + }) + It("should delete the resource and not delete the stream", func(ctx SpecContext) { + + By("Reconciling") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) + + By("Checking that the stream is not deleted") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + + By("Checking that the resource is deleted") + Eventually(k8sClient.Get). + WithArguments(ctx, typeNamespacedName, stream). + ShouldNot(Succeed()) + }) }) - }, - // Test cases provide functions for providing a configured the controller and setup preconditions - Entry("read only", func(ctx SpecContext) StreamReconciler { - readOnly, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{ReadOnly: true}) - Expect(err).NotTo(HaveOccurred()) - return StreamReconciler{readOnly} - }), - Entry("namespaced", func(ctx SpecContext) StreamReconciler { - readOnly, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{Namespace: "some-namespace"}) - Expect(err).NotTo(HaveOccurred()) - return StreamReconciler{readOnly} - }), - ) + When("controller is restricted to different namespace", func() { + BeforeEach(func(ctx SpecContext) { + namespaced, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{Namespace: "other-namespace"}) + Expect(err).NotTo(HaveOccurred()) + controller = &StreamReconciler{ + JetStreamController: namespaced, + } + }) + It("should not delete the resource and stream", func(ctx SpecContext) { + + By("Reconciling") + result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) + + By("Checking that the stream is not deleted") + _, err = jsClient.Stream(ctx, streamName) + Expect(err).NotTo(HaveOccurred()) + + By("Checking that the finalizer is not removed") + Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) + Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) + }) + }) + }) + }) It("should update stream on different server as specified in spec", func(ctx SpecContext) { By("Setting up the alternative server") @@ -500,15 +539,11 @@ var _ = Describe("Stream Controller", func() { Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) By("Reconciling the resource") - controllerReconciler := &StreamReconciler{ - baseController, - } - - result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + result, err := controller.Reconcile(ctx, reconcile.Request{ NamespacedName: typeNamespacedName, }) - Expect(result).To(Equal(ctrl.Result{})) Expect(err).NotTo(HaveOccurred()) + Expect(result.IsZero()).To(BeTrue()) By("Checking if the stream was created on the alternative server") altClient, closer, err := CreateJetStreamClient(&NatsConfig{ServerURL: altServer.ClientURL()}, true) @@ -705,9 +740,9 @@ func Test_mapSpecToConfig(t *testing.T) { // Compare nested structs assert.EqualValues(tt.want, got) - //if !reflect.DeepEqual(got, tt.want) { - // t.Errorf("mapSpecToConfig() \ngot = %v\nwant = %v", got, tt.want) - //} + // if !reflect.DeepEqual(got, tt.want) { + // t.Errorf("mapSpecToConfig() \ngot = %v\nwant = %v", got, tt.want) + // } }) } } diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 86f6906d..dc7a59b9 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -19,7 +19,6 @@ package controller import ( "fmt" "github.com/nats-io/nats-server/v2/server" - natsserver "github.com/nats-io/nats-server/v2/test" "github.com/nats-io/nats.go/jetstream" "os" "path/filepath" @@ -105,19 +104,3 @@ var _ = AfterSuite(func() { err = os.RemoveAll(storeDir) Expect(err).NotTo(HaveOccurred()) }) - -func CreateTestServer() *server.Server { - opts := &natsserver.DefaultTestOptions - opts.JetStream = true - opts.Port = -1 - opts.Debug = true - - dir, err := os.MkdirTemp("", "nats-*") - Expect(err).NotTo(HaveOccurred()) - opts.StoreDir = dir - - ns := natsserver.RunServer(opts) - Expect(err).NotTo(HaveOccurred()) - - return ns -} From 8331720f32d606eb498fa41f3172b3f6787fb66e Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 11 Dec 2024 14:14:00 +0100 Subject: [PATCH 24/35] make test description strings more uniform --- internal/controller/stream_controller_test.go | 111 +++++++++--------- 1 file changed, 54 insertions(+), 57 deletions(-) diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 26ca41a4..2e1bbc26 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -82,11 +82,11 @@ var _ = Describe("Stream Controller", func() { Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) } - By("Checking precondition: nats stream does not exist") + By("checking precondition: nats stream does not exist") _, err = jsClient.Stream(ctx, streamName) Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) - By("Setting up the tested controller") + By("setting up the tested controller") controller = &StreamReconciler{ baseController, } @@ -100,12 +100,12 @@ var _ = Describe("Stream Controller", func() { Expect(err).To(MatchError(k8serrors.IsNotFound, "Is not found")) } else { if controllerutil.ContainsFinalizer(resource, streamFinalizer) { - By("Removing the finalizer") + By("removing the finalizer") controllerutil.RemoveFinalizer(resource, streamFinalizer) Expect(k8sClient.Update(ctx, resource)).To(Succeed()) } - By("Removing the stream resource") + By("removing the stream resource") Expect(k8sClient.Delete(ctx, resource)). To(SatisfyAny( Succeed(), @@ -122,9 +122,8 @@ var _ = Describe("Stream Controller", func() { }) When("reconciling a not existing resource", func() { - It("stop reconciliation", func(ctx SpecContext) { - - By("Reconciling the created resource") + It("should stop reconciliation without error", func(ctx SpecContext) { + By("reconciling the created resource") result, err := controller.Reconcile(ctx, reconcile.Request{ NamespacedName: types.NamespacedName{ Namespace: "fake", @@ -167,13 +166,13 @@ var _ = Describe("Stream Controller", func() { When("reconciling an initialized resource", func() { BeforeEach(func(ctx SpecContext) { - By("Initializing the stream resource") + By("initializing the stream resource") - By("Setting the finalizer") + By("setting the finalizer") Expect(controllerutil.AddFinalizer(stream, streamFinalizer)).To(BeTrue()) Expect(k8sClient.Update(ctx, stream)).To(Succeed()) - By("Setting an unknown ready state") + By("setting an unknown ready state") stream.Status.Conditions = []api.Condition{{ Type: readyCondType, Status: v1.ConditionUnknown, @@ -187,7 +186,7 @@ var _ = Describe("Stream Controller", func() { It("should create a new stream", func(ctx SpecContext) { - By("Running Reconcile") + By("running Reconcile") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) @@ -195,14 +194,14 @@ var _ = Describe("Stream Controller", func() { // Fetch resource Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) - By("Checking if the ready state was updated") + By("checking if the ready state was updated") Expect(stream.Status.Conditions).To(HaveLen(1)) assertReadyStateMatches(stream.Status.Conditions[0], v1.ConditionTrue, "Reconciling", "created or updated", time.Now()) - By("Checking if the observed generation matches") + By("checking if the observed generation matches") Expect(stream.Status.ObservedGeneration).To(Equal(stream.Generation)) - By("Checking if the stream was created") + By("checking if the stream was created") natsStream, err := jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) streamInfo, err := natsStream.Info(ctx) @@ -215,12 +214,12 @@ var _ = Describe("Stream Controller", func() { When("PreventUpdate is set", func() { BeforeEach(func(ctx SpecContext) { - By("Setting preventDelete on the resource") + By("setting preventDelete on the resource") stream.Spec.PreventUpdate = true Expect(k8sClient.Update(ctx, stream)).To(Succeed()) }) It("should not create the stream", func(ctx SpecContext) { - By("Running Reconcile") + By("running Reconcile") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) @@ -234,7 +233,7 @@ var _ = Describe("Stream Controller", func() { _, err := jsClient.CreateStream(ctx, emptyStreamConfig) Expect(err).NotTo(HaveOccurred()) - By("Running Reconcile") + By("running Reconcile") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) @@ -248,7 +247,7 @@ var _ = Describe("Stream Controller", func() { When("read-only mode is enabled", func() { BeforeEach(func(ctx SpecContext) { - By("Setting read only on the controller") + By("setting read only on the controller") readOnly, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{ReadOnly: true}) Expect(err).NotTo(HaveOccurred()) controller = &StreamReconciler{ @@ -257,7 +256,7 @@ var _ = Describe("Stream Controller", func() { }) It("should not create the stream", func(ctx SpecContext) { - By("Running Reconcile") + By("running Reconcile") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) @@ -271,7 +270,7 @@ var _ = Describe("Stream Controller", func() { _, err := jsClient.CreateStream(ctx, emptyStreamConfig) Expect(err).NotTo(HaveOccurred()) - By("Running Reconcile") + By("running Reconcile") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) @@ -285,6 +284,7 @@ var _ = Describe("Stream Controller", func() { When("namespace restriction is enabled", func() { BeforeEach(func(ctx SpecContext) { + By("setting a namespace on the resource") namespaced, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{Namespace: "other-namespace"}) Expect(err).NotTo(HaveOccurred()) controller = &StreamReconciler{ @@ -293,7 +293,7 @@ var _ = Describe("Stream Controller", func() { }) It("should not create the stream", func(ctx SpecContext) { - By("Running Reconcile") + By("running Reconcile") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) @@ -307,7 +307,7 @@ var _ = Describe("Stream Controller", func() { _, err := jsClient.CreateStream(ctx, emptyStreamConfig) Expect(err).NotTo(HaveOccurred()) - By("Running Reconcile") + By("running Reconcile") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) @@ -319,7 +319,7 @@ var _ = Describe("Stream Controller", func() { }) It("should update an existing stream", func(ctx SpecContext) { - By("Reconciling once to create the stream") + By("reconciling once to create the stream") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) @@ -328,7 +328,7 @@ var _ = Describe("Stream Controller", func() { Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) previousTransitionTime := stream.Status.Conditions[0].LastTransitionTime - By("Updating the resource") + By("updating the resource") stream.Spec.Description = "new description" Expect(k8sClient.Update(ctx, stream)).To(Succeed()) @@ -340,14 +340,14 @@ var _ = Describe("Stream Controller", func() { // Fetch resource Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) - By("Checking if the state transition time was not updated") + By("checking if the state transition time was not updated") Expect(stream.Status.Conditions).To(HaveLen(1)) Expect(stream.Status.Conditions[0].LastTransitionTime).To(Equal(previousTransitionTime)) - By("Checking if the observed generation matches") + By("checking if the observed generation matches") Expect(stream.Status.ObservedGeneration).To(Equal(stream.Generation)) - By("Checking if the stream was updated") + By("checking if the stream was updated") natsStream, err := jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) @@ -360,7 +360,7 @@ var _ = Describe("Stream Controller", func() { It("should set an error state when the nats server is not available", func(ctx SpecContext) { - By("Setting up controller with unavailable nats server") + By("setting up controller with unavailable nats server") // Setup client for not running server // Use actual test server to ensure port not used by other service on test instance sv := CreateTestServer() @@ -383,7 +383,7 @@ var _ = Describe("Stream Controller", func() { err = k8sClient.Get(ctx, typeNamespacedName, stream) Expect(err).NotTo(HaveOccurred()) - By("Checking if the status was updated") + By("checking if the status was updated") Expect(stream.Status.Conditions).To(HaveLen(1)) assertReadyStateMatches( stream.Status.Conditions[0], @@ -393,25 +393,25 @@ var _ = Describe("Stream Controller", func() { time.Now(), ) - By("Checking if the observed generation does not match") + By("checking if the observed generation does not match") Expect(stream.Status.ObservedGeneration).ToNot(Equal(stream.Generation)) }) When("the resource is marked for deletion", func() { BeforeEach(func(ctx SpecContext) { - By("Marking the resource for deletion") + By("marking the resource for deletion") Expect(k8sClient.Delete(ctx, stream)).To(Succeed()) Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) // re-fetch after update }) It("should succeed deleting a not existing stream", func(ctx SpecContext) { - By("Reconciling") + By("reconciling") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) - By("Checking that the resource is deleted") + By("checking that the resource is deleted") Eventually(k8sClient.Get). WithArguments(ctx, typeNamespacedName, stream). ShouldNot(Succeed()) @@ -419,7 +419,7 @@ var _ = Describe("Stream Controller", func() { When("the underlying stream exists", func() { BeforeEach(func(ctx SpecContext) { - By("Creating the stream on the nats server") + By("creating the stream on the nats server") _, err := jsClient.CreateStream(ctx, emptyStreamConfig) Expect(err).NotTo(HaveOccurred()) }) @@ -432,16 +432,16 @@ var _ = Describe("Stream Controller", func() { }) It("should delete the stream", func(ctx SpecContext) { - By("Reconciling") + By("reconciling") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) - By("Checking that the stream is deleted") + By("checking that the stream is deleted") _, err = jsClient.Stream(ctx, streamName) Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) - By("Checking that the resource is deleted") + By("checking that the resource is deleted") Eventually(k8sClient.Get). WithArguments(ctx, typeNamespacedName, stream). ShouldNot(Succeed()) @@ -449,21 +449,21 @@ var _ = Describe("Stream Controller", func() { When("PreventDelete is set", func() { BeforeEach(func(ctx SpecContext) { - By("Setting preventDelete on the resource") + By("setting preventDelete on the resource") stream.Spec.PreventDelete = true Expect(k8sClient.Update(ctx, stream)).To(Succeed()) }) It("Should delete the resource and not delete the nats stream", func(ctx SpecContext) { - By("Reconciling") + By("reconciling") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) - By("Checking that the stream is not deleted") + By("checking that the stream is not deleted") _, err = jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) - By("Checking that the resource is deleted") + By("checking that the resource is deleted") Eventually(k8sClient.Get). WithArguments(ctx, typeNamespacedName, stream). ShouldNot(Succeed()) @@ -472,7 +472,7 @@ var _ = Describe("Stream Controller", func() { When("read only is set", func() { BeforeEach(func(ctx SpecContext) { - By("Setting read only on the controller") + By("setting read only on the controller") readOnly, err := NewJSController(k8sClient, &NatsConfig{ServerURL: testServer.ClientURL()}, &Config{ReadOnly: true}) Expect(err).NotTo(HaveOccurred()) controller = &StreamReconciler{ @@ -481,16 +481,16 @@ var _ = Describe("Stream Controller", func() { }) It("should delete the resource and not delete the stream", func(ctx SpecContext) { - By("Reconciling") + By("reconciling") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) - By("Checking that the stream is not deleted") + By("checking that the stream is not deleted") _, err = jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) - By("Checking that the resource is deleted") + By("checking that the resource is deleted") Eventually(k8sClient.Get). WithArguments(ctx, typeNamespacedName, stream). ShouldNot(Succeed()) @@ -507,16 +507,16 @@ var _ = Describe("Stream Controller", func() { }) It("should not delete the resource and stream", func(ctx SpecContext) { - By("Reconciling") + By("reconciling") result, err := controller.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) - By("Checking that the stream is not deleted") + By("checking that the stream is not deleted") _, err = jsClient.Stream(ctx, streamName) Expect(err).NotTo(HaveOccurred()) - By("Checking that the finalizer is not removed") + By("checking that the finalizer is not removed") Expect(k8sClient.Get(ctx, typeNamespacedName, stream)).To(Succeed()) Expect(stream.Finalizers).To(ContainElement(streamFinalizer)) }) @@ -525,27 +525,27 @@ var _ = Describe("Stream Controller", func() { }) It("should update stream on different server as specified in spec", func(ctx SpecContext) { - By("Setting up the alternative server") + By("setting up the alternative server") // Setup altClient for alternate server altServer := CreateTestServer() defer altServer.Shutdown() - By("Setting the server in the stream spec") + By("setting the server in the stream spec") stream.Spec.Servers = []string{altServer.ClientURL()} Expect(k8sClient.Update(ctx, stream)).To(Succeed()) - By("Checking precondition, that the stream does not yet exist") + By("checking precondition, that the stream does not yet exist") got, err := jsClient.Stream(ctx, streamName) Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) - By("Reconciling the resource") + By("reconciling the resource") result, err := controller.Reconcile(ctx, reconcile.Request{ NamespacedName: typeNamespacedName, }) Expect(err).NotTo(HaveOccurred()) Expect(result.IsZero()).To(BeTrue()) - By("Checking if the stream was created on the alternative server") + By("checking if the stream was created on the alternative server") altClient, closer, err := CreateJetStreamClient(&NatsConfig{ServerURL: altServer.ClientURL()}, true) defer closer.Close() Expect(err).NotTo(HaveOccurred()) @@ -554,7 +554,7 @@ var _ = Describe("Stream Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(got.CachedInfo().Created).To(BeTemporally("~", time.Now(), time.Second)) - By("Checking that the stream was NOT created on the original server") + By("checking that the stream was NOT created on the original server") _, err = jsClient.Stream(ctx, streamName) Expect(err).To(MatchError(jetstream.ErrStreamNotFound)) @@ -740,9 +740,6 @@ func Test_mapSpecToConfig(t *testing.T) { // Compare nested structs assert.EqualValues(tt.want, got) - // if !reflect.DeepEqual(got, tt.want) { - // t.Errorf("mapSpecToConfig() \ngot = %v\nwant = %v", got, tt.want) - // } }) } } From 2f04e7f22149cc1b898c3aab7dec18f9e07d4f3c Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 11 Dec 2024 15:23:58 +0100 Subject: [PATCH 25/35] Update docs and log messages --- internal/controller/stream_controller.go | 37 ++++++++++++++---------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 9946c756..8cf35766 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -44,6 +44,10 @@ type StreamReconciler struct { // - Initialize finalizer and ready condition if not present // - Delete stream if it is marked for deletion. // - Create or Update the stream +// +// A call to reconcile may perform only one action, expecting the reconciliation to be triggered again by an update. +// For example: Setting the finalizer triggers a second reconciliation. Reconcile returns after setting the finalizer, +// to prevent parallel reconciliations performing the same steps. func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := klog.FromContext(ctx) @@ -116,13 +120,9 @@ func (r *StreamReconciler) deleteStream(ctx context.Context, log logr.Logger, st return fmt.Errorf("update ready condition: %w", err) } - if stream.Spec.PreventDelete { - log.Info("PreventDelete set: skipping stream deletion.", "streamName", stream.Spec.Name) - } else if r.ReadOnly() { - log.Info("Read-only mode: skipping stream deletion.", "streamName", stream.Spec.Name) - } else { + if !stream.Spec.PreventDelete && !r.ReadOnly() { log.Info("Deleting stream.") - err := r.WithJetStreamClient(mapToConnectionOptions(stream.Spec), func(js jetstream.JetStream) error { + err := r.WithJetStreamClient(getConnOpts(stream.Spec), func(js jetstream.JetStream) error { return js.DeleteStream(ctx, stream.Spec.Name) }) if errors.Is(err, jetstream.ErrStreamNotFound) { @@ -130,13 +130,18 @@ func (r *StreamReconciler) deleteStream(ctx context.Context, log logr.Logger, st } else if err != nil { return fmt.Errorf("delete stream during finalization: %w", err) } + } else { + log.Info("Skipping stream deletion.", + "streamName", stream.Spec.Name, + "preventDelete", stream.Spec.PreventDelete, + "read-only", r.ReadOnly(), + ) } - log.Info("Stream deleted. Removing stream finalizer.") + log.Info("Removing stream finalizer.") if ok := controllerutil.RemoveFinalizer(stream, streamFinalizer); !ok { return errors.New("failed to remove stream finalizer") } - if err := r.Update(ctx, stream); err != nil { return fmt.Errorf("remove finalizer: %w", err) } @@ -147,11 +152,12 @@ func (r *StreamReconciler) deleteStream(ctx context.Context, log logr.Logger, st func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, stream *api.Stream) error { // Create or Update the stream based on the spec - if stream.Spec.PreventUpdate { - log.Info("PreventUpdate is set: skipping stream creation/update.", "streamName", stream.Spec.Name) - return nil - } else if r.ReadOnly() { - log.Info("Read-only mode: skipping stream creation/update.", "streamName", stream.Spec.Name) + if stream.Spec.PreventDelete || r.ReadOnly() { + log.Info("Skipping stream creation or update.", + "streamName", stream.Spec.Name, + "preventDelete", stream.Spec.PreventDelete, + "read-only", r.ReadOnly(), + ) return nil } @@ -163,7 +169,7 @@ func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, // CreateOrUpdateStream is called on every reconciliation when the stream is not to be deleted. // TODO(future-feature): Do we need to check if config differs? - err = r.WithJetStreamClient(mapToConnectionOptions(stream.Spec), func(js jetstream.JetStream) error { + err = r.WithJetStreamClient(getConnOpts(stream.Spec), func(js jetstream.JetStream) error { log.Info("create or update stream", "streamName", targetConfig.Name) _, err = js.CreateOrUpdateStream(ctx, targetConfig) return err @@ -193,7 +199,8 @@ func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, return nil } -func mapToConnectionOptions(spec api.StreamSpec) *connectionOptions { +// getConnOpts extracts nats connection relevant fields from the given stream spec as connectionOptions. +func getConnOpts(spec api.StreamSpec) *connectionOptions { return &connectionOptions{ Account: spec.Account, // TODO(review): Where does Spec.Account have to be considered? Creds: spec.Creds, From ffdc4c76a572fd6f3c5470622def38498d119f4e Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 11 Dec 2024 15:33:39 +0100 Subject: [PATCH 26/35] extract configuration to buildNatsConfig method --- internal/controller/jetstream_controller.go | 35 +++++++++++++-------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go index 3b959fe9..f665f872 100644 --- a/internal/controller/jetstream_controller.go +++ b/internal/controller/jetstream_controller.go @@ -25,6 +25,7 @@ type JetStreamController interface { // ReadOnly returns true when no changes should be made by the controller. ReadOnly() bool + // ValidNamespace ok if the controllers namespace restriction allows the given namespace. ValidNamespace(string string) bool @@ -62,16 +63,30 @@ func (c *jsController) ValidNamespace(targetNamespace string) bool { } func (c *jsController) WithJetStreamClient(opts *connectionOptions, op func(js jetstream.JetStream) error) error { + // Build single use client - // TODO(future feature): Use client-pool - serverUrl := strings.Join(opts.Servers, ",") + // TODO(future-feature): Use client-pool instead of single use client + cfg := c.buildNatsConfig(opts) + + jsClient, closer, err := CreateJetStreamClient(cfg, true) + if err != nil { + return fmt.Errorf("create jetstream client: %w", err) + } + defer closer.Close() + + return op(jsClient) +} + +// buildNatsConfig uses given opts to override the base NatsConfig. +func (c *jsController) buildNatsConfig(opts *connectionOptions) *NatsConfig { - // Build nats config from opts and controller base config. - // Takes opts values if present. + serverUrls := strings.Join(opts.Servers, ",") + + // Takes opts values if present cfg := &NatsConfig{ CRDConnect: false, ClientName: c.config.ClientName, - ServerURL: or(serverUrl, c.config.ServerURL), + ServerURL: or(serverUrls, c.config.ServerURL), TLSFirst: c.config.TLSFirst, // TODO(review): should this value depend on any opts? There is no TLSFirst in the spec } @@ -95,13 +110,7 @@ func (c *jsController) WithJetStreamClient(opts *connectionOptions, op func(js j cfg.Key = c.config.Key } - client, closer, err := CreateJetStreamClient(cfg, true) - if err != nil { - return fmt.Errorf("create jetstream client: %w", err) - } - defer closer.Close() - - return op(client) + return cfg } // or returns the value if it is not the null value. Otherwise, the fallback value is returned @@ -112,7 +121,7 @@ func or[T comparable](v T, fallback T) T { return v } -// updateReadyCondition returns the conditions with an added or updated ready condition +// updateReadyCondition returns the given conditions with an added or updated ready condition. func updateReadyCondition(conditions []api.Condition, status v1.ConditionStatus, reason string, message string) []api.Condition { var currentStatus v1.ConditionStatus From 617f3c01b49b45a0f6c54209b25be45788f53e66 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 11 Dec 2024 15:59:18 +0100 Subject: [PATCH 27/35] fix checking for preventDelete in the update step Instead check for preventUpdate. Introduced during refactor. --- internal/controller/stream_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 8cf35766..bf9df159 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -152,7 +152,7 @@ func (r *StreamReconciler) deleteStream(ctx context.Context, log logr.Logger, st func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, stream *api.Stream) error { // Create or Update the stream based on the spec - if stream.Spec.PreventDelete || r.ReadOnly() { + if stream.Spec.PreventUpdate || r.ReadOnly() { log.Info("Skipping stream creation or update.", "streamName", stream.Spec.Name, "preventDelete", stream.Spec.PreventDelete, From d603c91c20b1a505323165c5c0d9480a180930dc Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Thu, 12 Dec 2024 09:43:07 +0100 Subject: [PATCH 28/35] fix k8s binaries not downloaded for tests --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 5976b07b..e3e78b1a 100644 --- a/Makefile +++ b/Makefile @@ -202,6 +202,7 @@ $(ENVTEST): $(LOCALBIN) .PHONY: test test: envtest go vet ./controllers/... ./pkg/natsreloader/... ./internal/controller/... + $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path ## Get k8s binaries go test -race -cover -count=1 -timeout 10s ./controllers/... ./pkg/natsreloader/... ./internal/controller/... .PHONY: clean From ce91d085b963ac4a3036ce3a0a712ac4139c4708 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Thu, 12 Dec 2024 09:43:18 +0100 Subject: [PATCH 29/35] add /bin to gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 8571a5c6..8d5ae051 100644 --- a/.gitignore +++ b/.gitignore @@ -18,4 +18,5 @@ /nats-boot-config /nats-boot-config.docker /tools +/bin /.idea From 684a3b229d2087c664cfd9e176590f1ed8aa5d8c Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Tue, 17 Dec 2024 12:32:39 +0100 Subject: [PATCH 30/35] rename stream helper functions Prefix with stream to prevent conflict with other resources. --- internal/controller/stream_controller.go | 14 +++++++------- internal/controller/stream_controller_test.go | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index bf9df159..9eedb3cf 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -122,7 +122,7 @@ func (r *StreamReconciler) deleteStream(ctx context.Context, log logr.Logger, st if !stream.Spec.PreventDelete && !r.ReadOnly() { log.Info("Deleting stream.") - err := r.WithJetStreamClient(getConnOpts(stream.Spec), func(js jetstream.JetStream) error { + err := r.WithJetStreamClient(streamConnOpts(stream.Spec), func(js jetstream.JetStream) error { return js.DeleteStream(ctx, stream.Spec.Name) }) if errors.Is(err, jetstream.ErrStreamNotFound) { @@ -162,14 +162,14 @@ func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, } // Map spec to stream targetConfig - targetConfig, err := mapSpecToConfig(&stream.Spec) + targetConfig, err := streamSpecToConfig(&stream.Spec) if err != nil { return fmt.Errorf("map spec to stream targetConfig: %w", err) } // CreateOrUpdateStream is called on every reconciliation when the stream is not to be deleted. // TODO(future-feature): Do we need to check if config differs? - err = r.WithJetStreamClient(getConnOpts(stream.Spec), func(js jetstream.JetStream) error { + err = r.WithJetStreamClient(streamConnOpts(stream.Spec), func(js jetstream.JetStream) error { log.Info("create or update stream", "streamName", targetConfig.Name) _, err = js.CreateOrUpdateStream(ctx, targetConfig) return err @@ -199,8 +199,8 @@ func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, return nil } -// getConnOpts extracts nats connection relevant fields from the given stream spec as connectionOptions. -func getConnOpts(spec api.StreamSpec) *connectionOptions { +// streamConnOpts extracts nats connection relevant fields from the given stream spec as connectionOptions. +func streamConnOpts(spec api.StreamSpec) *connectionOptions { return &connectionOptions{ Account: spec.Account, // TODO(review): Where does Spec.Account have to be considered? Creds: spec.Creds, @@ -210,8 +210,8 @@ func getConnOpts(spec api.StreamSpec) *connectionOptions { } } -// mapSpecToConfig creates a jetstream.StreamConfig matching the given stream resource spec -func mapSpecToConfig(spec *api.StreamSpec) (jetstream.StreamConfig, error) { +// streamSpecToConfig creates a jetstream.StreamConfig matching the given stream resource spec +func streamSpecToConfig(spec *api.StreamSpec) (jetstream.StreamConfig, error) { // Set directly mapped fields config := jetstream.StreamConfig{ diff --git a/internal/controller/stream_controller_test.go b/internal/controller/stream_controller_test.go index 2e1bbc26..e80d327b 100644 --- a/internal/controller/stream_controller_test.go +++ b/internal/controller/stream_controller_test.go @@ -732,9 +732,9 @@ func Test_mapSpecToConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert := assert.New(t) - got, err := mapSpecToConfig(tt.spec) + got, err := streamSpecToConfig(tt.spec) if (err != nil) != tt.wantErr { - t.Errorf("mapSpecToConfig() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("streamSpecToConfig() error = %v, wantErr %v", err, tt.wantErr) return } From 0fd239dd0aedad5bede98b1a71a5f57a411b0482 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 18 Dec 2024 11:21:36 +0100 Subject: [PATCH 31/35] update naming as suggested --- internal/controller/jetstream_controller.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go index f665f872..cb589d8f 100644 --- a/internal/controller/jetstream_controller.go +++ b/internal/controller/jetstream_controller.go @@ -27,7 +27,7 @@ type JetStreamController interface { ReadOnly() bool // ValidNamespace ok if the controllers namespace restriction allows the given namespace. - ValidNamespace(string string) bool + ValidNamespace(namespace string) bool // WithJetStreamClient provides a jetStream client to the given operation. // The client uses the controllers connection configuration merged with opts. @@ -57,9 +57,9 @@ func (c *jsController) ReadOnly() bool { return c.controllerConfig.ReadOnly } -func (c *jsController) ValidNamespace(targetNamespace string) bool { +func (c *jsController) ValidNamespace(namespace string) bool { ns := c.controllerConfig.Namespace - return ns == "" || ns == targetNamespace + return ns == "" || ns == namespace } func (c *jsController) WithJetStreamClient(opts *connectionOptions, op func(js jetstream.JetStream) error) error { From a7a341eb30d3b04b632bcd2f1bdd0a12eeae2e09 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 18 Dec 2024 11:25:17 +0100 Subject: [PATCH 32/35] fix assumed reason in log message --- internal/controller/stream_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 9eedb3cf..52bbf70b 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -126,7 +126,7 @@ func (r *StreamReconciler) deleteStream(ctx context.Context, log logr.Logger, st return js.DeleteStream(ctx, stream.Spec.Name) }) if errors.Is(err, jetstream.ErrStreamNotFound) { - log.Info("Managed stream was already deleted.") + log.Info("Stream does not exist, unable to delete.", "streamName", stream.Spec.Name) } else if err != nil { return fmt.Errorf("delete stream during finalization: %w", err) } From 0b21494e8f71aac1ec45a61bdc23d16227d68db4 Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 18 Dec 2024 11:38:04 +0100 Subject: [PATCH 33/35] Update todo comments marked with review - Add note on opts.Account - Add comment on possible feature to expose TLSFirst in the spec. --- internal/controller/jetstream_controller.go | 6 +++++- internal/controller/stream_controller.go | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go index cb589d8f..f14bea99 100644 --- a/internal/controller/jetstream_controller.go +++ b/internal/controller/jetstream_controller.go @@ -87,9 +87,13 @@ func (c *jsController) buildNatsConfig(opts *connectionOptions) *NatsConfig { CRDConnect: false, ClientName: c.config.ClientName, ServerURL: or(serverUrls, c.config.ServerURL), - TLSFirst: c.config.TLSFirst, // TODO(review): should this value depend on any opts? There is no TLSFirst in the spec + TLSFirst: c.config.TLSFirst, // TODO(future-feature): expose TLSFirst in the spec config } + // Note: The opts.Account value coming from the resource spec is currently not considered. + // creds/nkey are associated with an account, the account field might be redundant. + // See https://github.com/nats-io/nack/pull/211#pullrequestreview-2511111670 + // Authentication either from opts or base config if opts.Creds != "" || opts.Nkey != "" { cfg.Credentials = opts.Creds diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index 52bbf70b..d2d5623f 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -202,7 +202,7 @@ func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, // streamConnOpts extracts nats connection relevant fields from the given stream spec as connectionOptions. func streamConnOpts(spec api.StreamSpec) *connectionOptions { return &connectionOptions{ - Account: spec.Account, // TODO(review): Where does Spec.Account have to be considered? + Account: spec.Account, Creds: spec.Creds, Nkey: spec.Nkey, Servers: spec.Servers, From 2bf9162abaa6f6acfd9f0dd1e854e5596089d76e Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 18 Dec 2024 11:49:23 +0100 Subject: [PATCH 34/35] separate CA config from client cert and key --- internal/controller/jetstream_controller.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/controller/jetstream_controller.go b/internal/controller/jetstream_controller.go index f14bea99..3796fc47 100644 --- a/internal/controller/jetstream_controller.go +++ b/internal/controller/jetstream_controller.go @@ -103,13 +103,18 @@ func (c *jsController) buildNatsConfig(opts *connectionOptions) *NatsConfig { cfg.NKey = c.config.NKey } - // Cert config either from opts or base config - if len(opts.TLS.RootCAs) > 0 || opts.TLS.ClientCert != "" || opts.TLS.ClientKey != "" { + // CAs from opts or base config + if len(opts.TLS.RootCAs) > 0 { cfg.CAs = opts.TLS.RootCAs + } else { + cfg.CAs = c.config.CAs + } + + // Client Cert and Key either from opts or base config + if opts.TLS.ClientCert != "" && opts.TLS.ClientKey != "" { cfg.Certificate = opts.TLS.ClientCert cfg.Key = opts.TLS.ClientKey } else { - cfg.CAs = c.config.CAs cfg.Certificate = c.config.Certificate cfg.Key = c.config.Key } From d93aa6337ebdc28cecc55d5a63de5c00177b6c3f Mon Sep 17 00:00:00 2001 From: Adrian Dieter Date: Wed, 18 Dec 2024 14:00:31 +0100 Subject: [PATCH 35/35] set streamName and consumerName fields once on logger Reword log messages. --- internal/controller/stream_controller.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/controller/stream_controller.go b/internal/controller/stream_controller.go index d2d5623f..1766c8ba 100644 --- a/internal/controller/stream_controller.go +++ b/internal/controller/stream_controller.go @@ -66,6 +66,8 @@ func (r *StreamReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, fmt.Errorf("get stream resource '%s': %w", req.NamespacedName.String(), err) } + log = log.WithValues("streamName", stream.Spec.Name) + // Update ready status to unknown when no status is set if stream.Status.Conditions == nil || len(stream.Status.Conditions) == 0 { log.Info("Setting initial ready condition to unknown.") @@ -132,7 +134,6 @@ func (r *StreamReconciler) deleteStream(ctx context.Context, log logr.Logger, st } } else { log.Info("Skipping stream deletion.", - "streamName", stream.Spec.Name, "preventDelete", stream.Spec.PreventDelete, "read-only", r.ReadOnly(), ) @@ -154,7 +155,6 @@ func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, // Create or Update the stream based on the spec if stream.Spec.PreventUpdate || r.ReadOnly() { log.Info("Skipping stream creation or update.", - "streamName", stream.Spec.Name, "preventDelete", stream.Spec.PreventDelete, "read-only", r.ReadOnly(), ) @@ -170,7 +170,7 @@ func (r *StreamReconciler) createOrUpdate(ctx context.Context, log logr.Logger, // CreateOrUpdateStream is called on every reconciliation when the stream is not to be deleted. // TODO(future-feature): Do we need to check if config differs? err = r.WithJetStreamClient(streamConnOpts(stream.Spec), func(js jetstream.JetStream) error { - log.Info("create or update stream", "streamName", targetConfig.Name) + log.Info("Creating or updating stream.") _, err = js.CreateOrUpdateStream(ctx, targetConfig) return err })