Introduce Event Notification Manager #645
Conversation
A new mechanism is introduced for generating events. The existing
socket-based notifications are enhanced to include file-based
notifications. The configuration of socket vs. file based notifies
is covered by the addition of a notification "mode" field in the
"notif" section, along with the path:
"notif": {
"enabled": true,
"mode": "file",
"events-dir": "/var/lib/opflex-agent-ovs/events"
}
The agent will look for files in this directory with .subscription
suffixes, and when present, will parse them to look for event
notification requests. The subscriptions file will use JSON
syntax to describe the events for notifications. For example:
{
"uuid": "2f5cd01d-7a7c-4156-b2cd-42565b33a52d",
"time-zone": "UTC",
"time-format": "UNUIX",
"subscriptions": [
{
"type": "class",
"state": "deleted",
"subject": "PlatformConfig"
}
]
}
This requests that the PlatformConfig MO's "deleted" event
is subscribed to. Supported types are "uri" or "class", for
either class-based or instance-based notifications. The state
can be "created", "updated", "deleted", or "any". The subject
is the MO class requested. An optional "uri" field must be
provided for instance-based subscriptions.
File-based notifications are profided in .notifications files
that have matching prefixes (e.g. "foo.subscriptions" would
generate a "foo.notifictions" file). The file is JSON formatted,
with content like the following:
{
"uuid": "unique-subscription-id",
"events": [
{
"uri": "PolicyUniverse/PlatformConfig/example-config/",
"timestamp": "2024-01-15 10:30:45.123",
"state": "deleted"
}
]
}
The timestamp field uses the time zone and format specified
in the .subscriptions file. The time zones supported are "UTC",
and "localtime" (default). The time formats supported include
"ISO8601", "RFC3339", and "UNIX".
The notifidation files are not historical - they are always
overwrititen using the most recently generated event.
The manager currently only supports the PlatformConfig class,
with the "deleted" state, but could be extended to support any
MO. A future patch should add support for the same subscription
mechanism and notifications via the notification socket. A future
patch should also include an interface class, which clients would
implement as callbacks, along with regisgtering with the manager
class to request those events.
(cherry picked from commit 9eaee8b)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| document.AddMember("events", events, allocator); | ||
|
|
||
| FILE* fp = fopen(filePath.string().c_str(), "w"); |
Check failure
Code scanning / CodeQL
File created without restricting permissions High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
The best way to fix this problem is to ensure that the file is created with restrictive permissions (user read/write only, i.e., 0600). Since fopen does not allow specifying permissions, the typical solution is to use the open() system call with flags O_WRONLY | O_CREAT | O_TRUNC and mode S_IRUSR | S_IWUSR, which ensures only the file's owner can read/write. The resulting file descriptor can be converted to a FILE* using fdopen so that existing code (which expects a FILE*) works without major changes. We only need to include <fcntl.h> and <sys/stat.h> for the correct flags and mode macros.
In summary:
- Include
<fcntl.h>and<sys/stat.h>up top if not already present. - In
writeNotificationFile, replacefopen(..., "w")with:- Use
open()with modeS_IRUSR | S_IWUSR(0600) - Use
fdopen()to convert the descriptor to aFILE*
- Use
- Handle errors and cleanup accordingly.
| @@ -25,6 +25,9 @@ | ||
| #include <iomanip> | ||
| #include <chrono> | ||
|
|
||
| #include <fcntl.h> | ||
| #include <sys/stat.h> | ||
|
|
||
| namespace opflexagent { | ||
|
|
||
| using std::string; | ||
| @@ -300,11 +303,18 @@ | ||
| } | ||
| document.AddMember("events", events, allocator); | ||
|
|
||
| FILE* fp = fopen(filePath.string().c_str(), "w"); | ||
| if (!fp) { | ||
| int fd = open(filePath.string().c_str(), O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR); | ||
| if (fd < 0) { | ||
| LOG(ERROR) << "Could not open notification file for writing: " << filePath; | ||
| return; | ||
| } | ||
|
|
||
| FILE* fp = fdopen(fd, "w"); | ||
| if (!fp) { | ||
| LOG(ERROR) << "Could not get FILE* from file descriptor: " << filePath; | ||
| close(fd); | ||
| return; | ||
| } | ||
|
|
||
| char writeBuffer[65536]; | ||
| rapidjson::FileWriteStream os(fp, writeBuffer, sizeof(writeBuffer)); |
| // Handle timezone - for now, support UTC and local time | ||
| std::tm* timeinfo; | ||
| if (timeZone && timeZone.get() == "UTC") { | ||
| timeinfo = std::gmtime(&time_t); |
Check failure
Code scanning / CodeQL
Use of potentially dangerous function Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the error, replace the call to std::gmtime (and symmetrically, std::localtime as well for consistency) with their thread-safe versions: std::gmtime_r and std::localtime_r. These are POSIX functions, not standard C++, but widely supported in Unix-like environments. With these, the caller provides their own struct tm buffer, eliminating the shared static data issue.
Concretely:
- Define a local
std::tmvariable, for example,std::tm timeinfo_buf;. - For the UTC option ("UTC" timezone), call
gmtime_r(&time_t, &timeinfo_buf). - For local time, call
localtime_r(&time_t, &timeinfo_buf). - Assign the returned pointer (
timeinfo = &timeinfo_buf;) so that further logic does not change. - No new imports or dependencies are necessary as
time.his implied in standard C++/C, and POSIX platforms provide the needed functions.
All changes are to be made within the function EventNotificationManager::getCurrentTimestamp in agent-ovs/lib/EventNotificationManager.cpp, lines 382–387.
| @@ -379,11 +379,12 @@ | ||
| } | ||
|
|
||
| // Handle timezone - for now, support UTC and local time | ||
| std::tm timeinfo_buf; | ||
| std::tm* timeinfo; | ||
| if (timeZone && timeZone.get() == "UTC") { | ||
| timeinfo = std::gmtime(&time_t); | ||
| timeinfo = gmtime_r(&time_t, &timeinfo_buf); | ||
| } else { | ||
| timeinfo = std::localtime(&time_t); | ||
| timeinfo = localtime_r(&time_t, &timeinfo_buf); | ||
| } | ||
|
|
||
| ss << std::put_time(timeinfo, format.c_str()); |
| if (timeZone && timeZone.get() == "UTC") { | ||
| timeinfo = std::gmtime(&time_t); | ||
| } else { | ||
| timeinfo = std::localtime(&time_t); |
Check failure
Code scanning / CodeQL
Use of potentially dangerous function Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix this problem, replace the call to the dangerous std::localtime function with the thread-safe localtime_r variant. localtime_r takes an additional parameter: a pointer to a user-provided struct tm buffer, where the result will be stored, thus avoiding any shared static memory.
Specifically, within EventNotificationManager::getCurrentTimestamp, replace:
std::tm* timeinfo;
if (timeZone && timeZone.get() == "UTC") {
timeinfo = std::gmtime(&time_t);
} else {
timeinfo = std::localtime(&time_t);
}with:
std::tm timeinfo;
if (timeZone && timeZone.get() == "UTC") {
gmtime_r(&time_t, &timeinfo);
} else {
localtime_r(&time_t, &timeinfo);
}Then, update the rest of the function to use &timeinfo instead of timeinfo.
No additional imports are required as <ctime> is transitively available, and these functions are part of the POSIX standard. However, this change is specific to POSIX systems (Linux, macOS, etc). If Windows compatibility is necessary, a conditional compilation block would be needed, but the current code appears to target POSIX platforms.
| @@ -379,14 +379,14 @@ | ||
| } | ||
|
|
||
| // Handle timezone - for now, support UTC and local time | ||
| std::tm* timeinfo; | ||
| std::tm timeinfo; | ||
| if (timeZone && timeZone.get() == "UTC") { | ||
| timeinfo = std::gmtime(&time_t); | ||
| gmtime_r(&time_t, &timeinfo); | ||
| } else { | ||
| timeinfo = std::localtime(&time_t); | ||
| localtime_r(&time_t, &timeinfo); | ||
| } | ||
|
|
||
| ss << std::put_time(timeinfo, format.c_str()); | ||
| ss << std::put_time(&timeinfo, format.c_str()); | ||
|
|
||
| if (includeMs && timeFormat && (timeFormat.get() == "ISO8601" || timeFormat.get() == "RFC3339")) { | ||
| ss << '.' << std::setfill('0') << std::setw(3) << ms.count(); |
mchalla
left a comment
mchalla
left a comment
There was a problem hiding this comment.
Some minor comments, at least fix the typo and log some errors where something needs to be logged for further debugging.
| subscription.state = SubscriptionState::ANY; | ||
| } | ||
|
|
||
| if (sub.HasMember("uri") && subscription.type == SubscrationType::URI) { |
There was a problem hiding this comment.
typo SubscrationType should be SubscriptionType
| EventNotificationManager::parseNotificationFile(const fs::path& filePath) { | ||
| std::ifstream file(filePath.string()); | ||
| if (!file.is_open()) { | ||
| return nullptr; |
There was a problem hiding this comment.
Actually, this isn't needed any more - will just delete this method.
| document.Parse(content.c_str()); | ||
|
|
||
| if (document.HasParseError() || !document.IsObject() || | ||
| !document.HasMember("uuid") || !document.HasMember("events")) { |
There was a problem hiding this comment.
Not needed now that this method is gone.
| void EventNotificationManager::deleted(const fs::path& filePath) { | ||
| string filename = filePath.filename().string(); | ||
|
|
||
| if (filename.size() < 14 || filename.substr(filename.size() - 14) != ".subscriptions") { |
| auto rit = elements.rbegin(); | ||
| for (; rit != elements.rend(); ++rit) { | ||
| if ((*rit) == subscription.subject) { | ||
| return true; |
There was a problem hiding this comment.
This loop / compare can be expensive for events that can happen frequently like policy events. we should think of a better way in future. but works ok for platform config delete that should be a rare event.
There was a problem hiding this comment.
Ack - we ideally would get the MO in the notification... but for delete events, there isn't an MO, but only the URI, which is why we did this. That said, I'll change this to a boost implementation.
There was a problem hiding this comment.
The boost method I was looking actually isn't any better - the above will work for now. Ideally we'd have a method in the URI class itself for something like this (i.e. seems reasonable that the URI should be able to know the type/class of the MO referenced).
| void EventNotificationManager::updated(const fs::path& filePath) { | ||
| string filename = filePath.filename().string(); | ||
|
|
||
| if (filename.size() < 14 || filename.substr(filename.size() - 14) != ".subscriptions") { |
There was a problem hiding this comment.
perhaps simpler if (filePath.extension() != ".subscriptions") return;
There was a problem hiding this comment.
Kept this consistent with the other check - added a log message.
|
Closing this in favor of updated PR. |
A new mechanism is introduced for generating events. The existing socket-based notifications are enhanced to include file-based notifications. The configuration of socket vs. file based notifies is covered by the addition of a notification "mode" field in the "notif" section, along with the path:
The agent will look for files in this directory with .subscription suffixes, and when present, will parse them to look for event notification requests. The subscriptions file will use JSON syntax to describe the events for notifications. For example:
{
"uuid": "2f5cd01d-7a7c-4156-b2cd-42565b33a52d",
"time-zone": "UTC",
"time-format": "UNUIX",
"subscriptions": [
{
"type": "class",
"state": "deleted",
"subject": "PlatformConfig"
}
]
}
This requests that the PlatformConfig MO's "deleted" event is subscribed to. Supported types are "uri" or "class", for either class-based or instance-based notifications. The state can be "created", "updated", "deleted", or "any". The subject is the MO class requested. An optional "uri" field must be provided for instance-based subscriptions.
File-based notifications are profided in .notifications files that have matching prefixes (e.g. "foo.subscriptions" would generate a "foo.notifictions" file). The file is JSON formatted, with content like the following:
{
"uuid": "unique-subscription-id",
"events": [
{
"uri": "PolicyUniverse/PlatformConfig/example-config/",
"timestamp": "2024-01-15 10:30:45.123",
"state": "deleted"
}
]
}
The timestamp field uses the time zone and format specified in the .subscriptions file. The time zones supported are "UTC", and "localtime" (default). The time formats supported include "ISO8601", "RFC3339", and "UNIX".
The notifidation files are not historical - they are always overwrititen using the most recently generated event.
The manager currently only supports the PlatformConfig class, with the "deleted" state, but could be extended to support any MO. A future patch should add support for the same subscription mechanism and notifications via the notification socket. A future patch should also include an interface class, which clients would implement as callbacks, along with regisgtering with the manager class to request those events.
(cherry picked from commit 9eaee8b)