Skip to content

Commit

Permalink
fix: Only set ConstraintTemplate's status.created on success (#2208)
Browse files Browse the repository at this point in the history 
* fix: clean up error consts in constrainttemplate controller

Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>

* fix: only set CT status.created on success

Only set ConstraintTemplate status.created to true if at least one
ConstraintTemplatePodStatus had no errors

Fixes #2053

Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>

* fix: add tests for CT status.created

Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>

* style: const for a constraint name in tests

Signed-off-by: Viktor Oreshkin <imselfish@stek29.rocks>
  • Loading branch information
@stek29
stek29 committed Aug 5, 2022
1 parent 33deaec commit e19a6c8
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 17 deletions.
15 changes: 10 additions & 5 deletions pkg/controller/constrainttemplate/constants.go
View file
@@ -1,7 +1,12 @@
package constrainttemplate

// ErrCreateCode indicates a problem creating a ConstraintTemplate.
const ErrCreateCode = "create_error"

// ErrIngestCode indicates a problem creating a ConstraintTemplate.
const ErrIngestCode = "ingest_error"
const (
// ErrCreateCode indicates a problem creating a ConstraintTemplate CRD.
ErrCreateCode = "create_error"
// ErrUpdateCode indicates a problem updating a ConstraintTemplate CRD.
ErrUpdateCode = "update_error"
// ErrConversionCode indicates a problem converting a ConstraintTemplate CRD.
ErrConversionCode = "conversion_error"
// ErrIngestCode indicates a problem ingesting a ConstraintTemplate Rego code.
ErrIngestCode = "ingest_error"
)
6 changes: 3 additions & 3 deletions pkg/controller/constrainttemplate/constrainttemplate_controller.go
View file
Expand Up @@ -356,7 +356,7 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
r.tracker.TryCancelTemplate(unversionedCT) // Don't track templates that failed compilation
r.metrics.registry.add(request.NamespacedName, metrics.ErrorStatus)
logError(request.NamespacedName.Name)
err := r.reportErrorOnCTStatus(ctx, "conversion_error", "Could not convert from unversioned resource", status, err)
err := r.reportErrorOnCTStatus(ctx, ErrConversionCode, "Could not convert from unversioned resource", status, err)
return reconcile.Result{}, err
}

