feature request: ovs_events output has nested double quote for summary.

aneeshkp commented 6 years ago

Version of collectd:5.8.0
Operating system / distribution:linux

Expected behavior

ovs_event plugin output for summary should be string without nested double quotes

"annotations":{"summary":"link state of br0 interface has been changed to UP" ,"uuid":"c52f2aca-3cb1-48e3-bba3-100b54303d84"}

Actual behavior

ovs_event plugin output for summary has nested double quote, which causes parsing issue when consuming by third-party applications.

"annotations":{"summary":"link state of \"br0\" interface has been changed to \"UP\"","uuid":"c52f2aca-3cb1-48e3-bba3-100b54303d84"}

/ fill the notification data / snprintf(n.message, sizeof(n.message), "link state of \"%s\" interface has been changed to \"%s\"", ifinfo->name, msg_link_status);

Steps to reproduce

Preparation OVS version - 2.5.2. OpenvSwitch configuration: Start ovs: service openvswitch-switch start Add new bridge: ovs-vsctl add-br br0 Allow connection to ovsdb-server: ovs-vsctl set-manager ptcp:6640

enable ovs_event
Interval 1 Port "6640" Address "127.0.0.1" Socket "/var/run/openvswitch/db.sock" Interfaces "br0" SendNotification true
step 2 Bring br0 down to trigger ovs_event ifconfig br0 down
step 3 Verify the output of the summary .

vmytnykx commented 6 years ago

There are few other places in collectd where nested double quotes are used in the notification message. E.g:cd collectd && grep --include="threshold.c" -rn '\\"' . Thus, I think this issue shouldn't be fixed at all. The issue should be fixed in your 3rd party application that you are using. @rubenk, @octo what do you think about this?

aneeshkp commented 6 years ago

I think it is just in two files ovs_events.c and threshold.c egrep -rn . -e '\"%s\" ' -A2 -B2 | grep snprint -A2 -B2

leifmadsen commented 5 years ago

@vmytnykx I think collectd should be liberal in what it accepts and conservative in what it sends; i.e. be as valid as possible on what is being sent rather than relying on the receiving end to implement a sanitizing function.

paramite commented 5 years ago

@vmytnykx Passing invalid json data is not correct IMO. And the fact that this bug is on more places is not a good reason why not to fix it :)

eero-t commented 9 months ago

@octo You added quoting sanitizing helpers to v6 recently, would they help here too?

octo commented 9 months ago

The code in src/ovs_events.c looks okay to me. The escaping needs to happen in the code that is wrapping the notification in JSON.

The code in src/utils/format_json/format_json.c that is responsible for the JSON formatting is also doing the right thing:

[
    {
        "labels": {
            "alertname": "collectd_unit_test",
            "instance": "example.com",
            "unit": "case",
            "severity": "WARNING",
            "service": "collectd"
        },
        "annotations": {
            "summary": "this is a \"message\""
        },
        "startsAt": "2015-11-23T13:16:46.125000000Z"
    }
]

The code was added in 30c111183bf529381359a411edf5e5b1feb6ba9e (i.e. version 5.6.0) and is largely unchanged since. I'm going to close this as not reproducible.

collectd / collectd