Merge pull request #883 from deniszh/backport/1.1.x/pr-869_pr-872_pr-858_pr-858_pr-858_pr-874_pr-875_pr-880_pr-881_pr-882

deniszh · web-flow · commit 4c60b0196258 · 2020-02-29T21:40:21.000+01:00
[1.1.x] s390x support for travis (#869) | Fix #871: Adjust aggregator-rules input_pattern match greediness to support numeric matching after captured field (#872) | sanitize names when using them as tag value (#858) | sanitize names when us
diff --git a/.travis.yml b/.travis.yml
@@ -1,9 +1,7 @@
 # http://travis-ci.org/#!/graphite-project/carbon
+dist: xenial
 language: python
 python: 2.7
-# sudo is needed to install libboost-python-dev, which is required by pyhash
-sudo: required
-dist: xenial
 
 matrix:
   include:
@@ -17,17 +15,43 @@ matrix:
       env:
         - TOXENV=py36
     - python: 3.7
-      sudo: true
       env:
         - TOXENV=py37
-    - python: "3.8-dev"
-      sudo: true
+    - python: 3.8
       env:
         - TOXENV=py38-pyhash
-    - python: "3.8-dev"
+    - python: 3.8
       env:
         - TOXENV=py38
-    - python: "3.8-dev"
+    - python: 3.8
+      env:
+        - TOXENV=lint
+    - python: 2.7
+      arch: s390x
+      env:
+        - TOXENV=py27
+    - python: 2.7
+      arch: s390x
+      env:
+        - TOXENV=lint
+    - python: 3.5
+      arch: s390x
+      env:
+        - TOXENV=py35
+    - python: 3.6
+      arch: s390x
+      env:
+        - TOXENV=py36
+    - python: 3.7
+      arch: s390x
+      env:
+        - TOXENV=py37
+    - python: 3.8
+      arch: s390x
+      env:
+        - TOXENV=py38
+    - python: 3.8
+      arch: s390x
       env:
         - TOXENV=lint
 
@@ -36,10 +60,10 @@ env:
   - TOXENV=py27-pyhash
   - TOXENV=lint
 
-before_install:
-  - bash -c "if [[ '$TOXENV' == *pyhash* ]]; then sudo apt-get -qq update; sudo apt-get install -y libboost-python-dev; fi"
-
 install:
+  - if [[ $(uname -m) == 's390x' ]]; then sudo rm -rf $HOME/.cache/pip; fi
+  - if echo "$TOXENV" | grep -q 'pyhash' ; then sudo apt-get -q install -y libboost-python-dev; fi
+  - if echo "$TOXENV" | grep -q '^py2'   ; then pip install --upgrade pip virtualenv; fi
   - pip install tox
 
 script:
diff --git a/conf/carbon.conf.example b/conf/carbon.conf.example
@@ -294,11 +294,18 @@ WHISPER_FALLOCATE_CREATE = True
 # you are familiar with the code. If you are not, please don't
 # mess with this, you are asking for trouble :)
 #
+# You need the bcrypt, cryptography and pyasn1 python modules installed for
+# manhole to work.
+#
+# Generate host keys with:
+# `ckeygen -t rsa -f /example/host_keys/ssh_host_key_rsa`
+#
 # ENABLE_MANHOLE = False
 # MANHOLE_INTERFACE = 127.0.0.1
 # MANHOLE_PORT = 7222
 # MANHOLE_USER = admin
 # MANHOLE_PUBLIC_KEY = ssh-rsa AAAAB3NzaC1yc2EAAAABiwAaAIEAoxN0sv/e4eZCPpi3N3KYvyzRaBaMeS2RsOQ/cDuKv11dlNzVeiyc3RFmCv5Rjwn/lQ79y0zyHxw67qLyhQ/kDzINc4cY41ivuQXm2tPmgvexdrBv5nsfEpjs3gLZfJnyvlcVyWK/lId8WUvEWSWHTzsbtmXAF2raJMdgLTbQ8wE=
+# MANHOLE_HOST_KEY_DIR = /example/host_keys
 
 # Patterns for all of the metrics this machine will store. Read more at
 # http://en.wikipedia.org/wiki/Advanced_Message_Queuing_Protocol#Bindings
diff --git a/lib/carbon/aggregator/rules.py b/lib/carbon/aggregator/rules.py
@@ -128,7 +128,7 @@ def build_regex(self):
         pre = input_part[:i]
         post = input_part[j + 2:]
         field_name = input_part[i + 2:j]
-        regex_part = '%s(?P<%s>.+)%s' % (pre, field_name, post)
+        regex_part = '%s(?P<%s>.+?)%s' % (pre, field_name, post)
 
       else:
         i = input_part.find('<')
@@ -137,7 +137,7 @@ def build_regex(self):
           pre = input_part[:i]
           post = input_part[j + 1:]
           field_name = input_part[i + 1:j]
-          regex_part = '%s(?P<%s>[^.]+)%s' % (pre, field_name, post)
+          regex_part = '%s(?P<%s>[^.]+?)%s' % (pre, field_name, post)
         elif input_part == '*':
           regex_part = '[^.]+'
         else:
diff --git a/lib/carbon/conf.py b/lib/carbon/conf.py
@@ -88,6 +88,7 @@
   MANHOLE_PORT=7222,
   MANHOLE_USER="",
   MANHOLE_PUBLIC_KEY="",
+  MANHOLE_HOST_KEY_DIR="",
   RELAY_METHOD='rules',
   DYNAMIC_ROUTER=False,
   DYNAMIC_ROUTER_MAX_RETRIES=5,
diff --git a/lib/carbon/manhole.py b/lib/carbon/manhole.py
@@ -3,14 +3,17 @@
 from twisted.conch.checkers import SSHPublicKeyDatabase
 from twisted.conch.manhole import Manhole
 from twisted.conch.manhole_ssh import TerminalRealm, ConchFactory
+from twisted.conch.openssh_compat.factory import OpenSSHFactory
 from twisted.internet import reactor
 from twisted.application.internet import TCPServer
 
 from carbon.protocols import CarbonServerProtocol
 from carbon.conf import settings
+import carbon
 
+from carbon.exceptions import CarbonConfigException
 
-namespace = {}
+namespace = {'carbon': carbon}
 
 
 class PublicKeyChecker(SSHPublicKeyDatabase):
@@ -31,16 +34,27 @@ def createManholeListener():
 
   if settings.MANHOLE_PUBLIC_KEY == 'None':
     credChecker = checkers.InMemoryUsernamePasswordDatabaseDontUse()
-    credChecker.addUser(settings.MANHOLE_USER, '')
+    credChecker.addUser(settings.MANHOLE_USER.encode('utf-8'),
+                        ''.encode('utf-8'))
   else:
     userKeys = {
-        settings.MANHOLE_USER: settings.MANHOLE_PUBLIC_KEY,
+        settings.MANHOLE_USER.encode('utf-8'):
+            settings.MANHOLE_PUBLIC_KEY.encode('utf-8'),
     }
     credChecker = PublicKeyChecker(userKeys)
 
   sshPortal = portal.Portal(sshRealm)
   sshPortal.registerChecker(credChecker)
   sessionFactory = ConchFactory(sshPortal)
+
+  # set ssh host keys
+  if settings.MANHOLE_HOST_KEY_DIR == "":
+    raise CarbonConfigException("MANHOLE_HOST_KEY_DIR not defined")
+  openSSHFactory = OpenSSHFactory()
+  openSSHFactory.dataRoot = settings.MANHOLE_HOST_KEY_DIR
+  sessionFactory.publicKeys = openSSHFactory.getPublicKeys()
+  sessionFactory.privateKeys = openSSHFactory.getPrivateKeys()
+
   return sessionFactory
 
 
diff --git a/lib/carbon/tests/test_client.py b/lib/carbon/tests/test_client.py
@@ -5,7 +5,6 @@
 )
 from carbon.routers import DatapointRouter
 from carbon.tests.util import TestSettings
