Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

data_dog: partially revert recent datadog PR to avoid provider ecs segfault #6785

Open
wants to merge 1 commit into
base: 1.9
Choose a base branch
from
Open

data_dog: partially revert recent datadog PR to avoid provider ecs segfault #6785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 5 additions & 24 deletions plugins/out_datadog/datadog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ static int datadog_format(struct flb_config *config,
return -1;
}
} else if (flb_sds_len(remapped_tags) < byte_cnt) {
tmp = flb_sds_increase(remapped_tags, flb_sds_len(remapped_tags) - byte_cnt);
tmp = flb_sds_increase(remapped_tags, byte_cnt - flb_sds_len(remapped_tags));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the data buffer resize fix from here: #6570

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, the 2nd arg to flb_sds_increase is the new size, so shouldn't that be byte_cnt? And we shouldn't subtract anything from it?

if (!tmp) {
flb_errno();
flb_sds_destroy(remapped_tags);
Expand Down Expand Up @@ -254,11 +254,8 @@ static int datadog_format(struct flb_config *config,
* (so they won't be packed as attr)
*/
if (ctx->remap && (ind = dd_attr_need_remapping(k, v)) >=0 ) {
ret = remapping[ind].remap_to_tag(remapping[ind].remap_tag_name, v,
&remapped_tags);
if (ret < 0) {
flb_plg_error(ctx->ins, "Failed to remap tag: %s, skipping", remapping[ind].remap_tag_name);
}
remapping[ind].remap_to_tag(remapping[ind].remap_tag_name, v,
remapped_tags);
continue;
}

Expand All @@ -280,25 +277,9 @@ static int datadog_format(struct flb_config *config,
/* here we concatenate ctx->dd_tags and remapped_tags, depending on their presence */
if (remap_cnt) {
if (ctx->dd_tags != NULL) {
tmp = flb_sds_cat(remapped_tags, FLB_DATADOG_TAG_SEPERATOR,
strlen(FLB_DATADOG_TAG_SEPERATOR));
if (!tmp) {
flb_errno();
flb_sds_destroy(remapped_tags);
msgpack_sbuffer_destroy(&mp_sbuf);
msgpack_unpacked_destroy(&result);
return -1;
}
remapped_tags = tmp;
flb_sds_cat(remapped_tags, FLB_DATADOG_TAG_SEPERATOR,
strlen(FLB_DATADOG_TAG_SEPERATOR));
flb_sds_cat(remapped_tags, ctx->dd_tags, strlen(ctx->dd_tags));
if (!tmp) {
flb_errno();
flb_sds_destroy(remapped_tags);
msgpack_sbuffer_destroy(&mp_sbuf);
msgpack_unpacked_destroy(&result);
return -1;
}
remapped_tags = tmp;
}
dd_msgpack_pack_key_value_str(&mp_pck,
FLB_DATADOG_DD_TAGS_KEY,
Expand Down
21 changes: 4 additions & 17 deletions plugins/out_datadog/datadog_conf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ struct flb_out_datadog *flb_datadog_conf_create(struct flb_output_instance *ins,
struct flb_upstream *upstream;
const char *api_key;
const char *tmp;
flb_sds_t tmp_sds;

int ret;
char *protocol = NULL;
Expand Down Expand Up @@ -76,18 +75,12 @@ struct flb_out_datadog *flb_datadog_conf_create(struct flb_output_instance *ins,
/* use TLS ? */
if (ins->use_tls == FLB_TRUE) {
io_flags = FLB_IO_TLS;
tmp_sds = flb_sds_create("https://");
ctx->scheme = flb_sds_create("https://");
}
else {
io_flags = FLB_IO_TCP;
tmp_sds = flb_sds_create("http://");
ctx->scheme = flb_sds_create("http://");
}
if (!tmp_sds) {
flb_errno();
flb_datadog_conf_destroy(ctx);
return NULL;
}
ctx->scheme = tmp_sds;
flb_plg_debug(ctx->ins, "scheme: %s", ctx->scheme);

/* configure URI */
Expand Down Expand Up @@ -133,17 +126,11 @@ struct flb_out_datadog *flb_datadog_conf_create(struct flb_output_instance *ins,

/* Get network configuration */
if (!ins->host.name) {
tmp_sds = flb_sds_create(FLB_DATADOG_DEFAULT_HOST);
ctx->host = flb_sds_create(FLB_DATADOG_DEFAULT_HOST);
}
else {
tmp_sds = flb_sds_create(ins->host.name);
}
if (!tmp_sds) {
flb_errno();
flb_datadog_conf_destroy(ctx);
return NULL;
ctx->host = flb_sds_create(ins->host.name);
}
ctx->host = tmp_sds;
flb_plg_debug(ctx->ins, "host: %s", ctx->host);

if (ins->host.port != 0) {
Expand Down
145 changes: 31 additions & 114 deletions plugins/out_datadog/datadog_remap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,172 +28,98 @@ const char *ECS_ARN_PREFIX = "arn:aws:ecs:";
const char *ECS_CLUSTER_PREFIX = "cluster/";
const char *ECS_TASK_PREFIX = "task/";

static int dd_remap_append_kv_to_ddtags(const char *key,
const char *val, size_t val_len, flb_sds_t *dd_tags_buf)
static void dd_remap_append_kv_to_ddtags(const char *key,
const char *val, size_t val_len, flb_sds_t dd_tags)
{
flb_sds_t tmp;

if (flb_sds_len(*dd_tags_buf) != 0) {
tmp = flb_sds_cat(*dd_tags_buf, FLB_DATADOG_TAG_SEPERATOR, strlen(FLB_DATADOG_TAG_SEPERATOR));
if (!tmp) {
flb_errno();
return -1;
}
*dd_tags_buf = tmp;
if (flb_sds_len(dd_tags) != 0) {
flb_sds_cat(dd_tags, FLB_DATADOG_TAG_SEPERATOR, strlen(FLB_DATADOG_TAG_SEPERATOR));
}

tmp = flb_sds_cat(*dd_tags_buf, key, strlen(key));
if (!tmp) {
flb_errno();
return -1;
}
*dd_tags_buf = tmp;

tmp = flb_sds_cat(*dd_tags_buf, ":", 1);
if (!tmp) {
flb_errno();
return -1;
}
*dd_tags_buf = tmp;

tmp = flb_sds_cat(*dd_tags_buf, val, val_len);
if (!tmp) {
flb_errno();
return -1;
}
*dd_tags_buf = tmp;

return 0;
flb_sds_cat(dd_tags, key, strlen(key));
flb_sds_cat(dd_tags, ":", 1);
flb_sds_cat(dd_tags, val, val_len);
}

/* default remapping: just move the key/val pair under dd_tags */
static int dd_remap_move_to_tags(const char *tag_name,
msgpack_object attr_value, flb_sds_t *dd_tags_buf)
static void dd_remap_move_to_tags(const char *tag_name,
msgpack_object attr_value, flb_sds_t dd_tags)
{
return dd_remap_append_kv_to_ddtags(tag_name, attr_value.via.str.ptr,
attr_value.via.str.size, dd_tags_buf);
dd_remap_append_kv_to_ddtags(tag_name, attr_value.via.str.ptr,
attr_value.via.str.size, dd_tags);
}

/* remapping function for container_name */
static int dd_remap_container_name(const char *tag_name,
msgpack_object attr_value, flb_sds_t *dd_tags_buf)
static void dd_remap_container_name(const char *tag_name,
msgpack_object attr_value, flb_sds_t dd_tags)
Comment on lines -78 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help me understand again, why are we reverting these changes which probably still are a real fix? Also if we are going to revert them, why not put it in a revert commit? Why same commit as this new fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the individual parts of the datadog pr and it turns out that completely unrelated error handling code was triggering a segfault somewhere random in the code (like on a network call).

We decided that this PR would keep only the essential portions of the recent PR and ditch the lower priority ones to avoid adding the segfault

{
/* remove the first / if present */
unsigned int adjust;
flb_sds_t buf = NULL;
int ret;
flb_sds_t buf;

adjust = attr_value.via.str.ptr[0] == '/' ? 1 : 0;
buf = flb_sds_create_len(attr_value.via.str.ptr + adjust,
attr_value.via.str.size - adjust);
if (!buf) {
flb_errno();
return -1;
}
ret = dd_remap_append_kv_to_ddtags(tag_name, buf, strlen(buf), dd_tags_buf);
dd_remap_append_kv_to_ddtags(tag_name, buf, strlen(buf), dd_tags);
flb_sds_destroy(buf);
if (ret < 0) {
return -1;
}

return 0;
}

/* remapping function for ecs_cluster */
static int dd_remap_ecs_cluster(const char *tag_name,
msgpack_object attr_value, flb_sds_t *dd_tags_buf)
static void dd_remap_ecs_cluster(const char *tag_name,
msgpack_object attr_value, flb_sds_t dd_tags)
{
flb_sds_t buf = NULL;
flb_sds_t buf;
char *cluster_name;
int ret;

buf = flb_sds_create_len(attr_value.via.str.ptr, attr_value.via.str.size);
if (!buf) {
flb_errno();
return -1;
}
cluster_name = strstr(buf, ECS_CLUSTER_PREFIX);

if (cluster_name != NULL) {
cluster_name += strlen(ECS_CLUSTER_PREFIX);
ret = dd_remap_append_kv_to_ddtags(tag_name, cluster_name, strlen(cluster_name), dd_tags_buf);
if (ret < 0) {
flb_sds_destroy(buf);
return -1;
}
dd_remap_append_kv_to_ddtags(tag_name, cluster_name, strlen(cluster_name), dd_tags);
}
else {
/*
* here the input is invalid: not in form of "XXXXXXcluster/"cluster-name
* we preverse the original value under tag "cluster_name".
*/
ret = dd_remap_append_kv_to_ddtags(tag_name, buf, strlen(buf), dd_tags_buf);
if (ret < 0) {
flb_sds_destroy(buf);
return -1;
}
dd_remap_append_kv_to_ddtags(tag_name, buf, strlen(buf), dd_tags);
}
flb_sds_destroy(buf);
return 0;
}

/* remapping function for ecs_task_definition */
static int dd_remap_ecs_task_definition(const char *tag_name,
msgpack_object attr_value, flb_sds_t *dd_tags_buf)
static void dd_remap_ecs_task_definition(const char *tag_name,
msgpack_object attr_value, flb_sds_t dd_tags)
{
flb_sds_t buf = NULL;
flb_sds_t buf;
char *split;
int ret;

buf = flb_sds_create_len(attr_value.via.str.ptr, attr_value.via.str.size);
if (!buf) {
flb_errno();
return -1;
}
split = strchr(buf, ':');

if (split != NULL) {
ret = dd_remap_append_kv_to_ddtags("task_family", buf, split-buf, dd_tags_buf);
if (ret < 0) {
flb_sds_destroy(buf);
return -1;
}
ret = dd_remap_append_kv_to_ddtags("task_version", split+1, strlen(split+1), dd_tags_buf);
if (ret < 0) {
flb_sds_destroy(buf);
return -1;
}
dd_remap_append_kv_to_ddtags("task_family", buf, split-buf, dd_tags);
dd_remap_append_kv_to_ddtags("task_version", split+1, strlen(split+1), dd_tags);
}
else {
/*
* here the input is invalid: not in form of task_name:task_version
* we preverse the original value under tag "ecs_task_definition".
*/
ret = dd_remap_append_kv_to_ddtags(tag_name, buf, strlen(buf), dd_tags_buf);
if (ret < 0) {
flb_sds_destroy(buf);
return -1;
}
dd_remap_append_kv_to_ddtags(tag_name, buf, strlen(buf), dd_tags);
}
flb_sds_destroy(buf);
return 0;
}

/* remapping function for ecs_task_arn */
static int dd_remap_ecs_task_arn(const char *tag_name,
msgpack_object attr_value, flb_sds_t *dd_tags_buf)
static void dd_remap_ecs_task_arn(const char *tag_name,
msgpack_object attr_value, flb_sds_t dd_tags)
{
flb_sds_t buf;
char *remain;
char *split;
char *task_arn;
int ret;

buf = flb_sds_create_len(attr_value.via.str.ptr, attr_value.via.str.size);
if (!buf) {
flb_errno();
return -1;
}

/*
* if the input is invalid, not in the form of "arn:aws:ecs:region:XXXX"
Expand All @@ -206,33 +132,24 @@ static int dd_remap_ecs_task_arn(const char *tag_name,
split = strchr(remain, ':');

if (split != NULL) {
ret = dd_remap_append_kv_to_ddtags("region", remain, split-remain, dd_tags_buf);
if (ret < 0) {
flb_sds_destroy(buf);
return -1;
}
dd_remap_append_kv_to_ddtags("region", remain, split-remain, dd_tags);
}
}

task_arn = strstr(buf, ECS_TASK_PREFIX);
if (task_arn != NULL) {
/* parse out the task_arn */
task_arn += strlen(ECS_TASK_PREFIX);
ret = dd_remap_append_kv_to_ddtags(tag_name, task_arn, strlen(task_arn), dd_tags_buf);
dd_remap_append_kv_to_ddtags(tag_name, task_arn, strlen(task_arn), dd_tags);
}
else {
/*
* if the input is invalid, not in the form of "XXXXXXXXtask/"task-arn
* then we preverse the original value under tag "task_arn".
*/
ret = dd_remap_append_kv_to_ddtags(tag_name, buf, strlen(buf), dd_tags_buf);
dd_remap_append_kv_to_ddtags(tag_name, buf, strlen(buf), dd_tags);
}
flb_sds_destroy(buf);
if (ret < 0) {
return -1;
}

return 0;
}

/*
Expand Down
4 changes: 1 addition & 3 deletions plugins/out_datadog/datadog_remap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,10 @@

#include "datadog.h"

typedef int (*dd_attr_remap_to_tag_fn)(const char*, msgpack_object, flb_sds_t*);

struct dd_attr_tag_remapping {
char* origin_attr_name; /* original attribute name */
char* remap_tag_name; /* tag name to remap to */
dd_attr_remap_to_tag_fn remap_to_tag; /* remapping function */
void (*remap_to_tag) (const char*, msgpack_object, flb_sds_t); /* remapping function */
};

extern const struct dd_attr_tag_remapping remapping[];
Expand Down