From dd7e466d0ec985831f3a976d1d1055a172cc4cb0 Mon Sep 17 00:00:00 2001 From: Domenico Francesco Bruscino Date: Fri, 14 Jun 2024 10:19:47 +0200 Subject: [PATCH] [#956] Fix broker property with ordinal parser --- controllers/activemqartemis_reconciler.go | 68 +++++++------- .../activemqartemis_reconciler_test.go | 89 +++++++++++++++++++ 2 files changed, 119 insertions(+), 38 deletions(-) diff --git a/controllers/activemqartemis_reconciler.go b/controllers/activemqartemis_reconciler.go index 107ea15ca..29b9f299b 100644 --- a/controllers/activemqartemis_reconciler.go +++ b/controllers/activemqartemis_reconciler.go @@ -2436,7 +2436,7 @@ func (reconciler *ActiveMQArtemisReconcilerImpl) addResourceForBrokerProperties( desired = obj.(*corev1.Secret) } - data := brokerPropertiesData(customResource.Spec.BrokerProperties) + data := BrokerPropertiesData(customResource.Spec.BrokerProperties) if desired == nil { reconciler.log.V(1).Info("desired brokerprop secret nil, create new one", "name", resourceName.Name) @@ -2467,41 +2467,33 @@ func alder32Of(props []string) []byte { return digest.Sum(nil) } -func brokerPropertiesData(props []string) map[string]string { +func BrokerPropertiesData(props []string) map[string]string { + contents := make(map[string]string) + buf := &bytes.Buffer{} fmt.Fprintln(buf, "# generated by crd") fmt.Fprintln(buf, "#") - hasOrdinalPrefix := false for _, propertyKeyVal := range props { - if strings.HasPrefix(propertyKeyVal, OrdinalPrefix) && strings.Index(propertyKeyVal, OrdinalPrefixSep) > 0 { - hasOrdinalPrefix = true - continue - } - fmt.Fprintf(buf, "%s\n", propertyKeyVal) - } + matches := ParseBrokerPropertyWithOrdinal(propertyKeyVal) - contents := map[string]string{BrokerPropertiesName: buf.String()} + if len(matches) > 0 { + mapKey := matches[1] + OrdinalPrefixSep + BrokerPropertiesName + value := matches[2] - if hasOrdinalPrefix { - for _, propertyKeyVal := range props { - if strings.HasPrefix(propertyKeyVal, OrdinalPrefix) { - if i := strings.Index(propertyKeyVal, OrdinalPrefixSep); i > 0 { - // use a key that will match the volume projection and broker status - mapKey := propertyKeyVal[:i+len(OrdinalPrefixSep)] + BrokerPropertiesName - value := propertyKeyVal[i+len(OrdinalPrefixSep):] - - existing, found := contents[mapKey] - if found { - contents[mapKey] = fmt.Sprintf("%s%s\n", existing, value) - } else { - contents[mapKey] = fmt.Sprintf("%s\n", value) - } - } + existing, found := contents[mapKey] + if found { + contents[mapKey] = fmt.Sprintf("%s%s\n", existing, value) + } else { + contents[mapKey] = fmt.Sprintf("%s\n", value) } + } else { + fmt.Fprintf(buf, "%s\n", propertyKeyVal) } } + contents[BrokerPropertiesName] = buf.String() + return contents } @@ -2637,8 +2629,9 @@ func (reconciler *ActiveMQArtemisReconcilerImpl) createExtraConfigmapsAndSecrets if secret == brokePropertiesResourceName && len(brokerPropsData) > 1 { // place ordinal data in subpath in order for _, key := range sortedKeys(brokerPropsData) { - if hasOrdinal, separatorIndex := extractOrdinalPrefixSeperatorIndex(key); hasOrdinal { - subPath := key[:separatorIndex] + matches := ParseBrokerPropertyWithOrdinal(key) + if len(matches) > 0 { + subPath := matches[1] secretVol.VolumeSource.Secret.Items = append(secretVol.VolumeSource.Secret.Items, corev1.KeyToPath{Key: key, Path: fmt.Sprintf("%s/%s", subPath, key)}) } else { secretVol.VolumeSource.Secret.Items = append(secretVol.VolumeSource.Secret.Items, corev1.KeyToPath{Key: key, Path: key}) @@ -2658,8 +2651,9 @@ func (reconciler *ActiveMQArtemisReconcilerImpl) createExtraConfigmapsAndSecrets if len(bpSecret.Data) > 0 { for _, key := range sortedKeysStringKeyByteValue(bpSecret.Data) { - if hasOrdinal, separatorIndex := extractOrdinalPrefixSeperatorIndex(key); hasOrdinal { - subPath := key[:separatorIndex] + matches := ParseBrokerPropertyWithOrdinal(key) + if len(matches) > 0 { + subPath := matches[1] secretVol.VolumeSource.Secret.Items = append(secretVol.VolumeSource.Secret.Items, corev1.KeyToPath{Key: key, Path: fmt.Sprintf("%s/%s", subPath, key)}) } else { secretVol.VolumeSource.Secret.Items = append(secretVol.VolumeSource.Secret.Items, corev1.KeyToPath{Key: key, Path: key}) @@ -2677,15 +2671,13 @@ func (reconciler *ActiveMQArtemisReconcilerImpl) createExtraConfigmapsAndSecrets return extraVolumes, extraVolumeMounts, nil } -func extractOrdinalPrefixSeperatorIndex(key string) (bool, int) { - - prefixIndex := strings.Index(key, OrdinalPrefix) - separatorIndex := strings.Index(key, OrdinalPrefixSep) +var brokerPropertyWithOrdinalRegex *regexp.Regexp - if prefixIndex == 0 && separatorIndex > len(OrdinalPrefix) { - return true, separatorIndex +func ParseBrokerPropertyWithOrdinal(property string) []string { + if brokerPropertyWithOrdinalRegex == nil { + brokerPropertyWithOrdinalRegex = regexp.MustCompile("^(" + regexp.QuoteMeta(OrdinalPrefix) + "[0-9]+)" + regexp.QuoteMeta(OrdinalPrefixSep) + "(.*)$") } - return false, -1 + return brokerPropertyWithOrdinalRegex.FindStringSubmatch(property) } func (reconciler *ActiveMQArtemisReconcilerImpl) NewStatefulSetForCR(customResource *brokerv1beta1.ActiveMQArtemis, namer common.Namers, currentStateFullSet *appsv1.StatefulSet, client rtclient.Client) (*appsv1.StatefulSet, error) { @@ -3115,8 +3107,8 @@ func checkProjectionStatus(cr *brokerv1beta1.ActiveMQArtemis, client rtclient.Cl if !present { // with ordinal prefix or extras in the map this can be the case - isForOrdinal, _ := extractOrdinalPrefixSeperatorIndex(name) - if !(name == JaasConfigKey || strings.HasPrefix(name, UncheckedPrefix) || isForOrdinal) { + matches := ParseBrokerPropertyWithOrdinal(name) + if !(name == JaasConfigKey || strings.HasPrefix(name, UncheckedPrefix) || len(matches) > 0) { missingKeys = append(missingKeys, name) } continue diff --git a/controllers/activemqartemis_reconciler_test.go b/controllers/activemqartemis_reconciler_test.go index b1f35eb9d..2fb7d2632 100644 --- a/controllers/activemqartemis_reconciler_test.go +++ b/controllers/activemqartemis_reconciler_test.go @@ -1350,3 +1350,92 @@ func TestGetBrokerHost(t *testing.T) { ingressHost = formatTemplatedString(&cr, specIngressHost, "2", "my-console", "abc") assert.Equal(t, "test-test-ns-my-console-2-abc.my-domain.com", ingressHost) } + +func TestParseBrokerPropertyWithOrdinal(t *testing.T) { + var matches []string + + matches = ParseBrokerPropertyWithOrdinal("broker-0.maxDiskUsage") + assert.Equal(t, 3, len(matches)) + assert.Equal(t, "broker-0.maxDiskUsage", matches[0]) + assert.Equal(t, "broker-0", matches[1]) + assert.Equal(t, "maxDiskUsage", matches[2]) + + matches = ParseBrokerPropertyWithOrdinal("broker-999.maxDiskUsage=97") + assert.Equal(t, 3, len(matches)) + assert.Equal(t, "broker-999.maxDiskUsage=97", matches[0]) + assert.Equal(t, "broker-999", matches[1]) + assert.Equal(t, "maxDiskUsage=97", matches[2]) + + matches = ParseBrokerPropertyWithOrdinal("maxDiskUsage=97") + assert.Equal(t, 0, len(matches)) + + matches = ParseBrokerPropertyWithOrdinal("a.broker-0.maxDiskUsage") + assert.Equal(t, 0, len(matches)) + + matches = ParseBrokerPropertyWithOrdinal("broker-0-maxDiskUsage") + assert.Equal(t, 0, len(matches)) + + matches = ParseBrokerPropertyWithOrdinal("broker-a.maxDiskUsage") + assert.Equal(t, 0, len(matches)) +} + +func TestBrokerPropertiesData(t *testing.T) { + + data := BrokerPropertiesData([]string{ + "maxDiskUsage=97", + "minDiskFree=5", + }) + + assert.Equal(t, 1, len(data)) + + assert.True(t, strings.Contains(data[BrokerPropertiesName], "maxDiskUsage=97")) + assert.True(t, strings.Contains(data[BrokerPropertiesName], "minDiskFree=5")) +} + +func TestBrokerPropertiesDataWithOrdinal(t *testing.T) { + + data := BrokerPropertiesData([]string{ + "broker-0.maxDiskUsage=98", + "broker-0.minDiskFree=6", + "broker-999.maxDiskUsage=99", + "broker-999.minDiskFree=7", + }) + + assert.Equal(t, 3, len(data)) + + assert.False(t, strings.Contains(data[BrokerPropertiesName], "maxDiskUsage")) + assert.False(t, strings.Contains(data[BrokerPropertiesName], "minDiskFree")) + + broker0BrokerPropertiesName := "broker-0" + OrdinalPrefixSep + BrokerPropertiesName + assert.True(t, strings.Contains(data[broker0BrokerPropertiesName], "maxDiskUsage=98")) + assert.True(t, strings.Contains(data[broker0BrokerPropertiesName], "minDiskFree=6")) + + broker999BrokerPropertiesName := "broker-999" + OrdinalPrefixSep + BrokerPropertiesName + assert.True(t, strings.Contains(data[broker999BrokerPropertiesName], "maxDiskUsage=99")) + assert.True(t, strings.Contains(data[broker999BrokerPropertiesName], "minDiskFree=7")) +} + +func TestBrokerPropertiesDataWithAndWithoutOrdinal(t *testing.T) { + + data := BrokerPropertiesData([]string{ + "maxDiskUsage=97", + "minDiskFree=5", + "broker-0.maxDiskUsage=98", + "broker-0.minDiskFree=6", + "broker-999.maxDiskUsage=99", + "broker-999.minDiskFree=7", + }) + + assert.Equal(t, 3, len(data)) + + assert.True(t, strings.Contains(data[BrokerPropertiesName], "maxDiskUsage=97")) + assert.True(t, strings.Contains(data[BrokerPropertiesName], "minDiskFree=5")) + + broker0BrokerPropertiesName := "broker-0" + OrdinalPrefixSep + BrokerPropertiesName + assert.True(t, strings.Contains(data[broker0BrokerPropertiesName], "maxDiskUsage=98")) + assert.True(t, strings.Contains(data[broker0BrokerPropertiesName], "minDiskFree=6")) + + broker999BrokerPropertiesName := "broker-999" + OrdinalPrefixSep + BrokerPropertiesName + assert.True(t, strings.Contains(data[broker999BrokerPropertiesName], "maxDiskUsage=99")) + assert.True(t, strings.Contains(data[broker999BrokerPropertiesName], "minDiskFree=7")) +}