Fix mbean parsing for mbeans with multiple quoted properties

CHANGELOG.next.asciidoc

@@ -135,6 +135,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]
 
 *Packetbeat*

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

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)
+			}
+		})
 	}
 
 }