From cece8fc6204775541ca5fc4765e5f46ba32ecbc4 Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Wed, 1 Apr 2020 19:13:02 +0200 Subject: [PATCH] Fix mbean parsing for mbeans with multiple quoted properties (#17374) 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 7b6d7c481c418608282aa6edf3b2b116b4ac0cad) --- CHANGELOG.next.asciidoc | 1 + metricbeat/module/jolokia/jmx/config.go | 36 ++++--- metricbeat/module/jolokia/jmx/config_test.go | 98 ++++++++++++++------ 3 files changed, 88 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 483f9443605..165e29ffdd2 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -62,6 +62,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Convert increments of 100 nanoseconds/ticks to milliseconds for WriteTime and ReadTime in diskio metricset (Windows) for consistency. {issue}14233[14233] - Fix diskio issue for windows 32 bit on disk_performance struct alignment. {issue}16680[16680] - Reduce memory usage in `elasticsearch/index` metricset. {issue}16503[16503] {pull}16538[16538] +- Fix issue in Jolokia module when mbean contains multiple quoted properties. {issue}17375[17375] {pull}17374[17374] *Packetbeat* diff --git a/metricbeat/module/jolokia/jmx/config.go b/metricbeat/module/jolokia/jmx/config.go index 60b9a60ff63..3cd61201c4c 100644 --- 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,7 +192,6 @@ 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) } @@ -203,6 +199,8 @@ func ParseMBeanName(mBeanName string) (*MBeanName, error) { // 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 index b6ccc1b7ccb..e873ccec866 100644 --- 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) + } + }) } }