Fix always fetch Zabbix version before issuing new requests (#2133)

Previously we were only fetching the version when the version was `0`.
This generally worked, but posed some problems when customers were
updating their Zabbix version, specifically when upgrading from a
version < `7.2.x` to `7.2.x` or above.

Before `7.2.x`, an `auth` parameter was still supported when issuing a
zabbix request, this was deprecated in `6.4.x` and later removed in
`7.2.x`. When a user was on a version < `7.2.x` all the outgoing
requests would add this `auth` parameter. When upgrading to `7.2.x` this
was a problem, because the version was not `0`, hence, not requiring
getting the version again, but also because we were still building the
request considering an older version and adding the `auth` parameter,
when this was no longer supported.

This PR removes the check for `version == 0`, though this now means that
every request that goes out will check the version before hand, I think
this will give us a more accurate representation of the version that
needs to be used.

fixes
https://github.com/orgs/grafana/projects/457/views/40?pane=issue&itemId=3683181283&issue=grafana%7Coss-big-tent-squad%7C135

.changeset/free-ideas-eat.md

@@ -0,0 +1,5 @@
+---
+'grafana-zabbix': minor
+---
+
+Fix when to fetch Zabbix version before issuing new requests

pkg/zabbix/methods_test.go

@@ -53,6 +53,9 @@ func TestGetHistory(t *testing.T) {
 			historyCalls = append(historyCalls, int(payload.Params["history"].(float64)))
 			return `{"result":[{"itemid":"1","clock":"1","value":"1.2","ns":"0"}]}`
 		}
+		if payload.Method ==  "apiinfo.version" {
+			return `{"result":"6.4.0"}`
+		}
 		return `{"result":null}`
 	})
 
@@ -80,6 +83,10 @@ func TestGetTrend(t *testing.T) {
 			}
 			return `{"result":[{"itemid":"1","clock":"1","value_min":"0","value_avg":"1","value_max":"2"}]}`
 		}
+
+		if payload.Method ==  "apiinfo.version" {
+			return `{"result":"6.4.0"}`
+		}
 		return `{"result":null}`
 	})
 
@@ -105,6 +112,7 @@ func TestGetItems(t *testing.T) {
 				{"itemid":"200","name":"Memory usage"}
 			]
 		}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	items, err := client.GetItems(context.Background(), "Servers", "web01", "", "/CPU/", "num", false)
@@ -130,6 +138,7 @@ func TestGetItemsBefore54(t *testing.T) {
 				{"itemid":"600","name":"API latency"}
 			]
 		}`,
+		"apiinfo.version": `{"result":"5.0.0"}`,
 	})
 
 	items, err := client.GetItemsBefore54(context.Background(), "Servers", "web01", "Databases", "/DB/", "num", false)
@@ -166,6 +175,7 @@ func TestGetApps(t *testing.T) {
 				{"applicationid":"60","name":"DB"}
 			]
 		}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	apps, err := client.GetApps(context.Background(), "Servers", "web01", "/^API/")
@@ -202,6 +212,7 @@ func TestGetItemTags(t *testing.T) {
 				{"itemid":"2","name":"Mem","tags":[{"tag":"Env","value":"stage"},{"tag":"Env","value":"prod"}]}
 			]
 		}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	tags, err := client.GetItemTags(context.Background(), "Servers", "web01", "/^Env/")
@@ -238,6 +249,7 @@ func TestGetHosts(t *testing.T) {
 				{"hostid":"20","name":"db01"}
 			]
 		}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	hosts, err := client.GetHosts(context.Background(), "Servers", "/web/")
@@ -267,6 +279,7 @@ func TestFilterHostsByQuery(t *testing.T) {
 func TestGetGroups(t *testing.T) {
 	client := NewZabbixClientWithResponses(t, map[string]string{
 		"hostgroup.get": `{"result":[{"groupid":"1","name":"Servers"},{"groupid":"2","name":"Apps"}]}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	groups, err := client.GetGroups(context.Background(), "/Apps/")
@@ -300,6 +313,11 @@ func TestGetAllItemsBuildsParams(t *testing.T) {
 			lastRequest = payload
 			return `{"result":[{"itemid":"1","name":"CPU $1","key_":"system.cpu[user]","value_type":"0","hosts":[{"hostid":"10","name":"web"}]}]}`
 		}
+
+		if payload.Method ==  "apiinfo.version" {
+			return `{"result":"6.4.0"}`
+		}
+
 		return `{"result":null}`
 	})
 
@@ -338,6 +356,11 @@ func TestGetItemsByIDs(t *testing.T) {
 			lastRequest = payload
 			return `{"result":[{"itemid":"1","name":"CPU"}]}`
 		}
