From 020ab587678821294deeb924e218075cd18ccbff Mon Sep 17 00:00:00 2001 From: Alessandro Costa Date: Wed, 3 May 2023 14:23:19 +0200 Subject: [PATCH 1/4] Add ocmClusterID parameter to package operator reconciler --- integration/pko_test.go | 8 +++++++- .../controllers/addon/addon_reconciler_options.go | 7 ++++--- .../controllers/addon/package_operator_reconciler.go | 11 +++++++---- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/integration/pko_test.go b/integration/pko_test.go index 01f31600d..e469fbdae 100644 --- a/integration/pko_test.go +++ b/integration/pko_test.go @@ -30,6 +30,7 @@ const ( addonName = "pko-test" addonNamespace = "pko-test-ns" deadMansSnitchUrlValue = "https://example.com/test-snitch-url" + ocmClusterIDValue = "foobar" pagerDutyKeyValue = "1234567890ABCDEF" // source: https://github.com/kostola/package-operator-packages/tree/v2.0/openshift/addon-operator/apnp-test-optional-params @@ -355,6 +356,9 @@ func clusterPackageChecker( clusterIDValueOk = err == nil } + ocmClusterID, present := addonsv1[addon.OcmClusterIDConfigKey] + ocmClusterIDValueOk := present && ocmClusterID == ocmClusterIDValue + addonParametersValueOk, deadMansSnitchUrlValueOk, pagerDutyValueOk := false, false, false if addonParametersValuePresent { value, present := addonsv1[addon.ParametersConfigKey] @@ -383,15 +387,17 @@ func clusterPackageChecker( pagerDutyValueOk = !present } - logger.Info(fmt.Sprintf("targetNamespace=%t, clusterID=%t, addonParameters=%t, deadMansSnitchUrl=%t, pagerDutyKey=%t", + logger.Info(fmt.Sprintf("targetNamespace=%t, clusterID=%t, ocmClusterID=%t, addonParameters=%t, deadMansSnitchUrl=%t, pagerDutyKey=%t", targetNamespaceValueOk, clusterIDValueOk, + ocmClusterIDValueOk, addonParametersValueOk, deadMansSnitchUrlValueOk, pagerDutyValueOk)) result := targetNamespaceValueOk && clusterIDValueOk && + ocmClusterIDValueOk && addonParametersValueOk && deadMansSnitchUrlValueOk && pagerDutyValueOk diff --git a/internal/controllers/addon/addon_reconciler_options.go b/internal/controllers/addon/addon_reconciler_options.go index 4b1166843..9cc52c6ab 100644 --- a/internal/controllers/addon/addon_reconciler_options.go +++ b/internal/controllers/addon/addon_reconciler_options.go @@ -37,9 +37,10 @@ type WithPackageOperatorReconciler struct { func (w WithPackageOperatorReconciler) ApplyToAddonReconciler(config *AddonReconciler) { poReconciler := &PackageOperatorReconciler{ - Client: w.Client, - Scheme: w.Scheme, - ClusterID: config.ClusterExternalID, + Client: w.Client, + Scheme: w.Scheme, + ClusterID: config.ClusterExternalID, + OcmClusterID: "foobar", // TODO: set real OCM cluster ID } config.subReconcilers = append(config.subReconcilers, poReconciler) } diff --git a/internal/controllers/addon/package_operator_reconciler.go b/internal/controllers/addon/package_operator_reconciler.go index d3cfe3e2e..ab04d65ea 100644 --- a/internal/controllers/addon/package_operator_reconciler.go +++ b/internal/controllers/addon/package_operator_reconciler.go @@ -30,7 +30,7 @@ spec: merge (.config | b64decMap) (hasKey .config "%[4]s" | ternary (dict "%[4]s" (index .config "%[4]s" | b64decMap)) (dict)) - (dict "%[5]s" "%[6]s" "%[7]s" "%[8]s") + (dict "%[5]s" "%[6]s" "%[7]s" "%[8]s" "%[9]s" "%[10]s") )}} ` @@ -38,15 +38,17 @@ const ( packageOperatorName = "packageOperatorReconciler" ClusterIDConfigKey = "clusterID" DeadMansSnitchUrlConfigKey = "deadMansSnitchUrl" + OcmClusterIDConfigKey = "ocmClusterID" PagerDutyKeyConfigKey = "pagerDutyKey" ParametersConfigKey = "parameters" TargetNamespaceConfigKey = "targetNamespace" ) type PackageOperatorReconciler struct { - Client client.Client - Scheme *runtime.Scheme - ClusterID string + Client client.Client + Scheme *runtime.Scheme + ClusterID string + OcmClusterID string } func (r *PackageOperatorReconciler) Name() string { return packageOperatorName } @@ -71,6 +73,7 @@ func (r *PackageOperatorReconciler) reconcileClusterObjectTemplate(ctx context.C addon.Spec.AddonPackageOperator.Image, ParametersConfigKey, ClusterIDConfigKey, r.ClusterID, + OcmClusterIDConfigKey, r.OcmClusterID, TargetNamespaceConfigKey, addonDestNamespace, ) From 676ff0315037ae3e00917a0c7b4785bbdde8df2a Mon Sep 17 00:00:00 2001 From: Alessandro Costa Date: Tue, 2 May 2023 16:43:05 +0200 Subject: [PATCH 2/4] Use real ocmClusterID --- integration/pko_test.go | 11 +++++------ .../controllers/addon/addon_reconciler_options.go | 2 +- internal/controllers/addon/controller.go | 15 ++++++++++++--- .../addon/package_operator_reconciler.go | 6 ++++-- internal/ocm/cluster.go | 4 ++++ internal/ocm/ocmtest/mock.go | 6 ++++++ 6 files changed, 32 insertions(+), 12 deletions(-) diff --git a/integration/pko_test.go b/integration/pko_test.go index e469fbdae..f341888f5 100644 --- a/integration/pko_test.go +++ b/integration/pko_test.go @@ -30,13 +30,12 @@ const ( addonName = "pko-test" addonNamespace = "pko-test-ns" deadMansSnitchUrlValue = "https://example.com/test-snitch-url" - ocmClusterIDValue = "foobar" pagerDutyKeyValue = "1234567890ABCDEF" - // source: https://github.com/kostola/package-operator-packages/tree/v2.0/openshift/addon-operator/apnp-test-optional-params - pkoImageOptionalParams = "quay.io/alcosta/package-operator-packages/openshift/addon-operator/apnp-test-optional-params:v2.0" - // source: https://github.com/kostola/package-operator-packages/tree/v2.0/openshift/addon-operator/apnp-test-required-params - pkoImageRequiredParams = "quay.io/alcosta/package-operator-packages/openshift/addon-operator/apnp-test-required-params:v2.0" + // source: https://github.com/kostola/package-operator-packages/tree/v3.0/openshift/addon-operator/apnp-test-optional-params + pkoImageOptionalParams = "quay.io/alcosta/package-operator-packages/openshift/addon-operator/apnp-test-optional-params:v3.0" + // source: https://github.com/kostola/package-operator-packages/tree/v3.0/openshift/addon-operator/apnp-test-required-params + pkoImageRequiredParams = "quay.io/alcosta/package-operator-packages/openshift/addon-operator/apnp-test-required-params:v3.0" ) func (s *integrationTestSuite) TestPackageOperatorReconcilerStatusPropagatedToAddon() { @@ -357,7 +356,7 @@ func clusterPackageChecker( } ocmClusterID, present := addonsv1[addon.OcmClusterIDConfigKey] - ocmClusterIDValueOk := present && ocmClusterID == ocmClusterIDValue + ocmClusterIDValueOk := present && len(fmt.Sprintf("%v", ocmClusterID)) > 0 addonParametersValueOk, deadMansSnitchUrlValueOk, pagerDutyValueOk := false, false, false if addonParametersValuePresent { diff --git a/internal/controllers/addon/addon_reconciler_options.go b/internal/controllers/addon/addon_reconciler_options.go index 9cc52c6ab..72c880e1b 100644 --- a/internal/controllers/addon/addon_reconciler_options.go +++ b/internal/controllers/addon/addon_reconciler_options.go @@ -40,7 +40,7 @@ func (w WithPackageOperatorReconciler) ApplyToAddonReconciler(config *AddonRecon Client: w.Client, Scheme: w.Scheme, ClusterID: config.ClusterExternalID, - OcmClusterID: "foobar", // TODO: set real OCM cluster ID + OcmClusterID: config.GetOCMClusterID, } config.subReconcilers = append(config.subReconcilers, poReconciler) } diff --git a/internal/controllers/addon/controller.go b/internal/controllers/addon/controller.go index e4d1bf9e5..63b3c9a31 100644 --- a/internal/controllers/addon/controller.go +++ b/internal/controllers/addon/controller.go @@ -129,6 +129,7 @@ func NewAddonReconciler( } type ocmClient interface { + GetClusterID() string GetCluster( ctx context.Context, req ocm.ClusterGetRequest, @@ -173,6 +174,13 @@ func (r *AddonReconciler) InjectOCMClient(ctx context.Context, c *ocm.Client) er return nil } +func (r *AddonReconciler) GetOCMClusterID() string { + if r.ocmClient == nil { + return "" + } + return r.ocmClient.GetClusterID() +} + // Pauses reconcilation of all Addon objects. Concurrency safe. func (r *AddonReconciler) EnableGlobalPause(ctx context.Context) error { return r.setGlobalPause(ctx, true) @@ -248,8 +256,8 @@ func (r *AddonReconciler) SetupWithManager(mgr ctrl.Manager, opts ...AddonReconc // AddonReconciler/Controller entrypoint func (r *AddonReconciler) Reconcile( - ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - + ctx context.Context, req ctrl.Request, +) (ctrl.Result, error) { logger := r.Log.WithValues("addon", req.NamespacedName.String()) ctx = controllers.ContextWithLogger(ctx, logger) @@ -298,7 +306,8 @@ func (r *AddonReconciler) syncWithExternalAPIs(ctx context.Context, logger logr. } func (r *AddonReconciler) reconcile(ctx context.Context, addon *addonsv1alpha1.Addon, - log logr.Logger) (ctrl.Result, error) { + log logr.Logger, +) (ctrl.Result, error) { ctx = controllers.ContextWithLogger(ctx, log) // Handle addon deletion before checking for pause condition. // This allows even paused addons to be deleted. diff --git a/internal/controllers/addon/package_operator_reconciler.go b/internal/controllers/addon/package_operator_reconciler.go index ab04d65ea..f10ed3d86 100644 --- a/internal/controllers/addon/package_operator_reconciler.go +++ b/internal/controllers/addon/package_operator_reconciler.go @@ -44,11 +44,13 @@ const ( TargetNamespaceConfigKey = "targetNamespace" ) +type OcmClusterIDGetter func() string + type PackageOperatorReconciler struct { Client client.Client Scheme *runtime.Scheme ClusterID string - OcmClusterID string + OcmClusterID OcmClusterIDGetter } func (r *PackageOperatorReconciler) Name() string { return packageOperatorName } @@ -73,7 +75,7 @@ func (r *PackageOperatorReconciler) reconcileClusterObjectTemplate(ctx context.C addon.Spec.AddonPackageOperator.Image, ParametersConfigKey, ClusterIDConfigKey, r.ClusterID, - OcmClusterIDConfigKey, r.OcmClusterID, + OcmClusterIDConfigKey, r.OcmClusterID(), TargetNamespaceConfigKey, addonDestNamespace, ) diff --git a/internal/ocm/cluster.go b/internal/ocm/cluster.go index e15db0398..9d57e0719 100644 --- a/internal/ocm/cluster.go +++ b/internal/ocm/cluster.go @@ -18,6 +18,10 @@ type Cluster struct { ExternalId string `json:"external_id"` } +func (c *Client) GetClusterID() string { + return c.opts.ClusterID +} + func (c *Client) GetCluster( ctx context.Context, req ClusterGetRequest, diff --git a/internal/ocm/ocmtest/mock.go b/internal/ocm/ocmtest/mock.go index 69515836e..a519fc213 100644 --- a/internal/ocm/ocmtest/mock.go +++ b/internal/ocm/ocmtest/mock.go @@ -8,6 +8,8 @@ import ( "github.com/openshift/addon-operator/internal/ocm" ) +const MockClusterID = "foobar" + type Client struct { mock.Mock } @@ -43,6 +45,10 @@ func (c *Client) GetCluster( args.Error(1) } +func (c *Client) GetClusterID() string { + return MockClusterID +} + func (c *Client) PostAddOnStatus( ctx context.Context, req ocm.AddOnStatusPostRequest, From 22acb89295caab475957275fff0f15d6f66fdfe7 Mon Sep 17 00:00:00 2001 From: Alessandro Costa Date: Wed, 3 May 2023 15:38:57 +0200 Subject: [PATCH 3/4] Fix TestPackageOperatorReconcilerLogic --- .../addon/package_operator_reconciler_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/controllers/addon/package_operator_reconciler_test.go b/internal/controllers/addon/package_operator_reconciler_test.go index be9a882b8..5f266fcd4 100644 --- a/internal/controllers/addon/package_operator_reconciler_test.go +++ b/internal/controllers/addon/package_operator_reconciler_test.go @@ -142,7 +142,14 @@ func TestPackageOperatorReconcilerLogic(t *testing.T) { ctx := context.Background() subTest.configureClient(ctx, c, identifier) - r := &addon.PackageOperatorReconciler{Client: c, Scheme: testutil.NewTestSchemeWithAddonsv1alpha1()} + r := &addon.PackageOperatorReconciler{ + Client: c, + Scheme: testutil.NewTestSchemeWithAddonsv1alpha1(), + ClusterID: "test-cluster-id", + OcmClusterID: func() string { + return "test-ocm-cluster-id" + }, + } res, err := r.Reconcile(ctx, test.addon) From 48f66b720b200a3b215db0bc50dfc0becdf8f880 Mon Sep 17 00:00:00 2001 From: Alessandro Costa Date: Wed, 3 May 2023 19:50:42 +0200 Subject: [PATCH 4/4] Use ocmClientMux in GetOCMClusterID --- internal/controllers/addon/controller.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/controllers/addon/controller.go b/internal/controllers/addon/controller.go index 63b3c9a31..b3551256d 100644 --- a/internal/controllers/addon/controller.go +++ b/internal/controllers/addon/controller.go @@ -175,6 +175,9 @@ func (r *AddonReconciler) InjectOCMClient(ctx context.Context, c *ocm.Client) er } func (r *AddonReconciler) GetOCMClusterID() string { + r.ocmClientMux.RLock() + defer r.ocmClientMux.RUnlock() + if r.ocmClient == nil { return "" }