Add package parameter in object creation API #8640
Conversation
mcodato
force-pushed
the
feature/api-parametrized-package-8639
branch
2 times, most recently
from
614efe4 to
a579f75
Compare
mcodato
force-pushed
the
feature/api-parametrized-package-8639
branch
from
a579f75 to
5aacae7
Compare
|
@lippserd Are we definitively gonna support this (so shall I review it)? |
Yes, please! |
There was a problem hiding this comment.
It looks like validation of the package name is missing completely (should use ConfigPackageUtility::ValidateName()).
Have you considered what happens when Icinga 2 is restarted after this was used but without deploying a new config? I haven't tried but given that this attempts to update config files if they happen to match the file structure usually present in the _api package, I have the feeling that this might lead to inconsistencies.
| if (object->GetPackage() == "_api") { | ||
| String file; | ||
|
|
||
| try { | ||
| file = ConfigObjectUtility::GetObjectConfigPath(object->GetReflectionType(), object->GetName()); | ||
| } catch (const std::exception& ex) { | ||
| Log(LogNotice, "ApiListener") | ||
| << "Cannot sync object '" << object->GetName() << "': " << ex.what(); | ||
| return; | ||
| } | ||
| String file; | ||
|
|
||
| std::ifstream fp(file.CStr(), std::ifstream::binary); | ||
| if (!fp) | ||
| return; | ||
| try { | ||
| file = ConfigObjectUtility::GetObjectConfigPath(object->GetReflectionType(), object->GetName(), | ||
| object->GetPackage()); | ||
| } catch (const std::exception& ex) { | ||
| Log(LogNotice, "ApiListener") | ||
| << "Cannot sync object '" << object->GetName() << "': " << ex.what(); | ||
| return; | ||
| } | ||
|
|
||
| std::ifstream fp(file.CStr(), std::ifstream::binary); | ||
| if (fp) { | ||
| String content((std::istreambuf_iterator<char>(fp)), std::istreambuf_iterator<char>()); | ||
| params->Set("config", content); | ||
| } |
There was a problem hiding this comment.
I don't understand this change. If the object is not the _api package, there's no guarantee that this file exists anyways. And if it's in the _api package, this changes the behavior when there's an error opening the file.
There was a problem hiding this comment.
With this change we support objects created via the API and objects created via the Director.
The config is used here in ConfigUpdateObjectAPIHandler in conjunction with the object to figure out if it is a new object.
If it is a new object created via API, the file will be present. If the object was created with the Director, when it is updated via API the file will not be found (the director uses different filenames) but the object will not be empty so the changes will be applied.
For sure if the file exists and an error occurs while reading, it will no longer fail here but fail here.
Do you suggest adding something like this in order to keep the same behavior for the _api package?
std::ifstream fp(file.CStr(), std::ifstream::binary);
if (fp) {
String content((std::istreambuf_iterator<char>(fp)), std::istreambuf_iterator<char>());
params->Set("config", content);
} else if (!fp && object->GetPackage() == "_api") {
return;
}
5aacae7 to
889d819
Compare
lib/remote/modifyobjecthandler.cpp
Outdated
| for (const Dictionary::Pair& kv : attrs) { | ||
| key = kv.first; | ||
| obj->ModifyAttribute(kv.first, kv.second); | ||
| if (obj->GetPackage() != package) { |
There was a problem hiding this comment.
Hi @julianbrost , @Al2Klimov
we found the following problem with this check:
- a host is created via the API in the package "director"
- then it is edited via the icingaweb2 page (Overview -> Hosts -> Feature Commands)
- the change is not applied because the code enters in this if
One possible solution is to validate the package only when the package parameter is specified in the API request.
Does this make sense?
mcodato
force-pushed
the
feature/api-parametrized-package-8639
branch
from
889d819 to
9e532e1
Compare
mcodato
force-pushed
the
feature/api-parametrized-package-8639
branch
2 times, most recently
from
775af2f to
3d953ea
Compare
Allow deletion via API of objects belonging to any package refs Icinga#8639
mcodato
force-pushed
the
feature/api-parametrized-package-8639
branch
from
3d953ea to
818cf21
Compare
There was a problem hiding this comment.
In general, as this is quite a heavy change to some core functionality, I think this will probably need quite some more work, especially in testing.
It looks like this PR is interacting with modified attributes (i.e. runtime updates for non-API-created objects) in a strange way, see the inline comments for more details.
| @@ -266,12 +271,6 @@ Value ApiListener::ConfigDeleteObjectAPIHandler(const MessageOrigin::Ptr& origin | |||
| return Empty; | |||
| } | |||
|
|
|||
| if (object->GetPackage() != "_api") { | |||
There was a problem hiding this comment.
Instead of removing this check completely, can the package instead be added to the cluster message and then be checked here, that is indeed the expected package?
| String package = "_api"; | ||
| if (params->Contains("package")) | ||
| package = params->Get("package"); |
There was a problem hiding this comment.
This value is only used as a parameter for ConfigObjectUtility::CreateObject(). I think it might also be a good idea to verify that the object is actually in the expected package in case of object updates.
| if (params->Contains("package")) { | ||
| package = HttpUtility::GetLastParameter(params, "package"); | ||
|
|
||
| if (!ConfigPackageUtility::ValidatePackageName(package) || package.GetData().at(0) == '_') { |
There was a problem hiding this comment.
Needs #include "remote/configpackageutility.hpp" (only gives an error when building with -DICINGA2_UNITY_BUILD=OFF)
| if (package == Empty) { | ||
| package = "_api"; | ||
| } else { | ||
| if (!ConfigPackageUtility::ValidatePackageName(package) || package.GetData().at(0) == '_') { |
There was a problem hiding this comment.
Needs #include "remote/configpackageutility.hpp" (only gives an error when building with -DICINGA2_UNITY_BUILD=OFF)
| if (params->Contains("package")) { | ||
| package = HttpUtility::GetLastParameter(params, "package"); | ||
|
|
||
| if (!ConfigPackageUtility::ValidatePackageName(package) || package.GetData().at(0) == '_') { |
There was a problem hiding this comment.
Needs #include "remote/configpackageutility.hpp" (only gives an error when building with -DICINGA2_UNITY_BUILD=OFF)
| if (object->GetPackage() != "_api" && object->GetVersion() == 0) | ||
| return; |
There was a problem hiding this comment.
With this PR, Icinga now sends loads of additional config update messages for all packages on startup, like this one:
debug/ApiListener: Sent update for object 'satellite-b-2!infrequent-check!dummy-service-notification': {"modified_attributes":{},"name":"satellite-b-2!infrequent-check!dummy-service-notification","original_attributes":[],"package":"_etc","type":"Notification","version":0,"zone":"master"}
I think this happens due to this check just being removed.
|
As discussed with @lippserd, this PR is no more needed and can be discarded. |
Hi, this is our proposed implementation for #8639
Test: create an object in a custom package
Request
Response
Logs
Test: delete an object of a custom package
Request
Response
Logs