From e0733829837b61bf31c6f6d282f30ec858d40a34 Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Fri, 5 Dec 2025 17:34:20 -0800 Subject: [PATCH] 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 | 5 +++++ pkg/zabbix/methods_test.go | 27 +++++++++++++++++++++++++++ pkg/zabbix/zabbix.go | 21 +++++++++------------ pkg/zabbix/zabbix_test.go | 22 +++++++++++++++------- 4 files changed, 56 insertions(+), 19 deletions(-) create mode 100644 .changeset/free-ideas-eat.md diff --git a/.changeset/free-ideas-eat.md b/.changeset/free-ideas-eat.md new file mode 100644 index 0000000..669c282 --- /dev/null +++ b/.changeset/free-ideas-eat.md @@ -0,0 +1,5 @@ +--- +'grafana-zabbix': minor +--- + +Fix when to fetch Zabbix version before issuing new requests diff --git a/pkg/zabbix/methods_test.go b/pkg/zabbix/methods_test.go index b7236a4..139bc9a 100644 --- a/pkg/zabbix/methods_test.go +++ b/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()) diff --git a/pkg/zabbix/zabbix.go b/pkg/zabbix/zabbix.go index dcb291e..506e6b3 100644 --- a/pkg/zabbix/zabbix.go +++ b/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) - if err != nil { - ds.logger.Error("Error querying Zabbix version", "error", err) - ds.version = -1 - } else { - ds.logger.Debug("Got Zabbix version", "version", version) - ds.version = version - } + + version, err := ds.GetVersion(ctx) + if err != nil { + ds.logger.Error("Error querying Zabbix version", "error", err) + ds.version = -1 + } else { + 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" { diff --git a/pkg/zabbix/zabbix_test.go b/pkg/zabbix/zabbix_test.go index 2091d0e..261722c 100644 --- a/pkg/zabbix/zabbix_test.go +++ b/pkg/zabbix/zabbix_test.go @@ -83,15 +83,23 @@ func TestNonCachedQuery(t *testing.T) { } func TestItemTagCache(t *testing.T) { - zabbixClient, _ := MockZabbixClient( - BasicDatasourceInfo, - `{"result":[{"itemid":"1","name":"test1"}]}`, - 200, - ) + callCount := 0 + zabbixClient := NewZabbixClientWithHandler(t, func(payload ApiRequestPayload) string { + switch payload.Method { + 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,