home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
72.17k stars 30.2k forks source link

Zabbix integration unable to login. #86879

Closed incama closed 2 weeks ago

incama commented 1 year ago

The problem

I ran the zabbix integration for over a year now, and since a few months I get the error "Unable to login to the zabbix api". I ran Zabbix 6.4 beta 3 with succes, but since beta 4 the message appears so I guess they made changes to the api.

What version of Home Assistant Core has the issue?

2023.1.7

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Zabbix

Link to integration documentation on our website

https://www.home-assistant.io/integrations/zabbix/

Diagnostics information

No response

Example YAML snippet

zabbix:
  host: xxx.xxx.xxx.xxx
  path: /
  ssl: false
  username: Admin
  password: ***
  publish_states_host: hostname-of-has
  exclude:
    domains:
      - device_tracker
    entities:
      - sun.sun
      - sensor.time

Anything in the logs that might be useful for us?

2023-01-29 07:22:17.741 ERROR (SyncWorker_5) [homeassistant.components.zabbix] Unable to login to the Zabbix API: {'code': -32602, 'message': 'Invalid params.', 'data': 'Invalid parameter "/": unexpected parameter "user".', 'json': "{'jsonrpc': '2.0', 'method': 'user.login', 'params': {'user': 'Admin', 'password': '********'}, 'id': '1'}"}
2023-01-29 07:22:17.830 ERROR (MainThread) [homeassistant.setup] Setup failed for zabbix: Integration failed to initialize.

Additional information

No response

dsteinkopf commented 1 year ago

Anyone got this working on Home Assistant OS? I have tried the above instructions, but on Home Assistant OS I am unable to find the api.py flle (/usr/local/lib/python3.10/site-packages/pyzabbix/api.py)

The file to be patched is now /usr/local/lib/python3.11/site-packages/pyzabbix/api.py

RdantasS commented 1 year ago

Why don't we use a parameter in config file? Like this: zbx_version: x.xx

vladimir-benc commented 11 months ago

I get it working against Zabbix 6.4 by patching the file pyzabbix/api.py

However the simplest fix would be to change underlaying library from the current (unmaintained) https://github.com/adubkov/py-zabbix

to newer (maintained) https://github.com/lukecyca/pyzabbix/

This replacement will fix the current issue and in addition will also allow to use the Apikey authentication instead.

vladimir-benc commented 11 months ago

Why don't we use a parameter in config file? Like this: zbx_version: x.xx

The version of Zabbix server is supported and available prior to any authentication calls: https://www.zabbix.com/documentation/current/en/manual/api/reference/apiinfo/version

and is also properly handled in the newer pyzabix library implementation: https://github.com/lukecyca/pyzabbix

igorenot commented 9 months ago

Any progress here?

Wonderer643 commented 9 months ago

Unfortunately it is not possible to use https://github.com/adubkov/py-zabbix instead of https://github.com/lukecyca/pyzabbix as py-zabbix also implements the zabbix_sender API. Actually this Zabbix integration is using two independent Zabbix APIs:

But luckily I have found https://pypi.org/project/zabbix/ which is replacing py-zabbix and it is supporting new authentication methods required by new Zabbix versions (api_token and username for 6.4+).

