WIP: Import policies in a policy

[vhdirk]

This is an implementation for #298 (referenced by #991)

[thjaeckle]

@vhdirk I will try to merge "master" onto my 2 year old experiment of adding policyImports - that should give a really good start for putting additional required work on top of it (e.g. how to handle search index updates then).

[vhdirk]

Oh, that'd be very cool! Thanks! Even if you don't have time to rebase it right now, I'm very interested to see where you ended up.

[thjaeckle]

I pushed a branch: https://github.com/eclipse/ditto/tree/lab/policy-imports I invested a little trying to merge master onto that - I think from Policy Model side I got very far - however there still is a lot of stuff not even compiling - I did not even come to the tricky part the "policy enforcement" and the policy cache loading / cache invalidation, etc.

To be honest: I think without deep understanding of the Ditto codebase this will be really hard to get finished.

*edit: pushed again now really merged master onto it

[thjaeckle]

@vhdirk I got the build working - did some first rough code adjustments right on your branch, I hope you don't mind. Will look into the functionality tomorrow.

What still is missing are unit tests, e.g. for:

• ImmutableEffectedImports
• ImmutableImportedLabels
• ImmutablePolicyImport
• ImmutablePolicyImports
• LabelInvalidException
• PolicyImportHelper

And also for:

• PolicyCacheLoader

[thjaeckle]

@vhdirk I added a "feature toggle" for the policy import feature - we recently already added something like this for the "merge things" feature.

I started Ditto on this feature branch, however the imported policies were not yet applied - I will now look into this, but I fear that currently the policy import is not yet working.

[thjaeckle]

@vhdirk I invested some time in getting the functionality to actually "work".

• For direct access to a thing/policy (knowing their IDs) I would say that the policy import does work perfectly

We have a few open topics before being able to merge this PR.

search index

However, the search index is only correctly updated for the "happy path":

• when a ThingUpdater responsible for updating a thing with ID x:y-1 is currently "alive" (is running), updating a policy which is imported in other policies will correctly update the things using the imported policy
  • so this is only the case when changes were recently done on a thing and e.g. not on a cluster restart

This is how the search index is updated when a policy is changed:

1. the PolicyEvent is received in PolicyEventForwarder of the search service
2. PolicyEventForwarder finds out all things which (directly) use this policy (via ThingsSearchUpdaterPersistence.getPolicyReferenceTags(Map) and creates for each matched thing one PolicyReferenceTag message
3. each created PolicyReferenceTag message is locally forwarded to ThingsUpdater
4. the ThingsUpdater invalidates the policyCache of that policy (on the local "search" instance)
5. the ThingsUpdater sends each PolicyReferenceTag via cluster sharding to a ThingUpdater in a search instance "responsible" for the shard of the thingId (consistently always the same instance for the same ID)

The problem is now that step 2. does not yet respect imported policies when calculating the PolicyReferenceTag message based on the update of a policy which was imported.

• therefore, we would need to adjust the MongoDB collection searchThings (or introduce another persistent MongoDB collection) in which such information is persisted
• basically we must answer the question: "which things are affected (because they link to a policy 'B' importing policy 'A') when policy 'A' is modified?"
• in the implementation MongoThingsSearchUpdaterPersistence.getPolicyReferenceTags(Map) those affected policyIds must additionally be returned as PolicyReferenceTags

When enhancing the search index to know that we:

• must ensure to stay backwards compatible (existing indexed things must still be found the same as before)
• must look into MongoDB indexes - so that those queries are fast
• have the freedom of add something like additionalPolicyIds - as the search index is "only" used in order to determine thingIds matching a search query .. the structure of it is never showed to the Ditto API user

importable

And one more thing we found in a peer-review with the team:

• a (potentially to be imported) policy must be able to declare single entries as "not importable" - otherwise when importing a complete policy there could be entries imported which are not desired to "include"
• as when creating existing policies this had not to be thought about we think that it's "safer" to assume that existing policies shall not be imported

We propose to add an optional "importable": true (the default value to assume if the field is not present is false - following the principle of least privilege) JSON field to the Policy Entry, e.g.:

{
  "policyId": "foo:bar",
  "entries": {
    "SUPER_ADMIN": {
      "importable": false,
      "subjects": {
        ...
      },
      "resources": {
        ...
       },
...

summing up

1. Search must be fixed to consistently update things in the index when imported policies are changed
2. Support principle of least privilege by adding "importable" enum to PolicyEntry defaulting to false - that way importing must be explicitly enabled
3. Documentation (OpenAPI + Ditto website docs) is missing
4. Additional (integration) tests testing applying the policies, e.g. added to ThingCommandEnforcementTest

I would say we are 70% through :-)

[thjaeckle]

@vhdirk @BobClaerhout I rebased this PR on the current Ditto master branch (took quite some effort because of some breaking Ditto 2.0 module changes) - I referenced @vhdirk as co-author in the commit.

Summarizing there are still some open TODOs:

• My comment https://github.com/eclipse/ditto/pull/993#discussion_r615890894 still is to be addressed - however I think that it would be easier to achieve when simply invalidating imported and affected policies cluster-wide when a policy was changed which was used in other policies as import
  • see my "// TODO TJ" comment in ThingsUpdater.processPolicyReferenceTag()
• Documentation (OpenAPI + Ditto website docs) is missing
• Additional (integration) tests testing applying the policies, e.g. added to ThingCommandEnforcementTest

[BobClaerhout]

@thjaeckle great! Thanks for the effort!
Just for my information: are these 3 things blockers for the PR to be merged or can we merge this and fix the open TODO's in future PRs?

[thjaeckle]

Yes, they are blockers - we won't merge a not sufficiently tested or undocumented feature. If this helps, we may continue the work on another fork (e.g. an official aloxy fork?) of Ditto instead of a private fork.

[BobClaerhout]

No, it's ok. I was just wondering. it gives me an insight on the status of the PR without diving in too deep. As you know, we're a little short on resources right now

[thjaeckle]

This should not be resumed before a Ditto 3.0 release where the "concierge" service will be removed, see #1339

[thjaeckle]

Superseded by #1555