store: remove refresh hold data in store requests for older proxies

"held" is supported starting v54 of the store API. Some store proxies might be running older versions of the store which will fail with: "Additional properties are not allowed ('held' was unexpected)". This approach retries bad requests, with the new knowledge of the store version parsed from the response header ""Snap-Store-Version"" and removes the "held" field if store version is less than v54. This approach is generic and can be used for other new fields using minStoreVersion(storeVer, <MIN_VER>). * o/snapstate: make debug message consistent and store version check explicit * o/snapstate: check Snap-Store-Version header is valid for retry logic and fix typo in test name * o/snapstate: extract refresh hold retrieval into a helper function * o/snapstate: move effectiveRefreshHold to autorefresh.go * o/snapstate: bump store api requirement to v55 for held field support	* v55 includes bug fixes related to held field Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com> 

5 files changed, 127 insertions, 32 deletions

diff --git a/overlord/snapstate/autorefresh.go b/overlord/snapstate/autorefresh.go
index b8cdab6e61..1abb549d77 100644
--- a/overlord/snapstate/autorefresh.go
+++ b/overlord/snapstate/autorefresh.go
@@ -163,9 +163,13 @@ func (m *autoRefresh) LastRefresh() (time.Time, error) {
 // EffectiveRefreshHold returns the time until to which refreshes are
 // held if refresh.hold configuration is set.
 func (m *autoRefresh) EffectiveRefreshHold() (time.Time, error) {
+	return effectiveRefreshHold(m.state)
+}
+
+func effectiveRefreshHold(st *state.State) (time.Time, error) {
	var holdValue string
 
-	tr := config.NewTransaction(m.state)
+	tr := config.NewTransaction(st)
	err := tr.Get("core", "refresh.hold", &holdValue)
	if err != nil && !config.IsNoOption(err) {
	return time.Time{}, err
diff --git a/overlord/snapstate/storehelpers.go b/overlord/snapstate/storehelpers.go
index 9d91293130..e825419c23 100644
--- a/overlord/snapstate/storehelpers.go
+++ b/overlord/snapstate/storehelpers.go
@@ -24,13 +24,11 @@ import (
	"errors"
	"fmt"
	"sort"
-	"time"
 
	"github.com/snapcore/snapd/asserts"
	"github.com/snapcore/snapd/asserts/snapasserts"
	"github.com/snapcore/snapd/logger"
	"github.com/snapcore/snapd/overlord/auth"
-	"github.com/snapcore/snapd/overlord/configstate/config"
	"github.com/snapcore/snapd/overlord/state"
	"github.com/snapcore/snapd/snap"
	"github.com/snapcore/snapd/snap/naming"
@@ -803,25 +801,11 @@ func refreshCandidates(ctx context.Context, st *state.State, names []string, rev
 // SnapHolds returns a map of held snaps to lists of holding snaps (including
 // "system" for user holds).
 func SnapHolds(st *state.State, snaps []string) (map[string][]string, error) {
-	var allSnapsHold string
-	tr := config.NewTransaction(st)
-	err := tr.Get("core", "refresh.hold", &allSnapsHold)
-	if err != nil && !config.IsNoOption(err) {
+	allSnapsHoldTime, err := effectiveRefreshHold(st)
+	if err != nil {
	return nil, err
	}
 
-	var allSnapsHoldTime time.Time
-	if allSnapsHold != "" {
-	if allSnapsHold == "forever" {
-	allSnapsHoldTime = timeNow().Add(maxDuration)
-	} else {
-	allSnapsHoldTime, err = time.Parse(time.RFC3339, allSnapsHold)
-	if err != nil {
-	return nil, err
-	}
-	}
-	}
-
	holds, err := HeldSnaps(st, HoldGeneral)
	if err != nil {
	return nil, err
diff --git a/overlord/snapstate/storehelpers_test.go b/overlord/snapstate/storehelpers_test.go
index 33f899ab22..8765a57016 100644
--- a/overlord/snapstate/storehelpers_test.go
+++ b/overlord/snapstate/storehelpers_test.go
@@ -495,15 +495,6 @@ func (s *snapmgrTestSuite) TestSnapHoldsSnapsOnly(c *C) {
	mockInstalledSnap(c, st, snapByaml, false)
	mockInstalledSnap(c, st, snapCyaml, false)
 
-	//mockLastRefreshed(c, st, "2021-05-09T10:00:00Z", "snap-a", "snap-b", "snap-c")
-
-	//now, err := time.Parse(time.RFC3339, "2021-05-10T10:00:00Z")
-	//c.Assert(err, IsNil)
-	//restore := snapstate.MockTimeNow(func() time.Time {
-	//	return now
-	//})
-	//defer restore()
-
	_, err := snapstate.HoldRefresh(st, snapstate.HoldGeneral, "snap-c", 24*time.Hour, "snap-a", "snap-b")
	c.Assert(err, IsNil)
 
diff --git a/store/store_action.go b/store/store_action.go
index 4a5112c6a7..45f0f87684 100644
--- a/store/store_action.go
+++ b/store/store_action.go
@@ -26,6 +26,7 @@ import (
	"encoding/base64"
	"encoding/json"
	"fmt"
+	"strconv"
	"time"
 
	"github.com/snapcore/snapd/asserts"
@@ -240,7 +241,7 @@ func (s *Store) SnapAction(ctx context.Context, currentSnaps []*CurrentSnap, act
 
	authRefreshes := 0
	for {
-	sars, ars, err := s.snapAction(ctx, currentSnaps, actions, assertQuery, toResolve, toResolveSeq, user, opts)
+	sars, ars, err := s.snapAction(ctx, currentSnaps, actions, assertQuery, toResolve, toResolveSeq, user, opts, 0)
 
	if saErr, ok := err.(*SnapActionError); ok && authRefreshes < 2 && len(saErr.Other) > 0 {
	// do we need to try to refresh auths?, 2 tries
@@ -309,7 +310,7 @@ type AssertionResult struct {
	StreamURLs []string
 }
 
-func (s *Store) snapAction(ctx context.Context, currentSnaps []*CurrentSnap, actions []*SnapAction, assertQuery AssertionQuery, toResolve map[asserts.Grouping][]*asserts.AtRevision, toResolveSeq map[asserts.Grouping][]*asserts.AtSequence, user *auth.UserState, opts *RefreshOptions) ([]SnapActionResult, []AssertionResult, error) {
+func (s *Store) snapAction(ctx context.Context, currentSnaps []*CurrentSnap, actions []*SnapAction, assertQuery AssertionQuery, toResolve map[asserts.Grouping][]*asserts.AtRevision, toResolveSeq map[asserts.Grouping][]*asserts.AtSequence, user *auth.UserState, opts *RefreshOptions, storeVer int) ([]SnapActionResult, []AssertionResult, error) {
	requestSalt := ""
	if opts != nil {
	requestSalt = opts.PrivacyKey
@@ -353,8 +354,8 @@ func (s *Store) snapAction(ctx context.Context, currentSnaps []*CurrentSnap, act
	CohortKey: curSnap.CohortKey,
	ValidationSets: valsetKeys,
	}
-
-	if len(curSnap.HeldBy) > 0 {
+	// `held` field was introduced in version 55 https://api.snapcraft.io/docs/
+	if len(curSnap.HeldBy) > 0 && (storeVer <= 0 || storeVer >= 55) {
	curSnapJSONs[i].Held = map[string][]string{"by": curSnap.HeldBy}
	}
	}
@@ -566,6 +567,18 @@ func (s *Store) snapAction(ctx context.Context, currentSnaps []*CurrentSnap, act
	}
 
	if resp.StatusCode != 200 {
+	// some fields might not be supported on proxies with old versions.
+	// we should retry with the snap store version known as we can now
+	// get it from the response header.
+	if resp.StatusCode == 400 && storeVer <= 0 {
+	verstr := resp.Header.Get("Snap-Store-Version")
+	ver, err := strconv.Atoi(verstr)
+	if err != nil || ver <= 0 {
+	logger.Debugf("cannot parse header value of Snap-Store-Version: expected positive int got %q", verstr)
+	} else {
+	return s.snapAction(ctx, currentSnaps, actions, assertQuery, toResolve, toResolveSeq, user, opts, ver)
+	}
+	}
	return nil, nil, respToError(resp, "query the store for updates")
	}
 
diff --git a/store/store_action_test.go b/store/store_action_test.go
index 4520dab8bb..d003cb4da8 100644
--- a/store/store_action_test.go
+++ b/store/store_action_test.go
@@ -2599,6 +2599,109 @@ func (s *storeActionSuite) TestSnapActionRefreshWithHeld(c *C) {
	c.Assert(results[0].Revision, Equals, snap.R(26))
 }
 
+func (s *storeActionSuite) TestSnapActionRefreshWithHeldUnsupportedProxy(c *C) {
+	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	assertRequest(c, r, "POST", snapActionPath)
+	// check device authorization is set, implicitly checking doRequest was used
+	c.Check(r.Header.Get("Snap-Device-Authorization"), Equals, `Macaroon root="device-macaroon"`)
+
+	// `held` field was introduced in version 55 https://api.snapcraft.io/docs/
+	// mock version that doesn't support `held` field (e.g. 52)
+	w.Header().Set("Snap-Store-Version", "52")
+
+	jsonReq, err := ioutil.ReadAll(r.Body)
+	c.Assert(err, IsNil)
+	var req struct {
+	Context []map[string]interface{} `json:"context"`
+	Actions []map[string]interface{} `json:"actions"`
+	}
+
+	err = json.Unmarshal(jsonReq, &req)
+	c.Assert(err, IsNil)
+	c.Assert(req.Context, HasLen, 1)
+	if _, exists := req.Context[0]["held"]; exists {
+	w.WriteHeader(400)
+	io.WriteString(w, `{
+ "error-list":[{
+ "code":"api-error",
+ "message":"Additional properties are not allowed ('held' was unexpected) at /context/0"
+ }]
+}`)
+	return
+	}
+	// snap action should retry without the `held` field
+	c.Assert(req.Context[0], DeepEquals, map[string]interface{}{
+	"snap-id": helloWorldSnapID,
+	"instance-key": helloWorldSnapID,
+	"revision": float64(1),
+	"tracking-channel": "stable",
+	"refreshed-date": helloRefreshedDateStr,
+	"epoch": iZeroEpoch,
+	})
+	c.Assert(req.Actions, HasLen, 1)
+	c.Assert(req.Actions[0], DeepEquals, map[string]interface{}{
+	"action": "refresh",
+	"instance-key": helloWorldSnapID,
+	"snap-id": helloWorldSnapID,
+	"channel": "stable",
+	})
+
+	io.WriteString(w, `{
+ "results": [{
+ "result": "refresh",
+ "instance-key": "buPKUD3TKqCOgLEjjHx5kSiCpIs5cMuQ",
+ "snap-id": "buPKUD3TKqCOgLEjjHx5kSiCpIs5cMuQ",
+ "name": "hello-world",
+ "snap": {
+ "snap-id": "buPKUD3TKqCOgLEjjHx5kSiCpIs5cMuQ",
+ "name": "hello-world",
+ "revision": 26,
+ "version": "6.1",
+ "publisher": {
+ "id": "canonical",
+ "username": "canonical",
+ "display-name": "Canonical"
+ }
+ }
+ }]
+}`)
+	}))
+
+	c.Assert(mockServer, NotNil)
+	defer mockServer.Close()
+
+	mockServerURL, _ := url.Parse(mockServer.URL)
+	cfg := store.Config{
+	StoreBaseURL: mockServerURL,
+	}
+	dauthCtx := &testDauthContext{c: c, device: s.device}
+	sto := store.New(&cfg, dauthCtx)
+
+	results, _, err := sto.SnapAction(s.ctx, []*store.CurrentSnap{
+	{
+	InstanceName: "hello-world",
+	SnapID: helloWorldSnapID,
+	TrackingChannel: "stable",
+	Revision: snap.R(1),
+	RefreshedDate: helloRefreshedDate,
+	HeldBy: []string{"foo", "bar"},
+	},
+	}, []*store.SnapAction{
+	{
+	Action: "refresh",
+	SnapID: helloWorldSnapID,
+	Channel: "stable",
+	InstanceName: "hello-world",
+	},
+	}, nil, nil, &store.RefreshOptions{PrivacyKey: "123"})
+
+	c.Assert(err, IsNil)
+	c.Assert(results, HasLen, 1)
+	c.Assert(results[0].SnapName(), Equals, "hello-world")
+	c.Assert(results[0].InstanceName(), Equals, "hello-world")
+	c.Assert(results[0].Revision, Equals, snap.R(26))
+}
+
 func (s *storeActionSuite) TestSnapActionRefreshWithValidationSets(c *C) {
	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	assertRequest(c, r, "POST", snapActionPath)