Merge pull request #261 from apahim/avoid_policy_crash

[MTSRE-773] Avoid crash on missing version

apis/addons/v1alpha1/addons_types.go

@@ -72,7 +72,8 @@ type AddonUpgradePolicyStatus struct {
 	// Upgrade policy value.
 	Value AddonUpgradePolicyValue `json:"value"`
 	// Upgrade Policy Version.
-	Version string `json:"version"`
+	// +optional
+	Version string `json:"version,omitempty"`
 	// The most recent generation a status update was based on.
 	ObservedGeneration int64 `json:"observedGeneration"`
 }

config/deploy/addons.managed.openshift.io_addons.yaml

@@ -446,7 +446,6 @@ spec:
                 - id
                 - observedGeneration
                 - value
-                - version
                 type: object
             type: object
         type: object

config/example/addon-operator.yaml

@@ -6,5 +6,5 @@ spec:
   ocm:
     endpoint: http://api-mock.api-mock.svc.cluster.local
     secret:
-      name: api-mock
+      name: pull-secret
       namespace: api-mock

config/example/reference-addon.yaml

@@ -16,6 +16,7 @@ spec:
       additionalCatalogSources:
       - name: test-1
         image: quay.io/osd-addons/reference-addon-index@sha256:58cb1c4478a150dc44e6c179d709726516d84db46e4e130a5227d8b76456b5bd
+  version: 1.0.0
   upgradePolicy:
     id: 123-456-789
   monitoring:

config/openshift/manifests/addons.crd.yaml

@@ -444,7 +444,6 @@ spec:
                 - id
                 - observedGeneration
                 - value
-                - version
                 type: object
             type: object
         type: object

docs/api-reference/_index.md

@@ -304,7 +304,7 @@ Tracks the last state last reported to the Upgrade Policy endpoint.
 | ----- | ----------- | ------ | -------- |
 | id | Upgrade policy id. | string | true |
 | value | Upgrade policy value. | AddonUpgradePolicyValue.addons.managed.openshift.io/v1alpha1 | true |
-| version | Upgrade Policy Version. | string | true |
+| version | Upgrade Policy Version. | string | false |
 | observedGeneration | The most recent generation a status update was based on. | int64 | true |
 
 [Back to Group]()

integration/fixtures_test.go

@@ -60,6 +60,7 @@ func addon_OwnNamespace_UpgradePolicyReporting() *addonsv1alpha1.Addon {
 					},
 				},
 			},
+			Version: "1.0.0",
 			UpgradePolicy: &addonsv1alpha1.AddonUpgradePolicy{
 				ID: "123-456-789",
 			},

internal/controllers/addon/controller.go

@@ -126,6 +126,10 @@ type ocmClient interface {
 		ctx context.Context,
 		req ocm.UpgradePolicyPatchRequest,
 	) (res ocm.UpgradePolicyPatchResponse, err error)