-from carbon import instrumentation
 import carbon.service  # NOQA
 
 from twisted.internet import reactor
@@ -44,7 +43,6 @@ def getDestinations(self, key):
       yield destination
 
 
-@patch('carbon.state.instrumentation', Mock(spec=instrumentation))
 class ConnectedCarbonClientProtocolTest(TestCase):
   def setUp(self):
     self.router_mock = Mock(spec=DatapointRouter)
@@ -88,7 +86,6 @@ def test_send_datapoints(self):
       self.protocol.sendLine.assert_called_with(expected_line_to_send)
 
 
-@patch('carbon.state.instrumentation', Mock(spec=instrumentation))
 class CarbonClientFactoryTest(TestCase):
   def setUp(self):
     self.router_mock = Mock(spec=DatapointRouter)
@@ -127,7 +124,6 @@ def test_send_queued_should_call_protocol_send_queued(self):
     self.protocol_mock.sendQueued.assert_called_once_with()
 
 
-@patch('carbon.state.instrumentation', Mock(spec=instrumentation))
 class CarbonClientManagerTest(TestCase):
   timeout = 1.0
 
@@ -190,7 +186,6 @@ def test_stop_client_removes_destination_from_router(self):
     self.router_mock.removeDestination.assert_called_once_with(dest)
 
 
