From f7bd38b5dc1c2afc5e46ead54e1ac34106da9bae Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Tue, 7 Apr 2020 20:47:12 +0200 Subject: [PATCH] Fix mbean parsing for mbeans with multiple quoted properties (#17374) (#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 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 9e85eb53a67f..9ad1a761cf7b 100644 --- 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 index 4925ee6a2937..3e3d623662e7 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 b6ccc1b7ccb5..e873ccec8662 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) + } + }) } }