+	GetUpgradePolicy(
+		ctx context.Context,
+		req ocm.UpgradePolicyGetRequest,
+	) (res ocm.UpgradePolicyGetResponse, err error)
 }
 
 func (r *AddonReconciler) InjectOCMClient(ctx context.Context, c *ocm.Client) error {

internal/controllers/addon/upgradepolicy_status.go

@@ -4,12 +4,13 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/openshift/addon-operator/internal/ocm"
+
 	"github.com/prometheus/client_golang/prometheus"
 
 	"github.com/go-logr/logr"
 
 	addonsv1alpha1 "github.com/openshift/addon-operator/apis/addons/v1alpha1"
-	"github.com/openshift/addon-operator/internal/ocm"
 )
 
 func (r *AddonReconciler) handleUpgradePolicyStatusReporting(
@@ -31,12 +32,49 @@ func (r *AddonReconciler) handleUpgradePolicyStatusReporting(
 		return nil
 	}
 
-	if addon.Spec.UpgradePolicy == nil || addon.UpgradeCompleteForCurrentVersion() {
+	if addon.Spec.Version == "" {
+		return nil
+	}
+
+	if addon.Spec.UpgradePolicy == nil {
 		return nil
 	}
-	if addon.Status.UpgradePolicy == nil || addon.Status.UpgradePolicy.Version != addon.Spec.Version {
+
+	if addon.UpgradeCompleteForCurrentVersion() {
+		return nil
+	}
+
+	if addon.Status.UpgradePolicy == nil {
+		return r.reportUpgradeStarted(ctx, addon)
+	}
+
+	var (
+		policyID = addon.Spec.UpgradePolicy.ID
+	)
+
+	if addon.Status.UpgradePolicy.Version == "" {
+		req := ocm.UpgradePolicyGetRequest{
+			ID: addon.Spec.UpgradePolicy.ID,
+		}
+		response, err := r.handleGetUpgradePolicyState(ctx, req)
+		if err != nil {
+			return fmt.Errorf(
+				"getting UpgradePolicy %q state: %w", policyID, err,
+			)
+		}
+		if response.Value == ocm.UpgradePolicyValueCompleted {
+			// When the upgrade policy is "completed" in the OCM API but we don't have
+			// a version in our status, we just have to populate the current version to our
+			// status as the "version" was just recently introduced
+			addon.SetUpgradePolicyStatus(addonsv1alpha1.AddonUpgradePolicyValueCompleted)
+			return nil
+		}
+	}
+
+	if addon.Status.UpgradePolicy.Version != addon.Spec.Version {
 		return r.reportUpgradeStarted(ctx, addon)
 	}
+
 	if addon.IsAvailable() {
 		return r.reportUpgradeCompleted(ctx, addon)
 	}
@@ -107,3 +145,17 @@ func (r *AddonReconciler) handlePatchUpgradePolicy(ctx context.Context,
 	_, err := r.ocmClient.PatchUpgradePolicy(ctx, req)
 	return err
 }
+
+func (r *AddonReconciler) handleGetUpgradePolicyState(ctx context.Context,
+	req ocm.UpgradePolicyGetRequest) (ocm.UpgradePolicyGetResponse, error) {
+	if r.Recorder != nil {
+		// TODO: do not count metrics when API returns 5XX response
+		timer := prometheus.NewTimer(prometheus.ObserverFunc(func(v float64) {
+			us := v * 1000000 // convert to microseconds
+			r.Recorder.RecordOCMAPIRequests(us)
+		}))
+		defer timer.ObserveDuration()
+	}
+	response, err := r.ocmClient.GetUpgradePolicy(ctx, req)
+	return response, err
+}

internal/controllers/addon/upgradepolicy_status_test.go

@@ -86,12 +86,15 @@ func TestAddonReconciler_handleUpgradePolicyStatusReporting(t *testing.T) {
 			Recorder:  recorder,
 		}
 
+		var Version = "1.0.0"
+
 		log := testutil.NewLogger(t)
 		addon := &addonsv1alpha1.Addon{
 			ObjectMeta: metav1.ObjectMeta{
 				Generation: 100,
 			},
 			Spec: addonsv1alpha1.AddonSpec{
+				Version: Version,
 				UpgradePolicy: &addonsv1alpha1.AddonUpgradePolicy{
 					ID: "1234",
 				},
@@ -102,7 +105,7 @@ func TestAddonReconciler_handleUpgradePolicyStatusReporting(t *testing.T) {
 			On("PatchUpgradePolicy", mock.Anything, ocm.UpgradePolicyPatchRequest{
 				ID:          "1234",
 				Value:       ocm.UpgradePolicyValueStarted,
-				Description: `Upgrading addon to version "".`,
+				Description: `Upgrading addon to version "1.0.0".`,
 			}).
 			Return(
 				ocm.UpgradePolicyPatchResponse{},
@@ -138,19 +141,23 @@ func TestAddonReconciler_handleUpgradePolicyStatusReporting(t *testing.T) {
 		}
 		log := testutil.NewLogger(t)
 
+		var Version = "1.0.0"
+
 		err := r.handleUpgradePolicyStatusReporting(
 			context.Background(),
 			log,
 			&addonsv1alpha1.Addon{
 				Spec: addonsv1alpha1.AddonSpec{
+					Version: Version,
 					UpgradePolicy: &addonsv1alpha1.AddonUpgradePolicy{
 						ID: "1234",
 					},
 				},
 				Status: addonsv1alpha1.AddonStatus{
 					UpgradePolicy: &addonsv1alpha1.AddonUpgradePolicyStatus{
-						ID:    "1234",
-						Value: addonsv1alpha1.AddonUpgradePolicyValueStarted,
+						ID:      "1234",
+						Version: Version,
+						Value:   addonsv1alpha1.AddonUpgradePolicyValueStarted,
 					},
 				},
 			},
@@ -166,6 +173,8 @@ func TestAddonReconciler_handleUpgradePolicyStatusReporting(t *testing.T) {
 		mockSummary := testutil.NewSummaryMock()
 		recorder.InjectOCMAPIRequestDuration(mockSummary)
 
+		var Version = "1.0.0"
+
 		r := &AddonReconciler{
 			Client:    client,
 			ocmClient: ocmClient,
@@ -177,6 +186,7 @@ func TestAddonReconciler_handleUpgradePolicyStatusReporting(t *testing.T) {
 				Generation: 100,
 			},
 			Spec: addonsv1alpha1.AddonSpec{
+				Version: Version,
 				UpgradePolicy: &addonsv1alpha1.AddonUpgradePolicy{
 					ID: "1234",
 				},
@@ -189,8 +199,9 @@ func TestAddonReconciler_handleUpgradePolicyStatusReporting(t *testing.T) {
 					},
 				},
 				UpgradePolicy: &addonsv1alpha1.AddonUpgradePolicyStatus{
-					ID:    "1234",
-					Value: addonsv1alpha1.AddonUpgradePolicyValueStarted,
+					ID:      "1234",
+					Version: Version,
+					Value:   addonsv1alpha1.AddonUpgradePolicyValueStarted,
 				},
 			},
 		}
@@ -199,7 +210,7 @@ func TestAddonReconciler_handleUpgradePolicyStatusReporting(t *testing.T) {
 			On("PatchUpgradePolicy", mock.Anything, ocm.UpgradePolicyPatchRequest{
 				ID:          "1234",
 				Value:       ocm.UpgradePolicyValueCompleted,
-				Description: `Addon was healthy at least once at version "".`,
+				Description: `Addon was healthy at least once at version "1.0.0".`,
 			}).
 			Return(
 				ocm.UpgradePolicyPatchResponse{},

internal/ocm/ocmtest/mock.go

@@ -25,6 +25,15 @@ func (c *Client) PatchUpgradePolicy(
 		args.Error(1)
 }
 
+func (c *Client) GetUpgradePolicy(
+	ctx context.Context,
+	req ocm.UpgradePolicyGetRequest,
+) (ocm.UpgradePolicyGetResponse, error) {
+	args := c.Called(ctx, req)
+	return args.Get(0).(ocm.UpgradePolicyGetResponse),
+		args.Error(1)
+}
+
 func (c *Client) GetCluster(
 	ctx context.Context,
 	req ocm.ClusterGetRequest,