-@patch('carbon.state.instrumentation', Mock(spec=instrumentation))
 class RelayProcessorTest(TestCase):
   timeout = 1.0
 
diff --git a/lib/carbon/tests/test_protobuf.py b/lib/carbon/tests/test_protobuf.py
@@ -1,7 +1,6 @@
 import carbon.client as carbon_client
 from carbon.routers import DatapointRouter
 from carbon.tests.util import TestSettings
-from carbon import instrumentation
 import carbon.service  # NOQA
 
 from carbon.carbon_pb2 import Payload
@@ -14,7 +13,7 @@
 from twisted.trial.unittest import TestCase
 from twisted.test.proto_helpers import StringTransport
 
-from mock import Mock, patch
+from mock import Mock
 from struct import unpack, calcsize
 
 
@@ -35,7 +34,6 @@ def decode_sent(data):
   return datapoints
 
 
-@patch('carbon.state.instrumentation', Mock(spec=instrumentation))
 class ConnectedCarbonClientProtocolTest(TestCase):
   def setUp(self):
     self.router_mock = Mock(spec=DatapointRouter)
diff --git a/lib/carbon/tests/test_util.py b/lib/carbon/tests/test_util.py
@@ -4,6 +4,7 @@
 
 from carbon.util import parseDestinations
 from carbon.util import enableTcpKeepAlive
+from carbon.util import TaggedSeries
 
 
 class UtilTest(TestCase):
@@ -21,6 +22,72 @@ def setTcpKeepAlive(self, value):
         enableTcpKeepAlive(_Transport(), True, None)
         self.assertEquals(s.getsockopt(socket.SOL_TCP, socket.SO_KEEPALIVE), 1)
 
