Skip to content

Commit

Permalink
fix(reports-controller): Allow UPDATE operation in validation
Browse files Browse the repository at this point in the history 
Signed-off-by: Bhargav Ravuri <bhargav.ravuri@infracloud.io>
  • Loading branch information
@Bhargav-InfraCloud
Bhargav-InfraCloud committed Feb 1, 2024
1 parent f1cf6ef commit a5df960
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 34 deletions.
2 changes: 1 addition & 1 deletion pkg/controllers/report/utils/scanner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (s *scanner) validateResource(ctx context.Context, resource unstructured.Un
WithNewResource(resource).
WithPolicy(policy).
WithNamespaceLabels(nsLabels)
response := s.engine.Validate(ctx, policyCtx)
response := s.engine.Validate(ctx, policyCtx, []kyvernov1.AdmissionOperation{kyvernov1.Create, kyvernov1.Update}...)
return &response, nil
}

Expand Down
1 change: 1 addition & 0 deletions pkg/engine/api/engine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type Engine interface {
Validate(
ctx context.Context,
policyContext PolicyContext,
operations ...kyvernov1.AdmissionOperation,
) EngineResponse

// Mutate performs mutation. Overlay first and then mutation patches
Expand Down
4 changes: 2 additions & 2 deletions pkg/engine/background.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,10 @@ func (e *engine) filterRule(
policy := policyContext.Policy()
gvk, subresource := policyContext.ResourceKind()

if err := engineutils.MatchesResourceDescription(newResource, rule, admissionInfo, namespaceLabels, policy.GetNamespace(), gvk, subresource, policyContext.Operation()); err != nil {
if err := engineutils.MatchesResourceDescription(newResource, rule, admissionInfo, namespaceLabels, policy.GetNamespace(), gvk, subresource, []kyvernov1.AdmissionOperation{policyContext.Operation()}); err != nil {
if ruleType == engineapi.Generation {
// if the oldResource matched, return "false" to delete GR for it
if err = engineutils.MatchesResourceDescription(oldResource, rule, admissionInfo, namespaceLabels, policy.GetNamespace(), gvk, subresource, policyContext.Operation()); err == nil {
if err = engineutils.MatchesResourceDescription(oldResource, rule, admissionInfo, namespaceLabels, policy.GetNamespace(), gvk, subresource, []kyvernov1.AdmissionOperation{policyContext.Operation()}); err == nil {
return engineapi.RuleFail(rule.Name, ruleType, "")
}
}
Expand Down
11 changes: 7 additions & 4 deletions pkg/engine/engine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,13 @@ func NewEngine(
func (e *engine) Validate(
ctx context.Context,
policyContext engineapi.PolicyContext,
operations ...kyvernov1.AdmissionOperation,
) engineapi.EngineResponse {
startTime := time.Now()
response := engineapi.NewEngineResponseFromPolicyContext(policyContext)
logger := internal.LoggerWithPolicyContext(logging.WithName("engine.validate"), policyContext)
if internal.MatchPolicyContext(logger, policyContext, e.configuration) {
policyResponse := e.validate(ctx, logger, policyContext)
policyResponse := e.validate(ctx, logger, policyContext, operations)
response = response.WithPolicyResponse(policyResponse)
}
response = response.WithStats(engineapi.NewExecutionStats(startTime, time.Now()))
Expand Down Expand Up @@ -191,6 +192,7 @@ func (e *engine) matches(
rule kyvernov1.Rule,
policyContext engineapi.PolicyContext,
resource unstructured.Unstructured,
operations []kyvernov1.AdmissionOperation,
) error {
if policyContext.AdmissionOperation() {
request := policyContext.AdmissionInfo()
Expand All @@ -207,7 +209,7 @@ func (e *engine) matches(
policyContext.Policy().GetNamespace(),
gvk,
subresource,
policyContext.Operation(),
operations,
)
if err == nil {
return nil
Expand All @@ -222,7 +224,7 @@ func (e *engine) matches(
policyContext.Policy().GetNamespace(),
gvk,
subresource,
policyContext.Operation(),
operations,
)
if err == nil {
return nil
Expand All @@ -239,14 +241,15 @@ func (e *engine) invokeRuleHandler(
resource unstructured.Unstructured,
rule kyvernov1.Rule,
ruleType engineapi.RuleType,
operations ...kyvernov1.AdmissionOperation,
) (unstructured.Unstructured, []engineapi.RuleResponse) {
return tracing.ChildSpan2(
ctx,
"pkg/engine",
fmt.Sprintf("RULE %s", rule.Name),
func(ctx context.Context, span trace.Span) (patchedResource unstructured.Unstructured, results []engineapi.RuleResponse) {
// check if resource and rule match
if err := e.matches(rule, policyContext, resource); err != nil {
if err := e.matches(rule, policyContext, resource, operations); err != nil {
logger.V(4).Info("rule not matched", "reason", err.Error())
return resource, nil
}
Expand Down
28 changes: 15 additions & 13 deletions pkg/engine/utils/match.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,12 @@ func doesResourceMatchConditionBlock(
namespaceLabels map[string]string,
gvk schema.GroupVersionKind,
subresource string,
operation kyvernov1.AdmissionOperation,
operations []kyvernov1.AdmissionOperation,
) []error {
if len(conditionBlock.Operations) > 0 {
if !slices.Contains(conditionBlock.Operations, operation) {
if !slices.ContainsFunc(operations, func(op kyvernov1.AdmissionOperation) bool {
return slices.Contains(conditionBlock.Operations, op)
}) {
// if operation does not match, return immediately
err := fmt.Errorf("operation does not match")
return []error{err}
Expand Down Expand Up @@ -173,7 +175,7 @@ func MatchesResourceDescription(
policyNamespace string,
gvk schema.GroupVersionKind,
subresource string,
operation kyvernov1.AdmissionOperation,
operations []kyvernov1.AdmissionOperation,
) error {
if resource.Object == nil {
return fmt.Errorf("resource is empty")
Expand All @@ -190,7 +192,7 @@ func MatchesResourceDescription(
oneMatched := false
for _, rmr := range rule.MatchResources.Any {
// if there are no errors it means it was a match
if len(matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)) == 0 {
if len(matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operations)) == 0 {
oneMatched = true
break
}
Expand All @@ -201,27 +203,27 @@ func MatchesResourceDescription(
} else if len(rule.MatchResources.All) > 0 {
// include object if ALL of the criteria match
for _, rmr := range rule.MatchResources.All {
reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operations)...)
}
} else {
rmr := kyvernov1.ResourceFilter{UserInfo: rule.MatchResources.UserInfo, ResourceDescription: rule.MatchResources.ResourceDescription}
reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operations)...)
}

// check exlude conditions only if match succeeds
if len(reasonsForFailure) == 0 {
if len(rule.ExcludeResources.Any) > 0 {
// exclude the object if ANY of the criteria match
for _, rer := range rule.ExcludeResources.Any {
reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operations)...)
}
} else if len(rule.ExcludeResources.All) > 0 {
// exclude the object if ALL the criteria match
excludedByAll := true
for _, rer := range rule.ExcludeResources.All {
// we got no errors inplying a resource did NOT exclude it
// "matchesResourceDescriptionExcludeHelper" returns errors if resource is excluded by a filter
if len(matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)) == 0 {
if len(matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operations)) == 0 {
excludedByAll = false
break
}
Expand All @@ -231,7 +233,7 @@ func MatchesResourceDescription(
}
} else {
rer := kyvernov1.ResourceFilter{UserInfo: rule.ExcludeResources.UserInfo, ResourceDescription: rule.ExcludeResources.ResourceDescription}
reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operations)...)
}
}

Expand All @@ -257,7 +259,7 @@ func matchesResourceDescriptionMatchHelper(
namespaceLabels map[string]string,
gvk schema.GroupVersionKind,
subresource string,
operation kyvernov1.AdmissionOperation,
operations []kyvernov1.AdmissionOperation,
) []error {
var errs []error
if datautils.DeepEqual(admissionInfo, kyvernov1beta1.RequestInfo{}) {
Expand All @@ -267,7 +269,7 @@ func matchesResourceDescriptionMatchHelper(
// checking if resource matches the rule
if !datautils.DeepEqual(rmr.ResourceDescription, kyvernov1.ResourceDescription{}) ||
!datautils.DeepEqual(rmr.UserInfo, kyvernov1.UserInfo{}) {
matchErrs := doesResourceMatchConditionBlock(rmr.ResourceDescription, rmr.UserInfo, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)
matchErrs := doesResourceMatchConditionBlock(rmr.ResourceDescription, rmr.UserInfo, admissionInfo, resource, namespaceLabels, gvk, subresource, operations)
errs = append(errs, matchErrs...)
} else {
errs = append(errs, fmt.Errorf("match cannot be empty"))
Expand All @@ -282,13 +284,13 @@ func matchesResourceDescriptionExcludeHelper(
namespaceLabels map[string]string,
gvk schema.GroupVersionKind,
subresource string,
operation kyvernov1.AdmissionOperation,
operations []kyvernov1.AdmissionOperation,
) []error {
var errs []error
// checking if resource matches the rule
if !datautils.DeepEqual(rer.ResourceDescription, kyvernov1.ResourceDescription{}) ||
!datautils.DeepEqual(rer.UserInfo, kyvernov1.UserInfo{}) {
excludeErrs := doesResourceMatchConditionBlock(rer.ResourceDescription, rer.UserInfo, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)
excludeErrs := doesResourceMatchConditionBlock(rer.ResourceDescription, rer.UserInfo, admissionInfo, resource, namespaceLabels, gvk, subresource, operations)
// it was a match so we want to exclude it
if len(excludeErrs) == 0 {
errs = append(errs, fmt.Errorf("resource excluded since one of the criteria excluded it"))
Expand Down
26 changes: 13 additions & 13 deletions pkg/engine/utils/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ func TestMatchesResourceDescription(t *testing.T) {
resource, _ := kubeutils.BytesToUnstructured(tc.Resource)

for _, rule := range autogen.ComputeRules(&policy) {
err := MatchesResourceDescription(*resource, rule, tc.AdmissionInfo, nil, "", resource.GroupVersionKind(), "", "CREATE")
err := MatchesResourceDescription(*resource, rule, tc.AdmissionInfo, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"})
if err != nil {
if !tc.areErrorsExpected {
t.Errorf("Testcase %d Unexpected error: %v\nmsg: %s", i+1, err, tc.Description)
Expand Down Expand Up @@ -1810,7 +1810,7 @@ func TestMatchesResourceDescription_GenerateName(t *testing.T) {
resource, _ := kubeutils.BytesToUnstructured(tc.Resource)

for _, rule := range autogen.ComputeRules(&policy) {
err := MatchesResourceDescription(*resource, rule, tc.AdmissionInfo, nil, "", resource.GroupVersionKind(), "", "CREATE")
err := MatchesResourceDescription(*resource, rule, tc.AdmissionInfo, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"})
if err != nil {
if !tc.areErrorsExpected {
t.Errorf("Testcase %d Unexpected error: %v\nmsg: %s", i+1, err, tc.Description)
Expand Down Expand Up @@ -1877,7 +1877,7 @@ func TestResourceDescriptionMatch_MultipleKind(t *testing.T) {
}
rule := v1.Rule{MatchResources: v1.MatchResources{ResourceDescription: resourceDescription}}

if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", "CREATE"); err != nil {
if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err != nil {
t.Errorf("Testcase has failed due to the following:%v", err)
}
}
Expand Down Expand Up @@ -1980,7 +1980,7 @@ func TestResourceDescriptionMatch_ExcludeDefaultGroups(t *testing.T) {
}

// First test: confirm that this above rule produces errors (and raise an error if err == nil)
if err := MatchesResourceDescription(*resource, rule, requestInfo, nil, "", resource.GroupVersionKind(), "", "CREATE"); err == nil {
if err := MatchesResourceDescription(*resource, rule, requestInfo, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err == nil {
t.Error("Testcase was expected to fail, but err was nil")
}

Expand All @@ -2000,7 +2000,7 @@ func TestResourceDescriptionMatch_ExcludeDefaultGroups(t *testing.T) {
}

// Second test: confirm that matching this rule does not create any errors (and raise if err != nil)
if err := MatchesResourceDescription(*resource, rule2, requestInfo, nil, "", resource.GroupVersionKind(), "", "CREATE"); err != nil {
if err := MatchesResourceDescription(*resource, rule2, requestInfo, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err != nil {
t.Errorf("Testcase was expected to not fail, but err was %s", err)
}

Expand All @@ -2014,7 +2014,7 @@ func TestResourceDescriptionMatch_ExcludeDefaultGroups(t *testing.T) {
}}

// Third test: confirm that now the custom exclude-snippet should run in CheckSubjects() and that should result in this rule failing (raise if err == nil for that reason)
if err := MatchesResourceDescription(*resource, rule2, requestInfo, nil, "", resource.GroupVersionKind(), "", "CREATE"); err == nil {
if err := MatchesResourceDescription(*resource, rule2, requestInfo, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err == nil {
t.Error("Testcase was expected to fail, but err was nil #1!")
}
}
Expand Down Expand Up @@ -2073,7 +2073,7 @@ func TestResourceDescriptionMatch_Name(t *testing.T) {
}
rule := v1.Rule{MatchResources: v1.MatchResources{ResourceDescription: resourceDescription}}

if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", "CREATE"); err != nil {
if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err != nil {
t.Errorf("Testcase has failed due to the following:%v", err)
}
}
Expand Down Expand Up @@ -2131,7 +2131,7 @@ func TestResourceDescriptionMatch_GenerateName(t *testing.T) {
}
rule := v1.Rule{MatchResources: v1.MatchResources{ResourceDescription: resourceDescription}}

if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", "CREATE"); err != nil {
if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err != nil {
t.Errorf("Testcase has failed due to the following:%v", err)
}
}
Expand Down Expand Up @@ -2190,7 +2190,7 @@ func TestResourceDescriptionMatch_Name_Regex(t *testing.T) {
}
rule := v1.Rule{MatchResources: v1.MatchResources{ResourceDescription: resourceDescription}}

if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", "CREATE"); err != nil {
if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err != nil {
t.Errorf("Testcase has failed due to the following:%v", err)
}
}
Expand Down Expand Up @@ -2248,7 +2248,7 @@ func TestResourceDescriptionMatch_GenerateName_Regex(t *testing.T) {
}
rule := v1.Rule{MatchResources: v1.MatchResources{ResourceDescription: resourceDescription}}

if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", "CREATE"); err != nil {
if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err != nil {
t.Errorf("Testcase has failed due to the following:%v", err)
}
}
Expand Down Expand Up @@ -2315,7 +2315,7 @@ func TestResourceDescriptionMatch_Label_Expression_NotMatch(t *testing.T) {
}
rule := v1.Rule{MatchResources: v1.MatchResources{ResourceDescription: resourceDescription}}

if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", "CREATE"); err != nil {
if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err != nil {
t.Errorf("Testcase has failed due to the following:%v", err)
}
}
Expand Down Expand Up @@ -2383,7 +2383,7 @@ func TestResourceDescriptionMatch_Label_Expression_Match(t *testing.T) {
}
rule := v1.Rule{MatchResources: v1.MatchResources{ResourceDescription: resourceDescription}}

if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", "CREATE"); err != nil {
if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err != nil {
t.Errorf("Testcase has failed due to the following:%v", err)
}
}
Expand Down Expand Up @@ -2464,7 +2464,7 @@ func TestResourceDescriptionExclude_Label_Expression_Match(t *testing.T) {
ExcludeResources: v1.MatchResources{ResourceDescription: resourceDescriptionExclude},
}

if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", "CREATE"); err == nil {
if err := MatchesResourceDescription(*resource, rule, v1beta1.RequestInfo{}, nil, "", resource.GroupVersionKind(), "", []v1.AdmissionOperation{"CREATE"}); err == nil {
t.Errorf("Testcase has failed due to the following:\n Function has returned no error, even though it was supposed to fail")
}
}
2 changes: 2 additions & 0 deletions pkg/engine/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ func (e *engine) validate(
ctx context.Context,
logger logr.Logger,
policyContext engineapi.PolicyContext,
operations []kyvernov1.AdmissionOperation,
) engineapi.PolicyResponse {
resp := engineapi.NewPolicyResponse()
policy := policyContext.Policy()
Expand Down Expand Up @@ -69,6 +70,7 @@ func (e *engine) validate(
matchedResource,
rule,
engineapi.Validation,
operations...,
)
matchedResource = resource
resp.Add(engineapi.NewExecutionStats(startTime, time.Now()), ruleResp...)
Expand Down
2 changes: 1 addition & 1 deletion pkg/webhooks/resource/generation/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ func (h *generationHandler) processRequest(ctx context.Context, policyContext *e
policy.GetNamespace(),
gvk,
subresource,
policyContext.Operation(),
[]kyvernov1.AdmissionOperation{policyContext.Operation()},
); err == nil {
h.log.V(4).Info("skip creating UR as the admission resource is both the source and the trigger")
continue
Expand Down

0 comments on commit a5df960

Please sign in to comment.