Open dnesting opened 2 years ago
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, ...)
}
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.
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?
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.
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...
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. 😞
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:
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:
I suggest:
For reference, here are some JSON blobs (sorted fields):
Network
Creating a WAN in the web UI:
Retrieving this object back from the controller adds an
_id
andsite
attributes.Creating a LAN in the web UI:
Note the entirely different field sets, such as
networkgroup
.Switch Port Profile
Separately, here's a GET of an existing switch port profile:
Here's the Terraform config:
But it sees this diff (with my commentary):
And here's what it sends to the controller:
(This particular update gets a 400
api.err.NoEdit
but that's another issue.)