Skip to content
New issue

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

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

Already on GitHub? # to your account

Enhancement/add support pihole v6 #371

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

KallanX
Copy link

@KallanX KallanX commented Feb 20, 2025

Adding support for the newly released Pi-hole major version 6. This update changed how you interact with the API.

@@ -27,6 +33,8 @@ type dnsStatsWidget struct {
AllowInsecure bool `yaml:"allow-insecure"`
URL string `yaml:"url"`
Token string `yaml:"token"`
AppPassword string `yaml:"app-password"`
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably get away with using the existing password though that's for the maintainer to decide

if token == "" {
return nil, errors.New("missing API token")
// piholeGetSID retrieves a new SID from Pi-hole using the app password.
func piholeGetSID(instanceURL, appPassword string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing allowInsecure, I was getting certificate error

Copy link
Contributor

@ralphocdol ralphocdol Feb 20, 2025

Choose a reason for hiding this comment

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

The API was rewritten, and so are the output, it looks like you're still using the same piholeStatsResponse as v5. I think you'd need to query multiple API from pihole to get those required data as /api/stats/summary doesn't have everything required for the widget to work.

# Probably for QueriesSeries and BlockedSeries, 
# or not
/api/stats/database/summary?from=1740020400&until=1740021160`
# TopBlockedDomains
/api/stats/top_domains?blocked=true&count=10

The rest should be available from the first query.

@KallanX
Copy link
Author

KallanX commented Feb 20, 2025

@ralphocdol I've been able to fix a good portion of your comments. The only thing left is for me to understand these two values:

QueriesSeries piholeQueriesSeries `json:"domains_over_time"`
BlockedSeries map[int64]int       `json:"ads_over_time"`

Are these in the original /api/stats/summary response or are they separate? I was able to grab the top blocked domains by hitting the separate /api/stats/top_domains?blocked=true.

@ralphocdol
Copy link
Contributor

@KallanX I think this has something to do with the widget's bar graph based on the comment on this code

// Pihole _should_ return data for the last 24 hours in a 10 minute interval, 6*24 = 144
if len(r.QueriesSeries) != 144 || len(r.BlockedSeries) != 144 {
	return stats
}

So the API /api/stats/database/summary?from=1740020400&until=1740021160 was incorrect.

Looking at the screenshot from the DNS Stats docs

The reddish one is most likely the BlockedSeries while the gray one is the QueriesSeries

@ralphocdol
Copy link
Contributor

/api/history looks closer

{
  "history": [
    {
      "timestamp": 1511819900.539157,
      "total": 2134,
      "cached": 525,
      "blocked": 413,
      "forwarded": 1196
    },
    {
      "timestamp": 1511820500.583821,
      "total": 2014,
      "cached": 52,
      "blocked": 43,
      "forwarded": 1910
    }
  ],
  "took": 0.003
}

BlockedSeries = blocked
QueriesSeries = total

@KallanX
Copy link
Author

KallanX commented Feb 26, 2025

@ralphocdol

This kind of got away from me. Took some time away to think about it all. I really don't understand why the Pi-hole developers had to change so much of their API. If it was for security reasons, I would argue they make it an opt-in feature, rather then a permanent feature. As I would assume most users of Pi-hole host their instances within their own home networks with no outside access (as I do) which would make the security argument a rather mute point.

I ran into an issue where SIDs were refused access after a period of time and hitting any endpoint would not revive them. So I had to include a check function along side a fetch function for the SIDs.

Anyways, check the latest. Let me know.

@ralphocdol
Copy link
Contributor

Made a pull request on your fork KallanX#1

Fix Graph and move SID to header
Copy link
Contributor

@ralphocdol ralphocdol left a comment

Choose a reason for hiding this comment

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

Let's wait for the maintainer @svilenmarkov to review this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants