Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OperatorPolicy Templates #235

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions controllers/configurationpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -971,28 +971,6 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi

// process object templates for go template usage
for i, rawData := range rawDataList {
// first check to make sure there are no hub-templates with delimiter - {{hub
// if one exists, it means the template resolution on the hub did not succeed.
if templates.HasTemplate(rawData, "{{hub", false) {
// check to see there is an annotation set to the hub error msg,
// if not ,set a generic msg
hubTemplatesErrMsg, ok := annotations["policy.open-cluster-management.io/hub-templates-error"]
if !ok || hubTemplatesErrMsg == "" {
// set a generic msg
hubTemplatesErrMsg = "Error occurred while processing hub-templates, " +
"check the policy events for more details."
}

log.Info(
"An error occurred while processing hub-templates on the Hub cluster. Cannot process the policy.",
"message", hubTemplatesErrMsg,
)

addTemplateErrorViolation("Error processing hub templates", hubTemplatesErrMsg)

return
}

if templates.HasTemplate(rawData, "", true) {
log.V(1).Info("Processing policy templates")

Expand Down
78 changes: 59 additions & 19 deletions controllers/operatorpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ import (
"reflect"
"regexp"
"slices"
"strconv"
"strings"

operatorv1 "github.com/operator-framework/api/pkg/operators/v1"
operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
templates "github.com/stolostron/go-template-utils/v4/pkg/templates"
depclient "github.com/stolostron/kubernetes-dependency-watches/client"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -316,7 +318,24 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p

validationErrors := make([]error, 0)

sub, subErr := buildSubscription(policy, r.DefaultNamespace)
disableTemplates := false

if disableAnnotation, ok := policy.GetAnnotations()["policy.open-cluster-management.io/disable-templates"]; ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this is an annotation instead of a spec field but I guess consistency with configuration policy is probably better than changing this at this point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed 😆

disableTemplates, _ = strconv.ParseBool(disableAnnotation) // on error, templates will not be disabled
}

var tmplResolver *templates.TemplateResolver

if !disableTemplates {
var err error

tmplResolver, err = templates.NewResolverWithDynamicWatcher(r.DynamicWatcher, templates.Config{})
if err != nil {
validationErrors = append(validationErrors, fmt.Errorf("unable to create template resolver: %w", err))
}
}

sub, subErr := buildSubscription(policy, r.DefaultNamespace, tmplResolver)
if subErr == nil {
err := r.applySubscriptionDefaults(ctx, sub)
if err != nil {
Expand All @@ -339,7 +358,7 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
opGroupNS = sub.Namespace
}

opGroup, ogErr := buildOperatorGroup(policy, opGroupNS)
opGroup, ogErr := buildOperatorGroup(policy, opGroupNS, tmplResolver)
if ogErr != nil {
validationErrors = append(validationErrors, ogErr)
} else {
Expand Down Expand Up @@ -434,13 +453,26 @@ func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
// If an error is returned, it will include details on why the policy spec if invalid and
// why the desired subscription can't be determined.
func buildSubscription(
policy *policyv1beta1.OperatorPolicy, defaultNS string,
policy *policyv1beta1.OperatorPolicy, defaultNS string, tmplResolver *templates.TemplateResolver,
) (*operatorv1alpha1.Subscription, error) {
subscription := new(operatorv1alpha1.Subscription)

rawSub := policy.Spec.Subscription.Raw

if tmplResolver != nil && templates.HasTemplate(rawSub, "", false) {
watcher := opPolIdentifier(policy.Namespace, policy.Name)

resolvedTmpl, err := tmplResolver.ResolveTemplate(rawSub, nil, &templates.ResolveOptions{Watcher: &watcher})
if err != nil {
return nil, fmt.Errorf("could not build subscription: %w", err)
}

rawSub = resolvedTmpl.ResolvedJSON
}

sub := make(map[string]interface{})

err := json.Unmarshal(policy.Spec.Subscription.Raw, &sub)
err := json.Unmarshal(rawSub, &sub)
if err != nil {
return nil, fmt.Errorf("the policy spec.subscription is invalid: %w", err)
}
Expand Down Expand Up @@ -510,7 +542,7 @@ func buildSubscription(
// buildOperatorGroup bootstraps the OperatorGroup spec defined in the operator policy
// with the apiversion and kind in preparation for resource creation
func buildOperatorGroup(
policy *policyv1beta1.OperatorPolicy, namespace string,
policy *policyv1beta1.OperatorPolicy, namespace string, tmplResolver *templates.TemplateResolver,
) (*operatorv1.OperatorGroup, error) {
operatorGroup := new(operatorv1.OperatorGroup)

Expand All @@ -526,9 +558,22 @@ func buildOperatorGroup(
return operatorGroup, nil
}

rawOG := policy.Spec.OperatorGroup.Raw

if tmplResolver != nil && templates.HasTemplate(rawOG, "", false) {
watcher := opPolIdentifier(policy.Namespace, policy.Name)

resolvedTmpl, err := tmplResolver.ResolveTemplate(rawOG, nil, &templates.ResolveOptions{Watcher: &watcher})
if err != nil {
return nil, fmt.Errorf("could not build operator group: %w", err)
}

rawOG = resolvedTmpl.ResolvedJSON
}

opGroup := make(map[string]interface{})

if err := json.Unmarshal(policy.Spec.OperatorGroup.Raw, &opGroup); err != nil {
if err := json.Unmarshal(rawOG, &opGroup); err != nil {
return nil, fmt.Errorf("the policy spec.operatorGroup is invalid: %w", err)
}

Expand Down Expand Up @@ -659,10 +704,8 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
return nil, false, fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
}

merged := opGroup.DeepCopy() // Copy it so that the value in the cache is not changed

updateNeeded, skipUpdate, err := r.mergeObjects(
ctx, desiredUnstruct, merged, string(policy.Spec.ComplianceType),
ctx, desiredUnstruct, &opGroup, string(policy.Spec.ComplianceType),
)
if err != nil {
return nil, false, fmt.Errorf("error checking if the OperatorGroup needs an update: %w", err)
Expand Down Expand Up @@ -702,7 +745,7 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(

desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion()

err = r.Update(ctx, merged)
err = r.Update(ctx, &opGroup)
if err != nil {
return nil, changed, fmt.Errorf("error updating the OperatorGroup: %w", err)
}
Expand Down Expand Up @@ -929,15 +972,13 @@ func (r *OperatorPolicyReconciler) musthaveSubscription(
return nil, nil, false, fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
}

merged := foundSub.DeepCopy() // Copy it so that the value in the cache is not changed

updateNeeded, skipUpdate, err := r.mergeObjects(ctx, desiredUnstruct, merged, string(policy.Spec.ComplianceType))
updateNeeded, skipUpdate, err := r.mergeObjects(ctx, desiredUnstruct, foundSub, string(policy.Spec.ComplianceType))
if err != nil {
return nil, nil, false, fmt.Errorf("error checking if the Subscription needs an update: %w", err)
}

mergedSub := new(operatorv1alpha1.Subscription)
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(merged.Object, mergedSub); err != nil {
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(foundSub.Object, mergedSub); err != nil {
return nil, nil, false, fmt.Errorf("error converting the retrieved Subscription to the go type: %w", err)
}

Expand Down Expand Up @@ -995,14 +1036,14 @@ func (r *OperatorPolicyReconciler) musthaveSubscription(
earlyConds = append(earlyConds, calculateComplianceCondition(policy))
}

err = r.Update(ctx, merged)
err = r.Update(ctx, foundSub)
if err != nil {
return mergedSub, nil, changed, fmt.Errorf("error updating the Subscription: %w", err)
}

merged.SetGroupVersionKind(subscriptionGVK) // Update stripped this information
foundSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information

updateStatus(policy, updatedCond("Subscription"), updatedObj(merged))
updateStatus(policy, updatedCond("Subscription"), updatedObj(foundSub))

return mergedSub, earlyConds, true, nil
}
Expand Down Expand Up @@ -1651,11 +1692,10 @@ func (r *OperatorPolicyReconciler) handleCatalogSource(

if !isMissing {
// CatalogSource is found, initiate health check
catalogSrcUnstruct := foundCatalogSrc.DeepCopy()
catalogSrc := new(operatorv1alpha1.CatalogSource)

err := runtime.DefaultUnstructuredConverter.
FromUnstructured(catalogSrcUnstruct.Object, catalogSrc)
FromUnstructured(foundCatalogSrc.Object, catalogSrc)
if err != nil {
return false, fmt.Errorf("error converting the retrieved CatalogSource to the Go type: %w", err)
}
Expand Down
6 changes: 3 additions & 3 deletions controllers/operatorpolicy_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestBuildSubscription(t *testing.T) {
}

// Check values are correctly bootstrapped to the Subscription
ret, err := buildSubscription(testPolicy, "my-operators")
ret, err := buildSubscription(testPolicy, "my-operators", nil)
assert.Equal(t, err, nil)
assert.Equal(t, ret.GroupVersionKind(), desiredGVK)
assert.Equal(t, ret.ObjectMeta.Name, "my-operator")
Expand Down Expand Up @@ -102,7 +102,7 @@ func TestBuildSubscriptionInvalidNames(t *testing.T) {
}

// Check values are correctly bootstrapped to the Subscription
_, err := buildSubscription(testPolicy, "my-operators")
_, err := buildSubscription(testPolicy, "my-operators", nil)
assert.Equal(t, err.Error(), test.expected)
},
)
Expand Down Expand Up @@ -138,7 +138,7 @@ func TestBuildOperatorGroup(t *testing.T) {
}

// Ensure OperatorGroup values are populated correctly
ret, err := buildOperatorGroup(testPolicy, "my-operators")
ret, err := buildOperatorGroup(testPolicy, "my-operators", nil)
assert.Equal(t, err, nil)
assert.Equal(t, ret.GroupVersionKind(), desiredGVK)
assert.Equal(t, ret.ObjectMeta.GetGenerateName(), "my-operators-")
Expand Down
6 changes: 3 additions & 3 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ require (
github.com/prometheus/client_golang v1.18.0
github.com/spf13/pflag v1.0.5
github.com/stolostron/go-log-utils v0.1.2
github.com/stolostron/go-template-utils/v4 v4.0.0
github.com/stolostron/kubernetes-dependency-watches v0.5.2
github.com/stolostron/go-template-utils/v4 v4.1.0
github.com/stolostron/kubernetes-dependency-watches v0.6.0
github.com/stretchr/testify v1.8.4
golang.org/x/mod v0.16.0
k8s.io/api v0.29.2
Expand Down Expand Up @@ -56,7 +56,7 @@ require (
github.com/google/gofuzz v1.2.0 // indirect
github.com/google/pprof v0.0.0-20240402174815-29b9bb013b0f // indirect
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
github.com/google/uuid v1.3.1 // indirect
github.com/google/uuid v1.4.0 // indirect
github.com/huandu/xstrings v1.4.0 // indirect
github.com/imdario/mergo v1.0.0 // indirect
github.com/josharian/intern v1.0.0 // indirect
Expand Down
12 changes: 6 additions & 6 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ github.com/google/pprof v0.0.0-20240402174815-29b9bb013b0f/go.mod h1:kf6iHlnVGwg
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaUGG7oYTSPP8MxqL4YI3kZKwcP4=
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ=
github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4=
github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/google/uuid v1.4.0 h1:MtMxsa51/r9yyhkyLsVeVt0B+BGQZzpQiTQ4eHZ8bc4=
github.com/google/uuid v1.4.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/huandu/xstrings v1.3.3/go.mod h1:y5/lhBue+AyNmUVz9RLU9xbLR0o4KIIExikq4ovT0aE=
github.com/huandu/xstrings v1.4.0 h1:D17IlohoQq4UcpqD7fDk80P7l+lwAmlFaBHgOipl2FU=
github.com/huandu/xstrings v1.4.0/go.mod h1:y5/lhBue+AyNmUVz9RLU9xbLR0o4KIIExikq4ovT0aE=
Expand Down Expand Up @@ -142,10 +142,10 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
github.com/stolostron/go-log-utils v0.1.2 h1:7l1aJWvBqU2+DUyimcslT5SJpdygVY/clRDmX5sO29c=
github.com/stolostron/go-log-utils v0.1.2/go.mod h1:8zrB8UJmp1rXhv3Ck9bBl5SpNfKk3SApeElbo96YRtQ=
github.com/stolostron/go-template-utils/v4 v4.0.0 h1:gvSfhXIoymo5Ql9MH/ofTTOtBVkaUBq8HokCGR4xkkc=
github.com/stolostron/go-template-utils/v4 v4.0.0/go.mod h1:svIOPUJpG/ObRn3WwZMBGMEMsBgKH8LVfhsaIArgAAQ=
github.com/stolostron/kubernetes-dependency-watches v0.5.2 h1:TtctOgPn+TYo1dJ+dr9TLR2rlpy5aFQ/1dzfcYZUDi4=
github.com/stolostron/kubernetes-dependency-watches v0.5.2/go.mod h1:5nwtuleCNR9Mno7SJRz6B2BavvXHbfR4rIwzKLkk9G8=
github.com/stolostron/go-template-utils/v4 v4.1.0 h1:i7S6gTmFixZB25zY03pR5rC30PVJGDFfFuLi15Oqq6g=
github.com/stolostron/go-template-utils/v4 v4.1.0/go.mod h1:pEezMPxvuvllc68oLUejh9tfM8r/Yi7s10ZlZhVkyM4=
github.com/stolostron/kubernetes-dependency-watches v0.6.0 h1:FxXsslLNH5hNojVvKX467iefCEwigdMftfmlYX/tddg=
github.com/stolostron/kubernetes-dependency-watches v0.6.0/go.mod h1:6v54aX8Bxx1m9YETZUxNGUrSHcRIArM/YnOqnUbIB/g=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
Expand Down
6 changes: 5 additions & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,11 @@ func main() {
depReconciler, depEvents := depclient.NewControllerRuntimeSource()

watcher, err := depclient.New(cfg, depReconciler,
&depclient.Options{DisableInitialReconcile: true, EnableCache: true})
&depclient.Options{
DisableInitialReconcile: true,
EnableCache: true,
ObjectCacheOptions: depclient.ObjectCacheOptions{UnsafeDisableDeepCopy: false},
})
if err != nil {
log.Error(err, "Unable to create dependency watcher")
os.Exit(1)
Expand Down
38 changes: 0 additions & 38 deletions test/e2e/case13_templatization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -461,44 +461,6 @@ var _ = Describe("Test templatization", Ordered, func() {
return configmap
}, defaultTimeoutSeconds, 1).ShouldNot(BeNil())

By("Patch with invalid hub template")
Copy link
Contributor

@yiraeChristineKim yiraeChristineKim Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now handled in the governance-policy-framework:
open-cluster-management-io/governance-policy-framework-addon#136

utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p",
`[{ "op": "replace",
"path": "/spec/object-templates/0/objectDefinition/data/test",
"value": '{{hub "default" "e2esecret" dddddd vvvvv d hub}}' }]`,
"-n", testNamespace)

By("By verifying that the configurationpolicy is NonCompliant")
Eventually(func() interface{} {
managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds)

return utils.GetComplianceState(managedPlc)
}, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant"))

By("By verifying that the configmap still exist ")
Consistently(func() interface{} {
configmap := utils.GetWithTimeout(clientManagedDynamic, gvrConfigMap,
case13PruneTmpErr+"-configmap", "default", true, defaultTimeoutSeconds)

return configmap
}, defaultConsistentlyDuration, 1).ShouldNot(BeNil())

By("Change to valid configmap")
utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p",
`[{ "op": "replace",
"path": "/spec/object-templates/0/objectDefinition/data/test",
"value": "working" }]`,
"-n", testNamespace)

By("By verifying that the configmap has no error")
Eventually(func() interface{} {
managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds)

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

By("Patch with invalid managed template")
utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p",
`[{ "op": "replace",
Expand Down
Loading