+
+		if payload.Method ==  "apiinfo.version" {
+			return `{"result":"6.4.0"}`
+		}
+
 		return `{"result":null}`
 	})
 
@@ -350,6 +373,7 @@ func TestGetItemsByIDs(t *testing.T) {
 func TestGetAllApps(t *testing.T) {
 	client := NewZabbixClientWithResponses(t, map[string]string{
 		"application.get": `{"result":[{"applicationid":"10","name":"API"}]}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	apps, err := client.GetAllApps(context.Background(), []string{"1"})
@@ -361,6 +385,7 @@ func TestGetAllApps(t *testing.T) {
 func TestGetAllHosts(t *testing.T) {
 	client := NewZabbixClientWithResponses(t, map[string]string{
 		"host.get": `{"result":[{"hostid":"10","name":"web01"}]}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	hosts, err := client.GetAllHosts(context.Background(), []string{"1"})
@@ -372,6 +397,7 @@ func TestGetAllHosts(t *testing.T) {
 func TestGetAllGroups(t *testing.T) {
 	client := NewZabbixClientWithResponses(t, map[string]string{
 		"hostgroup.get": `{"result":[{"groupid":"1","name":"Servers"}]}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	groups, err := client.GetAllGroups(context.Background())
@@ -391,6 +417,7 @@ func TestGetValueMappings(t *testing.T) {
 				}
 			]
 		}`,
+		"apiinfo.version": `{"result":"6.4.0"}`,
 	})
 
 	valueMaps, err := client.GetValueMappings(context.Background())

pkg/zabbix/zabbix.go

@@ -48,16 +48,13 @@ func (zabbix *Zabbix) GetAPI() *zabbixapi.ZabbixAPI {
 func (ds *Zabbix) Request(ctx context.Context, apiReq *ZabbixAPIRequest) (*simplejson.Json, error) {
 	var resultJson *simplejson.Json
 	var err error
-
+	
-	if ds.version == 0 {
+	version, err := ds.GetVersion(ctx)
-		version, err := ds.GetVersion(ctx)
+	if err != nil {
-		if err != nil {
+		ds.logger.Error("Error querying Zabbix version", "error", err)
-			ds.logger.Error("Error querying Zabbix version", "error", err)
+		ds.version = -1
-			ds.version = -1
+	} else {
-		} else {
+		ds.version = version
-			ds.logger.Debug("Got Zabbix version", "version", version)
-			ds.version = version
-		}
 	}
 
 	cachedResult, queryExistInCache := ds.cache.GetAPIRequest(apiReq)
@@ -68,7 +65,7 @@ func (ds *Zabbix) Request(ctx context.Context, apiReq *ZabbixAPIRequest) (*simpl
 		}
 
 		if IsCachedRequest(apiReq.Method) {
-			ds.logger.Debug("Writing result to cache", "method", apiReq.Method)
+			ds.logger.Debug("Writing result to cache", "method", apiReq.Method, "version", ds.version)
 			ds.cache.SetAPIRequest(apiReq, resultJson)
 		}
 	} else {
@@ -85,7 +82,7 @@ func (ds *Zabbix) Request(ctx context.Context, apiReq *ZabbixAPIRequest) (*simpl
 
 // request checks authentication and makes a request to the Zabbix API.
 func (zabbix *Zabbix) request(ctx context.Context, method string, params ZabbixAPIParams) (*simplejson.Json, error) {
-	zabbix.logger.Debug("Zabbix request", "method", method)
+	zabbix.logger.Debug("Zabbix request", "method", method, "version", zabbix.version)
 
 	// Skip auth for methods that are not required it
 	if method == "apiinfo.version" {

pkg/zabbix/zabbix_test.go

@@ -83,15 +83,23 @@ func TestNonCachedQuery(t *testing.T) {
 }
 
 func TestItemTagCache(t *testing.T) {
-	zabbixClient, _ := MockZabbixClient(
+	callCount := 0
-		BasicDatasourceInfo,
+	zabbixClient := NewZabbixClientWithHandler(t, func(payload ApiRequestPayload) string {
-		`{"result":[{"itemid":"1","name":"test1"}]}`,
+		switch payload.Method {
-		200,
+		case "apiinfo.version":
-	)
+			return `{"result":"6.4.0"}`
+		case "item.get":
+			callCount++
+			if callCount == 1 {
+				return `{"result":[{"itemid":"1","name":"test1"}]}`
+			}
+			return `{"result":[{"itemid":"2","name":"test2"}]}`
+		default:
+			return `{"result":null}`
+		}
+	})
 	// tag filtering is on >= 54 version
-	zabbixClient.version = 64
 	zabbixClient.settings.AuthType = settings.AuthTypeToken
-	zabbixClient.api.SetAuth("test")
 	items, err := zabbixClient.GetAllItems(
 		context.Background(),
 		nil,