Expand Down Expand Up @@ -426,7 +426,7 @@ func (r *ReconcileConstraintTemplate) handleUpdate(
if err := r.metrics.reportIngestDuration(ctx, metrics.ErrorStatus, time.Since(beginCompile)); err != nil {
logger.Error(err, "failed to report constraint template ingestion duration")
}
err := r.reportErrorOnCTStatus(ctx, "ingest_error", "Could not ingest Rego", status, err)
err := r.reportErrorOnCTStatus(ctx, ErrIngestCode, "Could not ingest Rego", status, err)
r.tracker.TryCancelTemplate(unversionedCT) // Don't track templates that failed compilation
return reconcile.Result{}, err
}
Expand Down Expand Up @@ -461,7 +461,7 @@ func (r *ReconcileConstraintTemplate) handleUpdate(
} else if !reflect.DeepEqual(newCRD, currentCRD) {
logger.Info("updating crd")
if err := r.Update(ctx, newCRD); err != nil {
err := r.reportErrorOnCTStatus(ctx, "update_error", "Could not update CRD", status, err)
err := r.reportErrorOnCTStatus(ctx, ErrUpdateCode, "Could not update CRD", status, err)
return reconcile.Result{}, err
}
}
Expand Down
8 changes: 7 additions & 1 deletion pkg/controller/constrainttemplatestatus/constrainttemplatestatus_controller.go
View file
Expand Up @@ -177,13 +177,19 @@ func (r *ReconcileConstraintStatus) Reconcile(ctx context.Context, request recon
sort.Sort(statusObjs)

var s []interface{}
// created is true if at least one Pod hasn't reported any errors
var created bool

for i := range statusObjs {
// Don't report status if it's not for the correct object. This can happen
// if a watch gets interrupted, causing the constraint status to be deleted
// out from underneath it
if statusObjs[i].Status.TemplateUID != template.GetUID() {
continue
}
if len(statusObjs[i].Status.Errors) == 0 {
created = true
}
j, err := json.Marshal(statusObjs[i].Status)
if err != nil {
return reconcile.Result{}, err
Expand All @@ -198,7 +204,7 @@ func (r *ReconcileConstraintStatus) Reconcile(ctx context.Context, request recon
return reconcile.Result{}, err
}

if err := unstructured.SetNestedField(template.Object, len(s) > 0, "status", "created"); err != nil {
if err := unstructured.SetNestedField(template.Object, created, "status", "created"); err != nil {
return reconcile.Result{}, err
}

Expand Down
88 changes: 80 additions & 8 deletions pkg/controller/constrainttemplatestatus/constrainttemplatestatus_controller_test.go
View file
Expand Up @@ -31,7 +31,12 @@ import (
"sigs.k8s.io/controller-runtime/pkg/manager"
)

const timeout = time.Second * 15
const (
timeout = 15 * time.Second

contraintTemplateName = "denyall"
constraintCRDName = "DenyAll"
)

// setupManager sets up a controller-runtime manager with registered watch manager.
func setupManager(t *testing.T) (manager.Manager, *watch.Manager) {
Expand Down Expand Up @@ -66,12 +71,12 @@ func setupManager(t *testing.T) (manager.Manager, *watch.Manager) {
func TestReconcile(t *testing.T) {
g := gomega.NewGomegaWithT(t)
template := &v1beta1.ConstraintTemplate{
ObjectMeta: metav1.ObjectMeta{Name: "denyall"},
ObjectMeta: metav1.ObjectMeta{Name: contraintTemplateName},
Spec: v1beta1.ConstraintTemplateSpec{
CRD: v1beta1.CRD{
Spec: v1beta1.CRDSpec{
Names: v1beta1.Names{
Kind: "DenyAll",
Kind: constraintCRDName,
},
},
},
Expand Down Expand Up @@ -157,6 +162,7 @@ violation[{"msg": "denied!"}] {
t.Fatal(err)
}
g.Eventually(verifyTStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
g.Eventually(verifyTStatusCreated(ctx, c, true), timeout).Should(gomega.BeNil())
g.Eventually(verifyTByPodStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
})

Expand All @@ -174,7 +180,7 @@ violation[{"msg": "denied!"}] {
fakePod := pod.DeepCopy()
fakePod.SetName("fake-pod")
t.Run("Multiple constraint template statuses are reported", func(t *testing.T) {
fakeTStatus, err := podstatus.NewConstraintTemplateStatusForPod(fakePod, "denyall", mgr.GetScheme())
fakeTStatus, err := podstatus.NewConstraintTemplateStatusForPod(fakePod, contraintTemplateName, mgr.GetScheme())
if err != nil {
t.Fatal(err)
}
Expand All @@ -188,6 +194,35 @@ violation[{"msg": "denied!"}] {
if err != nil {
t.Fatal(err)
}
g.Eventually(verifyTStatusCreated(ctx, c, true), timeout).Should(gomega.BeNil())
g.Eventually(verifyTByPodStatusCount(ctx, c, 2), timeout).Should(gomega.BeNil())
err = c.Delete(ctx, fakeTStatus)
if err != nil {
t.Fatal(err)
}
g.Eventually(verifyTByPodStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
})

t.Run("Constraint template status.created is true even if some pod has errors", func(t *testing.T) {
fakeTStatus, err := podstatus.NewConstraintTemplateStatusForPod(fakePod, contraintTemplateName, mgr.GetScheme())
if err != nil {
t.Fatal(err)
}
fakeTStatus.Status.TemplateUID = templateCpy.UID
fakeTStatus.Status.Errors = []*v1beta1.CreateCRDError{{
Code: constrainttemplate.ErrCreateCode,
Message: "Could not create CRD: error",
}}

// TODO: Test if this removal is necessary.
// https://github.com/open-policy-agent/gatekeeper/pull/1595#discussion_r722819552
t.Cleanup(testutils.DeleteObject(t, c, fakeTStatus))

err = c.Create(ctx, fakeTStatus)
if err != nil {
t.Fatal(err)
}
g.Eventually(verifyTStatusCreated(ctx, c, true), timeout).Should(gomega.BeNil())
g.Eventually(verifyTByPodStatusCount(ctx, c, 2), timeout).Should(gomega.BeNil())
err = c.Delete(ctx, fakeTStatus)
if err != nil {
Expand Down Expand Up @@ -260,6 +295,7 @@ violation[{"msg": "denied!"}] {
t.Fatal(err)
}
g.Eventually(verifyTStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
g.Eventually(verifyTStatusCreated(ctx, c, true), timeout).Should(gomega.BeNil())
g.Eventually(verifyTByPodStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
g.Eventually(verifyCStatusCount(ctx, c, 0), timeout).Should(gomega.BeNil())
err = c.Create(ctx, constraint)
Expand All @@ -269,7 +305,7 @@ violation[{"msg": "denied!"}] {
g.Eventually(verifyCStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
g.Eventually(verifyCByPodStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())

fakeTStatus, err := podstatus.NewConstraintTemplateStatusForPod(fakePod, "denyall", mgr.GetScheme())
fakeTStatus, err := podstatus.NewConstraintTemplateStatusForPod(fakePod, contraintTemplateName, mgr.GetScheme())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -306,6 +342,29 @@ violation[{"msg": "denied!"}] {
g.Eventually(verifyTStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
g.Eventually(verifyCStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
})

templateCpy = template.DeepCopy()
templateCpy.Spec.Targets[0].Rego = `
package foo
violation[invalid syntax error`

t.Run("Invalid ContraintTemplate has .status.created set to false", func(t *testing.T) {
g.Eventually(verifyTStatusCount(ctx, c, 0), timeout).Should(gomega.BeNil())
err := c.Create(ctx, templateCpy)
if err != nil {
t.Fatal(err)
}
g.Eventually(verifyTStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
g.Eventually(verifyTByPodStatusCount(ctx, c, 1), timeout).Should(gomega.BeNil())
g.Eventually(verifyTStatusCreated(ctx, c, false), timeout).Should(gomega.BeNil())

err = c.Delete(ctx, templateCpy)
if err != nil {
t.Fatal(err)
}
g.Eventually(verifyTStatusCount(ctx, c, 0), timeout).Should(gomega.BeNil())
})
}

func verifyTStatusCount(ctx context.Context, c client.Client, expected int) func() error {
Expand All @@ -321,10 +380,23 @@ func verifyTStatusCount(ctx context.Context, c client.Client, expected int) func
}
}

func verifyTStatusCreated(ctx context.Context, c client.Client, expected bool) func() error {
return func() error {
ct := &v1beta1.ConstraintTemplate{}
if err := c.Get(ctx, types.NamespacedName{Name: contraintTemplateName}, ct); err != nil {
return err
}
if ct.Status.Created != expected {
return fmt.Errorf("status.created = %t, wanted %t", ct.Status.Created, expected)
}
return nil
}
}

func verifyTByPodStatusCount(ctx context.Context, c client.Client, expected int) func() error {
return func() error {
ct := &v1beta1.ConstraintTemplate{}
if err := c.Get(ctx, types.NamespacedName{Name: "denyall"}, ct); err != nil {
if err := c.Get(ctx, types.NamespacedName{Name: contraintTemplateName}, ct); err != nil {
return err
}
if len(ct.Status.ByPod) != expected {
Expand Down Expand Up @@ -369,8 +441,8 @@ func newDenyAllConstraint() *unstructured.Unstructured {
constraint.SetGroupVersionKind(schema.GroupVersionKind{
Group: "constraints.gatekeeper.sh",
Version: "v1beta1",
Kind: "DenyAll",
Kind: constraintCRDName,
})
constraint.SetName("denyallconstraint")
constraint.SetName(contraintTemplateName + "constraint")
return constraint
}

0 comments on commit e19a6c8

Please sign in to comment.