Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Zabbix service widget #3905

Merged
merged 3 commits into from
Aug 29, 2024
Merged

Feature: Zabbix service widget #3905

merged 3 commits into from
Aug 29, 2024

Conversation

ping-localhost
Copy link
Contributor

@ping-localhost ping-localhost commented Aug 28, 2024

Proposed change

image

The user will be able to add the widget to the services.yaml file using the following configuration:

widget:
  type: zabbix
  url: http://zabbix.host.or.ip/zabbix
  key: your-api-key

Relevant feature request: #1953

Notes

I did somewhat break the Service Widget Guidelines.

  • Zabbix exposes 6 alert fields by default, but I can cut it down to just 4.
  • Zabbix requires an auth-field in the JSON RPC call for API tokens, which I could not figure out how to do with the generic jsonrpc proxy.

Let me know what I should change to have a valid pull request.

Type of change

  • New service widget
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation only
  • Other (please explain)

Checklist:

  • If applicable, I have added corresponding documentation changes.
  • If applicable, I have reviewed the feature and / or service widget guidelines.
  • I have checked that all code style checks pass using pre-commit hooks and linting checks.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.

Copy link
Collaborator

@shamoon shamoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did somewhat break the Service Widget Guidelines.

Zabbix exposes 6 alert fields by default, but I can cut it down to just 4.
Zabbix requires an auth-field in the JSON RPC call for API tokens, which I could not figure out how to do with the generic jsonrpc proxy.

Let me know what I should change to have a valid pull request.

Well, you need to change both of those things, that's why we have the guidelines.

Setting up a Zabbix instance to test does not seem trivial, so Im not inclined to do so, but Im certain the existing jsonrpc proxy can be modified to accept body params, or just simply adding logic to use widget.key if its supplied instead of username/pw.

@ping-localhost
Copy link
Contributor Author

ping-localhost commented Aug 29, 2024

Well, you need to change both of those things, that's why we have the guidelines.

I've removed the two lowest priority fields for now.

Im certain the existing jsonrpc proxy can be modified to accept body params, or just simply adding logic to use widget.key if its supplied instead of username/pw.

I've tried to do that, by passing the params via the URL itself and then parsing it again within the proxy. Hopefully this is okay.

We now also pass the actual widget object to the sendJsonRpcRequest so we can decide if we need to use username + password or the key.

Setting up a Zabbix instance to test does not seem trivial

If you're using Proxmox, you could use tteck's proxmox scripts to setup a Zabbix LXC.

@shamoon shamoon enabled auto-merge (squash) August 29, 2024 18:26
@ping-localhost
Copy link
Contributor Author

As long as Authorization stays capitalized, I'm happy with the changes in 4a0b18b. Zabbix doesn't accept the API token if the header is authorization.

Copy link
Collaborator

@shamoon shamoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the quick changes.

  1. I just reverted the change to the headers of the jsonrpc proxy just in case it affects other widgets. Presumably your version is more correct but zabbix didn't seem to care (I went ahead and did docker setup, pretty simple actually)
  2. Passing arbitrary params is a security vulnerability, so you can look how I've done this, basically they need to be hard-coded into the widget

Obviously let me know if you have any concerns

Thanks
Screenshot 2024-08-29 at 11 44 40 AM

@shamoon shamoon merged commit 50bb5e7 into gethomepage:main Aug 29, 2024
2 checks passed
@ping-localhost ping-localhost deleted the zabbix branch August 29, 2024 18:46
@ping-localhost
Copy link
Contributor Author

Perfect, thank you for fixing my mistake. I'll make sure to do better on the next widget 😁

shamoon added a commit that referenced this pull request Aug 29, 2024
@adm2k
Copy link

adm2k commented Aug 30, 2024

So is this widget usable at this time? I set it up but getting this:
image

@ping-localhost
Copy link
Contributor Author

So is this widget usable at this time? I set it up but getting this:

There is no new release yet, so it is not available yet.

@adm2k
Copy link

adm2k commented Aug 30, 2024

Ok, thanks for letting me know...

Plancke pushed a commit to Plancke/homepage that referenced this pull request Sep 6, 2024
@ping-localhost
Copy link
Contributor Author

@adm2k FYI, the widget is now available

@adm2k
Copy link

adm2k commented Sep 13, 2024

Thanks, working now...

Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion for related concerns. See our contributing guidelines for more details.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants