Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only allow update of proxy entities via the API #714

Closed
mercul3s opened this issue Dec 11, 2017 · 13 comments
Closed

Only allow update of proxy entities via the API #714

mercul3s opened this issue Dec 11, 2017 · 13 comments

Comments

@mercul3s
Copy link
Contributor

mercul3s commented Dec 11, 2017

In the entities API, validate the entity_class field is proxy before allowing an entity create or update.

In the case that entity_class is not proxy, return a human-readable error indicating that only proxy entities are allowed to be modified via the API. This would cause sensuctl entity update to return that human readable error.

In sensuctl disambiguate proxy entities and entities, providing a command to manage proxy entities: sensuctl proxy-entity .

subcommand being: get, list, update, delete, etc.

Update all proxy entity / proxy check documentation to reflect this change and refer to sensuctl proxy-entity.

@palourde
Copy link
Contributor

palourde commented Feb 16, 2018

@mercul3s Do you know if this is still relevant? I know 1.x just replace the existing entity and merging entities could potentially lead to some complications so I'm curious!

@mercul3s
Copy link
Contributor Author

@palourde I honestly can't remember the details of the discussion that led to this ticket! I think we do just replace the entity now as well. @grepory do you have any thoughts?

@grepory
Copy link
Contributor

grepory commented Feb 16, 2018

If someone modifies an entity via the API, which if we don’t support now we will in the future, the store needs to be the source of truth.

@calebhailey
Copy link

Would merging two entities update the other resources (e.g. check results / events) related to those entities? I'm thinking like a cascading update in a relational DB...

Example:

  • Entity 1 has event/incident id "101" associated with it
  • Entity 2 has event/incident id "102" associated with it
  • User merges Entity 2 into Entity 1 (effectively merging Entity 2 attributes into Entity 1, and then deleting Entity 2)

Will Entity 1 now have event/incident id 101 and 102 associated with it?

@grepory
Copy link
Contributor

grepory commented May 31, 2018

@calebhailey in short, no. We don't have a relational DB and we should avoid, as much as possible, any attempts to treat it as such. There are some exceptions to this rule, but those need to be minimized.

The original intent for "merging entities" was so that people could customize entities via the API.

There are a number of very thorny issues here.

Modifying entities via the API is currently possible due to the need for custom entities in proxy requests. I think perhaps proxy entities deserved their own type, but that ship has sailed for now.

Updating entities via the API is highly problematic. Processes that create and update entities (e.g. agents) should be the source of truth for that entity. Ideally, these processes are sending truths to sensu-backend. Given that, then any input from the API not from those processes are synthetic.

Merging two entities was a proposal for dealing with this circumstance.

Entity A is updated via the API to have subscriptions [sub1, sub2]. The agent is configured to have subscriptions [sub2, sub3]. The resulting merged entity would have [sub1, sub2, sub3]--however there is currently no mechanism for the agent to know it has sub1.

A more mischievous example would be:

Entity A is update via the API to have team "foo", but the agent is configured to have team "bar." This field isn't an array, so who do we trust? The configured agent or the update to the API?

I'm of the opinion that we should not allow this circumstance to happen. The configured agent should be the source of truth. Any entities with the reserved type of "agent" should be immutable via the API and their identity provided by the agent's configuration.

We should also, therefore, not

@mercul3s
Copy link
Contributor Author

OK - in this case, we shouldn't allow entities of class "agent" to be modified, only proxy entities. The API does not currently have that restriction.

@grepory
Copy link
Contributor

grepory commented May 31, 2018

I think we still have the outstanding question of "should we support this and to what extent?" I was just opining.

@calebhailey
Copy link

@mercul3s yeah, I think that's right ("proxy" class entities can be updated via sensuctl, but "agent" class entities cannot). It's not even that they "can't" be updated, it's just misleading to the user since the updates are wiped out on subsequent keepalive, right?

@annaplotkin
Copy link

This requires further investigation and subsequent discussion.

@grepory grepory changed the title Add support for merging two entities Only allow update of proxy entities via the API Nov 23, 2018
@grepory
Copy link
Contributor

grepory commented Nov 23, 2018

In the entities API, validate the entity_class field is proxy before allowing an entity create or update.

In the case that entity_class is not proxy, return a human-readable error indicating that only proxy entities are allowed to be modified via the API. This would cause sensuctl entity update to return that human readable error.

In sensuctl disambiguate proxy entities and entities, providing a command to manage proxy entities: sensuctl proxy-entity <subcommand>.

subcommand being: get, list, update, delete, etc.

@grepory
Copy link
Contributor

grepory commented Nov 23, 2018

This is going to require a documentation update, of course. Just let's not forget about it.

@grepory
Copy link
Contributor

grepory commented Nov 23, 2018

Updated description to reflect new spec.

@calebhailey calebhailey added the component:backend Sensu Backend improvements label May 5, 2020
@echlebek
Copy link
Contributor

We're going to be changing how all of this works shortly, so this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants