Icinga DB: dump the correct icinga:config:endpoint#zone_id

[Al2Klimov]

No description provided.

[Al2Klimov]

Before

$ redis-cli hgetall icinga:config:endpoint
1) "5ffd3470e558666b25ccd505c329faa0f1932596"
2) "{\"environment_id\":\"da39a3ee5e6b4b0d3255bfef95601890afd80709\",\"name\":\"Alexanders-MacBook-Pro.local\",\"name_checksum\":\"aa0db2d5375043173db4a7b4c95a919a056c8355\"}"
$

After

$ redis-cli hgetall icinga:config:endpoint
1) "5ffd3470e558666b25ccd505c329faa0f1932596"
2) "{\"environment_id\":\"da39a3ee5e6b4b0d3255bfef95601890afd80709\",\"name\":\"Alexanders-MacBook-Pro.local\",\"name_checksum\":\"aa0db2d5375043173db4a7b4c95a919a056c8355\",\"zone\":\"Alexanders-MacBook-Pro.local\",\"zone_id\":\"5ffd3470e558666b25ccd505c329faa0f1932596\"}"
$

[julianbrost]

This just looks really error-prone.

Is there any situation where it would make sense to actually set the zone attribute of an endpoint object to something else than the zone of which the endpoint is a member? Or in other words, can we make ConfigObject::GetZone() virtual (or set ConfigObject::m_Zone from Endpoint) instead?

I've skipped through all uses of GetZone() and the only other ones I could find that operate on the static type ConfigObject are the following:

• ApiListener::UpdateConfigObject():

  icinga2/lib/remote/apilistener-configsync.cpp

  Line 386 in 8dc069d

  Zone::Ptr target = static_pointer_cast<Zone>(object->GetZone());

• ApiListener::DeleteConfigObject():

  icinga2/lib/remote/apilistener-configsync.cpp

  Line 436 in 8dc069d

  Zone::Ptr target = static_pointer_cast<Zone>(object->GetZone());

• ApiListener::SyncRelayMessage():

  icinga2/lib/remote/apilistener.cpp

  Line 1235 in 8dc069d

  target_zone = static_pointer_cast<Zone>(secobj->GetZone());

• Zone::CanAccessObject():

  icinga2/lib/remote/zone.cpp

  Line 98 in 8dc069d

  object_zone = static_pointer_cast<Zone>(object->GetZone());

So it should be feasible to look over these if changing the behavior of ConfigObject::GetZone() for Endpoint objects would be a problem.

[Al2Klimov]

@lippserd What about zone in zone?

[julianbrost]

With file-based config sync this shouldn't be an issue as the config is synced based on the directory it is in. Maybe this could create issues with object-based config sync for API-created objects? I.e. if you'd add an Endpoint via the API, it would then be synced down to this endpoint where it yields an error as this already exists (as it has to be configured for the cluster to even connect)?

[lippserd]

@lippserd What about zone in zone?

I'm not entirely sure what the question is, but since configuring zone(s) in zone(s) is not an issue, Icinga 2 should dump the actual zone of the object, not the zone in which it was defined.

Does that answer your question?

[Al2Klimov]

My question: can endpoint objects also be inside zones, i.e. "non-locally"?

[lippserd]

My question: can endpoint objects also be inside zones, i.e. "non-locally"?

Yes.

[Al2Klimov]

So the Endpoint's Zone getter is actually used to determine the Zone where the Endpoint is defined.

@julianbrost So we shall not override the getter's semantics.

[julianbrost]

Ok, then we probably need a relationship with two different zones for each endpoint. I think we should then also make this explicit everywhere.

With the current state of this PR, zone_id would refer to the zone containing the object for all objects except endpoints. For these it would be the zone the endpoint is actually a member of. This would be inconsistent.

Why is the zone information that's currently dumped wrong in the first place? After all it's a zone that has a relationship to the endpoint and it's the same information that's currently returned when querying endpoint objects using the API:

$ curl -skSu root:icinga -H 'Accept: application/json' 'https://localhost:5665/v1/objects/endpoints/master-1?attrs=zone'
{"results":[{"attrs":{"zone":""},"joins":{},"meta":{},"name":"master-1","type":"Endpoint"}]}
$ curl -skSu root:icinga -H 'Accept: application/json' 'https://localhost:5665/v1/objects/endpoints/agent-1?attrs=zone'
{"results":[{"attrs":{"zone":"master"},"joins":{},"meta":{},"name":"agent-1","type":"Endpoint"}]}

I'd rather add the zone membership information as additional attributes, maybe called something like member_of_zone (I don't like this name but I have to suggest something).

In the long run (probably not as part of this PR), I'd also rename Endpoint::GetZone() accordingly so that it no longer shadows ConfigObject::GetZone().

[Al2Klimov]

@lippserd Any strong opinion on this?

[lippserd]

It should be the zone where the object is a member of.

[julianbrost]

Ok, then we're switching from the value of the zone attribute as known to Icinga to something the user hopefully expects the zone of an object to be. This should then be documented at some point (probably in some form of schema documentation, but this would fit better into the Icinga DB repo).