New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wip (draft pr) - reconcile sap-btp-manager secret changes #932

Closed

ukff wants to merge 28 commits into main from supportsecretchanges2

Contributor

ukff commented Dec 18, 2024

No description provided.

ukff added 13 commits

December 11, 2024 14:44

wip

80ac715

wip

c3cee01


          edit

abe3131


          edit

927f0aa


          edit2

67a8190


          edit3

8fc57d1


          edit3

76c50d5


          edit4

bf89e52


          Update

e19e4db

wip

7fe3cdc

wip

a195c89

wip

dcec865

wip

a9f02c3

kyma-bot added do-not-merge/work-in-progress cla: yes labels

github-actions bot commented Dec 18, 2024

Add one of following labels

- kind/feature -> Use it when you want to submit a new feature

- kind/enhancement -> Use it when you modify or improve an existing feature

- kind/bug -> Use it when you fix a bug

kyma-bot added the size/L label

wip

810a601

ukff force-pushed the supportsecretchanges2 branch from 15ac9a8 to 810a601 Compare

December 19, 2024 08:06

ukff added 11 commits

December 19, 2024 17:28

wip

586a89e

wip

304c09f

wip

ef771d6

wip

26f45a6

wip

d87c566

wip

4c3f94f

wip

cf1ace0

wip

649e6b8

wip

fbb46ab

wip

b5fe9f7

wip

457c28e

ukff added 3 commits

December 23, 2024 10:35

wip

e04d42c

wip

c105664

wip

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

		}

		if labels != nil {

Contributor

jaroslaw-pieszka Dec 19, 2024

We have already assigned labels and data to the structure fields.

controllers/btpoperator_controller.go

               	r.deleteCreationTimestamp(resourcesToApply...)
+              	logger.Info(fmt.Sprintf("check if deployment: %s in %s needs to be restarted.", DeploymentName, resolvedNamespace))
+              	clusterIdSecretChanged := r.checkIfOperatorNeedRestart(ctx, resolvedNamespace, baseSecret, &logger)

Contributor

jaroslaw-pieszka Dec 30, 2024

checkIfOperatorNeedRestart detects if clusterId was changed so the name is misleading.

Contributor

jaroslaw-pieszka Dec 30, 2024

On the other hand the implementation check both cases, so possibly the variable name is inappropriate.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              			Namespace: inNamespace,
+              		},
+              	}
+              	exists := make(chan bool)

Contributor

jaroslaw-pieszka Dec 30, 2024 •

edited

Loading

Could we use more informative channel name?
exists usually is used as boolean flag.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              	go r.checkResourceExistence(ctx, obj, exists, time.Second*10)
+              	ok := <-exists
+              	if !ok {
+              		return fmt.Errorf("secret: %s in %s didnt appear in given time", clusterIdSecretName, inNamespace)

Contributor

jaroslaw-pieszka Dec 30, 2024

Suggested change

      
            		return fmt.Errorf("secret: %s in %s didnt appear in given time", clusterIdSecretName, inNamespace)
          
            		return fmt.Errorf("secret: %s in %s did not appear in given time", clusterIdSecretName, inNamespace)

Contributor

jaroslaw-pieszka Dec 30, 2024

Still appear is not the best way to describe what we are expecting.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              				Namespace: ChartNamespace,
+              			},
+              		}
+              		err := r.Get(context.TODO(), client.ObjectKeyFromObject(currentConfigMap), currentConfigMap)

Contributor

jaroslaw-pieszka Dec 30, 2024

Non-existent config map is a different story. Restart would not help, since map is necessary for operator to start.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              			return true
+              		}
+              		return !reflect.DeepEqual(baseSecret.Data["management_namespace"], []byte(currentConfigMap.Data["RELEASE_NAMESPACE"])) ||

Contributor

jaroslaw-pieszka Dec 30, 2024

