From 4efcabffb5133ad6b8462482701adee85f27c349 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?=
<charled.breteche@gmail.com>
Date: Fri, 25 Mar 2022 15:43:47 +0100
Subject: [PATCH] refactor: use abstract policy interface in webhookconfig
(#3466)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>
Co-authored-by: shuting <shuting@nirmata.com>
---
api/kyverno/v1/clusterpolicy_types.go | 5 +
api/kyverno/v1/policy_types.go | 5 +
pkg/autogen/autogen.go | 4 +-
pkg/autogen/autogen_test.go | 2 +-
pkg/webhookconfig/configmanager.go | 180 ++++++++++++--------------
5 files changed, 94 insertions(+), 102 deletions(-)
diff --git a/api/kyverno/v1/clusterpolicy_types.go b/api/kyverno/v1/clusterpolicy_types.go
index ef0b24f681..f42cb0a8e7 100644
--- a/api/kyverno/v1/clusterpolicy_types.go
+++ b/api/kyverno/v1/clusterpolicy_types.go
@@ -31,6 +31,11 @@ type ClusterPolicy struct {
Status PolicyStatus `json:"status,omitempty" yaml:"status,omitempty"`
}
+// GetSpec returns the policy spec
+func (p *ClusterPolicy) GetSpec() Spec {
+ return p.Spec
+}
+
// GetRules returns the policy rules
func (p *ClusterPolicy) GetRules() []Rule {
return p.Spec.GetRules()
diff --git a/api/kyverno/v1/policy_types.go b/api/kyverno/v1/policy_types.go
index 744ba15850..2439241a51 100755
--- a/api/kyverno/v1/policy_types.go
+++ b/api/kyverno/v1/policy_types.go
@@ -32,6 +32,11 @@ type Policy struct {
Status PolicyStatus `json:"status,omitempty" yaml:"status,omitempty"`
}
+// GetSpec returns the policy spec
+func (p *Policy) GetSpec() Spec {
+ return p.Spec
+}
+
// GetRules returns the policy rules
func (p *Policy) GetRules() []Rule {
return p.Spec.GetRules()
diff --git a/pkg/autogen/autogen.go b/pkg/autogen/autogen.go
index 872709b8ea..ae0d1d1b5c 100644
--- a/pkg/autogen/autogen.go
+++ b/pkg/autogen/autogen.go
@@ -118,7 +118,7 @@ func GetSupportedControllers(spec *kyverno.Spec, log logr.Logger) []string {
}
// GetRequestedControllers returns the requested autogen controllers based on object annotations.
-func GetRequestedControllers(meta metav1.ObjectMeta) []string {
+func GetRequestedControllers(meta *metav1.ObjectMeta) []string {
annotations := meta.GetAnnotations()
if annotations == nil {
return nil
@@ -135,7 +135,7 @@ func GetRequestedControllers(meta metav1.ObjectMeta) []string {
// GetControllers computes the autogen controllers that should be applied to a policy.
// It returns the requested, supported and effective controllers (intersection of requested and supported ones).
-func GetControllers(meta metav1.ObjectMeta, spec *kyverno.Spec, log logr.Logger) ([]string, []string, []string) {
+func GetControllers(meta *metav1.ObjectMeta, spec *kyverno.Spec, log logr.Logger) ([]string, []string, []string) {
// compute supported and requested controllers
supported := GetSupportedControllers(spec, log)
requested := GetRequestedControllers(meta)
diff --git a/pkg/autogen/autogen_test.go b/pkg/autogen/autogen_test.go
index b29da042f7..08c6816bc0 100644
--- a/pkg/autogen/autogen_test.go
+++ b/pkg/autogen/autogen_test.go
@@ -241,7 +241,7 @@ func Test_GetRequestedControllers(t *testing.T) {
}
for _, test := range testCases {
- controllers := GetRequestedControllers(test.meta)
+ controllers := GetRequestedControllers(&test.meta)
assert.DeepEqual(t, test.expectedControllers, controllers)
}
}
diff --git a/pkg/webhookconfig/configmanager.go b/pkg/webhookconfig/configmanager.go
index aba1d445a7..cdaefe1944 100644
--- a/pkg/webhookconfig/configmanager.go
+++ b/pkg/webhookconfig/configmanager.go
@@ -36,6 +36,12 @@ import (
var DefaultWebhookTimeout int64 = 10
+// policy abstracts the concrete policy type (Policy vs ClusterPolicy)
+type policy interface {
+ metav1.Object
+ GetSpec() kyverno.Spec
+}
+
// webhookConfigManager manges the webhook configuration dynamically
// it is NOT multi-thread safe
type webhookConfigManager struct {
@@ -139,13 +145,11 @@ func (m *webhookConfigManager) handleErr(err error, key interface{}) {
m.queue.Forget(key)
return
}
-
if m.queue.NumRequeues(key) < 3 {
logger.Error(err, "failed to sync policy", "key", key)
m.queue.AddRateLimited(key)
return
}
-
utilruntime.HandleError(err)
logger.V(2).Info("dropping policy out of queue", "key", key)
m.queue.Forget(key)
@@ -153,26 +157,22 @@ func (m *webhookConfigManager) handleErr(err error, key interface{}) {
func (m *webhookConfigManager) addClusterPolicy(obj interface{}) {
p := obj.(*kyverno.ClusterPolicy)
- if hasWildcard(p) {
+ if hasWildcard(&p.Spec) {
atomic.AddInt64(&m.wildcardPolicy, int64(1))
}
m.enqueue(p)
}
func (m *webhookConfigManager) updateClusterPolicy(old, cur interface{}) {
- oldP := old.(*kyverno.ClusterPolicy)
- curP := cur.(*kyverno.ClusterPolicy)
-
+ oldP, curP := old.(*kyverno.ClusterPolicy), cur.(*kyverno.ClusterPolicy)
if reflect.DeepEqual(oldP.Spec, curP.Spec) {
return
}
-
- if hasWildcard(oldP) && !hasWildcard(curP) {
+ if hasWildcard(&oldP.Spec) && !hasWildcard(&curP.Spec) {
atomic.AddInt64(&m.wildcardPolicy, ^int64(0))
- } else if !hasWildcard(oldP) && hasWildcard(curP) {
+ } else if !hasWildcard(&oldP.Spec) && hasWildcard(&curP.Spec) {
atomic.AddInt64(&m.wildcardPolicy, int64(1))
}
-
m.enqueue(curP)
}
@@ -191,8 +191,7 @@ func (m *webhookConfigManager) deleteClusterPolicy(obj interface{}) {
}
m.log.V(4).Info("Recovered deleted ClusterPolicy '%s' from tombstone", "name", p.GetName())
}
-
- if hasWildcard(p) {
+ if hasWildcard(&p.Spec) {
atomic.AddInt64(&m.wildcardPolicy, ^int64(0))
}
m.enqueue(p)
@@ -200,30 +199,23 @@ func (m *webhookConfigManager) deleteClusterPolicy(obj interface{}) {
func (m *webhookConfigManager) addPolicy(obj interface{}) {
p := obj.(*kyverno.Policy)
- if hasWildcard(p) {
+ if hasWildcard(&p.Spec) {
atomic.AddInt64(&m.wildcardPolicy, int64(1))
}
-
- pol := kyverno.ClusterPolicy(*p)
- m.enqueue(&pol)
+ m.enqueue(p)
}
func (m *webhookConfigManager) updatePolicy(old, cur interface{}) {
- oldP := old.(*kyverno.Policy)
- curP := cur.(*kyverno.Policy)
-
+ oldP, curP := old.(*kyverno.Policy), cur.(*kyverno.Policy)
if reflect.DeepEqual(oldP.Spec, curP.Spec) {
return
}
-
- if hasWildcard(oldP) && !hasWildcard(curP) {
+ if hasWildcard(&oldP.Spec) && !hasWildcard(&curP.Spec) {
atomic.AddInt64(&m.wildcardPolicy, ^int64(0))
- } else if !hasWildcard(oldP) && hasWildcard(curP) {
+ } else if !hasWildcard(&oldP.Spec) && hasWildcard(&curP.Spec) {
atomic.AddInt64(&m.wildcardPolicy, int64(1))
}
-
- pol := kyverno.ClusterPolicy(*curP)
- m.enqueue(&pol)
+ m.enqueue(curP)
}
func (m *webhookConfigManager) deletePolicy(obj interface{}) {
@@ -239,15 +231,12 @@ func (m *webhookConfigManager) deletePolicy(obj interface{}) {
utilruntime.HandleError(fmt.Errorf("error decoding object tombstone, invalid type"))
return
}
- m.log.V(4).Info("Recovered deleted ClusterPolicy '%s' from tombstone", "name", p.GetName())
+ m.log.V(4).Info("Recovered deleted Policy '%s/%s' from tombstone", "name", p.GetNamespace(), p.GetName())
}
-
- if hasWildcard(p) {
+ if hasWildcard(&p.Spec) {
atomic.AddInt64(&m.wildcardPolicy, ^int64(0))
}
-
- pol := kyverno.ClusterPolicy(*p)
- m.enqueue(&pol)
+ m.enqueue(p)
}
func (m *webhookConfigManager) deleteWebhook(obj interface{}) {
@@ -263,19 +252,17 @@ func (m *webhookConfigManager) deleteWebhook(obj interface{}) {
func (m *webhookConfigManager) enqueueAllPolicies() {
logger := m.log.WithName("enqueueAllPolicies")
-
policies, err := m.listAllPolicies()
if err != nil {
logger.Error(err, "unabled to list policies")
}
-
for _, policy := range policies {
m.enqueue(policy)
logger.V(4).Info("added policy to the queue", "namespace", policy.GetNamespace(), "name", policy.GetName())
}
}
-func (m *webhookConfigManager) enqueue(policy *kyverno.ClusterPolicy) {
+func (m *webhookConfigManager) enqueue(policy interface{}) {
logger := m.log
key, err := cache.MetaNamespaceKeyFunc(policy)
if err != nil {
@@ -330,7 +317,6 @@ func (m *webhookConfigManager) processNextWorkItem() bool {
defer m.queue.Done(key)
err := m.sync(key.(string))
m.handleErr(err, key)
-
return true
}
@@ -341,13 +327,11 @@ func (m *webhookConfigManager) sync(key string) error {
defer func() {
logger.V(4).Info("finished syncing policy", "key", key, "processingTime", time.Since(startTime).String())
}()
-
namespace, name, err := cache.SplitMetaNamespaceKey(key)
if err != nil {
logger.Info("invalid resource key", "key", key)
return nil
}
-
return m.reconcileWebhook(namespace, name)
}
@@ -378,7 +362,7 @@ func (m *webhookConfigManager) reconcileWebhook(namespace, name string) error {
}
}
- if err := m.updateStatus(policy, ready); err != nil {
+ if err := m.updateStatus(namespace, name, ready); err != nil {
return errors.Wrapf(err, "failed to update policy status %s/%s", namespace, name)
}
@@ -388,39 +372,30 @@ func (m *webhookConfigManager) reconcileWebhook(namespace, name string) error {
return nil
}
-func (m *webhookConfigManager) getPolicy(namespace, name string) (*kyverno.ClusterPolicy, error) {
+func (m *webhookConfigManager) getPolicy(namespace, name string) (policy, error) {
if namespace == "" {
return m.pLister.Get(name)
+ } else {
+ return m.npLister.Policies(namespace).Get(name)
}
-
- nsPolicy, err := m.npLister.Policies(namespace).Get(name)
- if err == nil && nsPolicy != nil {
- p := kyverno.ClusterPolicy(*nsPolicy)
- return &p, err
- }
-
- return nil, err
}
-func (m *webhookConfigManager) listAllPolicies() ([]*kyverno.ClusterPolicy, error) {
- policies := []*kyverno.ClusterPolicy{}
-
+func (m *webhookConfigManager) listAllPolicies() ([]policy, error) {
+ policies := []policy{}
polList, err := m.npLister.Policies(metav1.NamespaceAll).List(labels.Everything())
if err != nil {
return nil, errors.Wrapf(err, "failed to list Policy")
}
-
- for _, pol := range polList {
- p := kyverno.ClusterPolicy(*pol)
- policies = append(policies, &p)
+ for _, p := range polList {
+ policies = append(policies, p)
}
-
cpolList, err := m.pLister.List(labels.Everything())
if err != nil {
return nil, errors.Wrapf(err, "failed to list ClusterPolicy")
}
-
- policies = append(policies, cpolList...)
+ for _, p := range cpolList {
+ policies = append(policies, p)
+ }
return policies, nil
}
@@ -463,19 +438,20 @@ func (m *webhookConfigManager) buildWebhooks(namespace string) (res []*webhook,
}
for _, p := range policies {
- if p.HasValidate() || p.HasGenerate() {
- if p.Spec.FailurePolicy != nil && *p.Spec.FailurePolicy == kyverno.Ignore {
- m.mergeWebhook(validateIgnore, p, true)
+ spec := p.GetSpec()
+ if spec.HasValidate() || spec.HasGenerate() {
+ if spec.FailurePolicy != nil && *spec.FailurePolicy == kyverno.Ignore {
+ m.mergeWebhook(validateIgnore, &spec, true)
} else {
- m.mergeWebhook(validateFail, p, true)
+ m.mergeWebhook(validateFail, &spec, true)
}
}
- if p.HasMutate() || p.HasVerifyImages() {
- if p.Spec.FailurePolicy != nil && *p.Spec.FailurePolicy == kyverno.Ignore {
- m.mergeWebhook(mutateIgnore, p, false)
+ if spec.HasMutate() || spec.HasVerifyImages() {
+ if spec.FailurePolicy != nil && *spec.FailurePolicy == kyverno.Ignore {
+ m.mergeWebhook(mutateIgnore, &spec, false)
} else {
- m.mergeWebhook(mutateFail, p, false)
+ m.mergeWebhook(mutateFail, &spec, false)
}
}
}
@@ -722,29 +698,45 @@ func (m *webhookConfigManager) compareAndUpdateWebhook(webhookKind, webhookName
return nil
}
-func (m *webhookConfigManager) updateStatus(policy *kyverno.ClusterPolicy, status bool) error {
- policyCopy := policy.DeepCopy()
- requested, supported, activated := autogen.GetControllers(policy.ObjectMeta, &policy.Spec, m.log)
- policyCopy.Status.SetReady(status)
- policyCopy.Status.Autogen.Requested = requested
- policyCopy.Status.Autogen.Supported = supported
- policyCopy.Status.Autogen.Activated = activated
- policyCopy.Status.Rules = policy.Spec.Rules
- if reflect.DeepEqual(policyCopy.Status, policy.Status) {
- return nil
+func (m *webhookConfigManager) updateStatus(namespace, name string, ready bool) error {
+ update := func(meta *metav1.ObjectMeta, spec *kyverno.Spec, status *kyverno.PolicyStatus) bool {
+ copy := status.DeepCopy()
+ requested, supported, activated := autogen.GetControllers(meta, spec, m.log)
+ status.SetReady(ready)
+ status.Autogen.Requested = requested
+ status.Autogen.Supported = supported
+ status.Autogen.Activated = activated
+ status.Rules = spec.Rules
+ return !reflect.DeepEqual(status, copy)
}
- if policy.GetNamespace() == "" {
- _, err := m.kyvernoClient.KyvernoV1().ClusterPolicies().UpdateStatus(context.TODO(), policyCopy, metav1.UpdateOptions{})
- return err
+ if namespace == "" {
+ p, err := m.pLister.Get(name)
+ if err != nil {
+ return err
+ }
+ if update(&p.ObjectMeta, &p.Spec, &p.Status) {
+ if _, err := m.kyvernoClient.KyvernoV1().ClusterPolicies().UpdateStatus(context.TODO(), p, metav1.UpdateOptions{}); err != nil {
+ return err
+ }
+ }
+ } else {
+ p, err := m.npLister.Policies(namespace).Get(name)
+ if err != nil {
+ return err
+ }
+ if update(&p.ObjectMeta, &p.Spec, &p.Status) {
+ if _, err := m.kyvernoClient.KyvernoV1().Policies(namespace).UpdateStatus(context.TODO(), p, metav1.UpdateOptions{}); err != nil {
+ return err
+ }
+ }
}
- _, err := m.kyvernoClient.KyvernoV1().Policies(policyCopy.GetNamespace()).UpdateStatus(context.TODO(), (*kyverno.Policy)(policyCopy), metav1.UpdateOptions{})
- return err
+ return nil
}
// mergeWebhook merges the matching kinds of the policy to webhook.rule
-func (m *webhookConfigManager) mergeWebhook(dst *webhook, policy *kyverno.ClusterPolicy, updateValidate bool) {
+func (m *webhookConfigManager) mergeWebhook(dst *webhook, spec *kyverno.Spec, updateValidate bool) {
matchedGVK := make([]string, 0)
- for _, rule := range policy.GetRules() {
+ for _, rule := range spec.GetRules() {
// matching kinds in generate policies need to be added to both webhook
if rule.HasGenerate() {
matchedGVK = append(matchedGVK, rule.MatchKinds()...)
@@ -826,9 +818,9 @@ func (m *webhookConfigManager) mergeWebhook(dst *webhook, policy *kyverno.Cluste
dst.rule[apiVersions] = removeDuplicates(versions)
dst.rule[resources] = removeDuplicates(rsrcs)
- if policy.Spec.WebhookTimeoutSeconds != nil {
- if dst.maxWebhookTimeout < int64(*policy.Spec.WebhookTimeoutSeconds) {
- dst.maxWebhookTimeout = int64(*policy.Spec.WebhookTimeoutSeconds)
+ if spec.WebhookTimeoutSeconds != nil {
+ if dst.maxWebhookTimeout < int64(*spec.WebhookTimeoutSeconds) {
+ dst.maxWebhookTimeout = int64(*spec.WebhookTimeoutSeconds)
}
}
}
@@ -857,20 +849,10 @@ func webhookKey(webhookKind, failurePolicy string) string {
return strings.Join([]string{webhookKind, failurePolicy}, "/")
}
-func hasWildcard(policy interface{}) bool {
- if p, ok := policy.(*kyverno.ClusterPolicy); ok {
- for _, rule := range p.GetRules() {
- if kinds := rule.MatchKinds(); utils.ContainsString(kinds, "*") {
- return true
- }
- }
- }
-
- if p, ok := policy.(*kyverno.Policy); ok {
- for _, rule := range p.GetRules() {
- if kinds := rule.MatchKinds(); utils.ContainsString(kinds, "*") {
- return true
- }
+func hasWildcard(spec *kyverno.Spec) bool {
+ for _, rule := range spec.GetRules() {
+ if kinds := rule.MatchKinds(); utils.ContainsString(kinds, "*") {
+ return true
}
}
return false