pkg/cvo: Reason granularity for RemoteFailed

We want to be able to distinguish these conditions, which can be due
to internal misconfiguration or external Cincinnati/network errors
[1].  The former can be fixed by cluster admins.  The latter could go
either way.

I dropped the len(upstream) guard from checkForUpdate because there's
already an earlier guard in syncAvailableUpdates.  The guard I'm
removing is from db150e6 (cvo: Perform status updates in a single
thread, 2018-11-03, #45).  The covering guard is from the later
286641d (api: Update to objects from openshift/api, 2018-11-15, #55).

Personally, I'd rather have GetUpdates return an *Error, so we could
dispense with the cast and unused Unknown-reason fallback.  But
Abhinav wanted the explicit cast in return for a more familiar error
type [2].

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1685338
[2]: #268 (comment)

Author: wking

pkg/cincinnati/cincinnati.go

@@ -34,6 +34,23 @@ func NewClient(id uuid.UUID, proxyURL *url.URL, tlsConfig *tls.Config) Client {
 // Update is a single node from the update graph.
 type Update node
 
+// Error is returned when are unable to get updates.
+type Error struct {
+	// Reason is the reason suggested for the ClusterOperator status condition.
+	Reason string
+
+	// Message is the message suggested for the ClusterOperator status condition.
+	Message string
+
+	// cause is the upstream error, if any, being wrapped by this error.
+	cause error
+}
+
+// Error serializes the error as a string, to satisfy the error interface.
+func (err *Error) Error() string {
+	return fmt.Sprintf("%s: %s", err.Reason, err.Message)
+}
+
 // GetUpdates fetches the next-applicable update payloads from the specified
 // upstream Cincinnati stack given the current version and channel. The next-
 // applicable updates are determined by downloading the update graph, finding
@@ -46,7 +63,7 @@ func (c Client) GetUpdates(upstream string, arch string, channel string, version
 	// Prepare parametrized cincinnati query.
 	cincinnatiURL, err := url.Parse(upstream)
 	if err != nil {
-		return nil, fmt.Errorf("failed to parse upstream URL: %s", err)
+		return nil, &Error{Reason: "InvalidURI", Message: fmt.Sprintf("failed to parse upstream URL: %s", err)}
 	}
 	queryParams := cincinnatiURL.Query()
 	queryParams.Add("arch", arch)
@@ -58,7 +75,7 @@ func (c Client) GetUpdates(upstream string, arch string, channel string, version
 	// Download the update graph.
 	req, err := http.NewRequest("GET", cincinnatiURL.String(), nil)
 	if err != nil {
-		return nil, err
+		return nil, &Error{Reason: "InvalidRequest", Message: err.Error(), cause: err}
 	}
 	req.Header.Add("Accept", GraphMediaType)
 	if c.tlsConfig != nil {
@@ -72,23 +89,23 @@ func (c Client) GetUpdates(upstream string, arch string, channel string, version
 	client := http.Client{Transport: &transport}
 	resp, err := client.Do(req)
 	if err != nil {
-		return nil, err
+		return nil, &Error{Reason: "RemoteFailed", Message: err.Error(), cause: err}
 	}
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("unexpected HTTP status: %s", resp.Status)
+		return nil, &Error{Reason: "ResponseFailed", Message: fmt.Sprintf("unexpected HTTP status: %s", resp.Status)}
 	}
 
 	// Parse the graph.
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
-		return nil, err
+		return nil, &Error{Reason: "ResponseFailed", Message: err.Error(), cause: err}
 	}
 
 	var graph graph
 	if err = json.Unmarshal(body, &graph); err != nil {
-		return nil, err
+		return nil, &Error{Reason: "ResponseInvalid", Message: err.Error(), cause: err}
 	}
 
 	// Find the current version within the graph.
@@ -102,7 +119,10 @@ func (c Client) GetUpdates(upstream string, arch string, channel string, version
 		}
 	}
 	if !found {
-		return nil, fmt.Errorf("currently installed version %s not found in the %q channel", version, channel)
+		return nil, &Error{
+			Reason:  "VersionNotFound",
+			Message: fmt.Sprintf("currently installed version %s not found in the %q channel", version, channel),
+		}
 	}
 
 	// Find the children of the current version.

pkg/cincinnati/cincinnati_test.go

@@ -49,7 +49,7 @@ func TestGetUpdates(t *testing.T) {
 		name:          "unknown version",
 		version:       "4.0.0-3",
 		expectedQuery: "arch=test-arch&channel=test-channel&id=01234567-0123-0123-0123-0123456789ab&version=4.0.0-3",
-		err:           "currently installed version 4.0.0-3 not found in the \"test-channel\" channel",
+		err:           "VersionNotFound: currently installed version 4.0.0-3 not found in the \"test-channel\" channel",
 	}}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {

pkg/cvo/availableupdates.go

@@ -127,6 +127,14 @@ func calculateAvailableUpdatesStatus(clusterID string, proxyURL *url.URL, tlsCon
 		}
 	}
 
+	uuid, err := uuid.Parse(string(clusterID))
+	if err != nil {
+		return nil, configv1.ClusterOperatorStatusCondition{
+			Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: "InvalidID",
+			Message: fmt.Sprintf("invalid cluster ID: %s", err),
+		}
+	}
+
 	if len(arch) == 0 {
 		return nil, configv1.ClusterOperatorStatusCondition{
 			Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: "NoArchitecture",
@@ -157,12 +165,19 @@ func calculateAvailableUpdatesStatus(clusterID string, proxyURL *url.URL, tlsCon
 		}
 	}
 
-	updates, err := checkForUpdate(clusterID, proxyURL, tlsConfig, upstream, arch, channel, currentVersion)
+	updates, err := cincinnati.NewClient(uuid, proxyURL, tlsConfig).GetUpdates(upstream, arch, channel, currentVersion)
 	if err != nil {
 		klog.V(2).Infof("Upstream server %s could not return available updates: %v", upstream, err)
+		if updateError, ok := err.(*cincinnati.Error); ok {
+			return nil, configv1.ClusterOperatorStatusCondition{
+				Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: updateError.Reason,
+				Message: fmt.Sprintf("Unable to retrieve available updates: %s", updateError.Message),
+			}
+		}
+		// this should never happen
 		return nil, configv1.ClusterOperatorStatusCondition{
-			Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: "RemoteFailed",
-			Message: fmt.Sprintf("Unable to retrieve available updates: %v", err),
+			Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: "Unknown",
+			Message: fmt.Sprintf("Unable to retrieve available updates: %s", err),
 		}
 	}
 
@@ -182,18 +197,6 @@ func calculateAvailableUpdatesStatus(clusterID string, proxyURL *url.URL, tlsCon
 	}
 }
 
-func checkForUpdate(clusterID string, proxyURL *url.URL, tlsConfig *tls.Config, upstream, arch, channel string, currentVersion semver.Version) ([]cincinnati.Update, error) {
-	uuid, err := uuid.Parse(string(clusterID))
-	if err != nil {
-		return nil, err
-	}
-
-	if len(upstream) == 0 {
-		return nil, fmt.Errorf("no upstream URL set for cluster version")
-	}
-	return cincinnati.NewClient(uuid, proxyURL, tlsConfig).GetUpdates(upstream, arch, channel, currentVersion)
-}
-
 // getHTTPSProxyURL returns a url.URL object for the configured
 // https proxy only. It can be nil if does not exist or there is an error.
 func (optr *Operator) getHTTPSProxyURL() (*url.URL, string, error) {

pkg/cvo/cvo_test.go

@@ -2451,7 +2451,7 @@ func TestOperator_availableUpdatesSync(t *testing.T) {
 				Condition: configv1.ClusterOperatorStatusCondition{
 					Type:    configv1.RetrievedUpdates,
 					Status:  configv1.ConditionFalse,
-					Reason:  "RemoteFailed",
+					Reason:  "ResponseFailed",
 					Message: "Unable to retrieve available updates: unexpected HTTP status: 500 Internal Server Error",
 				},
 			},