From 408f89c4ecac711caec9a69e678d147abb522235 Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Fri, 19 Jul 2024 02:00:28 -0700 Subject: [PATCH 1/9] feat: codify custom search attributes in TemporalNamespace --- api/v1beta1/temporalnamespace_types.go | 5 + api/v1beta1/zz_generated.deepcopy.go | 17 +++- .../crds/temporal-operator.crds.yaml | 8 ++ .../bases/temporal.io_temporalnamespaces.yaml | 8 ++ controllers/temporalnamespace_controller.go | 93 +++++++++++++++++++ docs/api/v1beta1.md | 28 ++++++ pkg/version/zz_generated.deepcopy.go | 1 - 7 files changed, 155 insertions(+), 5 deletions(-) diff --git a/api/v1beta1/temporalnamespace_types.go b/api/v1beta1/temporalnamespace_types.go index 1ab2d45a..9b57a372 100644 --- a/api/v1beta1/temporalnamespace_types.go +++ b/api/v1beta1/temporalnamespace_types.go @@ -55,6 +55,11 @@ type TemporalNamespaceSpec struct { // Only applicable if the namespace is a global namespace. // +optional Clusters []string `json:"clusters,omitempty"` + // Search attributes are key-value pairs of metadata objects included in a workflow + // execution's visibility information. Temporal uses some default search attributes + // but also supports custom search attributes. + // +optional + CustomSearchAttributes map[string]string `json:"customSearchAttributes,omitempty"` // The name of active Temporal Cluster. // Only applicable if the namespace is a global namespace. // +optional diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index d42b3b6c..4100c77e 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated // Licensed to Alexandre VILAIN under one or more contributor // license agreements. See the NOTICE file distributed with @@ -431,7 +430,8 @@ func (in *DynamicConfigSpec) DeepCopyInto(out *DynamicConfigSpec) { if val == nil { (*out)[key] = nil } else { - in, out := &val, &outVal + inVal := (*in)[key] + in, out := &inVal, &outVal *out = make([]ConstrainedValue, len(*in)) for i := range *in { (*in)[i].DeepCopyInto(&(*out)[i]) @@ -641,7 +641,8 @@ func (in *MetricsSpec) DeepCopyInto(out *MetricsSpec) { if val == nil { (*out)[key] = nil } else { - in, out := &val, &outVal + inVal := (*in)[key] + in, out := &inVal, &outVal *out = make([]string, len(*in)) copy(*out, *in) } @@ -656,7 +657,8 @@ func (in *MetricsSpec) DeepCopyInto(out *MetricsSpec) { if val == nil { (*out)[key] = nil } else { - in, out := &val, &outVal + inVal := (*in)[key] + in, out := &inVal, &outVal *out = make([]string, len(*in)) copy(*out, *in) } @@ -1837,6 +1839,13 @@ func (in *TemporalNamespaceSpec) DeepCopyInto(out *TemporalNamespaceSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.CustomSearchAttributes != nil { + in, out := &in.CustomSearchAttributes, &out.CustomSearchAttributes + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } if in.Archival != nil { in, out := &in.Archival, &out.Archival *out = new(TemporalNamespaceArchivalSpec) diff --git a/charts/temporal-operator/crds/temporal-operator.crds.yaml b/charts/temporal-operator/crds/temporal-operator.crds.yaml index 662fc639..08a210e5 100644 --- a/charts/temporal-operator/crds/temporal-operator.crds.yaml +++ b/charts/temporal-operator/crds/temporal-operator.crds.yaml @@ -4479,6 +4479,14 @@ spec: items: type: string type: array + customSearchAttributes: + additionalProperties: + type: string + description: |- + Search attributes are key-value pairs of metadata objects included in a workflow + execution's visibility information. Temporal uses some default search attributes + but also supports custom search attributes. + type: object data: additionalProperties: type: string diff --git a/config/crd/bases/temporal.io_temporalnamespaces.yaml b/config/crd/bases/temporal.io_temporalnamespaces.yaml index 3dd44f87..5957b34d 100644 --- a/config/crd/bases/temporal.io_temporalnamespaces.yaml +++ b/config/crd/bases/temporal.io_temporalnamespaces.yaml @@ -130,6 +130,14 @@ spec: items: type: string type: array + customSearchAttributes: + additionalProperties: + type: string + description: |- + Search attributes are key-value pairs of metadata objects included in a workflow + execution's visibility information. Temporal uses some default search attributes + but also supports custom search attributes. + type: object data: additionalProperties: type: string diff --git a/controllers/temporalnamespace_controller.go b/controllers/temporalnamespace_controller.go index 61fb86d8..576afd34 100644 --- a/controllers/temporalnamespace_controller.go +++ b/controllers/temporalnamespace_controller.go @@ -21,9 +21,12 @@ import ( "context" "errors" "fmt" + "strings" "time" "github.com/alexandrevilain/controller-tools/pkg/patch" + "go.temporal.io/api/enums/v1" + "go.temporal.io/api/operatorservice/v1" "go.temporal.io/api/serviceerror" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -136,6 +139,11 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re } } + err = r.reconcileCustomSearchAttributes(ctx, namespace, cluster) + if err != nil { + return r.handleError(namespace, v1beta1.ReconcileErrorReason, err) + } + logger.Info("Successfully reconciled namespace", "namespace", namespace.GetName()) v1beta1.SetTemporalNamespaceReady(namespace, metav1.ConditionTrue, v1beta1.TemporalNamespaceCreatedReason, "Namespace successfully created") @@ -143,6 +151,91 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re return r.handleSuccess(namespace) } +// reconcileCustomSearchAttributes ensures that the custom search attributes on the Temporal server exactly match those defined in the spec +func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx context.Context, namespace *v1beta1.TemporalNamespace, cluster *v1beta1.TemporalCluster) error { + ns := namespace.GetName() + + client, err := temporal.GetClusterClient(ctx, r.Client, cluster) + if err != nil { + return err + } + + // List search attributes currently on the Temporal server + listReq := &operatorservice.ListSearchAttributesRequest{Namespace: ns} + searchAttributesOnServer, err := client.OperatorService().ListSearchAttributes(ctx, listReq) + if err != nil { + return err + } + // just focus on the CUSTOM search attributes. + customSearchAttributesOnServer := &searchAttributesOnServer.CustomAttributes // a pointer avoids unecessary copying for the sake of just a named variable + + // Note that the CustomSearchAttributes map data structure that is built using the Spec merely maps string->string. + // To rigorously compare search attributes between the spec and the Temporal server, the types need to be consistent. + // Therefore, we need to construct a string->enums.IndexedValueType map from the string->string map. + customSearchAttributesInSpec := make(map[string]enums.IndexedValueType, len(namespace.Spec.CustomSearchAttributes)) + for searchAttrNameString, searchAttrTypeString := range namespace.Spec.CustomSearchAttributes { + indexedValueType, err := searchAttributeTypeStringToEnum(searchAttrTypeString) + if err != nil { + return fmt.Errorf("unable to parse search attribute type %s: %w", searchAttrTypeString, err) + } + customSearchAttributesInSpec[searchAttrNameString] = indexedValueType + } + + // Remove those custom search attributes from the Temporal server whose name does not exist in the Spec, + // or whose name exists in the Spec but whose type doesn't match the type in the Spec. + customSearchAttributesToRemove := make([]string, 0) + for serverSearchAttrName, serverSearchAttrType := range *customSearchAttributesOnServer { + specSearchAttrType, serverSearchAttrNameExistsInSpec := customSearchAttributesInSpec[serverSearchAttrName] + if !serverSearchAttrNameExistsInSpec || serverSearchAttrType != specSearchAttrType { + customSearchAttributesToRemove = append(customSearchAttributesToRemove, serverSearchAttrName) + } + } + removeReq := &operatorservice.RemoveSearchAttributesRequest{ + Namespace: ns, + SearchAttributes: customSearchAttributesToRemove, + } + _, err = client.OperatorService().RemoveSearchAttributes(ctx, removeReq) + if err != nil { + return fmt.Errorf("unable to remove search attributes: %w", err) + } + + // Create custom search attributes from the Spec which don't yet exist on the Temporal server. + // If the Temporal server already has a custom search attribute with the same name but a different type, + // then return an error. + customSearchAttributesToCreate := make(map[string]enums.IndexedValueType) + for specSearchAttrName, specSearchAttrType := range customSearchAttributesInSpec { + serverSearchAttrType, specSearchAttrNameExistsOnServer := (*customSearchAttributesOnServer)[specSearchAttrName] + if specSearchAttrNameExistsOnServer { + if specSearchAttrType != serverSearchAttrType { + return fmt.Errorf("search attribute %s already exists and has different type %s", specSearchAttrName, serverSearchAttrType.String()) + } + } else { + customSearchAttributesToCreate[specSearchAttrName] = specSearchAttrType + } + } + createReq := &operatorservice.AddSearchAttributesRequest{ + Namespace: ns, + SearchAttributes: customSearchAttributesToCreate, + } + _, err = client.OperatorService().AddSearchAttributes(ctx, createReq) + if err != nil { + return fmt.Errorf("unable to add search attributes: %w", err) + } + + + return nil +} + +// searchAttributeTypeStringToEnum returns the IndexedValueType for a given string or an error. +func searchAttributeTypeStringToEnum(search string) (enums.IndexedValueType, error) { + for k, v := range enums.IndexedValueType_shorthandValue { + if strings.EqualFold(search, k) { + return enums.IndexedValueType(v), nil + } + } + return enums.INDEXED_VALUE_TYPE_UNSPECIFIED, fmt.Errorf("unsupported search attribute type: %v", search) +} + // ensureFinalizer ensures the deletion finalizer is set on the object if the user allowed namespace deletion using the CRD. func (r *TemporalNamespaceReconciler) ensureFinalizer(namespace *v1beta1.TemporalNamespace) { if namespace.ObjectMeta.DeletionTimestamp.IsZero() && namespace.Spec.AllowDeletion { diff --git a/docs/api/v1beta1.md b/docs/api/v1beta1.md index 50822bd3..0837c22f 100644 --- a/docs/api/v1beta1.md +++ b/docs/api/v1beta1.md @@ -5220,6 +5220,20 @@ Only applicable if the namespace is a global namespace.

+customSearchAttributes
+ +map[string]string + + + +(Optional) +

Search attributes are key-value pairs of metadata objects included in a workflow +execution’s visibility information. Temporal uses some default search attributes +but also supports custom search attributes.

+ + + + activeClusterName
string @@ -5444,6 +5458,20 @@ Only applicable if the namespace is a global namespace.

+customSearchAttributes
+ +map[string]string + + + +(Optional) +

Search attributes are key-value pairs of metadata objects included in a workflow +execution’s visibility information. Temporal uses some default search attributes +but also supports custom search attributes.

+ + + + activeClusterName
string diff --git a/pkg/version/zz_generated.deepcopy.go b/pkg/version/zz_generated.deepcopy.go index f203531a..59dd5200 100644 --- a/pkg/version/zz_generated.deepcopy.go +++ b/pkg/version/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated // Licensed to Alexandre VILAIN under one or more contributor // license agreements. See the NOTICE file distributed with From 895333fc3073f8a44aace1ba63eb9b60ead1bd76 Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Mon, 22 Jul 2024 18:11:53 -0700 Subject: [PATCH 2/9] add guard condition for empty payload in operatorservice requests --- controllers/temporalnamespace_controller.go | 129 ++++++++++++-------- 1 file changed, 78 insertions(+), 51 deletions(-) diff --git a/controllers/temporalnamespace_controller.go b/controllers/temporalnamespace_controller.go index 576afd34..89e9f42c 100644 --- a/controllers/temporalnamespace_controller.go +++ b/controllers/temporalnamespace_controller.go @@ -25,6 +25,7 @@ import ( "time" "github.com/alexandrevilain/controller-tools/pkg/patch" + "github.com/go-logr/logr" "go.temporal.io/api/enums/v1" "go.temporal.io/api/operatorservice/v1" "go.temporal.io/api/serviceerror" @@ -139,8 +140,9 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re } } - err = r.reconcileCustomSearchAttributes(ctx, namespace, cluster) + err = r.reconcileCustomSearchAttributes(ctx, logger, namespace, cluster) if err != nil { + logger.Info(fmt.Sprintf("Failed to reconcile custom search attributes: %v", err)) return r.handleError(namespace, v1beta1.ReconcileErrorReason, err) } @@ -152,88 +154,113 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re } // reconcileCustomSearchAttributes ensures that the custom search attributes on the Temporal server exactly match those defined in the spec -func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx context.Context, namespace *v1beta1.TemporalNamespace, cluster *v1beta1.TemporalCluster) error { - ns := namespace.GetName() - +func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx context.Context, logger logr.Logger, namespace *v1beta1.TemporalNamespace, cluster *v1beta1.TemporalCluster) error { + // To talk to the Temporal server, construct a client client, err := temporal.GetClusterClient(ctx, r.Client, cluster) if err != nil { return err } + // The Temporal OperatorService API requires requests to specify the namespace name, so capture it. + ns := namespace.GetName() - // List search attributes currently on the Temporal server + // List the current search attributes on the Temporal server listReq := &operatorservice.ListSearchAttributesRequest{Namespace: ns} - searchAttributesOnServer, err := client.OperatorService().ListSearchAttributes(ctx, listReq) + serverSearchAttributes, err := client.OperatorService().ListSearchAttributes(ctx, listReq) if err != nil { return err } - // just focus on the CUSTOM search attributes. - customSearchAttributesOnServer := &searchAttributesOnServer.CustomAttributes // a pointer avoids unecessary copying for the sake of just a named variable + + // Narrow the focus to custom search attributes only. + serverCustomSearchAttributes := &serverSearchAttributes.CustomAttributes // use a pointer to avoid unecessary copying // Note that the CustomSearchAttributes map data structure that is built using the Spec merely maps string->string. // To rigorously compare search attributes between the spec and the Temporal server, the types need to be consistent. - // Therefore, we need to construct a string->enums.IndexedValueType map from the string->string map. - customSearchAttributesInSpec := make(map[string]enums.IndexedValueType, len(namespace.Spec.CustomSearchAttributes)) - for searchAttrNameString, searchAttrTypeString := range namespace.Spec.CustomSearchAttributes { - indexedValueType, err := searchAttributeTypeStringToEnum(searchAttrTypeString) + // We therefore construct a string->enums.IndexedValueType map from the "weaker" string->string map. + specCustomSearchAttributes := make(map[string]enums.IndexedValueType, len(namespace.Spec.CustomSearchAttributes)) + for searchAttributeNameString, searchAttributeTypeString := range namespace.Spec.CustomSearchAttributes { + indexedValueType, err := searchAttributeTypeStringToEnum(searchAttributeTypeString) if err != nil { - return fmt.Errorf("unable to parse search attribute type %s: %w", searchAttrTypeString, err) + return fmt.Errorf("failed to parse search attribute %s because its type is %s: %w", searchAttributeNameString, searchAttributeTypeString, err) } - customSearchAttributesInSpec[searchAttrNameString] = indexedValueType + specCustomSearchAttributes[searchAttributeNameString] = indexedValueType } - // Remove those custom search attributes from the Temporal server whose name does not exist in the Spec, - // or whose name exists in the Spec but whose type doesn't match the type in the Spec. + /* + NOTE: At this point, we're ready to start comparing the current state (search attributes on the server) + to the desired state (search attributes in the spec). + + Reconciling custom search attributes is accomplished in simple steps: + + 1. Retrieve the custom search attributes which are currently on the Temporal server. (Already completed in above code) + 2. Determine which custom search attributes need to be removed, if any. + 3. Determine which custom search attributes need to be created, if any. + 4. Make any necessary requests to the Temporal server to remove/create custom search attributes. + + Some of these steps may fail if some Temporal search attribute constraint is violated; in which case, this function will return early + with a helpful error message. + */ + + // Remove those custom search attributes from the Temporal server whose name does not exist in the Spec. customSearchAttributesToRemove := make([]string, 0) - for serverSearchAttrName, serverSearchAttrType := range *customSearchAttributesOnServer { - specSearchAttrType, serverSearchAttrNameExistsInSpec := customSearchAttributesInSpec[serverSearchAttrName] - if !serverSearchAttrNameExistsInSpec || serverSearchAttrType != specSearchAttrType { - customSearchAttributesToRemove = append(customSearchAttributesToRemove, serverSearchAttrName) + for serverSearchAttributeName := range *serverCustomSearchAttributes { + _, serverSearchAttributeNameExistsInSpec := specCustomSearchAttributes[serverSearchAttributeName] + if !serverSearchAttributeNameExistsInSpec { + customSearchAttributesToRemove = append(customSearchAttributesToRemove, serverSearchAttributeName) } } - removeReq := &operatorservice.RemoveSearchAttributesRequest{ - Namespace: ns, - SearchAttributes: customSearchAttributesToRemove, - } - _, err = client.OperatorService().RemoveSearchAttributes(ctx, removeReq) - if err != nil { - return fmt.Errorf("unable to remove search attributes: %w", err) - } - // Create custom search attributes from the Spec which don't yet exist on the Temporal server. - // If the Temporal server already has a custom search attribute with the same name but a different type, - // then return an error. - customSearchAttributesToCreate := make(map[string]enums.IndexedValueType) - for specSearchAttrName, specSearchAttrType := range customSearchAttributesInSpec { - serverSearchAttrType, specSearchAttrNameExistsOnServer := (*customSearchAttributesOnServer)[specSearchAttrName] - if specSearchAttrNameExistsOnServer { - if specSearchAttrType != serverSearchAttrType { - return fmt.Errorf("search attribute %s already exists and has different type %s", specSearchAttrName, serverSearchAttrType.String()) - } - } else { - customSearchAttributesToCreate[specSearchAttrName] = specSearchAttrType + // Add custom search attributes from the Spec which don't yet exist on the Temporal server. + // If the Temporal server already has a custom search attribute with the same name but a different type, then return an error. + customSearchAttributesToAdd := make(map[string]enums.IndexedValueType) + for specSearchAttributeName, specSearchAttributeType := range specCustomSearchAttributes { + serverSearchAttributeType, specSearchAttributeNameExistsOnServer := (*serverCustomSearchAttributes)[specSearchAttributeName] + if !specSearchAttributeNameExistsOnServer { + customSearchAttributesToAdd[specSearchAttributeName] = specSearchAttributeType + } else if specSearchAttributeType != serverSearchAttributeType { + return fmt.Errorf("search attribute %s already exists and has different type %s", specSearchAttributeName, serverSearchAttributeType.String()) } } - createReq := &operatorservice.AddSearchAttributesRequest{ - Namespace: ns, - SearchAttributes: customSearchAttributesToCreate, - } - _, err = client.OperatorService().AddSearchAttributes(ctx, createReq) - if err != nil { - return fmt.Errorf("unable to add search attributes: %w", err) + + // If there are search attributes that should be removed, then make a request to the Temporal server to remove them. + if len(customSearchAttributesToRemove) > 0 { + removeReq := &operatorservice.RemoveSearchAttributesRequest{ + Namespace: ns, + SearchAttributes: customSearchAttributesToRemove, + } + _, err = client.OperatorService().RemoveSearchAttributes(ctx, removeReq) + if err != nil { + return fmt.Errorf("failed to remove search attributes: %w", err) + } + logger.Info(fmt.Sprintf("removed custom search attributes: %v", customSearchAttributesToRemove)) } + // If there are search attributes that should be added, then make a request the Temporal server to create them. + if len(customSearchAttributesToAdd) > 0 { + createReq := &operatorservice.AddSearchAttributesRequest{ + Namespace: ns, + SearchAttributes: customSearchAttributesToAdd, + } + _, err = client.OperatorService().AddSearchAttributes(ctx, createReq) + if err != nil { + return fmt.Errorf("failed to add search attributes: %w", err) + } + logger.Info(fmt.Sprintf("added custom search attributes: %v", customSearchAttributesToAdd)) + } return nil } -// searchAttributeTypeStringToEnum returns the IndexedValueType for a given string or an error. -func searchAttributeTypeStringToEnum(search string) (enums.IndexedValueType, error) { +// searchAttributeTypeStringToEnum retrieves the actual IndexedValueType for a given string. +// It expects searchAttributeTypeString to be a string representation of the valid Go type. +// Returns the IndexedValueType if parsing is successful, otherwise an error. +// See https://docs.temporal.io/visibility#supported-types for supported types. +func searchAttributeTypeStringToEnum(searchAttributeTypeString string) (enums.IndexedValueType, error) { for k, v := range enums.IndexedValueType_shorthandValue { - if strings.EqualFold(search, k) { + if strings.EqualFold(searchAttributeTypeString, k) { return enums.IndexedValueType(v), nil } } - return enums.INDEXED_VALUE_TYPE_UNSPECIFIED, fmt.Errorf("unsupported search attribute type: %v", search) + return enums.INDEXED_VALUE_TYPE_UNSPECIFIED, fmt.Errorf("unsupported search attribute type: %v", searchAttributeTypeString) } // ensureFinalizer ensures the deletion finalizer is set on the object if the user allowed namespace deletion using the CRD. From c6d6336d1e796de08a313d4eaa124a0824ccd447 Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Mon, 22 Jul 2024 21:29:58 -0700 Subject: [PATCH 3/9] pass logger by reference --- controllers/temporalnamespace_controller.go | 49 ++++++++++----------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/controllers/temporalnamespace_controller.go b/controllers/temporalnamespace_controller.go index 89e9f42c..19a7210c 100644 --- a/controllers/temporalnamespace_controller.go +++ b/controllers/temporalnamespace_controller.go @@ -140,7 +140,7 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re } } - err = r.reconcileCustomSearchAttributes(ctx, logger, namespace, cluster) + err = r.reconcileCustomSearchAttributes(ctx, &logger, namespace, cluster) if err != nil { logger.Info(fmt.Sprintf("Failed to reconcile custom search attributes: %v", err)) return r.handleError(namespace, v1beta1.ReconcileErrorReason, err) @@ -153,19 +153,31 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re return r.handleSuccess(namespace) } -// reconcileCustomSearchAttributes ensures that the custom search attributes on the Temporal server exactly match those defined in the spec -func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx context.Context, logger logr.Logger, namespace *v1beta1.TemporalNamespace, cluster *v1beta1.TemporalCluster) error { +// reconcileCustomSearchAttributes ensures that custom search attributes on the Temporal server exactly match those defined in the spec. +func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx context.Context, logger *logr.Logger, namespace *v1beta1.TemporalNamespace, cluster *v1beta1.TemporalCluster) error { + /* + NOTE: Reconciliation of custom search attributes is accomplished using these steps: + + 1. Retrieve the custom search attributes which are currently on the Temporal server. + 2. Determine which custom search attributes need to be removed, if any. + 3. Determine which custom search attributes need to be added, if any. + 4. If needed, request the Temporal server to remove the necessary custom search attributes. + 5. If needed, request the Temporal server to add the necessary custom search attributes. + + Some of these steps may fail if some Temporal search attribute constraint is violated; in which case, this function will return early with a helpful error message. + */ + // To talk to the Temporal server, construct a client client, err := temporal.GetClusterClient(ctx, r.Client, cluster) if err != nil { return err } - // The Temporal OperatorService API requires requests to specify the namespace name, so capture it. + // Requests to Temporal's OperatorService API need to specify the namespace name, so capture it here for future use. ns := namespace.GetName() - // List the current search attributes on the Temporal server - listReq := &operatorservice.ListSearchAttributesRequest{Namespace: ns} - serverSearchAttributes, err := client.OperatorService().ListSearchAttributes(ctx, listReq) + // List all search attributes that are currently on the Temporal server + listRequest := &operatorservice.ListSearchAttributesRequest{Namespace: ns} + serverSearchAttributes, err := client.OperatorService().ListSearchAttributes(ctx, listRequest) if err != nil { return err } @@ -185,21 +197,6 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex specCustomSearchAttributes[searchAttributeNameString] = indexedValueType } - /* - NOTE: At this point, we're ready to start comparing the current state (search attributes on the server) - to the desired state (search attributes in the spec). - - Reconciling custom search attributes is accomplished in simple steps: - - 1. Retrieve the custom search attributes which are currently on the Temporal server. (Already completed in above code) - 2. Determine which custom search attributes need to be removed, if any. - 3. Determine which custom search attributes need to be created, if any. - 4. Make any necessary requests to the Temporal server to remove/create custom search attributes. - - Some of these steps may fail if some Temporal search attribute constraint is violated; in which case, this function will return early - with a helpful error message. - */ - // Remove those custom search attributes from the Temporal server whose name does not exist in the Spec. customSearchAttributesToRemove := make([]string, 0) for serverSearchAttributeName := range *serverCustomSearchAttributes { @@ -223,11 +220,11 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex // If there are search attributes that should be removed, then make a request to the Temporal server to remove them. if len(customSearchAttributesToRemove) > 0 { - removeReq := &operatorservice.RemoveSearchAttributesRequest{ + removeRequest := &operatorservice.RemoveSearchAttributesRequest{ Namespace: ns, SearchAttributes: customSearchAttributesToRemove, } - _, err = client.OperatorService().RemoveSearchAttributes(ctx, removeReq) + _, err = client.OperatorService().RemoveSearchAttributes(ctx, removeRequest) if err != nil { return fmt.Errorf("failed to remove search attributes: %w", err) } @@ -236,11 +233,11 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex // If there are search attributes that should be added, then make a request the Temporal server to create them. if len(customSearchAttributesToAdd) > 0 { - createReq := &operatorservice.AddSearchAttributesRequest{ + addRequest := &operatorservice.AddSearchAttributesRequest{ Namespace: ns, SearchAttributes: customSearchAttributesToAdd, } - _, err = client.OperatorService().AddSearchAttributes(ctx, createReq) + _, err = client.OperatorService().AddSearchAttributes(ctx, addRequest) if err != nil { return fmt.Errorf("failed to add search attributes: %w", err) } From 49126d70bf3a06d59671de270137bd5d8bf8f7d3 Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Mon, 22 Jul 2024 21:31:17 -0700 Subject: [PATCH 4/9] add customSearchAttributes to an example TemporalNamespace Spec --- examples/cluster-postgres/03-temporal-namespace.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/examples/cluster-postgres/03-temporal-namespace.yaml b/examples/cluster-postgres/03-temporal-namespace.yaml index ced40546..a032c6e6 100644 --- a/examples/cluster-postgres/03-temporal-namespace.yaml +++ b/examples/cluster-postgres/03-temporal-namespace.yaml @@ -9,3 +9,11 @@ spec: description: Default namespace retentionPeriod: 168h #7 days allowDeletion: true + customSearchAttributes: + attr1: Bool + attr2: Datetime + attr3: Double + attr4: Int + attr5: Keyword + attr6: KeywordList + attr7: Text From 80f393526d0a20c9adc383863a9d5f8804be6fe8 Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Thu, 1 Aug 2024 13:35:08 -0700 Subject: [PATCH 5/9] add feature mentioning that the operator manages custom search attrs --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 16052645..0a8860c5 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,7 @@ Please note this table only reports end-to-end tests suite coverage, others vers - [x] Automatic mTLS certificates management (using cert-manager). - [x] Support for integration in meshes: istio & linkerd. - [x] Namespace management using CRDs. +- [x] Custom search attribute management. - [x] Cluster version upgrades. - [x] Cluster monitoring. - [x] Complete end2end test suite. From 511d1e607aeb094cfcca6b118c479e7510e69d36 Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Thu, 1 Aug 2024 13:35:08 -0700 Subject: [PATCH 6/9] add feature mentioning that the operator manages custom search attrs --- controllers/temporalnamespace_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/temporalnamespace_controller.go b/controllers/temporalnamespace_controller.go index 19a7210c..3ee0ab1f 100644 --- a/controllers/temporalnamespace_controller.go +++ b/controllers/temporalnamespace_controller.go @@ -183,7 +183,7 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex } // Narrow the focus to custom search attributes only. - serverCustomSearchAttributes := &serverSearchAttributes.CustomAttributes // use a pointer to avoid unecessary copying + serverCustomSearchAttributes := &serverSearchAttributes.CustomAttributes // use a pointer to avoid unnecessary copying // Note that the CustomSearchAttributes map data structure that is built using the Spec merely maps string->string. // To rigorously compare search attributes between the spec and the Temporal server, the types need to be consistent. From 7e2f787ea7fecc69909c1c892256f46922b0cb0a Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Tue, 6 Aug 2024 15:30:52 -0700 Subject: [PATCH 7/9] fix typo --- tests/e2e/namespace_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e/namespace_test.go b/tests/e2e/namespace_test.go index 19f882f9..f6e6862c 100644 --- a/tests/e2e/namespace_test.go +++ b/tests/e2e/namespace_test.go @@ -39,7 +39,7 @@ func TestNamespaceCreation(t *testing.T) { var cluster *v1beta1.TemporalCluster var temporalNamespace *v1beta1.TemporalNamespace - namespaceFature := features.New("namespace creation using CRD"). + namespaceFeature := features.New("namespace creation using CRD"). Setup(func(ctx context.Context, _ *testing.T, cfg *envconf.Config) context.Context { namespace := GetNamespaceForFeature(ctx) @@ -189,7 +189,7 @@ func TestNamespaceCreation(t *testing.T) { // }). Feature() - testenv.Test(t, namespaceFature) + testenv.Test(t, namespaceFeature) } func TestNamespaceDeletionWhenClusterDoesNotExist(rt *testing.T) { From a9eb3845263d02524f8dd306a0580251ef6c5e86 Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Tue, 6 Aug 2024 22:44:53 -0700 Subject: [PATCH 8/9] extract map construction logic into separate func --- controllers/temporalnamespace_controller.go | 48 ++++++++++++--------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/controllers/temporalnamespace_controller.go b/controllers/temporalnamespace_controller.go index 3ee0ab1f..9227beb9 100644 --- a/controllers/temporalnamespace_controller.go +++ b/controllers/temporalnamespace_controller.go @@ -154,20 +154,18 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re } // reconcileCustomSearchAttributes ensures that custom search attributes on the Temporal server exactly match those defined in the spec. +// +// NOTE: Reconciliation of custom search attributes is accomplished using these steps: +// +// 1. Retrieve the custom search attributes which are currently on the Temporal server. +// 2. Determine which custom search attributes need to be removed, if any. +// 3. Determine which custom search attributes need to be added, if any. +// 4. If needed, request the Temporal server to remove the necessary custom search attributes. +// 5. If needed, request the Temporal server to add the necessary custom search attributes. +// +// Some of these steps may fail if some Temporal search attribute constraint is violated; in which case this function will return early with a helpful error message. func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx context.Context, logger *logr.Logger, namespace *v1beta1.TemporalNamespace, cluster *v1beta1.TemporalCluster) error { - /* - NOTE: Reconciliation of custom search attributes is accomplished using these steps: - - 1. Retrieve the custom search attributes which are currently on the Temporal server. - 2. Determine which custom search attributes need to be removed, if any. - 3. Determine which custom search attributes need to be added, if any. - 4. If needed, request the Temporal server to remove the necessary custom search attributes. - 5. If needed, request the Temporal server to add the necessary custom search attributes. - - Some of these steps may fail if some Temporal search attribute constraint is violated; in which case, this function will return early with a helpful error message. - */ - - // To talk to the Temporal server, construct a client + // Construct a client to talk to the Temporal server client, err := temporal.GetClusterClient(ctx, r.Client, cluster) if err != nil { return err @@ -188,13 +186,9 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex // Note that the CustomSearchAttributes map data structure that is built using the Spec merely maps string->string. // To rigorously compare search attributes between the spec and the Temporal server, the types need to be consistent. // We therefore construct a string->enums.IndexedValueType map from the "weaker" string->string map. - specCustomSearchAttributes := make(map[string]enums.IndexedValueType, len(namespace.Spec.CustomSearchAttributes)) - for searchAttributeNameString, searchAttributeTypeString := range namespace.Spec.CustomSearchAttributes { - indexedValueType, err := searchAttributeTypeStringToEnum(searchAttributeTypeString) - if err != nil { - return fmt.Errorf("failed to parse search attribute %s because its type is %s: %w", searchAttributeNameString, searchAttributeTypeString, err) - } - specCustomSearchAttributes[searchAttributeNameString] = indexedValueType + specCustomSearchAttributes, err := createIndexedValueTypeMap(&namespace.Spec.CustomSearchAttributes) + if err != nil { + return err } // Remove those custom search attributes from the Temporal server whose name does not exist in the Spec. @@ -247,6 +241,20 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex return nil } +// createIndexedValueTypeMap is a helper function takes in a map[string]string and returns another +// map where each entry value from the old map is translated from a plain string into an enums.IndexedValueType. +func createIndexedValueTypeMap(m *map[string]string) (map[string]enums.IndexedValueType, error) { + specCustomSearchAttributes := make(map[string]enums.IndexedValueType, len(*m)) + for searchAttributeNameString, searchAttributeTypeString := range *m { + indexedValueType, err := searchAttributeTypeStringToEnum(searchAttributeTypeString) + if err != nil { + return nil, fmt.Errorf("failed to parse search attribute %s because its type is %s: %w", searchAttributeNameString, searchAttributeTypeString, err) + } + specCustomSearchAttributes[searchAttributeNameString] = indexedValueType + } + return specCustomSearchAttributes, nil +} + // searchAttributeTypeStringToEnum retrieves the actual IndexedValueType for a given string. // It expects searchAttributeTypeString to be a string representation of the valid Go type. // Returns the IndexedValueType if parsing is successful, otherwise an error. From db0a0ba2204e6e3e1d395a92f0389d67a82e5d1f Mon Sep 17 00:00:00 2001 From: TheHiddenLayer <37908451+TheHiddenLayer@users.noreply.github.com> Date: Thu, 8 Aug 2024 11:43:16 -0700 Subject: [PATCH 9/9] testing with log statements --- controllers/temporalnamespace_controller.go | 51 ++++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/controllers/temporalnamespace_controller.go b/controllers/temporalnamespace_controller.go index 9227beb9..e049d86b 100644 --- a/controllers/temporalnamespace_controller.go +++ b/controllers/temporalnamespace_controller.go @@ -62,16 +62,19 @@ type TemporalNamespaceReconciler struct { func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) { logger := log.FromContext(ctx) - logger.Info("Starting reconciliation") + logger.Info("=== (NS) STARTING RECONCILIATION") + // logger.Info("Starting reconciliation") namespace := &v1beta1.TemporalNamespace{} err := r.Get(ctx, req.NamespacedName, namespace) if err != nil { if apierrors.IsNotFound(err) { + logger.Info("=== (NS) COULD NOT GET NAMESPACE") return reconcile.Result{}, nil } return reconcile.Result{}, err } + logger.Info("=== (NS) GOT NAMESPACE") patchHelper, err := patch.NewHelper(namespace, r.Client) if err != nil { @@ -84,6 +87,7 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re if err != nil { reterr = kerrors.NewAggregate([]error{reterr, err}) } + logger.Info("=== (NS) PATCHING CLUSTER") }() cluster := &v1beta1.TemporalCluster{} @@ -94,20 +98,25 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re // - TemporalCluster has not been created yet. In this case, if the TemporalNamespace is deleted, no point in waiting for the TemporalCluster to be healthy. // - TemporalCluster existed at some point, but now is deleted. In this case, the underlying namespace in the Temporal server is already gone. controllerutil.RemoveFinalizer(namespace, deletionFinalizer) + logger.Info("=== (NS) ERROR GETTING CLUSTER FROM k8s") return reconcile.Result{}, nil } + logger.Info("=== (NS) ERROR GETTING CLUSTER FROM k8s") return r.handleError(namespace, v1beta1.ReconcileErrorReason, err) } + logger.Info("=== (NS) GOT CLUSTER FROM k8s") if !cluster.IsReady() { - logger.Info("Skipping namespace reconciliation until referenced cluster is ready") + logger.Info("=== (NS) SKIPPING NAMESPACE RECONCILIATION UNTIL REFERENCED CLUSTER IS READY, REQUEUEING") + // logger.Info("Skipping namespace reconciliation until referenced cluster is ready") return reconcile.Result{RequeueAfter: 10 * time.Second}, nil } // Check if the resource has been marked for deletion if !namespace.ObjectMeta.DeletionTimestamp.IsZero() { - logger.Info("Deleting namespace") + logger.Info("=== (NS) DELETING NAMESPACE") + // logger.Info("Deleting namespace") err := r.ensureNamespaceDeleted(ctx, namespace, cluster) if err != nil { @@ -135,18 +144,23 @@ func (r *TemporalNamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Re return r.handleError(namespace, v1beta1.ReconcileErrorReason, err) } err = client.Update(ctx, temporal.NamespaceToUpdateNamespaceRequest(cluster, namespace)) + logger.Info("=== (NS) UPDATING NAMESPACE") if err != nil { return r.handleError(namespace, v1beta1.ReconcileErrorReason, err) } } + logger.Info("=== (CSA) BEFORE RECONCILING SEARCH ATTRIBUTES") err = r.reconcileCustomSearchAttributes(ctx, &logger, namespace, cluster) if err != nil { - logger.Info(fmt.Sprintf("Failed to reconcile custom search attributes: %v", err)) + logger.Info(fmt.Sprintf("=== (CSA) FAILED TO RECONCILE CUSTOM SEARCH ATTRIBUTES: %v", err)) + //logger.Info(fmt.Sprintf("Failed to reconcile custom search attributes: %v", err)) return r.handleError(namespace, v1beta1.ReconcileErrorReason, err) } + logger.Info("=== (CSA) AFTER RECONCILING SEARCH ATTRIBUTES") - logger.Info("Successfully reconciled namespace", "namespace", namespace.GetName()) + logger.Info("=== (NS) SUCCESSFULLY RECONCILED NAMESPACE", "namespace", namespace.GetName()) + // logger.Info("Successfully reconciled namespace", "namespace", namespace.GetName()) v1beta1.SetTemporalNamespaceReady(namespace, metav1.ConditionTrue, v1beta1.TemporalNamespaceCreatedReason, "Namespace successfully created") @@ -168,28 +182,36 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex // Construct a client to talk to the Temporal server client, err := temporal.GetClusterClient(ctx, r.Client, cluster) if err != nil { + logger.Info("=== (CSA) COULDN'T CREATE CLIENT TO TALK TO TEMPORAL SERVER") return err } + logger.Info("=== (CSA) CREATED CLIENT TO TALK TO TEMPORAL SERVER") // Requests to Temporal's OperatorService API need to specify the namespace name, so capture it here for future use. ns := namespace.GetName() // List all search attributes that are currently on the Temporal server listRequest := &operatorservice.ListSearchAttributesRequest{Namespace: ns} + logger.Info("=== (CSA) CONSTRUCTED LIST SEARCH ATTR REQUEST") serverSearchAttributes, err := client.OperatorService().ListSearchAttributes(ctx, listRequest) if err != nil { + logger.Info("=== (CSA) COULDN'T RECEIVE SEARCH ATTRIBUTES ON SERVER") return err } + logger.Info("=== (CSA) RECEIVED SEARCH ATTRIBUTES ON SERVER") // Narrow the focus to custom search attributes only. serverCustomSearchAttributes := &serverSearchAttributes.CustomAttributes // use a pointer to avoid unnecessary copying + logger.Info(fmt.Sprintf("=== (CSA) CUSTOM SEARCH ATTRIBUTES ON SERVER: %v", serverCustomSearchAttributes)) // Note that the CustomSearchAttributes map data structure that is built using the Spec merely maps string->string. // To rigorously compare search attributes between the spec and the Temporal server, the types need to be consistent. // We therefore construct a string->enums.IndexedValueType map from the "weaker" string->string map. specCustomSearchAttributes, err := createIndexedValueTypeMap(&namespace.Spec.CustomSearchAttributes) if err != nil { + logger.Info(fmt.Sprintf("=== (CSA) ERROR READING CUSTOM SEARCH ATTRIBUTES IN SPEC: %v", specCustomSearchAttributes)) return err } + logger.Info(fmt.Sprintf("=== (CSA) CUSTOM SEARCH ATTRIBUTES IN SPEC: %v", specCustomSearchAttributes)) // Remove those custom search attributes from the Temporal server whose name does not exist in the Spec. customSearchAttributesToRemove := make([]string, 0) @@ -199,6 +221,7 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex customSearchAttributesToRemove = append(customSearchAttributesToRemove, serverSearchAttributeName) } } + logger.Info(fmt.Sprintf("=== (CSA) CUSTOM SEARCH ATTRIBUTES TO REMOVE: %v", customSearchAttributesToRemove)) // Add custom search attributes from the Spec which don't yet exist on the Temporal server. // If the Temporal server already has a custom search attribute with the same name but a different type, then return an error. @@ -208,9 +231,11 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex if !specSearchAttributeNameExistsOnServer { customSearchAttributesToAdd[specSearchAttributeName] = specSearchAttributeType } else if specSearchAttributeType != serverSearchAttributeType { + logger.Info(fmt.Sprintf("=== (CSA) CUSTOM SEARCH ATTRIBUTE %s ALREADY EXISTS AND HAS DIFFERENT TYPE %s", specSearchAttributeName, serverSearchAttributeType.String())) return fmt.Errorf("search attribute %s already exists and has different type %s", specSearchAttributeName, serverSearchAttributeType.String()) } } + logger.Info(fmt.Sprintf("=== (CSA) CUSTOM SEARCH ATTRIBUTES TO ADD: %v", customSearchAttributesToAdd)) // If there are search attributes that should be removed, then make a request to the Temporal server to remove them. if len(customSearchAttributesToRemove) > 0 { @@ -218,11 +243,16 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex Namespace: ns, SearchAttributes: customSearchAttributesToRemove, } + logger.Info("=== (CSA) CONSTRUCTED REMOVE ATTR REQUEST") _, err = client.OperatorService().RemoveSearchAttributes(ctx, removeRequest) + logger.Info("=== (CSA) SERVER RESPONDED TO REMOVE REQUEST") if err != nil { return fmt.Errorf("failed to remove search attributes: %w", err) } - logger.Info(fmt.Sprintf("removed custom search attributes: %v", customSearchAttributesToRemove)) + logger.Info(fmt.Sprintf("=== (CSA) REMOVED CUSTOM SEARCH ATTRIBUTES: %v", customSearchAttributesToRemove)) + // logger.Info(fmt.Sprintf("removed custom search attributes: %v", customSearchAttributesToRemove)) + } else { + logger.Info(fmt.Sprintf("=== (CSA) NO SEARCH ATTRIBUTES TO REMOVE %v", customSearchAttributesToRemove)) } // If there are search attributes that should be added, then make a request the Temporal server to create them. @@ -231,13 +261,20 @@ func (r *TemporalNamespaceReconciler) reconcileCustomSearchAttributes(ctx contex Namespace: ns, SearchAttributes: customSearchAttributesToAdd, } + logger.Info("=== (CSA) CONSTRUCTED ADD ATTR REQUEST") _, err = client.OperatorService().AddSearchAttributes(ctx, addRequest) + logger.Info("=== (CSA) SERVER RESPONDED TO ADD REQUEST") if err != nil { return fmt.Errorf("failed to add search attributes: %w", err) } - logger.Info(fmt.Sprintf("added custom search attributes: %v", customSearchAttributesToAdd)) + logger.Info(fmt.Sprintf("=== (CSA) ADDED CUSTOM SEARCH ATTRIBUTES: %v", customSearchAttributesToAdd)) + // logger.Info(fmt.Sprintf("added custom search attributes: %v", customSearchAttributesToAdd)) + } else { + logger.Info(fmt.Sprintf("=== (CSA) NO SEARCH ATTRIBUTES TO ADD %v", customSearchAttributesToAdd)) } + + logger.Info("=== (CSA) CUSTOM SEARCH ATTRIBUTE RECONCILIATION LOOP FINISHED") return nil }