Anyway some changes required in the homeassistant/components/zabbix/ to make it working.

  1. manifest.json.patch
    --- component-zabbix-original/manifest.json     2023-12-27 14:11:04.000000000 +0400
    +++ component-zabbix-fixed/manifest.json        2024-01-03 16:57:06.000000000 +0400
    @@ -5,5 +5,5 @@
    "documentation": "https://www.home-assistant.io/integrations/zabbix",
    "iot_class": "local_polling",
    "loggers": ["pyzabbix"],
    -  "requirements": ["py-zabbix==1.1.7"]
    +  "requirements": ["zabbix==2.0.2"]
    }
  2. __init__.py.patch

    
    --- component-zabbix-original/__init__.py       2023-12-27 14:11:04.000000000 +0400
    +++ component-zabbix-fixed/__init__.py  2024-01-03 21:35:18.265358859 +0400
    @@ -14,6 +14,8 @@
    
    from homeassistant.const import (
     CONF_HOST,
    +    CONF_URL,
    +    CONF_TOKEN,
     CONF_PASSWORD,
     CONF_PATH,
     CONF_SSL,
    @@ -53,8 +55,10 @@
     {
         DOMAIN: INCLUDE_EXCLUDE_BASE_FILTER_SCHEMA.extend(
             {
    +                vol.Required(CONF_URL): cv.string,
                 vol.Required(CONF_HOST): cv.string,
                 vol.Optional(CONF_PASSWORD): cv.string,
    +                vol.Optional(CONF_TOKEN): cv.string,
                 vol.Optional(CONF_PATH, default=DEFAULT_PATH): cv.string,
                 vol.Optional(CONF_SSL, default=DEFAULT_SSL): cv.boolean,
                 vol.Optional(CONF_USERNAME): cv.string,
    @@ -72,16 +76,21 @@
     conf = config[DOMAIN]
     protocol = "https" if conf[CONF_SSL] else "http"

As in my setup Zabbix server (trapper) and Zabbix Web UI are running in different containers I had to introduce another host configuration variable in the HA configuration.yaml:

zabbix:
  url: zabbix-web:8080
  host: zabbix-server
  path: api_jsonrpc.php
  publish_states_host: homeassistant
  ssl: false
  username: USERNAME
  password: PASSWORD
  token: ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00

sensor:
  - platform: zabbix
    triggers:
      hostids: [10606,10608]
      individual: true

Also it is possible to define token and if defined it will be used to login via url to the Zabbix API instead of username and password which are ignored in that case. Note: username, password and toeken are not used for sending metrics from HA to to zabbix-server host.

I am running HomeAssistant as docker container and in order to implement these changes I added to docker-compose.yml changed files in volumes section:

    volumes:
      - ./config:/config
      - /etc/localtime:/etc/localtime:ro
      - /run/dbus:/run/dbus:ro
      - ./component-zabbix-fixed/manifest.json:/usr/src/homeassistant/homeassistant/components/zabbix/manifest.json:ro
      - ./component-zabbix-fixed/__init__.py:/usr/src/homeassistant/homeassistant/components/zabbix/__init__.py:ro

Original files you can get from homeassistant container using docker cp like: docker cp homeassistant:/usr/src/homeassistant/homeassistant/components/zabbix/manifest.json ./ and docker cp homeassistant:/usr/src/homeassistant/homeassistant/components/zabbix/__init__.py ./

And then apply patch like: patch -b __init__.py <__init__.py.patch and patch -b manifest.json <manifest.json.patch

WebSpider commented 9 months ago

Thanks for this, I will turn this into a PR in a few hours. I do not have a zabbix instance myself to nl test this, so some feedback would be welcome

dsteinkopf commented 9 months ago

Sounds great. Thank you very much. I am still using my manual patches (mentioned above) on every release. So it should be relatively easy to

  1. add "url" to configuration.yml (same value as host in my case as both are actually running in different containers but ports are mapped to the same external IP)
  2. Use your two patches instead of mine
  3. Check if zabbix values keep coming in.

(anything missing?)

I hope I'll be able to do this tomorrow.

dsteinkopf commented 9 months ago

I've just done the steps described in my comment yesterday.

My first try was unsuccessful because the zabbix ssl option in configuration.yml was true - which was wrong, but has never been a problem. I don't know why.

After setting ssl correctly to false (besides adding host, see above) and "ha core restart" everythings seems ok again 👍

So as far as I can say this patch is fine!

dsteinkopf commented 9 months ago

P.S. I am using a recent Zabbix 6.4.

@WebSpider, @Wonderer643: Who will create a PR? If you want me to, I can do it (but I think I am not the one who deserves the honour)

Wonderer643 commented 9 months ago

Hi @dsteinkopf, thank you for confirming that patch is working. Glad to hear this. Feel free to create PR yourself (if not yet done by @WebSpider as promised before). I am not looking for honour, but for a well working product :) :) Also I am pretty new to HA and not actually a developer, thus it will require some time from my side to get used for HA development workflow.

dsteinkopf commented 9 months ago

I've also never developed for HA. What I'd do is just create a commit containing your two patches and submit a PR from that.

WebSpider commented 9 months ago

I'm working on a PR today, apologies for the delay :)

dsteinkopf commented 9 months ago

Don't hurry, we're able to live without it a bit longer 😉

WebSpider commented 9 months ago

Please test above PR and leave comments in this issue, thanks :+1:

Wonderer643 commented 9 months ago

Hi @WebSpider I've tested the PR and it is working fine. I am able to receive the information to the zabbix server from the HA running with this PR. However I have comment on the PR description - "This moves the support for zabbix from the unmaintained py-zabbix library to the maintained pyzabbix library..." is not correct, it should be "... from the unmaintained py-zabbix library to the maintained zabbix library ..." as in manifest.json it is now "requirements": ["zabbix==2.0.2"].

P.S. Do you want me to copy the same to the PR comments / reviews?

WebSpider commented 9 months ago

it should be "... from the unmaintained py-zabbix library to the maintained zabbix library ..."

Thanks for your tests! Yep. Will update. I will need to make a seperate PR to update the documentation for homeassistant anyways.

P.S. Do you want me to copy the same to the PR comments / reviews?

No, this should be OK. Reviews are for the maintainers of homeassistant. They will probably have something to remark as well :)

Wonderer643 commented 9 months ago

Apart of the token, please, don't forget in documentation that now it is required to define both host and url configuration variables. Like I wrote before. Example configuration.yaml:

zabbix:
  url: zabbix-web:8080
  host: zabbix-server
  path: api_jsonrpc.php
  publish_states_host: homeassistant
  ssl: false
  username: USERNAME
  password: PASSWORD
  token: ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00ff00

sensor:
  - platform: zabbix
    triggers:
      hostids: [10606,10608]
      individual: true
dsteinkopf commented 8 months ago

What about changing the logic to use host as url value, if url is missing?

I assume, this would prevent questions from users who upgraded without reading.

WebSpider commented 8 months ago

Ah yes, missed that one. Let me fix it :)

