Too many attributes being sent with Create and Update operations

[dnesting]

Presently the client serializes all attributes it knows about when sending Create and Update requests to the controller. By contrast, the official web UI only sends a subset of attributes that appear relevant. When retrieving resources with the Get operation, some attributes that the controller doesn't think we should care about get omitted, and others that we've set previously (that the controller never expected to see) get returned. Because we set far more attributes than the web UI sets, this results in two classes of bugs:

1. There appears to be perpetual differences between local state and what's stored on the controller, because we are trying to set an attribute that the controller omits in its response. This results in Terraform diffs that never converge.
2. When attributes that the controller isn't expecting are later made available to the web UI client, this causes the web UI to behave incorrectly. For instance, a network configured as WAN will show "LAN" on the network list. There may be more serious bugs lurking.

Acknowledging that this is a poorly-behaved API service (it's not exactly documented or supported), it's still an issue that needs a solution. Has anyone given this any thought?

It seems like there needs to be a few decisions:

1. Do we try to fix this?
2. How should we go about suppressing fields? How do we figure out what to suppress?
3. Knowing the best plan for (2), do we still think this is a tractable problem?

I suggest:

1. Yes
2. Making requests, cataloging behavior, and hand-crafting a set of conditions that need to be true for a list of fields to exist in the request (and contribute to a diff), along with whether fields are required, and what their defaults are.
3. I don't know.

For reference, here are some JSON blobs (sorted fields):

Network

Creating a WAN in the web UI:

{
  "name": "LTE",
  "purpose": "wan",
  "report_wan_event": true,
  "wan_dhcp_options": [],
  "wan_egress_qos": "",
  "wan_ip_aliases": [],
  "wan_load_balance_type": "failover-only",
  "wan_load_balance_weight": 50,
  "wan_networkgroup": "WAN2",
  "wan_type": "dhcp",
  "wan_type_v6": "disabled"
}

Retrieving this object back from the controller adds an _id and site attributes.

Creating a LAN in the web UI:

{
  "dhcp_relay_enabled": false,
  "dhcpd_dns_enabled": false,
  "dhcpd_enabled": true,
  "dhcpd_gateway_enabled": false,
  "dhcpd_leasetime": 86400,
  "dhcpd_start": "10.0.0.6",
  "dhcpd_stop": "10.0.0.254",
  "dhcpd_time_offset_enabled": false,
  "enabled": true,
  "gateway_type": "default",
  "ip_subnet": "10.0.0.1/24",
  "ipv6_interface_type": "none",
  "ipv6_pd_start": "::2",
  "ipv6_pd_stop": "::7d1",
  "is_nat": true,
  "name": "Test",
  "nat_outbound_ip_addresses": [],
  "networkgroup": "LAN",
  "purpose": "corporate",
  "vlan": "22",
  "vlan_enabled": true
}

Note the entirely different field sets, such as networkgroup.

Switch Port Profile

Separately, here's a GET of an existing switch port profile:

  {
   "_id": "...omitted...",
   "site_id": "...omitted...",
   "name": "IoT",
   "forward": "native",
   "isolation": false,
   "native_networkconf_id": "...omitted...",
   "attr_no_edit": true
  }

Here's the Terraform config:

resource "unifi_port_profile" "iot" {
  name                  = "IoT"
  native_networkconf_id = unifi_network.iot.id
}

But it sees this diff (with my commentary):

  ~ resource "unifi_port_profile" "iot" {
      ~ autoneg                        = false -> true  // the web UI says "link speed" = "Autonegotiate"
      + dot1x_ctrl                     = "force_authorized" // the web UI says "802.1X Control" = "Force authorized"
      ~ dot1x_idle_timeout             = 0 -> 300
        id                             = "...omitted..."
      ~ lldpmed_enabled                = false -> true // Enabled in the web UI
        name                           = "IoT"
      + op_mode                        = "switch"
      ~ stp_port_mode                  = false -> true // Enabled in the web UI
        # (25 unchanged attributes hidden)
    }

And here's what it sends to the controller:

{
 "_id": "5d6f172bf25e119aa7e3005e",
 "site_id": "default",
 "autoneg": true,
 "dot1x_ctrl": "force_authorized",
 "dot1x_idle_timeout": 300,
 "egress_rate_limit_kbps_enabled": false,
 "forward": "native",
 "full_duplex": false,
 "isolation": false,
 "lldpmed_enabled": true,
 "lldpmed_notify_enabled": false,
 "native_networkconf_id": "5d6f172bf25e119aa7e3005d",
 "name": "IoT",
 "op_mode": "switch",
 "port_security_enabled": false,
 "stormctrl_bcast_enabled": false,
 "stormctrl_mcast_enabled": false,
 "stormctrl_ucast_enabled": false,
 "stp_port_mode": true,
 "voice_networkconf_id": ""
}

(This particular update gets a 400 api.err.NoEdit but that's another issue.)

[dnesting]

Idea: Manually work out what conditions mean some fields are required, optional, or should be suppressed, and find a way to suppress them when serializing (json omitifempty?). Maybe something like:

switch obj.Purpose {
  "wan":
    suppress(obj.NetworkGroup, obj.VLAN, obj.DhcpdEnabled, ...)
    require(obj.WANType, obj.WANTypeV6, ...)
  "lan":
    suppress(obj.WanType, ...)
}

if obj.DhcpdEnabled {
  require(obj.DhcpdStart, ...)
}

[dnesting]

Idea: At least with the Network type, the field sets for purpose:"LAN" and purpose:"WAN" are completely different. Maybe there needs to be two separate objects, like LANNetwork and WANNetwork that just use the same API endpoint? I haven't even begun to dive into the Device type, which I think might have a similar issue with wildly different field sets depending on what type of device it is.

[dnesting]

Trying to work out how the web UI does this, I found in app.js code like this (for the 'network' save operation):

angular.module("app-unifi-manage").controller("NetworkFormLanController", ["$translate", "CollectionView", "devices", "deviceFeatureUtils", "deviceUtils", "dhcpOptions", "gatewayUtils", "heyToasts", "ipUtils", "ipv6Utils", "BaseFormController", "networks", "networkUtils", "portForwards", "settings", "siteUtils", "switchUtils", "system", "_", "DHCP_USER_OPTION", "NETWORK_DEFAULTS", "REGEX_PATTERNS", "UI", function(e, t, n, i, d, p, f, m, h, _, g, v, y, E, C, b, S, T, A, I, O, w, R) {
    ... {
            key: "save",
            value: function() {
                var e = this;
                this.isSubmitting = !0;
                var t = angular.extend({}, this.modelCopy.attributes, {
                    setting_preference: "manual",
                    enabled: !0,
                    is_nat: !0,
                    dhcpd_enabled: "server" === this.dhcpModeCopy,
                    dhcp_relay_enabled: "relay" === this.dhcpModeCopy
                });
                t.dhcpd_time_offset_enabled || delete t.dhcpd_time_offset,
                "static" !== t.ipv6_interface_type && "dhcpdv6_enabled"in t && (t.dhcpdv6_enabled = !1);
                var n = Object.keys(this.modelCopy.attributes).filter(function(t) {
                    return t.startsWith(e.DHCP_USER_OPTION_PREFIX)
                });
                return this.dhcpOptions.repository.collection.forEach(function(i) {
                    var r = e.DHCP_USER_OPTION_PREFIX + i.id
                      , o = t[r];
                    void 0 !== o && null !== o && "" !== o || delete t[r];
                    var a = n.indexOf(r);
                    -1 !== a && n.splice(a, 1)
                }),
                t = A.omit(t, n),
                this.model.get("attr_hidden_id") || (t.vlan_enabled = !!this.modelCopy.get("vlan")),
                "none" === t.ipv6_interface_type && (delete t.ipv6_ra_enabled,
                delete t.ipv6_ra_priority,
                delete t.ipv6_ra_valid_lifetime,
                delete t.ipv6_ra_preferred_lifetime,
                delete t.dhcpdv6_enabled,
                delete t.dhcpdv6_start,
                delete t.dhcpdv6_stop,
                delete t.dhcpdv6_leasetime,
                delete t.dhcpdv6_dns_auto,
                delete t.dhcpdv6_dns_1,
                delete t.dhcpdv6_dns_2,
                delete t.dhcpdv6_dns_3,
                delete t.dhcpdv6_dns_4),
                "static" !== t.ipv6_interface_type && delete t.ipv6_subnet,
                "corporate" !== t.purpose && "guest" !== t.purpose && (delete t.gateway_device,
                delete t.gateway_type),
                "switch" !== t.gateway_type && delete t.gateway_device,
                "pd" !== t.ipv6_interface_type ? (delete t.ipv6_pd_interface,
                delete t.ipv6_pd_prefixid) : this.isUbios() ? delete t.ipv6_pd_prefixid : null === t.ipv6_pd_prefixid && (t.ipv6_pd_prefixid = ""),
    ...

It looks like this represents a fairly complex set of conditions that cause the web UI to delete fields in the JSON structure that gets sent to the server. Based on my own experimentation, the deletion of these fields is necessary to keep the resources in a valid state and failing to do so causes problems with some resources in the future, especially in the form of display bugs in the web UI and inability of Terraform updates to converge.

Should we try and make go-unifi behavior match the web UI here and delete these fields? If so, how should we approach it? It looks like things like createNetwork and updateNetwork are auto-generated and it's not clear how we could auto-generate these instructions. Maybe an optional var networkFilter func(n Network) (filtered Network) = nil that could be called in updateNetwork or createNetwork to zero out fields that the web UI would have omitted?

[dnesting]

Ugh it's even more complicated than that. Somewhere it's actually treating things like "WAN" and "LAN" as distinct data types, so we have an entirely different set of operations that occur for the WAN case:

angular.module("app-unifi-manage").controller("NetworkFormWanController", ["$translate", "deviceFeatureUtils", "deviceUtils", "featureFlag", "frame", "gatewayUtils", "heyToasts", "ipUtils", "BaseFormController", "networks", "networkUtils", "settings", "speedTest", "system", "NETWORK_DEFAULTS", "REGEX_PATTERNS", "UI", function(e, t, n, d, p, f, m, h, _, g, v, y, E, C, b, S, T) {
    return new (function(n) {
        ...
            key: "save",
            value: function() {
                var e = this;
                this.isSubmitting = !0;
                var t = angular.extend({}, this.modelCopy.attributes, {});
                return t.setting_preference = "manual",
                t.wan_dns1 || t.wan_dns2 ? t.wan_dns_preference = "manual" : t.wan_dns_preference = "auto",
                null === t.wan_vlan && (t.wan_vlan = ""),
                t.wan_ip_aliases && (["static", "pppoe"].includes(this.showTypeForm) ? t.wan_ip_aliases = t.wan_ip_aliases.filter(function(e) {
                    return !!e
                }) : t.wan_ip_aliases = []),
                v.saveNetwork(this.model, t).then(function(n) {
                    return m.pushSuccess(n),
                    !e.isPortReassignmentSupported() || f.tryAssignToDefaultPort(t.wan_networkgroup).then(function() {
                        return !0
                    })
                }).catch(function(e) {
                    return m.pushError(e),
                    !1
                }).finally(function() {
                    e.isSubmitting = !1
                })
            }
        ...

The digger I deep the more it looks like this needs a fair amount of customized code to do some of the same data sanitization/defaulting/filtering that the web UI does in routines like this.

[red-hood]

From my experience, the Unifi controller does not really have sth that could be regarded as an API. That thing randomly returns strings and integers for the same types, depending if it's a response to a POST or PUT. Sometimes it also returns only a few attributes, if it can't create the object, but actually does not return an error. All validation is done on the UI, the http endpoints seem to allow creation of arbitrarily broken objects, which then bring down the whole controller. Using the http endpoints for automation seems to be a Russian roulette...

[dnesting]

Agreed. I personally decided this was too much of a mine field, reset my controller to clear out all of the warts my experimentation here created in my environment, and have abandoned this approach. UniFi needs to rethink their API from scratch using API/backend engineers rather than frontend engineers.

The only way a package like this will be safe is if it fully emulates a frontend with all of these special exceptions and edge cases, and exposes a "real" API on top of that. I spent tens of hours building something to capture all of the API calls my browser made for every kind of interaction with my controller, clustering operations based on what fields were getting set, and the resulting TODO list was just overwhelming, especially considering that it's going to continue to change over time. It's just not sustainable in my opinion until Unifi decides to make its API real.

Until then I believe this approach is unsafe. 😞