Creation of CompassManagerMapping CRD after successfull registration Runtime in Compass #27

mvshao · 2023-09-19T12:53:45Z

Description

Changes proposed in this pull request:

reconcile triggers only while the Application Connector module is present in Kyma CR
changed the way of marking Runtime as connected to Compass from labeling Kyma CR to creating Compass Manager Mapping CRD with all the data needed
added WhiteSource and Protecode scans for module
~~Compass Manager is capable of refreshing Compass one-time token~~ commented -> moved to different PR

Related issue(s)

akgalwas

I think the PR is ok, we are close to finalise it.

akgalwas · 2023-09-24T10:59:10Z

controllers/compassmanager_controller.go

+func (cm *CompassManagerReconciler) createCompassMappingResource(compassRuntimeID string, kymaLabels map[string]string) error {
+	compassMapping := &v1beta1.CompassManagerMapping{}
+	compassMapping.Name = kymaLabels[KymaNameLabel]
+	compassMapping.Namespace = DefaultResourceNamespace


Namespace name for the Compass Manager Mapping is hardcoded. Please consider using namespace name from Kyma CR.

akgalwas · 2023-09-24T11:06:43Z

controllers/compassmanager_controller.go

-func (cm *CompassManagerReconciler) markRuntimeRegistered(objKey types.NamespacedName, compassID string) error {
-	instance := &kyma.Kyma{}
+func (cm *CompassManagerReconciler) createCompassMappingResource(compassRuntimeID string, kymaLabels map[string]string) error {
+	compassMapping := &v1beta1.CompassManagerMapping{}


I'm wondering whether we should add operator.kyma-project.io/managed-by label with Compass Manager value to determine that Compass Manager controls the CRs lifecycle.

Good idea, I will add this

akgalwas · 2023-09-24T11:10:11Z

controllers/compassmanager_controller.go

+	BrokerInstanceIDLabel          = "kyma-project.io/instance-id"
+	ShootNameLabel                 = "kyma-project.io/shoot-name"
+	SubaccountIDLabel              = "kyma-project.io/subaccount-id"
+	KymaIDLabel                    = "kyma-project.io/kyma-id"


In the original version of the specification we use kyma-id. This was my mistake, let's consistently use operator.kyma-project.io/kyma-name. Its better to have the same label on Kyma, and Compass Manager to avoid confusion.

akgalwas · 2023-09-24T11:40:49Z

controllers/compassmanager_controller.go

+		}
+	}
+
+	// If kcp-system Namespace contains Compass Manager CR with compass-id and runtime-id correlated with given in Kyma CR, skip reconciliation. Cluster is already connected


I think this comment is outdated.

akgalwas · 2023-09-24T11:52:12Z

controllers/compassmanager_controller.go

 	// KubeconfigKey is the name of the key in the secret storing cluster credentials.
 	// The secret is created by KEB: https://github.com/kyma-project/control-plane/blob/main/components/kyma-environment-broker/internal/process/steps/lifecycle_manager_kubeconfig.go
 	KubeconfigKey = "config"
 )

 //+kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch
-//+kubebuilder:rbac:groups=operator.kyma-project.io,resources=kymas,verbs=get;list;watch;update
+//+kubebuilder:rbac:groups=operator.kyma-project.io,resources=kymas,verbs=get;list;watch
+//+kubebuilder:rbac:groups=operator.kyma-project.io,resources=compassmanagermappings,verbs=create;get;list;watch;update;delete


Probably we could limit privileges for Compass Manager Mappings to the following: create;get;list;delete

The watch privilege is required. Because the controller returns an error

[reflector.go:148] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:231: Failed to watch *v1beta1.CompassManagerMapping: unknown (get compassmanagermappings.operator.kyma-project.io)

akgalwas · 2023-09-24T11:57:38Z

controllers/compassmanager_controller.go

@@ -94,7 +105,7 @@ func (cm *CompassManagerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return ctrl.Result{RequeueAfter: requeueTime}, err
 	}

-	compassRuntimeID, err := cm.Configurator.RegisterInCompass(createCompassRuntimeLabels(kymaLabels))
+	compassRuntimeID, err := cm.Registrator.RegisterInCompass(createCompassRuntimeLabels(kymaLabels))


Before registration is done, we should check if the Compass Manager Mapping already exists.

I have this feature uncommited, but implemetned.

akgalwas · 2023-09-25T04:16:37Z

config/samples/operator_v1beta1_compassmanagermapping.yaml

+    app.kubernetes.io/managed-by: kustomize
+    app.kubernetes.io/created-by: compass-manager
+  name: compassmanagermapping-sample
+spec:


I think we could have missed one thing here. As noted in the issue concerning resource watching we should consider setting statuses for operations. As we can't set status on the Kyma resource, CompassManagerMapping seems to be the right place.

Why this is needed?

Let's imagine situation when we have Compass outage. It will we surely visible in the logs, as Compass Manager will constantly retry requests. If we had proper alerting/monitoring we could also be notified that some problem occurred.
I see the following benefits of adding the status property:

We would get additional insight into the system:

We could see how many runtimes that have module enabled cannot be registered due to Compass failures.

We could see when Compass Manager noticed module is enabled, and when runtime was registered, and secret with configuration created

We could add some dashboards

You can see the example of status structures here.

I think this needs some discussions before we decide to implement it. If we decide to go with statuses let's do that in the separate PR.

