Fix mbean parsing for mbeans with multiple quoted properties (elastic…

…#17374) (elastic#17407) There are mbeans that can contain multiple quoted properties, and Metricbeat was failing to correctly parse them. Fix the regular expression used for parsing to cover this case. Also small refactors in tests and to remove an almost duplicated regular expression. (cherry picked from commit 46f74f6)
leweafan · Apr 7, 2020 · 32373f9 · 32373f9
1 parent 6e7ff5e
commit 32373f9
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 47 deletions.
diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc
@@ -183,6 +183,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 - Use max in k8s overview dashboard aggregations. {pull}17015[17015]
 - Fix Disk Used and Disk Usage visualizations in the Metricbeat System dashboards. {issue}12435[12435] {pull}17272[17272]
 - Fix missing Accept header for Prometheus and OpenMetrics module. {issue}16870[16870] {pull}17291[17291]
+- Fix issue in Jolokia module when mbean contains multiple quoted properties. {issue}17375[17375] {pull}17374[17374]
 - Combine cloudwatch aggregated metrics into single event. {pull}17345[17345]
 - Fix how we filter services by name in system/service {pull}17400[17400]
 - Fix cloudwatch metricset missing tags collection. {issue}17419[17419] {pull}17424[17424]

diff --git a/metricbeat/module/jolokia/jmx/config.go b/metricbeat/module/jolokia/jmx/config.go
@@ -117,7 +117,14 @@ type MBeanName struct {
 	Properties map[string]string
 }
 
-var mbeanRegexp = regexp.MustCompile("([^,=:*?]+)=([^,=:\"]+|\".*\")")
+// Parse strings with properties with the format key=value, being:
+// - Key a nonempty string of characters which may not contain any of the characters,
+//   comma (,), equals (=), colon, asterisk, or question mark.
+// - Value a string that can be quoted or unquoted, if unquoted it cannot be empty and
+//   cannot contain any of the characters comma, equals, colon, or quote.
+//   If quoted, it can contain any character, including newlines, but quote needs to be
+//   escaped with a backslash.
+var mbeanRegexp = regexp.MustCompile(`([^,=:*?]+)=([^,=:"]+|"([^\\"]|\\.)*?")`)
 
 // This replacer is responsible for adding a "!" before special characters in GET request URIs
 // For more information refer: https://jolokia.org/reference/html/protocol.html
@@ -158,7 +165,6 @@ func (m *MBeanName) Canonicalize(escape bool) string {
 // The Mbean string has to abide by the rules which are imposed by Java.
 // For more info: https://docs.oracle.com/javase/8/docs/api/javax/management/ObjectName.html#getCanonicalName--
 func ParseMBeanName(mBeanName string) (*MBeanName, error) {
-
 	// Split mbean string in two parts: the bean domain and the properties
 	parts := strings.SplitN(mBeanName, ":", 2)
 	if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
@@ -170,15 +176,6 @@ func ParseMBeanName(mBeanName string) (*MBeanName, error) {
 		Domain: parts[0],
 	}
 
-	// First of all verify that all bean properties are
-	// in the form key=value
-	tmpProps := propertyRegexp.FindAllString(parts[1], -1)
-	propertyList := strings.Join(tmpProps, ",")
-	if len(propertyList) != len(parts[1]) {
-		// Some property didn't match
-		return nil, fmt.Errorf("mbean properties must be in the form key=value: %s", mBeanName)
-	}
-
 	// Using this regexp we will split the properties in a 2 dimensional array
 	// instead of just splitting by commas because values can be quoted
 	// and contain commas, what complicates the parsing.
@@ -195,14 +192,15 @@ func ParseMBeanName(mBeanName string) (*MBeanName, error) {
 	// }
 	properties := mbeanRegexp.FindAllStringSubmatch(parts[1], -1)
 
-	// If we could not parse MBean properties
 	if properties == nil {
 		return nil, fmt.Errorf("mbean properties must be in the form key=value: %s", mBeanName)
 	}
 
 	// Initialise properties map
 	mybean.Properties = make(map[string]string)
 
+	// Get the parsed string to check that everything has been parsed
+	var parsed []string
 	for _, prop := range properties {
 
 		// If every row does not have 3 columns, then
@@ -212,9 +210,16 @@ func ParseMBeanName(mBeanName string) (*MBeanName, error) {
 			return nil, fmt.Errorf("mbean properties must be in the form key=value: %s", mBeanName)
 		}
 
+		parsed = append(parsed, prop[0])
+
 		mybean.Properties[prop[1]] = prop[2]
 	}
 
+	// Not all the properties definition has been parsed
+	if parsed := strings.Join(parsed, ","); len(parts[1]) != len(parsed) {
+		return nil, fmt.Errorf("not all properties could be parsed: %s, parsed: %s", mBeanName, parsed)
+	}
+
 	return mybean, nil
 }
 
@@ -418,13 +423,6 @@ func (pc *JolokiaHTTPPostFetcher) BuildRequestsAndMappings(configMappings []JMXM
 	return httpRequests, mapping, nil
 }
 
-// Parse strings with properties with the format key=value, being:
-// - key a nonempty string of characters which may not contain any of the characters,
-//   comma (,), equals (=), colon, asterisk, or question mark.
-// - value a string that can be quoted or unquoted, if unquoted it cannot be empty and
-//   cannot contain any of the characters comma, equals, colon, or quote.
-var propertyRegexp = regexp.MustCompile("[^,=:*?]+=([^,=:\"]+|\".*\")")
-
 func (pc *JolokiaHTTPPostFetcher) buildRequestBodyAndMapping(mappings []JMXMapping) ([]byte, AttributeMapping, error) {
 	responseMapping := make(AttributeMapping)
 	var blocks []RequestBlock

diff --git a/metricbeat/module/jolokia/jmx/config_test.go b/metricbeat/module/jolokia/jmx/config_test.go
@@ -75,32 +75,32 @@ func TestBuildJolokiaGETUri(t *testing.T) {
 
 func TestParseMBean(t *testing.T) {
 
-	cases := []struct {
+	cases := map[string]struct {
 		mbean    string
 		expected *MBeanName
 		ok       bool
 	}{
-		{
+		"empty": {
 			mbean: ``,
 			ok:    false,
 		},
-		{
+		"no domain": {
 			mbean: `type=Runtime`,
 			ok:    false,
 		},
-		{
+		"no properties": {
 			mbean: `java.lang`,
 			ok:    false,
 		},
-		{
+		"no properties, with colon": {
 			mbean: `java.lang:`,
 			ok:    false,
 		},
-		{
+		"property without value": {
 			mbean: `java.lang:type=Runtime,name`,
 			ok:    false,
 		},
-		{
+		"single property": {
 			mbean: `java.lang:type=Runtime`,
 			expected: &MBeanName{
 				Domain: `java.lang`,
@@ -110,18 +110,17 @@ func TestParseMBean(t *testing.T) {
 			},
 			ok: true,
 		},
-		{
-			mbean: `java.lang:name=Foo,type=Runtime`,
+		"other single property": {
+			mbean: `java.lang:type=Memory`,
 			expected: &MBeanName{
 				Domain: `java.lang`,
 				Properties: map[string]string{
-					"name": "Foo",
-					"type": "Runtime",
+					"type": "Memory",
 				},
 			},
 			ok: true,
 		},
-		{
+		"multiple properties": {
 			mbean: `java.lang:name=Foo,type=Runtime`,
 			expected: &MBeanName{
 				Domain: `java.lang`,
@@ -132,7 +131,7 @@ func TestParseMBean(t *testing.T) {
 			},
 			ok: true,
 		},
-		{
+		"property with wildcard": {
 			mbean: `java.lang:type=Runtime,name=Foo*`,
 			expected: &MBeanName{
 				Domain: `java.lang`,
@@ -143,7 +142,7 @@ func TestParseMBean(t *testing.T) {
 			},
 			ok: true,
 		},
-		{
+		"property with wildcard as value": {
 			mbean: `java.lang:type=Runtime,name=*`,
 			expected: &MBeanName{
 				Domain: `java.lang`,
@@ -154,7 +153,7 @@ func TestParseMBean(t *testing.T) {
 			},
 			ok: true,
 		},
-		{
+		"quoted property": {
 			mbean: `java.lang:name="foo,bar",type=Runtime`,
 			expected: &MBeanName{
 				Domain: `java.lang`,
@@ -165,17 +164,41 @@ func TestParseMBean(t *testing.T) {
 			},
 			ok: true,
 		},
-		{
-			mbean: `java.lang:type=Memory`,
+		"multiple quoted properties": {
+			mbean: `java.lang:name="foo",othername="bar",type=Runtime`,
 			expected: &MBeanName{
 				Domain: `java.lang`,
 				Properties: map[string]string{
-					"type": "Memory",
+					"name":      `"foo"`,
+					"othername": `"bar"`,
+					"type":      "Runtime",
 				},
 			},
 			ok: true,
 		},
-		{
+		"escaped quote in quoted property": {
+			mbean: `java.lang:name="foo,\"bar\"",type=Runtime`,
+			expected: &MBeanName{
+				Domain: `java.lang`,
+				Properties: map[string]string{
+					"name": `"foo,\"bar\""`,
+					"type": "Runtime",
+				},
+			},
+			ok: true,
+		},
+		"newline in quoted property": {
+			mbean: `java.lang:name="foo\nbar",type=Runtime`,
+			expected: &MBeanName{
+				Domain: `java.lang`,
+				Properties: map[string]string{
+					"name": `"foo\nbar"`,
+					"type": "Runtime",
+				},
+			},
+			ok: true,
+		},
+		"real catalina mbean": {
 			mbean: `Catalina:name=HttpRequest1,type=RequestProcessor,worker="http-nio-8080"`,
 			expected: &MBeanName{
 				Domain: `Catalina`,
@@ -187,17 +210,36 @@ func TestParseMBean(t *testing.T) {
 			},
 			ok: true,
 		},
+		"real activemq artemis mbean": {
+			mbean: `org.apache.activemq.artemis:broker="0.0.0.0",component=addresses,address="helloworld",subcomponent=queues,routing-type="anycast",queue="helloworld"`,
+			expected: &MBeanName{
+				Domain: `org.apache.activemq.artemis`,
+				Properties: map[string]string{
+					"broker":       `"0.0.0.0"`,
+					"component":    `addresses`,
+					"address":      `"helloworld"`,
+					"subcomponent": `queues`,
+					"routing-type": `"anycast"`,
+					"queue":        `"helloworld"`,
+				},
+			},
+			ok: true,
+		},
 	}
 
-	for _, c := range cases {
-		beanObj, err := ParseMBeanName(c.mbean)
-
-		if c.ok {
-			assert.NoError(t, err, "failed parsing for: "+c.mbean)
-			assert.Equal(t, c.expected, beanObj, "mbean: "+c.mbean)
-		} else {
-			assert.Error(t, err, "should have failed for: "+c.mbean)
-		}
+	for title, c := range cases {
+		t.Run(title, func(t *testing.T) {
+			beanObj, err := ParseMBeanName(c.mbean)
+
+			if c.ok {
+				if assert.NoError(t, err, "failed parsing for: "+c.mbean) {
+					t.Log("Canonicalized mbean: ", beanObj.Canonicalize(true))
+				}
+				assert.Equal(t, c.expected, beanObj, "mbean: "+c.mbean)
+			} else {
+				assert.Error(t, err, "should have failed for: "+c.mbean)
+			}
+		})
 	}
 
 }