Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
  • Loading branch information
@ritazh
ritazh committed Feb 16, 2024
1 parent 766e1ff commit 8b5048a
Show file tree
Hide file tree
Showing 9 changed files with 137 additions and 171 deletions.
6 changes: 3 additions & 3 deletions .github/workflows/workflow.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ jobs:
build_test:
name: "Build and Test"
runs-on: ubuntu-22.04
timeout-minutes: 15
timeout-minutes: 20
strategy:
matrix:
KUBERNETES_VERSION: ["1.25.8", "1.26.3", "1.27.1", "1.28.0"]
Expand Down Expand Up @@ -177,7 +177,7 @@ jobs:
IMG=gatekeeper-e2e:latest \
USE_LOCAL_IMG=true
make test-e2e
make test-e2e KUBERNETES_VERSION=${{ matrix.KUBERNETES_VERSION }}
- name: Save logs
if: ${{ always() }}
Expand Down Expand Up @@ -263,7 +263,7 @@ jobs:
build_test_generator_expansion:
name: "[Generator Resource Expansion] Build and Test"
runs-on: ubuntu-22.04
timeout-minutes: 15
timeout-minutes: 20

steps:
- name: Harden Runner
Expand Down
87 changes: 38 additions & 49 deletions pkg/controller/constraint/constraint_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ import (
"context"
"errors"
"fmt"
"reflect"
"strings"
"sync"

"github.com/go-logr/logr"
v1beta1 "github.com/open-policy-agent/frameworks/constraint/pkg/apis/templates/v1beta1"
constraintclient "github.com/open-policy-agent/frameworks/constraint/pkg/client"
"github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/k8scel/transform"
"github.com/open-policy-agent/frameworks/constraint/pkg/core/constraints"
constraintstatusv1beta1 "github.com/open-policy-agent/gatekeeper/v3/apis/status/v1beta1"
"github.com/open-policy-agent/gatekeeper/v3/pkg/controller/config/process"
"github.com/open-policy-agent/gatekeeper/v3/pkg/controller/constraintstatus"
Expand All @@ -36,7 +36,7 @@ import (
"github.com/open-policy-agent/gatekeeper/v3/pkg/readiness"
"github.com/open-policy-agent/gatekeeper/v3/pkg/util"
"github.com/open-policy-agent/gatekeeper/v3/pkg/watch"
admissionregistrationv1alpha1 "k8s.io/api/admissionregistration/v1alpha1"
admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
Expand All @@ -59,6 +59,8 @@ import (

var log = logf.Log.WithName("controller").WithValues(logging.Process, "constraint_controller")

var vapMux sync.RWMutex

var VapAPIEnabled *bool

var VapEnforcement VapFlagType
Expand All @@ -85,6 +87,15 @@ func (v *VapFlagType) Set(value string) error {
return fmt.Errorf("invalid value %s. Allowed values are %s, %s, %s", value, VapFlagNone, VapFlagGatekeeperDefault, VapFlagVapDefault)
}

// setting defaults when not set; required for unit test

Check failure on line 90 in pkg/controller/constraint/constraint_controller.go

View workflow job for this annotation

GitHub Actions / Lint 

Comment should end in a period (godot)
func (v *VapFlagType) SetDefaultIfEmpty() {
if *v == "" {
*v = VapFlagType(VapFlagGatekeeperDefault)
VapAPIEnabled = new(bool)
*VapAPIEnabled = true
}
}

type Adder struct {
CFClient *constraintclient.Client
ConstraintsCache *ConstraintsCache
Expand Down Expand Up @@ -145,9 +156,8 @@ type ConstraintsCache struct {
}

type tags struct {
enforcementAction util.EnforcementAction
status metrics.Status
generateVapBinding bool
enforcementAction util.EnforcementAction
status metrics.Status
}

// newReconciler returns a new reconcile.Reconciler.
Expand Down Expand Up @@ -230,8 +240,7 @@ type ReconcileConstraint struct {
// that would otherwise trigger a watch. The bool returns whether
// the function was executed, which can be used to determine
// whether the reconciler should infer the object has been deleted
ifWatching func(schema.GroupVersionKind, func() error) (bool, error)
generateVapBinding bool
ifWatching func(schema.GroupVersionKind, func() error) (bool, error)
}

// +kubebuilder:rbac:groups=constraints.gatekeeper.sh,resources=*,verbs=get;list;watch;create;update;patch;delete
Expand Down Expand Up @@ -263,6 +272,7 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
return reconcile.Result{}, nil
}

generateVapBinding := false
deleted := false
instance := &unstructured.Unstructured{}
instance.SetGroupVersionKind(gvk)
Expand Down Expand Up @@ -294,7 +304,7 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
}
// unless constraint vap label is false, default to parent
if useVap == "no" {
r.generateVapBinding = false
generateVapBinding = false
} else {
log.Info("constraint resource use-vap label is not no; will default to parent constraint template label")
parentCTUseVap, err := r.getCTVapLabel(ctx, instance.GetKind())
Expand All @@ -303,8 +313,8 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
return reconcile.Result{}, err
}
log.Info("constraint resource", "parentCTUseVap", parentCTUseVap)
r.generateVapBinding = ShouldGenerateVap(parentCTUseVap)
log.Info("constraint resource", "generateVapBinding", r.generateVapBinding)
generateVapBinding = ShouldGenerateVap(parentCTUseVap)
log.Info("constraint resource", "generateVapBinding", generateVapBinding)
}
constraintKey := strings.Join([]string{instance.GetKind(), instance.GetName()}, "/")
enforcementAction, err := util.GetEnforcementAction(instance.Object)
Expand All @@ -329,23 +339,20 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
status.Status.ConstraintUID = instance.GetUID()
status.Status.ObservedGeneration = instance.GetGeneration()
status.Status.Errors = nil
cachedTags := r.constraintsCache.getTagsByConstraintKey(constraintKey)
cachedGenerateVapBinding := cachedTags.generateVapBinding
log.Info("constraint", "cachedGenerateVapBinding", cachedGenerateVapBinding)

if c, err := r.cfClient.GetConstraint(instance); err != nil || !constraints.SemanticEqual(instance, c) || r.generateVapBinding != cachedGenerateVapBinding {
if c, err := r.cfClient.GetConstraint(instance); err != nil || !reflect.DeepEqual(instance, c) {
// generate vapbinding resources
if r.generateVapBinding && IsVapAPIEnabled() {
currentVapBinding := &admissionregistrationv1alpha1.ValidatingAdmissionPolicyBinding{}
if generateVapBinding && IsVapAPIEnabled() {
currentVapBinding := &admissionregistrationv1beta1.ValidatingAdmissionPolicyBinding{}
vapBindingName := fmt.Sprintf("gatekeeper-%s", instance.GetName())
log.Info("check if vapbinding exists", "vapBindingName", vapBindingName)
if err := r.reader.Get(ctx, types.NamespacedName{Name: vapBindingName}, currentVapBinding); err != nil {
if !apierrors.IsNotFound(err) {
if !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "failed to get API group resources") {
return reconcile.Result{}, err
}
currentVapBinding = nil
}
newVapBinding := &admissionregistrationv1alpha1.ValidatingAdmissionPolicyBinding{}
newVapBinding := &admissionregistrationv1beta1.ValidatingAdmissionPolicyBinding{}
transformedVapBinding, err := transform.ConstraintToBinding(instance)
if err != nil {
status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: err.Error()})
Expand Down Expand Up @@ -375,18 +382,7 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
return reconcile.Result{}, err
}
} else {

Check failure on line 384 in pkg/controller/constraint/constraint_controller.go

View workflow job for this annotation

GitHub Actions / Lint 

elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
uc, err := runtime.DefaultUnstructuredConverter.ToUnstructured(currentVapBinding)
if err != nil {
log.Error(err, "error converting to unstructured")
return reconcile.Result{}, err
}
un, err := runtime.DefaultUnstructuredConverter.ToUnstructured(newVapBinding)
if err != nil {
log.Error(err, "error converting to unstructured")
return reconcile.Result{}, err
}

if !constraints.SemanticEqual(&unstructured.Unstructured{Object: un}, &unstructured.Unstructured{Object: uc}) {
if !reflect.DeepEqual(currentVapBinding, newVapBinding) {
log.Info("updating vapbinding")
if err := r.writer.Update(ctx, newVapBinding); err != nil {
status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: err.Error()})
Expand All @@ -400,12 +396,12 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
}
// do not generate vapbinding resources
// remove if exists
if !r.generateVapBinding && r.generateVapBinding != cachedGenerateVapBinding && IsVapAPIEnabled() {
currentVapBinding := &admissionregistrationv1alpha1.ValidatingAdmissionPolicyBinding{}
if !generateVapBinding && IsVapAPIEnabled() {
currentVapBinding := &admissionregistrationv1beta1.ValidatingAdmissionPolicyBinding{}
vapBindingName := fmt.Sprintf("gatekeeper-%s", instance.GetName())
log.Info("check if vapbinding exists", "vapBindingName", vapBindingName)
if err := r.reader.Get(ctx, types.NamespacedName{Name: vapBindingName}, currentVapBinding); err != nil {
if !apierrors.IsNotFound(err) {
if !apierrors.IsNotFound(err) && !strings.Contains(err.Error(), "failed to get API group resources") {
return reconcile.Result{}, err
}
currentVapBinding = nil
Expand All @@ -423,9 +419,8 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
}
if err := r.cacheConstraint(ctx, instance); err != nil {
r.constraintsCache.addConstraintKey(constraintKey, tags{
enforcementAction: enforcementAction,
status: metrics.ErrorStatus,
generateVapBinding: r.generateVapBinding,
enforcementAction: enforcementAction,
status: metrics.ErrorStatus,
})
status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: err.Error()})
if err2 := r.writer.Update(ctx, status); err2 != nil {
Expand All @@ -444,9 +439,8 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R

// adding constraint to cache and sending metrics
r.constraintsCache.addConstraintKey(constraintKey, tags{
enforcementAction: enforcementAction,
status: metrics.ActiveStatus,
generateVapBinding: r.generateVapBinding,
enforcementAction: enforcementAction,
status: metrics.ActiveStatus,
})
reportMetrics = true
} else {
Expand Down Expand Up @@ -586,9 +580,8 @@ func (c *ConstraintsCache) addConstraintKey(constraintKey string, t tags) {
defer c.mux.Unlock()

c.cache[constraintKey] = tags{
enforcementAction: t.enforcementAction,
status: t.status,
generateVapBinding: t.generateVapBinding,
enforcementAction: t.enforcementAction,
status: t.status,
}
}

Expand All @@ -599,13 +592,6 @@ func (c *ConstraintsCache) deleteConstraintKey(constraintKey string) {
delete(c.cache, constraintKey)
}

func (c *ConstraintsCache) getTagsByConstraintKey(constraintKey string) tags {
c.mux.Lock()
defer c.mux.Unlock()

return c.cache[constraintKey]
}

func (c *ConstraintsCache) reportTotalConstraints(ctx context.Context, reporter StatsReporter) {
c.mux.RLock()
defer c.mux.RUnlock()
Expand Down Expand Up @@ -640,6 +626,9 @@ func ShouldGenerateVap(useVapLabel string) bool {
}

func IsVapAPIEnabled() bool {
vapMux.Lock()
defer vapMux.Unlock()

if VapAPIEnabled != nil {
return *VapAPIEnabled
}
Expand Down

0 comments on commit 8b5048a

Please sign in to comment.