akgalwas · 2023-09-25T04:39:11Z

controllers/compassmanager_controller.go

@@ -108,7 +119,7 @@ func (cm *CompassManagerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	}
 	cm.Log.Infof("Compass Runtime Agent for Runtime %s configured.", compassRuntimeID)

-	return ctrl.Result{}, cm.markRuntimeRegistered(req.NamespacedName, compassRuntimeID)
+	return ctrl.Result{}, cm.createCompassMappingResource(compassRuntimeID, kymaLabels)


One note: if we decide to provide statuses on the CR we should create it even in case the Compass registration failed. This is just a reminder, I think we should discuss if we go with the status field, and then create a new PR.

akgalwas · 2023-09-28T06:17:05Z

controllers/compassmanager_controller.go

@@ -88,13 +101,25 @@ func (cm *CompassManagerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return ctrl.Result{RequeueAfter: requeueTime}, nil
 	}

+	compassMappingLabels, err := cm.getCompassMappingLabels(req.Name)


I'm confused a bit. I though we will implement obtaining token in other PR.

akgalwas · 2023-09-28T06:19:49Z

controllers/compassmanager_controller.go

+	return err
+}
+
+func (cm *CompassManagerReconciler) getCompassMappingLabels(mappingName string) (map[string]string, error) {


I would rename mappingName to kymaName to avoid confusion.

CLAassistant · 2023-09-29T04:39:33Z

All committers have signed the CLA.

CLAassistant · 2023-09-29T04:39:33Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ mvshao
❌ akgalwas
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…ccount

…ger Mapping

mvshao · 2023-10-02T13:02:45Z

/test pre-compass-manager-presubmit-scanner

mvshao added the area/application-connector Issues or PRs related to application connectivity label Sep 19, 2023

mvshao requested a review from a team as a code owner September 19, 2023 12:53

kyma-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2023

mvshao changed the title ~~Creation of CompassManagerMapping CRD after succesfull registration Runetime in Compass~~ Creation of CompassManagerMapping CRD after successfull registration Runtime in Compass Sep 19, 2023

kyma-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 21, 2023

akgalwas reviewed Sep 25, 2023

View reviewed changes

mvshao mentioned this pull request Sep 25, 2023

Compass Manager - implement registering, and unregistering Runtimes in Compass Director #10

Closed

4 tasks

Disper assigned akgalwas Sep 26, 2023

akgalwas reviewed Sep 28, 2023

View reviewed changes

kyma-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 28, 2023

mvshao and others added 15 commits September 29, 2023 14:49

Watching on App Conn module enable, todo unit tests

3a1a31f

Creation of CompassManagerMapping after succesfull registration

97db56c

Fix important lint errors

20220ee

Fix important lint errors v2

bce309b

Reorganize interface

707ee7d

Add security scans

aa83185

Add security scans

2625d69

Update image in scans

b1580ba

Token refreshing and unit tests

4c891b2

Apply review suggestions

4978725

Fix linting errors

1493a28

Apply review suggestions, renaming ID to compassID, tenant to globalA…

33b642f

…ccount

Refactored control loop, and added error handling around Compass Mana…

0a6d02e

…ger Mapping

Inline registerRuntimeIfNotExists

bae79f9

linter

b1c0758

add branch to ga lint

6899845

mvshao force-pushed the crd-creation branch from 9ac2f9d to 6899845 Compare September 29, 2023 12:51

mvshao added 4 commits October 2, 2023 10:37

Lint logging file name

c31096b

Fix linting errors

0f2db41

Checking another output format for lint

cc2317c

Fix errors in linter

43a9e71

mvshao added 3 commits October 2, 2023 15:43

Fix

5e0c83b

Fix lint error

09bf456

Fix lint error v2

706da82

akgalwas approved these changes Oct 4, 2023

View reviewed changes

kyma-bot added the lgtm Looks good to me! label Oct 4, 2023

kyma-bot merged commit f50ad5a into kyma-project:main Oct 4, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creation of CompassManagerMapping CRD after successfull registration Runtime in Compass #27

Creation of CompassManagerMapping CRD after successfull registration Runtime in Compass #27

mvshao commented Sep 19, 2023 •

edited

Loading

akgalwas left a comment

akgalwas Sep 24, 2023

akgalwas Sep 24, 2023

mvshao Sep 25, 2023

akgalwas Sep 24, 2023

akgalwas Sep 24, 2023

akgalwas Sep 24, 2023

mvshao Sep 27, 2023

akgalwas Sep 24, 2023

mvshao Sep 25, 2023

akgalwas Sep 25, 2023

akgalwas Sep 25, 2023

akgalwas Sep 28, 2023 •

edited

Loading

akgalwas Sep 28, 2023

CLAassistant commented Sep 29, 2023 •

edited

Loading

CLAassistant commented Sep 29, 2023

mvshao commented Oct 2, 2023

Creation of CompassManagerMapping CRD after successfull registration Runtime in Compass #27

Creation of CompassManagerMapping CRD after successfull registration Runtime in Compass #27

Conversation

mvshao commented Sep 19, 2023 • edited Loading

akgalwas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akgalwas Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Sep 29, 2023 • edited Loading

CLAassistant commented Sep 29, 2023

mvshao commented Oct 2, 2023

mvshao commented Sep 19, 2023 •

edited

Loading

akgalwas Sep 28, 2023 •

edited

Loading

CLAassistant commented Sep 29, 2023 •

edited

Loading