Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Renamed config key for Graylog protocol to use.
Fixed wrong byte packing when sending chunked UDP packets.
Improved test code and comments.

Signed-off-by: Thomas Pulzer <[email protected]>
  • Loading branch information
Faldon committed Mar 28, 2019
commit 38caffe5a134840df8ca8afc0274aedb2e98f0a1
4 changes: 2 additions & 2 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -786,7 +786,7 @@
/**
* Your graylog host server name, for example ``localhost``, ``hostname``,
* ``hostname.example.com``, or the IP address. To specify a port use
* ``hostname:####``
* ``hostname:####``. The default port is 5410
* Only effective when ``log_type`` set to ``graylog``
*/
'graylog_host' => '',
Expand All @@ -797,7 +797,7 @@
*
* The default value is ``udp``.
*/
'graylog_method' => 'udp',
'graylog_proto' => 'udp',

/**
* Log condition for log level increase based on conditions. Once one of these
Expand Down
19 changes: 11 additions & 8 deletions lib/private/Log/Graylog.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class Graylog implements IWriter {

public function __construct(IConfig $config) {
$this->host = gethostname();
$this->protocol = $config->getSystemValue('graylog_method', 'udp');
$this->protocol = $config->getSystemValue('graylog_proto', 'udp');
$address = $config->getSystemValue('graylog_host', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Code is fine. I would pick a slightly different approach but everyone has a different code style so this is up to you.

		$address = explode(':', $config->getSystemValue('graylog_host'));
		if (count($address) === 2) {
			$this->target = $address[0];
			$this->port = (int)$address[1];
		} else {
			$this->target = $address[0];
			$this->port = 514;
		}

if (false !== strpos($address, ':')) {
$this->target = explode(':', $address)[0];
Expand All @@ -72,27 +72,30 @@ public function write(string $app, $message, int $level) {
time() . '}';
switch ($this->protocol) {
case 'udp':
$chunks = str_split($msg, 8000);
$chunks = str_split($msg, 1024);
break;
case 'tcp':
$chunks[0] = $msg;
break;
}
$count = count($chunks);
$errno = 0;
$errstr = '';
$errNo = 0;
$errStr = '';
$fp = stream_socket_client(
$this->protocol . '://' . $this->target . ':' . $this->port,
$errno,
$errstr,
$errNo,
$errStr,
5
);
if(false === $fp) {
return;
}
switch ($count > 1) {
case true:
$id = random_bytes(8);
for ($i = 0; $i < $count; $i++) {
fwrite($fp, pack('n', 0x1e0f) . $id . $i . $count .
$chunks[$i] . pack('x'));
fwrite($fp, pack('n', 0x1e0f) . $id . pack('CC', $i, $count)
. $chunks[$i]);
}
break;
case false:
Expand Down
83 changes: 58 additions & 25 deletions tests/lib/Log/GraylogTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@
class GraylogTest extends TestCase {

/** @var string */
private $graylog_method_restore;
private $protocol_restore;
/** @var string */
private $graylog_host_restore;
private $target_restore;
/** @var SystemConfig */
private $config;
/** @var string */
Expand All @@ -45,66 +45,99 @@ class GraylogTest extends TestCase {
protected function setUp() {
parent::setUp();
$this->config = \OC::$server->getSystemConfig();
$this->graylog_method_restore = $this->config->getValue('graylog_method');
$this->graylog_host_restore = $this->config->getValue('graylog_host');
$this->protocol_restore = $this->config->getValue('graylog_proto');
$this->target_restore = $this->config->getValue('graylog_host');
$this->buf = '';
$this->from = '';
$this->port = 0;
}

public function testUnchunkedUdp() {
$this->config->setValue('graylog_method', 'udp');
$this->config->setValue('graylog_proto', 'udp');
$this->config->setValue('graylog_host', '127.0.0.1:5140');

// Create a mock server to send a test message to
$s = socket_create(AF_INET, SOCK_DGRAM, SOL_UDP);
socket_bind($s, '127.0.0.1', 5140);
socket_set_nonblock($s);

$id = 'GraylogTest';
$msg = 'UDP Graylog test < 1kb';
$graylog = new Graylog(\OC::$server->getConfig());
$graylog->write('GraylogTest', 'UDP Graylog test < 8kb', 1);
$graylog->write('GraylogTest', $msg, 1);

socket_recvfrom($s, $this->buf, 8000, 0, $this->from, $this->port);
socket_recvfrom($s, $this->buf, 1025, 0, $this->from, $this->port);
socket_close($s);

// The resulting GELF message has to be 130 characters long
$this->assertEquals(130, strlen($this->buf));
// The resulting GELF message has a length of 81 + length of host name +
// length of app name + length of log message + 3 formatting characters.
$expected = 81 + strlen(gethostname()) + strlen($msg) + strlen($id) + 3;
$this->assertEquals($expected, strlen($this->buf));
}

public function testChunkedUdp() {
$this->config->setValue('graylog_method', 'udp');
$this->config->setValue('graylog_proto', 'udp');
$this->config->setValue('graylog_host', '127.0.0.1:5140');

// Create a mock server to send a test message to
$s = socket_create(AF_INET, SOCK_DGRAM, SOL_UDP);
socket_bind($s, '127.0.0.1', 5140);
socket_set_nonblock($s);

$graylog = new Graylog(\OC::$server->getConfig());
$msg = "Very log message filled with garbage to execeed 8kb limit. ";
for($i = 0; $i < 8000; $i++) {
$msg .= "A";
$id = 'GraylogTest';
$msg = "Very log message filled with garbage to exceed 1kb limit. ";
for($i = 0; $i < 1024; $i++) {
$msg .= "A";
}
$graylog->write('GraylogTest', $msg, 3);
$graylog = new Graylog(\OC::$server->getConfig());
$graylog->write($id, $msg, 3);

socket_recvfrom($s, $this->buf, 8000, 0, $this->from, $this->port);
socket_recvfrom($s, $this->buf, 1034, 0, $this->from, $this->port);
socket_close($s);

// The first response should start with 0x1E 0x0F, has sequence 0
// at position 10 and total count 2 at position 11
// The chunked GELF message must start start with 0x1E 0x0F, followed
// by 8 byte message id, 1 byte current sequence and 1 byte total chunk
// count. In this test the total chunk count is 2 and we examine the
// first chunk (zero-indexed).
$this->assertEquals(0x1e0f, unpack('n', $this->buf)[1]);
$this->assertEquals(0, intval(substr($this->buf, 10, 1)));
$this->assertEquals(2, intval(substr($this->buf, 11, 1)));
$this->assertEquals(0, unpack('C', substr($this->buf, 10, 1))[1]);
$this->assertEquals(2, unpack('C', substr($this->buf, 11, 1))[1]);
}

public function testTcp() {
$this->config->setValue('graylog_proto', 'tcp');
$this->config->setValue('graylog_host', '127.0.0.1:5140');

// Create a mock server to send a test message to
$s = socket_create(AF_INET, SOCK_STREAM, SOL_TCP);
socket_bind($s, '127.0.0.1', 5140);
socket_listen($s);
socket_set_nonblock($s);

$id = 'GraylogTest';
$msg = 'TCP Graylog test < 1kb';
$graylog = new Graylog(\OC::$server->getConfig());
$graylog->write($id, $msg, 3);

$c = socket_accept($s);
$this->buf = socket_read($c, 1025);
socket_close($c);
socket_close($s);

// The resulting GELF message has a length of 81 + length of host name +
// length of app name + length of log message + 3 formatting characters.
$expected = 81 + strlen(gethostname()) + strlen($msg) + strlen($id) + 3;
$this->assertEquals($expected, strlen($this->buf));
}

protected function tearDown() {
if (isset($this->graylog_method_restore)) {
$this->config->setValue('graylog_method', $this->graylog_method_restore);
if (isset($this->protocol_restore)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rullzer @ChristophWurst do we need this logic? I think we should properly mock OC_Config here 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I would keep it, because then the proper "is everything set as wanted and also set together via config" covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

is everything set as wanted and also set together via config

=> Integration test. I see your point but there is a good chance that in the future this unit test on drugs will fail 😎

$this->config->setValue('graylog_proto', $this->protocol_restore);
} else {
$this->config->deleteValue('graylog_method');
$this->config->deleteValue('graylog_proto');
}
if (isset($this->graylog_host_restore)) {
$this->config->setValue('graylog_host', $this->graylog_host_restore);
if (isset($this->target_restore)) {
$this->config->setValue('graylog_host', $this->target_restore);
} else {
$this->config->deleteValue('graylog_host');
}
Expand Down