refactor: simplify identity types, move API code to daemon package

[benhoyt]

This is a follow-up PR after #744. It's not related to the package move, but it's to simplify the number of identity types from 3 to 2, and make it more consistent with how we do other API handling: internal type for internal usage, and then separate type or conversion functions in the daemon/API package to do the conversion to and from the API format.

This is now ready for review, though I still also want to do a few more manual tests on it.

[dmitry-lyfar]

I looked into the logic separation, validation and tests, not so much into the specifics of the identities types implementation. LGTM, it makes it easier to port these to another snapd-based codebase.

For my understanding: If I have:

{
  "action": "replace",
  "identities": {
    "alice": null,
    "bob": {
       ...
    }
  }
}

it would result in a nil apiIdent which would return nil as an identityFromApi. This looks like a legitimate case for the remove action, so that that identity should not only be mentioned but must be explicitly set to null by the client. Ditto for the replace.

I cannot seem to grasp the difference between replace and a combination of add, update and remove. My guess was that this action introduced for the client's convenience to reduce the number of round trips for cases when an entity does not exist but has to be created without an extra GET to ensure that? When one should use update vs. replace?

[benhoyt]

Yeah, your understanding is correct. The "null" identity is only for remove and replace (in the replace case "null" means remove it). The add/update/remove commands were more intended for manual usage; the replace command was more intended for API usage (it's idempotent and does all three).

[dimaqq]

Yeah, your understanding is correct. The "null" identity is only for remove and replace (in the replace case "null" means remove it).

Let's add a unit test for that.

Edit: there are tests for update and remove already, they were just not visible in the change set.

[dimaqq]

Looks good to me!

[benhoyt]

Yeah, your understanding is correct. The "null" identity is only for remove and replace (in the replace case "null" means remove it).

Let's add a unit test for that.

Yep, there's "null" tests for replace too:

• pebble/internals/overlord/identities/identities_test.go

  Line 571 in 00aaceb

  "bob": nil, // nil means remove it

• pebble/internals/daemon/api_identities_test.go

  Line 265 in 00aaceb

  "bob": null,