mirror of
https://github.com/kyverno/kyverno.git
synced 2024-12-14 11:57:48 +00:00
Fix check for generated webhook rules being equal to what the API server has (#3407)
* Add webhookRulesEqual function and test Signed-off-by: Thomas Hartland <thomas.hartland@diamond.ac.uk> * Handle edge cases in webhookRulesEqual function Signed-off-by: Thomas Hartland <thomas.hartland@diamond.ac.uk>
This commit is contained in:
Thomas Hartland
GitHub
parent
c8c631d4a7
commit
0360ad25c1
2 changed files with 202 additions and 1 deletions
|
|
@ -579,6 +579,61 @@ func (m *webhookConfigManager) getWebhook(webhookKind, webhookName string) (reso
|
|||
return resourceWebhook, nil
|
||||
}
|
||||
|
||||
// webhookRulesEqual compares webhook rules between
|
||||
// the representation returned by the API server,
|
||||
// and the internal representation that is generated.
|
||||
//
|
||||
// The two representations are slightly different,
|
||||
// so this function handles those differences.
|
||||
func webhookRulesEqual(apiRules []interface{}, internalRules []interface{}) (bool, error) {
|
||||
// Handle edge case when both are empty.
|
||||
// API representation is a nil slice,
|
||||
// internal representation is one rule
|
||||
// but with no selectors.
|
||||
if len(apiRules) == 0 && len(internalRules) == 1 {
|
||||
if len(internalRules[0].(map[string]interface{})) == 0 {
|
||||
return true, nil
|
||||
}
|
||||
}
|
||||
|
||||
// Both *should* be length 1, but as long
|
||||
// as they are equal the next loop works.
|
||||
if len(apiRules) != len(internalRules) {
|
||||
return false, nil
|
||||
}
|
||||
|
||||
for i := range internalRules {
|
||||
internal, ok := internalRules[i].(map[string]interface{})
|
||||
if !ok {
|
||||
return false, errors.New("type conversion of internal rules failed")
|
||||
}
|
||||
api, ok := apiRules[i].(map[string]interface{})
|
||||
if !ok {
|
||||
return false, errors.New("type conversion of API rules failed")
|
||||
}
|
||||
|
||||
// Range over the fields of internal, as the
|
||||
// API rule has extra fields (operations, scope)
|
||||
// that can't be checked on the internal rules.
|
||||
for field := range internal {
|
||||
// Convert the API rules values to []string.
|
||||
apiValues, _, err := unstructured.NestedStringSlice(api, field)
|
||||
if err != nil {
|
||||
return false, errors.Wrapf(err, "error getting string slice for API rules field %s", field)
|
||||
}
|
||||
|
||||
// Internal type is already []string.
|
||||
internalValues := internal[field]
|
||||
|
||||
if !reflect.DeepEqual(internalValues, apiValues) {
|
||||
return false, nil
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return true, nil
|
||||
}
|
||||
|
||||
func (m *webhookConfigManager) compareAndUpdateWebhook(webhookKind, webhookName string, webhooksMap map[string]interface{}) error {
|
||||
logger := m.log.WithName("compareAndUpdateWebhook").WithValues("kind", webhookKind, "name", webhookName)
|
||||
resourceWebhook, err := m.getWebhook(webhookKind, webhookName)
|
||||
|
|
@ -621,7 +676,13 @@ func (m *webhookConfigManager) compareAndUpdateWebhook(webhookKind, webhookName
|
|||
continue
|
||||
}
|
||||
|
||||
if !reflect.DeepEqual(rules, []interface{}{w.rule}) {
|
||||
rulesEqual, err := webhookRulesEqual(rules, []interface{}{w.rule})
|
||||
if err != nil {
|
||||
logger.Error(err, "failed to compare webhook rules")
|
||||
continue
|
||||
}
|
||||
|
||||
if !rulesEqual {
|
||||
changed = true
|
||||
|
||||
tmpRules, ok := newWebooks[i].(map[string]interface{})["rules"].([]interface{})
|
||||
|
|
|
|||
140
pkg/webhookconfig/configmanager_test.go
Normal file
140
pkg/webhookconfig/configmanager_test.go
Normal file
|
|
@ -0,0 +1,140 @@
|
|||
package webhookconfig
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"gotest.tools/assert"
|
||||
)
|
||||
|
||||
var (
|
||||
emptyInternalRules []interface{}
|
||||
emptyAPIRules []interface{}
|
||||
|
||||
configmapsInternalRules []interface{}
|
||||
configmapsAPIRules []interface{}
|
||||
|
||||
configmapsSecretsInternalRules []interface{}
|
||||
configmapsSecretsAPIRules []interface{}
|
||||
|
||||
secretsConfigmapsInternalRules []interface{}
|
||||
secretsConfigmapsAPIRules []interface{}
|
||||
|
||||
badAPIRules []interface{}
|
||||
)
|
||||
|
||||
func init() {
|
||||
// No rules.
|
||||
// Internal representation is a rule with no selectors.
|
||||
// API server representation is no rule (nil).
|
||||
emptyInternalRules = []interface{}{map[string]interface{}{}}
|
||||
emptyAPIRules = nil
|
||||
|
||||
// Rule selecting configmaps.
|
||||
// API server representation matches the internal
|
||||
// representation but has extra fields "operations" and "scope",
|
||||
// and is of interface types instead of strings.
|
||||
configmapsInternalRules = []interface{}{
|
||||
map[string]interface{}{
|
||||
"apiGroups": []string{""},
|
||||
"apiVersions": []string{"v1"},
|
||||
"resources": []string{"configmaps"},
|
||||
},
|
||||
}
|
||||
configmapsAPIRules = []interface{}{
|
||||
map[string]interface{}{
|
||||
"apiGroups": []interface{}{""},
|
||||
"apiVersions": []interface{}{"v1"},
|
||||
"resources": []interface{}{"configmaps"},
|
||||
"operations": []interface{}{"CREATE", "UPDATE", "DELETE", "CONNECT"},
|
||||
"scope": "*",
|
||||
},
|
||||
}
|
||||
|
||||
// Rule selecting configmaps and secrets.
|
||||
// API server representation matches the internal
|
||||
// representation but has extra fields "operations" and "scope",
|
||||
// and is of interface types instead of strings.
|
||||
configmapsSecretsInternalRules = []interface{}{
|
||||
map[string]interface{}{
|
||||
"apiGroups": []string{""},
|
||||
"apiVersions": []string{"v1"},
|
||||
"resources": []string{"configmaps", "secrets"},
|
||||
},
|
||||
}
|
||||
configmapsSecretsAPIRules = []interface{}{
|
||||
map[string]interface{}{
|
||||
"apiGroups": []interface{}{""},
|
||||
"apiVersions": []interface{}{"v1"},
|
||||
"resources": []interface{}{"configmaps", "secrets"},
|
||||
"operations": []interface{}{"CREATE", "UPDATE", "DELETE", "CONNECT"},
|
||||
"scope": "*",
|
||||
},
|
||||
}
|
||||
|
||||
// Same as previous but reversing the order of configmaps and secrets.
|
||||
secretsConfigmapsInternalRules = []interface{}{
|
||||
map[string]interface{}{
|
||||
"apiGroups": []string{""},
|
||||
"apiVersions": []string{"v1"},
|
||||
"resources": []string{"secrets", "configmaps"},
|
||||
},
|
||||
}
|
||||
secretsConfigmapsAPIRules = []interface{}{
|
||||
map[string]interface{}{
|
||||
"apiGroups": []interface{}{""},
|
||||
"apiVersions": []interface{}{"v1"},
|
||||
"resources": []interface{}{"secrets", "configmaps"},
|
||||
"operations": []interface{}{"CREATE", "UPDATE", "DELETE", "CONNECT"},
|
||||
"scope": "*",
|
||||
},
|
||||
}
|
||||
|
||||
// API rules with missing fields.
|
||||
badAPIRules = []interface{}{
|
||||
map[string]interface{}{
|
||||
"apiGroups": []interface{}{""},
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
func TestRulesEqual(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
internal []interface{}
|
||||
apiserver []interface{}
|
||||
equal bool
|
||||
shouldErr bool
|
||||
}{
|
||||
// Both empty. Should be equal.
|
||||
{"empty-equal", emptyInternalRules, emptyAPIRules, true, false},
|
||||
|
||||
// Both rules select configmaps. Should be equal.
|
||||
{"configmaps-equal", configmapsInternalRules, configmapsAPIRules, true, false},
|
||||
|
||||
// Both rules select configmaps and secrets. Should be equal.
|
||||
{"cm-secrets-equal", configmapsSecretsInternalRules, configmapsSecretsAPIRules, true, false},
|
||||
|
||||
// Both rules select secrets and configmaps (reversed compared to previous). Should be equal.
|
||||
{"secrets-cm-equal", secretsConfigmapsInternalRules, secretsConfigmapsAPIRules, true, false},
|
||||
|
||||
// Internal is updated from nothing to configmaps. Not equal.
|
||||
{"add-configmaps", configmapsInternalRules, emptyAPIRules, false, false},
|
||||
|
||||
// Internal is updated from configmaps to configmaps and secrets. Not equal.
|
||||
{"add-secrets", configmapsSecretsInternalRules, configmapsAPIRules, false, false},
|
||||
|
||||
// Order of configmaps and secrets is switched. Not equal.
|
||||
{"order-switched", configmapsSecretsInternalRules, secretsConfigmapsAPIRules, false, false},
|
||||
|
||||
// Malformed API rules, if modified by user or something like that. Not equal.
|
||||
{"bad-api-rules", configmapsInternalRules, badAPIRules, false, false},
|
||||
}
|
||||
|
||||
for _, test := range tests {
|
||||
t.Run(test.name, func(t *testing.T) {
|
||||
equal, err := webhookRulesEqual(test.apiserver, test.internal)
|
||||
assert.Equal(t, err != nil, test.shouldErr)
|
||||
assert.Equal(t, equal, test.equal)
|
||||
})
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue