Skip to content

Commit

Permalink
Bug: ACM-5052, Policy gets shortly into non-compliant state
Browse files Browse the repository at this point in the history
Solution: decode automatically converted stringData prior to evaluation

Signed-off-by: Jeffrey Luo <jeluo@redhat.com>
  • Loading branch information
JeffeyL committed Jun 21, 2023
1 parent 418c94c commit 40fe477
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 0 deletions.
25 changes: 25 additions & 0 deletions controllers/configurationpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package controllers

import (
"context"
"encoding/base64"
"errors"
"fmt"
"reflect"
Expand Down Expand Up @@ -2505,6 +2506,7 @@ func handleSingleKey(

desiredValue := formatTemplate(desiredObj, key)
existingValue := existingObj.UnstructuredContent()[key]

typeErr := ""

// We will compare the existing field to a "merged" field which has the fields in the template
Expand Down Expand Up @@ -2556,6 +2558,29 @@ func handleSingleKey(
mergedValue.(map[string]interface{}), existingValue.(map[string]interface{}))
}

if key == "stringData" && existingObj.UnstructuredContent()["kind"] == "Secret" {
// override automatic conversion from stringData to data prior to evaluation
existingValue = existingObj.UnstructuredContent()["data"]
value, ok := existingValue.(map[string]interface{})

if !ok {
message := "Error casting encoded data"

return message, false, mergedValue, false
}

for k, val := range value {
decodedVal, err := base64.StdEncoding.DecodeString(val.(string))
if err != nil {
message := fmt.Sprintf("Error decoding secret: %s", k)

return message, false, mergedValue, false
}

existingValue.(map[string]interface{})[k] = string(decodedVal)
}
}

// sort objects before checking equality to ensure they're in the same order
if !equalObjWithSort(mergedValue, existingValue) {
updateNeeded = true
Expand Down
69 changes: 69 additions & 0 deletions test/e2e/case32_secret_stringdata_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright Contributors to the Open Cluster Management project

package e2e

import (
"strconv"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

"open-cluster-management.io/config-policy-controller/test/utils"
)

const (
case32ConfigPolicyName string = "case32config"
case32CreatePolicyYaml string = "../resources/case32_secret_stringdata/case32_create_secret.yaml"
)

var _ = Describe("Test converted stringData being decoded before comparison for Secrets", Ordered, func() {
It("Config should be created properly on the managed cluster", func() {
By("Creating " + case32ConfigPolicyName + " on managed")
utils.Kubectl("apply", "-f", case32CreatePolicyYaml, "-n", testNamespace)
cfg := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
case32ConfigPolicyName, testNamespace, true, defaultTimeoutSeconds)
Expect(cfg).NotTo(BeNil())
})

It("Verifies the config policy is initially compliant "+case32ConfigPolicyName+" in "+testNamespace, func() {
By("Waiting for " + case32ConfigPolicyName + " to become Compliant for up to " +
strconv.Itoa(defaultTimeoutSeconds) + " seconds")
Eventually(func() interface{} {
cfgplc := utils.GetWithTimeout(
clientManagedDynamic, gvrConfigPolicy,
case32ConfigPolicyName, testNamespace,
true, defaultTimeoutSeconds,
)

return utils.GetComplianceState(cfgplc)
}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
})

It("Verifies that a secret is created by "+case32ConfigPolicyName+" in openshift-config", func() {
By("Grabbing " + "htpasswd-secret from namespace openshift-config")
scrt := utils.GetWithTimeout(
clientManagedDynamic, gvrSecret, "htpasswd-secret", testNamespace, true, defaultTimeoutSeconds,
)

Expect(scrt).NotTo(BeNil())
})

It("Verifies the config policy stays compliant "+case32ConfigPolicyName+" in "+testNamespace, func() {
By("Checking the events on the configuration policy" + case32ConfigPolicyName)
Consistently(func() int {
eventlen := len(utils.GetMatchingEvents(
clientManaged, testNamespace,
case32ConfigPolicyName, case32ConfigPolicyName,
"NonCompliant;", defaultTimeoutSeconds))

return eventlen
}, defaultTimeoutSeconds, 5).Should(BeNumerically("<", 3))
})

AfterAll(func() {
utils.Kubectl("delete", "configurationpolicy", case32ConfigPolicyName, "-n", testNamespace)
utils.Kubectl("delete", "secret", "htpasswd-secret", "-n", testNamespace)
utils.Kubectl("delete", "event",
"--field-selector=involvedObject.name="+case32ConfigPolicyName, "-n", "managed")
})
})
24 changes: 24 additions & 0 deletions test/resources/case32_secret_stringdata/case32_create_secret.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
metadata:
name: case32config
spec:
evaluationInterval:
compliant: 15s
noncompliant: 15s
object-templates:
- complianceType: musthave
objectDefinition:
apiVersion: v1
kind: Secret
metadata:
name: htpasswd-secret
namespace: managed
stringData:
htpasswd: |
alice:$QGJM$2qku.rrSEWRFD/tjOf.dZQ1q.RaR5tTmfNyZn0$1uI6eDx/Ont37ws
bob:S1KPovd0N/iDb9$dlTBH7weXG5rQV$5EPf9KrbJjw2HXy35ujhPw2y0$SIto
cadence:W$MeWCMWuKi0ApUyZ8byG5$DNL.3FD$c72kFhyZIX8Sf0yJH2AEOGFsRgEHJ
pruneObjectBehavior: None
remediationAction: enforce
severity: medium

0 comments on commit 40fe477

Please sign in to comment.