Why we compare management_namespace with release namespace?

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              }
+              func (r *BtpOperatorReconciler) checkIfOperatorNeedRestart(ctx context.Context, resolvedNamespace string, baseSecret *corev1.Secret, logger *logr.Logger) bool {
+              	detectConfigMapChanges := func() bool {

Contributor

jaroslaw-pieszka Dec 30, 2024

Functional objects seem to be overkill here.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              		}
+              		err := r.Client.Get(ctx, client.ObjectKeyFromObject(&clusterIdSecret), &clusterIdSecret)
+              		if k8serrors.IsNotFound(err) {
+              			logger.Info(fmt.Sprintf("secret %s in %s not found, so restart is needed", clusterIdSecretName, resolvedNamespace))

Contributor

jaroslaw-pieszka Dec 30, 2024

How restart would help in this case?

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              			logger.Info(fmt.Sprintf("secret %s in %s not found, so restart is needed", clusterIdSecretName, resolvedNamespace))
+              			return true
+              		}
+              		return !reflect.DeepEqual(baseSecret.Data["cluster_id"], clusterIdSecret.Data["INITIAL_CLUSTER_ID"])

Contributor

jaroslaw-pieszka Dec 30, 2024

Why DeepEqual?
Simple comparison is not enough?

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

-              	return unstructured.SetNestedField(u.Object, string(secret.Data["cluster_id"]), "data", "CLUSTER_ID")
+              func (r *BtpOperatorReconciler) setConfigMapValues(baseSecret *corev1.Secret, configMap *unstructured.Unstructured, resolvedNamespace string) error {
+              	err := unstructured.SetNestedField(configMap.Object, resolvedNamespace, "data", "MANAGEMENT_NAMESPACE")
+              	err = unstructured.SetNestedField(configMap.Object, resolvedNamespace, "data", "RELEASE_NAMESPACE")

Contributor

jaroslaw-pieszka Dec 30, 2024

AFAIK we should support change of management namespace not release namespace.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

-              func (r *BtpOperatorReconciler) setConfigMapValues(secret *corev1.Secret, u *unstructured.Unstructured) error {
-              	return unstructured.SetNestedField(u.Object, string(secret.Data["cluster_id"]), "data", "CLUSTER_ID")
+              func (r *BtpOperatorReconciler) setConfigMapValues(baseSecret *corev1.Secret, configMap *unstructured.Unstructured, resolvedNamespace string) error {
+              	err := unstructured.SetNestedField(configMap.Object, resolvedNamespace, "data", "MANAGEMENT_NAMESPACE")

Contributor

jaroslaw-pieszka Dec 30, 2024

We overwrite error in the next line. So named variable is not needed.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

               }
               func (r *BtpOperatorReconciler) setSecretValues(secret *corev1.Secret, u *unstructured.Unstructured) error {
               	for k := range secret.Data {
-              		if err := unstructured.SetNestedField(u.Object, base64.StdEncoding.EncodeToString(secret.Data[k]), "data", k); err != nil {
-              			return err
+              		for _, r := range requiredSecretKeys {

Contributor

jaroslaw-pieszka Dec 30, 2024

This is over-engineered.
We just need to change one value of the map.
I think no loop is needed, at least not two nested loops.

Contributor Author

ukff Dec 31, 2024

why it is over engineered? we want only fields from requiresSecretsKeys. We dont want add any more keys to base secret than are required (for example namespace)

Contributor

jaroslaw-pieszka Dec 31, 2024

We will discuss it later on.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

               		logger.Error(err, "resources reconciliation failed")
               	}
               }
+              func (r *BtpOperatorReconciler) restartOperator(ctx context.Context, inNamespace string, logger *logr.Logger) error {
+              	logger.Info(fmt.Sprintf("restarting of deployment: %s in %s started", DeploymentName, inNamespace))

Contributor

jaroslaw-pieszka Dec 30, 2024 •

edited

Loading

Suggested change

      
            	logger.Info(fmt.Sprintf("restarting of deployment: %s in %s started", DeploymentName, inNamespace))
          
            	logger.Info(fmt.Sprintf("restarting deployment: %s in %s restarted", DeploymentName, inNamespace))

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

               		logger.Error(err, "resources reconciliation failed")
               	}
               }
+              func (r *BtpOperatorReconciler) restartOperator(ctx context.Context, inNamespace string, logger *logr.Logger) error {

Contributor

jaroslaw-pieszka Dec 30, 2024

This secret with cluster_id is always located in release namespace.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              func (r *BtpOperatorReconciler) restartOperator(ctx context.Context, inNamespace string, logger *logr.Logger) error {
+              	logger.Info(fmt.Sprintf("restarting of deployment: %s in %s started", DeploymentName, inNamespace))
+              	clusterIdSecret := &corev1.Secret{

Contributor

jaroslaw-pieszka Dec 30, 2024

Should be extracted to function with the proper responsibility - deletion of the secret.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              	if !k8serrors.IsNotFound(err) {
+              		return "", "", false, fmt.Errorf("failed to get configmap %s in %s, %w", ConfigName, ChartNamespace, err)
+              	}
+              	currentNamespace := currentConfigMap.Data["management_namespace"]

Contributor

jaroslaw-pieszka Dec 30, 2024 •

edited

Loading

The key in the map is in uppercase: MANAGEMENT_NAMESPACE

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              	return nil
+              }
+              func (r *BtpOperatorReconciler) resolveNamespace(ctx context.Context, secret *corev1.Secret, logger *logr.Logger) (string, string, bool, error) {

Contributor

jaroslaw-pieszka Dec 30, 2024

It is not clear what is the responsibility here.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

               	}
               	logger.Info(fmt.Sprintf("got %d module resources to apply based on %s directory", len(resourcesToApply), r.getResourcesToApplyPath()))
+              	resolvedNamespace, _, namespaceChanged, err := r.resolveNamespace(ctx, baseSecret, &logger)

Contributor

jaroslaw-pieszka Dec 30, 2024

Too many responsibilities for this function.
And why to return value which is ignored (always).

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

+              	err := unstructured.SetNestedField(configMap.Object, resolvedNamespace, "data", "MANAGEMENT_NAMESPACE")
+              	err = unstructured.SetNestedField(configMap.Object, resolvedNamespace, "data", "RELEASE_NAMESPACE")
+              	err = unstructured.SetNestedField(configMap.Object, string(baseSecret.Data["cluster_id"]), "data", "CLUSTER_ID")

Contributor

jaroslaw-pieszka Dec 30, 2024

Shouldn't we extract literals, so commonly used: CLUSTER_ID et al.

jaroslaw-pieszka reviewed

View reviewed changes

controllers/btpoperator_controller.go

               	return nil
               }
-              func (r *BtpOperatorReconciler) reconcileResources(ctx context.Context, s *corev1.Secret) error {
+              func (r *BtpOperatorReconciler) reconcileResources(ctx context.Context, baseSecret *corev1.Secret) error {

Contributor

jaroslaw-pieszka Dec 30, 2024

How about State of the resource?
Is it in line with what is happening?
Restart could take some time, operator is not operational and we enter reconcileResources either from Ready state or from Processing state.

Contributor Author

ukff commented Dec 31, 2024 •

edited

Loading

@jaroslaw-pieszka this pr is draft and wip. it was not to be supposed to review now :D but i will takie into considerations

ukff closed this

ukff changed the title ~~wip - reconcile sap-btp-manager secret changes~~ wip (draft pr) - reconcile sap-btp-manager secret changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes do-not-merge/work-in-progress size/L