30 Aug
2021
30 Aug
'21
4:22 p.m.
#### Pre-Submission Checklist
- [X] Commit message has the format required by CONTRIBUTING guide
- [X] Commits are split per component (core, individual modules, libs, utils, ...)
- [X] Each component has a single commit (if not, squash them into one commit)
- [X] No commits to README files for modules (changes must be done to docbook files
in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change
- [ ] Small bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds new functionality)
- [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist:
<!-- Go over all points below, and after creating the PR, tick the checkboxes that
apply -->
- [ ] PR should be backported to stable branches
- [X] Tested changes locally
- [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
this is a tiny module which provides integration with slack over webhooks. currently it
exports only one function which accepts formatted string as an argument, evaluates
pseudo-variables if any and sends the msg to slack channel (channel name, username and
emoji are set as a module parameters). Kemi function is exported as well.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2838
-- Commit Summary --
* slack: new module, send message to slack channel
-- File Changes --
A src/modules/slack/Makefile (31)
A src/modules/slack/README (154)
A src/modules/slack/doc/Makefile (4)
A src/modules/slack/doc/slack.html (468)
A src/modules/slack/doc/slack.xml (28)
A src/modules/slack/doc/slack_admin.xml (175)
A src/modules/slack/slack.c (298)
A src/modules/slack/slack.h (58)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2838.patch
https://github.com/kamailio/kamailio/pull/2838.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2838
30 Aug
30 Aug
6:38 p.m.
@henningw commented on this pull request.
Thanks for the pull request. I just had a quick look and added a few comments, related to
parameter naming and error checking.
+ return -1;
+ }
+
+ if(strncmp(val, "https://hooks.slack.com", 23)) {
+ LM_ERR("slack invalid webhook url [%s]\n", val);
+ return -1;
+ }
+
+ // TODO: parse webhook to multiple channels? eg.: chan1=>https://AAA/BBB/CC,
chan2=>...
+
+ slack_url = (char*)pkg_malloc(len + 1);
+ if (slack_url==NULL) {
+ PKG_MEM_ERROR;
+ return -1;
+ }
+ strcpy(slack_url, val);
would be good to use strncpy, as you already have the len.
+ return;
+}
+
+/**
+ * send message with curl
+ * @return 0 on success, -1 on error
+ */
+static int _curl_send(const char* uri, str *post_data)
+{
+ int datasz;
+ char* send_data;
+ CURL *curl_handle;
+ CURLcode res;
+ // LM_DBG("sending to[%s]\n", uri);
+
+ datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s,
slack_icon);
snprintf could fail, so you should check for error (negative return value).
+ */
+static int _curl_send(const char* uri, str
*post_data)
+{
+ int datasz;
+ char* send_data;
+ CURL *curl_handle;
+ CURLcode res;
+ // LM_DBG("sending to[%s]\n", uri);
+
+ datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s,
slack_icon);
+ send_data = (char*)pkg_malloc((datasz+1)*sizeof(char));
+ if(send_data==NULL) {
+ LM_ERR("Error: can not allocate pkg memory [%d] bytes\n", datasz);
+ return -1;
+ }
+ snprintf(send_data, datasz+1, BODY_FMT, slack_channel, slack_username,
post_data->s, slack_icon);
see above
+ <section id="slack.p.icon_emogi">
+ <title><varname>icon_emogi</varname> (str)</title>
+ <para>
+ specify an emoji (using colon shortcodes, eg. :white_check_mark:)
+ to use as the profile photo alongside the message.
+ </para>
+ <para>
+ <emphasis>
+ Default value is :ghost:
+ </emphasis>
+ </para>
+ <example>
+ <title>Set <varname>icon_emogi</varname> parameter</title>
+ <programlisting format="linespecific">
+...
+modparam("slack", "icon_emogi", ":ghost:")
I think the parameter _icon_emogi_ is mispelled, i think it should be named _icon_emoji_.
Would be good to fix this in code and docs.
+ * Exported functions
+ */
+static cmd_export_t cmds[] = {
+ {"slack_send", (cmd_function)slack_send1, 1, slack_fixup, 0,
ANY_ROUTE},
+ {0, 0, 0, 0, 0, 0}
+};
+
+
+/**
+ * Exported parameters
+ */
+static param_export_t mod_params[] = {
+ { "slack_url", PARAM_STRING|USE_FUNC_PARAM, (void*)_slack_url_param},
+ { "channel", PARAM_STRING, &slack_channel }, // channel starts with #
+ { "username", PARAM_STRING, &slack_username },
+ { "icon_emogi", PARAM_STRING, &slack_icon },
I think the parameter _icon_emogi_ is mispelled, i think it should be named _icon_emoji_.
Would be good to fix this in code and docs.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2838#pullrequestreview-741842887
31 Aug
31 Aug
7:57 a.m.
@arsperger pushed 1 commit.
3438be7b095453d129babba1eb92936f2be63cd9 slack: fix param typo; strncpy, snprintf error
check; removed pre-built html doc
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2838/files/b7c95fa59f1ffcf3aabc48…
8:08 a.m.
@arsperger commented on this pull request.
+ */
+static int _curl_send(const char* uri, str
*post_data)
+{
+ int datasz;
+ char* send_data;
+ CURL *curl_handle;
+ CURLcode res;
+ // LM_DBG("sending to[%s]\n", uri);
+
+ datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s,
slack_icon);
+ send_data = (char*)pkg_malloc((datasz+1)*sizeof(char));
+ if(send_data==NULL) {
+ LM_ERR("Error: can not allocate pkg memory [%d] bytes\n", datasz);
+ return -1;
+ }
+ snprintf(send_data, datasz+1, BODY_FMT, slack_channel, slack_username,
post_data->s, slack_icon);
thank you for your notes. I am wondering, if the above snprintf check is passed, what
could be a reason for this one to fail? do we really need to error check this one?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2838#discussion_r699009988
9:16 a.m.
@henningw commented on this pull request.
+ */
+static int _curl_send(const char* uri, str
*post_data)
+{
+ int datasz;
+ char* send_data;
+ CURL *curl_handle;
+ CURLcode res;
+ // LM_DBG("sending to[%s]\n", uri);
+
+ datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s,
slack_icon);
+ send_data = (char*)pkg_malloc((datasz+1)*sizeof(char));
+ if(send_data==NULL) {
+ LM_ERR("Error: can not allocate pkg memory [%d] bytes\n", datasz);
+ return -1;
+ }
+ snprintf(send_data, datasz+1, BODY_FMT, slack_channel, slack_username,
post_data->s, slack_icon);
This of course depends on the working of snprintf, the docs unfortunately do not give much
more details. But generally it is good "defensive" programming practice to check
for errors on outside library calls that you do not control/own.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2838#discussion_r699048847
9:36 a.m.
We have too many modules who call the curl library, which can lead to issues with the TLS
initialisation. This is why we created an API in the curl module. Please look into using
that API instead of calling the curl library directly.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2838#issuecomment-908978800
10:14 a.m.
The snprintf() for getting datasz should be checked, because can fail on unexpected
characters - from the manual:
```
[EILSEQ] An invalid wide character code was encountered.
```
That will make it safer when wanting to send data taken from SIP traffic (e.g., body of
requests).
On negative return code, likely pkg_malloc() will fail, requested size being big.
The second snprintf() is unlikely to fail, but I would use pkg_mallocxz() or set
send_data[0] = '\0' before it so in case it fails for what so ever unknown reason,
then the CURL library doesn't get a buffer with garbage and does not go beyond
allocated buffer to find the '\0'.
For reusing http_client, it would be good to review all modules. Can be done for this one
also after merging, it is easier to work directly in repo than as PR.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2838#issuecomment-909005630
12:56 p.m.
@arsperger pushed 1 commit.
4d71d13d02afdc51865f44669874ec95b538de7d slack: allocate zeroed memory for send_data
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2838/files/3438be7b095453d129babb…
1 Sep
1 Sep
12:32 p.m.
Thanks @arsperger! I am going to merge the PR and invite you to get commit access so you
can develop and maintain the module in Kamailio repo. You can still do pull requests for
this module, if you prefer it.
If you want to push code to other components or module, it is highly recommended to do
pull requests, so other developers can review and be sure there is no conflict with
ongoing work on those components.
Do consider using the API provided by http_client module as suggested by @oej -- we should
do it for the other modules as well.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2838#issuecomment-910154089
12:33 p.m.
Merged #2838 into master.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2838#event-5238610125
990
days inactive
992
days old
9 comments
4 participants
participants (4)
-
Arsen Semenov -
Daniel-Constantin Mierla
-
Henning Westerholt
-
Olle E. Johansson