Wonderer643 commented 8 months ago

What about changing the logic to use host as url value, if url is missing?

I assume, this would prevent questions from users who upgraded without reading.

Sounds reasonable. It took some time, but please check this patch:

--- component-zabbix-original/__init__.py       2023-12-27 14:11:04.000000000 +0400
+++ component-zabbix-fixed/__init__.py  2024-01-10 23:17:30.946123276 +0400
@@ -7,13 +7,15 @@
 import threading
 import time
 from urllib.error import HTTPError
-from urllib.parse import urljoin
+from urllib.parse import urljoin, urlsplit

 from pyzabbix import ZabbixAPI, ZabbixAPIException, ZabbixMetric, ZabbixSender
 import voluptuous as vol

 from homeassistant.const import (
     CONF_HOST,
+    CONF_URL,
+    CONF_TOKEN,
     CONF_PASSWORD,
     CONF_PATH,
     CONF_SSL,
@@ -53,8 +55,10 @@
     {
         DOMAIN: INCLUDE_EXCLUDE_BASE_FILTER_SCHEMA.extend(
             {
+                vol.Required(CONF_URL): cv.string,
                 vol.Required(CONF_HOST): cv.string,
                 vol.Optional(CONF_PASSWORD): cv.string,
+                vol.Optional(CONF_TOKEN): cv.string,
                 vol.Optional(CONF_PATH, default=DEFAULT_PATH): cv.string,
                 vol.Optional(CONF_SSL, default=DEFAULT_SSL): cv.boolean,
                 vol.Optional(CONF_USERNAME): cv.string,
@@ -72,16 +76,52 @@
     conf = config[DOMAIN]
     protocol = "https" if conf[CONF_SSL] else "http"

-    url = urljoin(f"{protocol}://{conf[CONF_HOST]}", conf[CONF_PATH])
+    url = None
+    if conf[CONF_URL]:  # 'url' is set, use it as zabbix_web URL instead of value in 'host'
+        t_url = conf[CONF_URL]
+        if not t_url.startswith(("http://", "https://")):
+            t_url = f"{protocol}://{t_url}"
+        try:
+            url_parts = urlsplit(t_url)
+            _ = url_parts.port  # to raise ValueError if port in 'url' is not from range
+        except ValueError:
+            _LOGGER.error("Validation error in configuration.yml for 'host': %s", conf[CONF_URL])
+            return False
+        path = conf[CONF_PATH] if (url_parts.path == '/' or url_parts.path == '') else url_parts.path
+        url = urljoin(f"{url_parts.scheme}://{url_parts.netloc}", path)
+
+    host = conf[CONF_HOST]
+    if not host.startswith(("http://", "https://")):
+        host = f"{protocol}://{host}"
+    try:
+        url_parts = urlsplit(host)
+        _ = url_parts.port  # to raise ValueError if port in 'host' is not from range
+    except ValueError:
+        _LOGGER.error("Validation error in configuration.yml for 'host': %s", conf[CONF_HOST])
+        return False
+    if url:  # if already set, then 'host' should be used as zabbix_server trapper host:port
+        zabbix_server = url_parts.hostname
+        zabbix_port = int(url_parts.port) if url_parts.port else 10051
+    else:
+        path = conf[CONF_PATH] if (url_parts.path == '/' or url_parts.path == '') else url_parts.path
+        url = urljoin(f"{url_parts.scheme}://{url_parts.netloc}", path)
+        zabbix_server = url_parts.hostname
+        zabbix_port = 10051  # use default zabbix_server port, as port from 'host' is for Zabbix WEB API
+
     username = conf.get(CONF_USERNAME)
     password = conf.get(CONF_PASSWORD)
+    token = conf.get(CONF_TOKEN)

     publish_states_host = conf.get(CONF_PUBLISH_STATES_HOST)

     entities_filter = convert_include_exclude_filter(conf)

     try:
-        zapi = ZabbixAPI(url=url, user=username, password=password)
+        zapi = ZabbixAPI(server=url)
+        if token is not None:
+          zapi.login(api_token=token)
+        else:
+          zapi.login(user=username, password=password)
         _LOGGER.info("Connected to Zabbix API Version %s", zapi.api_version())
     except ZabbixAPIException as login_exception:
         _LOGGER.error("Unable to login to the Zabbix API: %s", login_exception)
@@ -158,7 +198,7 @@
         return metrics

     if publish_states_host:
-        zabbix_sender = ZabbixSender(zabbix_server=conf[CONF_HOST])
+        zabbix_sender = ZabbixSender(zabbix_server=zabbix_server, zabbix_port=zabbix_port)
         instance = ZabbixThread(hass, zabbix_sender, event_to_metrics)
         instance.setup(hass)

Configuration is also changed to be backward compatible with the old format.

Now only host is required. If only host is defined without url then value is used as URL to connect to Zabbix WEB API, and hostname part of URL (without port) is used to connect to zabbix_server using default port 10051. If both host and url are defined, then url is used as URL to connect to the Zabbix WEB API, and host is used as hostname and port to connect to zabbix_server with defined port. In both cases url parts (url scheme like "http" and url path) defined as URL overwrite other configuration variables like ssl and path. For example:

zabbix:
    host: 'http://zabbix-server:8080/api'
    path: '/zabbix'
    ssl: true

this config will connect to Zabbix WEB API via non SSL URL "http://http://zabbix-server:8080/api" despite that both path and ssl are defined. Also will connect to zabbix_server:10051 for sending metrics.

But:

zabbix:
    host: 'zabbix-server:8080'
    path: '/zabbix'
    ssl: true

will connect to Zabbix WEB API via SSL URL "https://zabbix-server:8080/zabbix".

If Zabbix Web and zabbix-server are running on different hosts, or need to use non default zabbix_server port then config will look like:

zabbix:
  host: zabbix-server:11151
  url: zabbix-web:8080
  path: api_jsonrpc.php
  publish_states_host: homeassistant
  ssl: false

this will connect to Zabbix WEB API via URL "http://zabbix-web:8080/api_jsonrpc.php" and to zabbix_server:11151 for sending metrics.

@dsteinkopf Can you check this patch with your old configuration file where only host is defined?

WebSpider commented 8 months ago

What about changing the logic to use host as url value, if url is missing? I assume, this would prevent questions from users who upgraded without reading.

Sounds reasonable. It took some time, but please check this patch:

Wow, cool. I was going to start on basically the same approach today, calculate URL from all the parts, if not defined. Thanks for this help, I'll make sure to mention it in the commit :)

dsteinkopf commented 8 months ago

Great. Looks like the most luxury solution.

I've done 2 tests:

  1. Using the new config (containing url): Works fine.
  2. Using the old config (no url): Invalid config for 'zabbix' at configuration.yaml, line 60: required key 'url' not provided, please check the docs at https://www.home-assistant.io/integrations/zabbix 09:51:40 – (ERROR) config.py
WebSpider commented 8 months ago

Great. Looks like the most luxury solution.

I've done 2 tests:

1. Using the new config (containing `url`): Works fine.

2. Using the old config (no `url`):
   `Invalid config for 'zabbix' at configuration.yaml, line 60: required key 'url' not provided, please check the docs at https://www.home-assistant.io/integrations/zabbix 09:51:40 – (ERROR) config.py`

Yeah, I've took @Wonderer643 's code and adapted it at a few points:

PR is updated, please test that version.

Wonderer643 commented 8 months ago

I've checked the PR. And I see the logic is broken now. My idea was: If no url python variable defined (as not set due to missing conf[CONF_URL] and url in config), then use host config parameter for both Zabbix WEB API URL and zabbix_server trapper with default port 10051. If url python variable is already defined, than it is used for Zabbix WEB API URL. And host should be exclusively used for zabbix_server trapper. Thus extra urlsplits in PR code are breaking the logic. See below my inline comments on your comments :

    if url:  # if already set, derive trapper host:port from it # not from it, but from CONF_HOST
        # url_parts = urlsplit(url)  # Not needed here as url_parts are set right above at line 101 from CONF_HOST. And we need "CONF_HOST" for zabbix_server, not "CONF_URL" (url var is set from CONF_URL above at lines 81, 85, 95 ) 
        zabbix_server = url_parts.hostname
        zabbix_port = int(url_parts.port) if url_parts.port else 10051
    else:  # url not set. Derive trapper host from CONF_HOST  # and set url variable from CONF_HOST as well
        # url_parts = urlsplit(host) # This is not breaking anything, but not needed as url_parts are already set from CONF_HOST at lines 97, 101
        path = conf[CONF_PATH] if (url_parts.path in ("/", "")) else url_parts.path
        url = urljoin(f"{ url_parts.scheme }://{ url_parts.netloc }", path)
        zabbix_server = url_parts.hostname
        zabbix_port = 10051
dsteinkopf commented 8 months ago

May I humbly suggest to split the new logic part into some def get_url_and_host(conf_url, conf_host) -> Tuple[str, str]: ... which returns host and url.

This way you are also easily able to add several test cases in the form testcases = [("conf_url1", "conf_host1", "wanted_url1", "wanted_host1"), ...] and iterate over this list containing many cases incl. edge cases.

I think this will be a good idea because the logic is not as simple as it looks on a first sight.

...just an idea, I don't know how to do this in the HA context. Maybe it helps anyway.

WebSpider commented 8 months ago

I've checked the PR. And I see the logic is broken now. My idea was: If no url python variable defined (as not set due to missing conf[CONF_URL] and url in config), then use host config parameter for both Zabbix WEB API URL and zabbix_server trapper with default port 10051. If url python variable is already defined, than it is used for Zabbix WEB API URL. And host should be exclusively used for zabbix_server trapper. Thus extra urlsplits in PR code are breaking the logic. See below my inline comments on your comments :

Ah. OK :)

    if url:  # if already set, derive trapper host:port from it # not from it, but from CONF_HOST
        # url_parts = urlsplit(url)  # Not needed here as url_parts are set right above at line 101 from CONF_HOST. And we need "CONF_HOST" for zabbix_server, not "CONF_URL" (url var is set from CONF_URL above at lines 81, 85, 95 ) 

Ah, I missed that, not knowing Zabbix a lot :-)

        zabbix_server = url_parts.hostname
        zabbix_port = int(url_parts.port) if url_parts.port else 10051
    else:  # url not set. Derive trapper host from CONF_HOST  # and set url variable from CONF_HOST as well
        # url_parts = urlsplit(host) # This is not breaking anything, but not needed as url_parts are already set from CONF_HOST at lines 97, 101

True, I'll remove this.

WebSpider commented 8 months ago

May I humbly suggest to split the new logic part into some def get_url_and_host(conf_url, conf_host) -> Tuple[str, str]: ... which returns host and url.

That is definately a good thing to do, as well as add tests. First, let's get this going again, then maybe someone can reformat the integration, since it's in need of some love.

Wonderer643 commented 8 months ago

Hi @WebSpider I've checked latest PR, and there are TypeError: slice indices must be integers or None or have an __index__ method for lines 82 and 98 from __init__.py. This is about missing pair of extra ( ) in startswith. I can't directly commit to your PR due to missing permissions, thus can you, please, correct using below patch:

--- __init__.py.bad     2024-01-12 06:03:54.723530872 +0000
+++ __init__.py 2024-01-12 05:32:35.093022731 +0000
@@ -79,7 +79,7 @@
     url = None
     if conf[CONF_URL]:  # Validate URL
         t_url = conf[CONF_URL]
-        if not t_url.startswith("http://", "https://"):
+        if not t_url.startswith(("http://", "https://")):
             t_url = f"{ protocol }://{ t_url }"
         try:
             url_parts = urlsplit(t_url)
@@ -95,7 +95,7 @@
         url = urljoin(f"{ url_parts.scheme }://{url_parts.netloc}", path)

     host = conf[CONF_HOST]  # Validate HOST
-    if not host.startswith("http://", "https://"):
+    if not host.startswith(("http://", "https://")):
         host = f"{ protocol }://{ host }"
     try:
         url_parts = urlsplit(host)
WebSpider commented 8 months ago

Hi @WebSpider I've checked latest PR, and there are TypeError: slice indices must be integers or None or have an __index__ method for lines 82 and 98 from __init__.py. This is about missing pair of extra ( ) in startswith. I can't directly commit to your PR due to missing permissions, thus can you, please, correct using below patch:

This is now applied to the PR. I'll look into if I can give you permissions in my branch, might work a bit better :-)

WebSpider commented 8 months ago

The PR was closed because the integration needs a lot of love before we're allowed merge it in.

I would like to invite @Wonderer643 to pick this up, without a running Zabbix, and no desire to run one myself, I don't have the proper setup (or time, priority, etc) to fix this.

Too bad, we were on the right track!

dsteinkopf commented 8 months ago

:-( For convenience (future readers), here again the link to the (now closed) PR: https://github.com/home-assistant/core/pull/107562

For integrations that connect to devices or services, we no longer accept new YAML configuration or changes.

This integration needs to be refactored first to use a configuration flow and config entries first.

More information about this can be found in Architecture Decision Record:

ADR-0010: https://github.com/home-assistant/architecture/blob/master/adr/0010-integration-configuration.md#decision See our developer documentation on how to get started creating a configuration flow for this integration:

https://developers.home-assistant.io/docs/config_entries_config_flow_handler

As these changes often involve a bit of work and some significant shift in the current code, we will close this PR for now.

We (and our community!) would really appreciate it if you could start on the refactoring of this integration in a new PR.

Thanks already! 👍

dsteinkopf commented 8 months ago

This config flow concept seems to be a good thing... But this means that one has to learn how to setup a local HA dev environment and everything that is needed...

Is this the right starting point? https://developers.home-assistant.io/docs/development_environment/

Wonderer643 commented 8 months ago

Yes, I started with this one, with the devcontainers via WSL. This is how I was testing the PRs. But there are a lot of other to read. And this one from Config Flow - "Integrations with a config flow require full test coverage of all code in config_flow.py to be accepted into core." raises the bar even higher for the begginers.

Anyway, I'll try, but no promises about the timeline.

dsteinkopf commented 8 months ago

I am also trying. With limited success: https://discord.com/channels/330944238910963714/554842238073700352/1195337211709767710

dsteinkopf commented 8 months ago

Here is my proposal for my above suggested config function: https://gist.github.com/dsteinkopf/fa188b97e16c8cc13a38cec5fb2364c6

It can by used def setup in this way:

    protocol = "https" if conf[CONF_SSL] else "http"

    zabbix_base_url, zabbix_server, zabbix_port = get_url_server_and_port(
        conf[CONF_URL], conf[CONF_HOST], protocol
    )
    zabbix_url = urljoin(zabbix_base_url, conf[CONF_PATH])

It includes several positive and negative test cases.

I don't like the auto formatting for the testcases as the become rather large and unreadable, but I don't know how to avoid this.

autoantwort commented 8 months ago

I planed to migrate this integration to use the config flow. I read that for the sensor the web api is required. I thought about simply dropping that feature to make the integration easier or are you actually using it (sensor for the number of active triggers in zabbix)?

dsteinkopf commented 8 months ago

Me, I am NOT using the number of active triggers. I am only using the sensors being mirrored from ha to zabbix.

autoantwort commented 8 months ago

Just after after I was done implementing the zabbix api and the user login I realized that this is completely unnecessary if you only want to send metrics to zabbix ... 🤦

autoantwort commented 8 months ago

@frenck https://github.com/home-assistant/core/pull/107562#discussion_r1450044041 got closed because the configuration should use the config flow pattern, but an important part of the integration is sending data to zabbix just like the https://next.home-assistant.io/integrations/mqtt_statestream integration which is also using a yaml config. If you still think that the integration should use the config flow, can you explain how https://next.home-assistant.io/integrations/mqtt_statestream#configure-filter could be realized as config/option flow?

dsteinkopf commented 8 months ago

In between, zabbix released an official Python library: https://blog.zabbix.com/python-zabbix-utils/27056/

autoantwort commented 7 months ago

I have created https://github.com/home-assistant/core/pull/110132 if somebody wants to review it :)

dsteinkopf commented 7 months ago

Thank you very much. I hope I am not the only one to be happy about this progress!

kazoob commented 7 months ago

I planed to migrate this integration to use the config flow. I read that for the sensor the web api is required. I thought about simply dropping that feature to make the integration easier or are you actually using it (sensor for the number of active triggers in zabbix)?

I personally use the active triggers sensor in my Home Assistant Dashboard. I would be sad to see it be removed, however I suppose it is not the end of the world.

dsteinkopf commented 6 months ago

I have created #110132 if somebody wants to review it :)

Thank you, again. Who is able to do the review?

airdolomiti commented 6 months ago

I have HAOS on Yellow and no pyzabbix file exist, so I can't edit the params to use the Zabbix components as mentioned in this video https://www.youtube.com/watch?v=6G_wec2pLw4. Any ideas?

cbergmann commented 6 months ago

hi, I patched my installation with this patch and it seems to work fine.

I have two suggestions but these are out of scope for this Issue:

vkoutecky commented 6 months ago

I have HAOS on Yellow and no pyzabbix file exist, so I can't edit the params to use the Zabbix components as mentioned in this video https://www.youtube.com/watch?v=6G_wec2pLw4. Any ideas?

I had the same problem, and in the end, I went the API route. And ultimately, it's a better solution for me. I've created a brief tutorial:

Monitoring HA in Zabbix

dsteinkopf commented 6 months ago

Yes, you're right, this seems to be the more strightforward and stable way (from my personal today's view at least...)

What I have in mind (additionally) is some discovery that

  1. automatically creates the items in Zabbix (based on the json containing every entity)
  2. with the same names as now (using the zabbix ha integration).

So I can neatlessly switch...

vkoutecky commented 6 months ago

Discovery was a great idea. I tested it, and it works - each entity is created as a separate Item.

I updated the guide a bit. Updated tutorial