+    def test_sanitizing_name_as_tag_value(self):
+        test_cases = [
+            {
+                'original': "my~.test.abc",
+                'expected': "my~.test.abc",
+            }, {
+                'original': "a.b.c",
+                'expected': "a.b.c",
+            }, {
+                'original': "~~a~~.~~~b~~~.~~~c~~~",
+                'expected': "a~~.~~~b~~~.~~~c~~~",
+            }, {
+                'original': "a.b.c~",
+                'expected': "a.b.c~",
+            }, {
+                'original': "~a.b.c",
+                'expected': "a.b.c",
+            }, {
+                'original': "~a~",
+                'expected': "a~",
+            }, {
+                'original': "~~~",
+                'raises': True,
+            }, {
+                'original': "~",
+                'raises': True,
+            },
+        ]
+
+        for test_case in test_cases:
+            if test_case.get('raises', False):
+                self.assertRaises(
+                    Exception,
+                    TaggedSeries.sanitize_name_as_tag_value,
+                    test_case['original'],
+                )
+            else:
+                result = TaggedSeries.sanitize_name_as_tag_value(test_case['original'])
+                self.assertEquals(result, test_case['expected'])
+
+    def test_validate_tag_key_and_value(self):
+        # assert that it raises exception when sanitized name is still not valid
+        with self.assertRaises(Exception):
+            # sanitized name is going to be '', which is not a valid tag value
+            TaggedSeries.sanitize_name_as_tag_value('~~~~')
+
+        with self.assertRaises(Exception):
+            # given tag value is invalid because it has length 0
+            TaggedSeries.validateTagAndValue('metric.name;tag=')
+
+        with self.assertRaises(Exception):
+            # given tag key is invalid because it has length 0
+            TaggedSeries.validateTagAndValue('metric.name;=value')
+
+        with self.assertRaises(Exception):
+            # given tag is missing =
+            TaggedSeries.validateTagAndValue('metric.name;tagvalue')
+
+        with self.assertRaises(Exception):
+            # given tag value is invalid because it starts with ~
+            TaggedSeries.validateTagAndValue('metric.name;tag=~value')
+
+        with self.assertRaises(Exception):
+            # given tag key is invalid because it contains !
+            TaggedSeries.validateTagAndValue('metric.name;ta!g=value')
+
 
 # Destinations have the form:
 # <host> ::= <string without colons> | "[" <string> "]"
diff --git a/lib/carbon/util.py b/lib/carbon/util.py
@@ -336,6 +336,28 @@ def __init__(classObj, name, bases, members):
 
 
 class TaggedSeries(object):
+  prohibitedTagChars = ';!^='
+
+  @classmethod
+  def validateTagAndValue(cls, tag, value):
+    """validate the given tag / value based on the specs in the documentation"""
+    if len(tag) == 0:
+      raise Exception('Tag may not be empty')
+    if len(value) == 0:
+      raise Exception('Value for tag "{tag}" may not be empty'.format(tag=tag))
+
+    for char in cls.prohibitedTagChars:
+      if char in tag:
+        raise Exception(
+          'Character "{char}" is not allowed in tag "{tag}"'.format(char=char, tag=tag))
+
+    if ';' in value:
+      raise Exception(
+        'Character ";" is not allowed in value "{value}" of tag {tag}'.format(value=value, tag=tag))
+
+    if value[0] == '~':
+      raise Exception('Tag values are not allowed to start with "~" in tag "{tag}"'.format(tag=tag))
+
   @classmethod
   def parse(cls, path):
     # if path is in openmetrics format: metric{tag="value",...}
@@ -362,10 +384,15 @@ def parse_openmetrics(cls, path):
       if not m:
         raise Exception('Cannot parse path %s, invalid segment %s' % (path, rawtags))
 
-      tags[m.group(1)] = m.group(2).replace(r'\"', '"').replace(r'\\', '\\')
+      tag = m.group(1)
+      value = m.group(2).replace(r'\"', '"').replace(r'\\', '\\')
+
+      cls.validateTagAndValue(tag, value)
+
+      tags[tag] = value
       rawtags = rawtags[len(m.group(0)):]
 
-    tags['name'] = metric
+    tags['name'] = cls.sanitize_name_as_tag_value(metric)
     return cls(metric, tags)
 
   @classmethod
@@ -384,11 +411,23 @@ def parse_carbon(cls, path):
       if len(tag) != 2 or not tag[0]:
         raise Exception('Cannot parse path %s, invalid segment %s' % (path, segment))
 
+      cls.validateTagAndValue(*tag)
+
       tags[tag[0]] = tag[1]
 
-    tags['name'] = metric
+    tags['name'] = cls.sanitize_name_as_tag_value(metric)
     return cls(metric, tags)
 
+  @staticmethod
+  def sanitize_name_as_tag_value(name):
+    """take a metric name and sanitize it so it is guaranteed to be a valid tag value"""
+    sanitized = name.lstrip('~')
+
+    if len(sanitized) == 0:
+      raise Exception('Cannot use metric name %s as tag value, results in emptry string' % (name))
+
+    return sanitized
+
   @staticmethod
   def format(tags):
     return tags.get('name', '') + ''.join(sorted([
