Group records in database by ipVersion

[felixwrt]

Currently, items in the database (updates.json) are grouped by domain and host. This leads to problems when there are multiple config items that contain the same domain and host.

I'm using two separate config items with the same host and domain but different ipVersion values to update both the ipv4 and ipv6 address. In this setup, ddns-updater currently uses the history of both ip versions which leads to incorrect info displayed in the UI.

This PR adds ipVersion as an additional item that is used to group IPs in the database. With this change, the history is displayed correctly.

Example config.json:

{
    "settings": [
        {
            "provider": "strato",
            "domain": "my.domain",
            "host": "@",
            "password": "pw",
            "ip_version": "ipv4",
            "provider_ip": false
        },
        {
            "provider": "strato",
            "domain": "my.domain",
            "host": "@",
            "password": "pw",
            "ip_version": "ipv6",
            "provider_ip": false
        }
    ]
}

updates.json using current master:

{
    "records": [
      {
        "domain": "my.domain",
        "host": "@",
        "ips": [
          {
            "ip": "2001:fff:8987:4000:3e18:ffff:fe03:ffff",
            "time": "2023-07-30T01:44:10.54280786Z"
          },
          {
            "ip": "97.155.132.1",
            "time": "2023-07-31T01:44:16.306989718Z"
          },
          {
            "ip": "2001:fff:89a4:9a00:3e18:ffff:fe03:ffff",
            "time": "2023-07-31T01:44:16.505670661Z"
          },
          {
            "ip": "2001:fff:8984:1d00:3e18:ffff:fe03:ffff",
            "time": "2023-08-01T01:44:10.520411731Z"
          },
          {
            "ip": "97.155.133.27",
            "time": "2023-08-02T01:39:10.529543713Z"
          },
          {
            "ip": "2001:fff:89a0:f500:3e18:ffff:fe03:ffff",
            "time": "2023-08-02T01:39:10.724642214Z"
          }
        ]
      }
    ]
  }

updates.json with the changes proposed in this PR:

{
  "records": [
    {
      "domain": "my.domain",
      "host": "@",
      "ipVersion": "ipv4",
      "ips": [
        {
          "ip": "97.155.132.1",
          "time": "2023-07-31T01:44:16.306989718Z"
        },
        {
          "ip": "97.155.133.27",
          "time": "2023-08-02T01:39:10.529543713Z"
        }
      ]
    },
    {
      "domain": "wirth.one",
      "host": "@",
      "ipVersion": "ipv6",
      "ips": [
        {
          "ip": "2001:fff:8987:4000:3e18:ffff:fe03:ffff",
          "time": "2023-07-30T01:44:10.54280786Z"
        },
        {
          "ip": "2001:fff:89a4:9a00:3e18:ffff:fe03:ffff",
          "time": "2023-07-31T01:44:16.505670661Z"
        },
        {
          "ip": "2001:fff:8984:1d00:3e18:ffff:fe03:ffff",
          "time": "2023-08-01T01:44:10.520411731Z"
        },
        {
          "ip": "2001:fff:89a0:f500:3e18:ffff:fe03:ffff",
          "time": "2023-08-02T01:39:10.724642214Z"
        }
      ]
    }
  ]
}

UI (current master):

UI (with the changes proposed here):

Let me know if this looks good to you. Thanks!

[qdm12]

Nice work 🎖️ Also good job on the high quality PR description 💯

However, I think we should have some migration from the older updates.json format to this newer one. We can run this migration when creating the persistent database object I think, and re-write the file (sort the ipv4 apart from the ipv6 events). Let me know if you need some help doing this, but if you feel comfortable enough with Go, go for it! 😉

[felixwrt]

I thought a bit about migration but didn't find an obvious way to do it. The problem is that the result of the migration depends on the config. For example, if the database contains both ipv4 and ipv6 addresses, we don't know whether they should be splitted into two groups or stay within one. In my example above, splitting would be correct. If the config contained a single item with ipVersion "ipv4 or ipv6", the migration should keep the addresses in a single group and add the key ipConfig: "ipv4 or ipv6" to that group.

With access to the config, the migration could be done correctly, but I don't know how much effort it would be to get the config info to the place where the database is written. Smells like bad design to me, but I might be overseeing something.

While we're talking about changing the database format: I think it would make sense to include a version field. This would make future changes to the database format easier.

[felixwrt]

I found a solution I'm quite happy with. I implemented a method on Database that does the migration from the old format to the new one. As this needs access to the Providers, I'm calling it from the main function. To me, this looks better than passing the providers as a parameter when creating the database.

The migration itself adds a version key to the file and sets the ip_version keys for all records. Providers are used to decide what value the ip_version key should have. If there's only a single provider, the ip version of it is used. If there's no provider (e.g. when the database contains old data and the config has been changed), the ip version is set to ipv4 or ipv6. If there are multiple providers that use different ip version settings, the record is split into one record for each ip version setting used. Events with ipv4 addresses are put in the ipv4 record if that exists and into the ipv4 or ipv6 record otherwise. ipv6 events are handled the same.

I've written some tests for the common cases.

I'm still very much a beginner with Go, so feel free to nit-pick on all the stuff I did wrong ;-)

[felixwrt]

@qdm12 did you get a chance to look at this yet?

[qdm12]

Yes it's in progress 😄 I'll probably submit a review tomorrow 🙏 Sorry for the delay and thanks for your patience (and work!!)!

[felixwrt]

Hi, thanks for the review. I'll try to work through it within the next couple of days.

[felixwrt]

Hey @qdm12,

I just tried to get back into this work but noticed that all my knowledge of this project and Go is gone. I also don't have time currently to pursue this further, sry. I hope that you'll find the time to finish this PR and get it merged.

[qdm12]

Hey @felixwrt I have been reworking/twisting this around, and I think I came up with a very simple solution: just filter the IPs by IP version (v4 or v6 or v4Orv6) when getting events, without a database migration/change. Effectively the ips field is still a mixed bag of IPv4 and Ipv6, but the program takes care to filter them according to the ip version defined. The commit is 942ae51 with about ~20 lines. This should fix it without migration etc. 😉

[qdm12]

I will merge this since it should do the trick with minimal code changes.
I have a decent database migration code stashed locally in case it's needed, feel free to let me know. Thanks 💯 !

[CamFlyerCH]

@qdm12 Thanks for solving this issue ! Works now for me.