Fix resource update reconciling, increase log level for users reconci…

…ler (#727)

### Description
Fixes #633
Fixes #731 

Based on investigations
[here](#633 (comment))
and
[here](#731 (comment))
it was found that a discrepancy between `<nil slice>` and `<[] slice>`
is causing the operator to constantly update User, Role, IndexTemplate,
etc.

Also, the current log level for User reconciles produces too many logs,
so increasing its level may be a good idea here.

Because of the issue with comparing `nil` and empty slices, I have
replaced `reflect.DeepEqual` with `comp.Equal`. Also, added a helper
function that will sort nested json keys in cases when API returns
unsorted keys which is casing comparing to return diff.
### Issues Resolved
Closes #633 
Closes #731

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Yevhenii Tiutiunnyk <[email protected]>

opensearch-operator/controllers/opensearchuser_controller.go

@@ -53,7 +53,7 @@ type OpensearchUserReconciler struct {
 // move the current state of the cluster closer to the desired state.
 func (r *OpensearchUserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	r.Logger = log.FromContext(ctx).WithValues("user", req.NamespacedName)
-	r.Logger.Info("Reconciling OpensearchUser")
+	r.Logger.V(4).Info("Reconciling OpensearchUser")
 
 	r.Instance = &opsterv1.OpensearchUser{}
 	err := r.Get(ctx, req.NamespacedName, r.Instance)

opensearch-operator/go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/cisco-open/k8s-objectmatcher v1.9.0
 	github.com/cisco-open/operator-tools v0.30.0
 	github.com/go-logr/logr v1.2.4
+	github.com/google/go-cmp v0.5.9
 	github.com/hashicorp/go-version v1.6.0
 	github.com/jarcoal/httpmock v1.2.0
 	github.com/kralicky/kmatch v0.0.0-20220713045459-85a252b9275e
@@ -48,7 +49,6 @@ require (
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/google/gnostic v0.5.7-v3refs // indirect
-	github.com/google/go-cmp v0.5.9 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // indirect
 	github.com/google/uuid v1.3.0 // indirect

opensearch-operator/opensearch-gateway/services/os_data_service.go

@@ -4,7 +4,8 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"reflect"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"strings"
 
 	"github.com/Opster/opensearch-k8s-operator/opensearch-operator/opensearch-gateway/requests"
@@ -349,7 +350,22 @@ func ShouldUpdateIndexTemplate(
 	if indexTemplateResponse.Name != indexTemplateName {
 		return false, fmt.Errorf("returned index template named '%s' does not equal the requested name '%s'", indexTemplateResponse.Name, indexTemplateName)
 	}
-	if reflect.DeepEqual(indexTemplate, indexTemplateResponse.IndexTemplate) {
+
+	if indexTemplateResponse.IndexTemplate.Template.Settings != nil {
+		indexTemplateResponse.IndexTemplate.Template.Settings, err = helpers.SortedJsonKeys(indexTemplateResponse.IndexTemplate.Template.Settings)
+		if err != nil {
+			return false, err
+		}
+	}
+
+	if indexTemplateResponse.IndexTemplate.Template.Mappings != nil {
+		indexTemplateResponse.IndexTemplate.Template.Mappings, err = helpers.SortedJsonKeys(indexTemplateResponse.IndexTemplate.Template.Mappings)
+		if err != nil {
+			return false, err
+		}
+	}
+
+	if cmp.Equal(indexTemplate, indexTemplateResponse.IndexTemplate, cmpopts.EquateEmpty()) {
 		return false, nil
 	}
 
@@ -460,7 +476,7 @@ func ShouldUpdateComponentTemplate(
 		return false, fmt.Errorf("returned component template named '%s' does not equal the requested name '%s'", componentTemplateResponse.Name, componentTemplateName)
 	}
 
-	if reflect.DeepEqual(componentTemplate, componentTemplateResponse.ComponentTemplate) {
+	if cmp.Equal(componentTemplate, componentTemplateResponse.ComponentTemplate, cmpopts.EquateEmpty()) {
 		return false, nil
 	}
 

opensearch-operator/opensearch-gateway/services/os_ism_service.go

@@ -5,9 +5,9 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"reflect"
-
 	"github.com/Opster/opensearch-k8s-operator/opensearch-operator/opensearch-gateway/requests"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/opensearch-project/opensearch-go/opensearchutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
@@ -16,7 +16,7 @@ var ErrNotFound = errors.New("Policy not found")
 
 // ShouldUpdateISMPolicy checks if the passed policy is same as existing or needs update
 func ShouldUpdateISMPolicy(ctx context.Context, newPolicy, existingPolicy requests.Policy) (bool, error) {
-	if reflect.DeepEqual(newPolicy, existingPolicy) {
+	if cmp.Equal(newPolicy, existingPolicy, cmpopts.EquateEmpty()) {
 		return false, nil
 	}
 	lg := log.FromContext(ctx).WithValues("os_service", "policy")

opensearch-operator/opensearch-gateway/services/os_security_service.go

@@ -4,10 +4,10 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"reflect"
-
 	"github.com/Opster/opensearch-k8s-operator/opensearch-operator/opensearch-gateway/requests"
 	"github.com/Opster/opensearch-k8s-operator/opensearch-operator/opensearch-gateway/responses"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/opensearch-project/opensearch-go/opensearchutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
@@ -59,7 +59,7 @@ func ShouldUpdateUser(
 		return false, fmt.Errorf("kubernetes resource conflict; uids don't match")
 	}
 
-	if reflect.DeepEqual(user, userResponse[username]) {
+	if cmp.Equal(user, userResponse[username], cmpopts.EquateEmpty()) {
 		return false, nil
 	}
 
@@ -176,7 +176,7 @@ func ShouldUpdateRole(
 		return false, err
 	}
 
-	if reflect.DeepEqual(role, roleResponse[rolename]) {
+	if cmp.Equal(role, roleResponse[rolename], cmpopts.EquateEmpty()) {
 		return false, nil
 	}
 
@@ -331,7 +331,7 @@ func ShouldUpdateActionGroup(
 		return false, err
 	}
 
-	if reflect.DeepEqual(actionGroup, actionGroupResponse[actionGroupName]) {
+	if cmp.Equal(actionGroup, actionGroupResponse[actionGroupName], cmpopts.EquateEmpty()) {
 		return false, nil
 	}
 
@@ -415,7 +415,7 @@ func ShouldUpdateTenant(
 		return false, err
 	}
 
-	if reflect.DeepEqual(tenant, tenantResponse[tenantName]) {
+	if cmp.Equal(tenant, tenantResponse[tenantName], cmpopts.EquateEmpty()) {
 		return false, nil
 	}
 

opensearch-operator/pkg/helpers/helpers.go

@@ -1,8 +1,10 @@
 package helpers
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"reflect"
 	"sort"
 	"time"
@@ -149,6 +151,22 @@ func SortedKeys(input map[string]string) []string {
 	return keys
 }
 
+// SortedJsonKeys helps to sort JSON object keys
+// E.g. if API returns unsorted JSON object like this: {"resp": {"b": "2", "a": "1"}}
+// this function could sort it and return {"resp": {"a": "1", "b": "2"}}
+// This is useful for comparing Opensearch CRD objects and API responses
+func SortedJsonKeys(obj *apiextensionsv1.JSON) (*apiextensionsv1.JSON, error) {
+	m := make(map[string]interface{})
+	if err := json.Unmarshal(obj.Raw, &m); err != nil {
+		return nil, err
+	}
+	rawBytes, err := json.Marshal(m)
+	if err != nil {
+		return nil, err
+	}
+	return &apiextensionsv1.JSON{Raw: rawBytes}, err
+}
+
 func ResolveClusterManagerRole(ver string) string {
 	masterRole := "master"
 	osVer, err := version.NewVersion(ver)

opensearch-operator/pkg/reconcilers/indextemplate_test.go

@@ -52,7 +52,9 @@ var _ = Describe("indextemplate reconciler", func() {
 				Name:          "my-template",
 				IndexPatterns: []string{"my-logs-*"},
 				Template: opsterv1.OpensearchIndexSpec{
-					Settings: &apiextensionsv1.JSON{},
+					Settings: &apiextensionsv1.JSON{
+						Raw: []byte(`{"index":{"number_of_replicas":"1","number_of_shards":"5","refresh_interval":"60s"}}`),
+					},
 					Mappings: &apiextensionsv1.JSON{},
 					Aliases:  make(map[string]opsterv1.OpensearchIndexAliasSpec),
 				},
@@ -273,7 +275,9 @@ var _ = Describe("indextemplate reconciler", func() {
 						IndexTemplate: requests.IndexTemplate{
 							IndexPatterns: []string{"my-logs-*"},
 							Template: requests.Index{
-								Settings: &apiextensionsv1.JSON{},
+								Settings: &apiextensionsv1.JSON{
+									Raw: []byte(`{"index":{"refresh_interval":"60s","number_of_shards":"5","number_of_replicas":"1"}}`),
+								},
 								Mappings: &apiextensionsv1.JSON{},
 								Aliases:  make(map[string]requests.IndexAlias),
 							},
@@ -310,7 +314,9 @@ var _ = Describe("indextemplate reconciler", func() {
 						IndexTemplate: requests.IndexTemplate{
 							IndexPatterns: []string{"my-logs-*"},
 							Template: requests.Index{
-								Settings: &apiextensionsv1.JSON{},
+								Settings: &apiextensionsv1.JSON{
+									Raw: []byte(`{"index":{"refresh_interval":"60s","number_of_shards":"5","number_of_replicas":"1"}}`),
+								},
 								Mappings: &apiextensionsv1.JSON{},
 								Aliases:  make(map[string]requests.IndexAlias),
 							},