Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement the match logic #932

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Implement the match logic #932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21359d7
Implement matching logic
fedepaol Nov 2, 2020
55aaaff
Integrate the mutators with the match logic
fedepaol Nov 25, 2020
5ca2951
Merge branch 'master' into matchlogic
ritazh Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions pkg/mutation/assign_mutator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import (
"github.com/open-policy-agent/gatekeeper/pkg/mutation/path/parser"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
)

// AssignMutator is a mutator object built out of a
Expand All @@ -22,9 +22,13 @@ type AssignMutator struct {
// AssignMutator implements mutatorWithSchema
var _ MutatorWithSchema = &AssignMutator{}

func (m *AssignMutator) Matches(obj metav1.Object, ns *corev1.Namespace) bool {
// TODO implement using matches function
return false
func (m *AssignMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
matches, err := Matches(m.assign.Spec.Match, obj, ns)
if err != nil {
log.Error(err, "AssignMutator.Matches failed", "assign", m.assign.Name)
return false
}
return matches
}

func (m *AssignMutator) Mutate(obj *unstructured.Unstructured) error {
Expand Down
12 changes: 8 additions & 4 deletions pkg/mutation/assignmeta_mutator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import (
"github.com/open-policy-agent/gatekeeper/pkg/mutation/path/parser"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
)

var (
Expand Down Expand Up @@ -44,9 +44,13 @@ type AssignMetadataMutator struct {
// assignMetadataMutator implements mutator
var _ Mutator = &AssignMetadataMutator{}

func (m *AssignMetadataMutator) Matches(obj metav1.Object, ns *corev1.Namespace) bool {
// TODO implement using matches function
return false
func (m *AssignMetadataMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
matches, err := Matches(m.assignMetadata.Spec.Match, obj, ns)
if err != nil {
log.Error(err, "AssignMetadataMutator.Matches failed", "assignMeta", m.assignMetadata.Name)
return false
}
return matches
}

func (m *AssignMetadataMutator) Mutate(obj *unstructured.Unstructured) error {
Expand Down
157 changes: 157 additions & 0 deletions pkg/mutation/match.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
package mutation

import (
"fmt"

mutationsv1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"
corev1 "k8s.io/api/core/v1"
apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: merge import blocks

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
)

// Matches verifies if the given object belonging to the given namespace
// matches the current mutator.
func Matches(match mutationsv1.Match, obj runtime.Object, ns *corev1.Namespace) (bool, error) {
meta, err := meta.Accessor(obj)
if err != nil {
return false, fmt.Errorf("Accessor failed for %s", obj.GetObjectKind().GroupVersionKind().Kind)
}

foundMatch := false

for _, kk := range match.Kinds {
kindMatches := false
groupMatches := false

for _, k := range kk.Kinds {
if k == "*" || k == obj.GetObjectKind().GroupVersionKind().Kind {
kindMatches = true
break
}
}
if len(kk.Kinds) == 0 {
kindMatches = true
}

for _, g := range kk.APIGroups {
if g == "*" || g == obj.GetObjectKind().GroupVersionKind().Group {
groupMatches = true
break
}
}
if len(kk.APIGroups) == 0 {
groupMatches = true
}

if kindMatches && groupMatches {
foundMatch = true
}
}
if len(match.Kinds) == 0 {
foundMatch = true
}

if !foundMatch {
return false, nil
}

if match.Scope == apiextensionsv1beta1.ClusterScoped &&
meta.GetNamespace() != "" {
return false, nil
}

if match.Scope == apiextensionsv1beta1.NamespaceScoped &&
meta.GetNamespace() == "" {
return false, nil
}

found := false
for _, n := range match.Namespaces {
if meta.GetNamespace() == n {
found = true
break
}
}
if !found && len(match.Namespaces) > 0 {
return false, nil
}

for _, n := range match.ExcludedNamespaces {
if meta.GetNamespace() == n {
return false, nil
}
}
if match.LabelSelector != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also match the label selector against old object?

It seems weird, but that's how MutatingWebhookConfiguration's logic works:

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#mutatingwebhook-v1-admissionregistration-k8s-io

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed it was only against the old object, as it's the criteria for applying a given mutation.

Given all the stability premises and the fact that we are only adding (and not changing) labels, I think it's safe to assume that if the old object matches the selector, the new one will. If only the new one will, it will be a matter of another round of mutation before the stability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the old object/new object premise doesn't really work for mutation. It's a bit weird to be deviating from K8s convention and what we do with admission control, but I think it makes sense.

@shomron @ritazh @sozercan @brycecr thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the last bit, I think apart from this discussion this is good to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the above people to the review, hopefully that kick-starts the discussion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on not deviating from K8s mutatingwebhookconfig logic to avoid surprises to users

we are only adding (and not changing) labels

what if users want to match label on the old object then mutate the object to delete that label? is that a supported scenario for MVP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understood.
Here defining "the old" vs "the new" is a bit tricky, as we apply a bunch of mutations at the same time, and we can't just refer to the old as "before all mutations" and new as "after all mutations" because the decision of whether applying a mutation or not is based on the match logic. I think what we do here is to test only against "the old" object, with respect to the single mutation.

See

gatekeeper/pkg/mutation/system.go

Line 85 in 624bfcc

if m.Matches(obj, ns) {

In that case, what is referred as old vs new? And what should happen if we find out that after a mutation the object does not match anymore?
Is it fine to assume that we match against the old, we mutate and then we "rollback" the mutation (i.e restoring a backup of the object) if match of the single mutation fails afterwards?

Re: deleting labels, this is not possible with the current design as we are only adding non-existing labels / annotations. This is needed for making sure the mutation converges.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given all the reasons above, +1 on not testing OldObject for now

Re: deleting labels, this is not possible with the current design as we are only adding non-existing labels / annotations. This is needed for making sure the mutation converges.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lemme make sure I'm getting this straight:
If we also match against newObject, then, if a mutation were to add a label to a resource, then all instances of that resource type would match. The other angle on that would be that you couldn't add a label to a subset of objects of one GVK by selecting by label.

I think I don't grok the purpose of applying to both old and new objects unless you allow the mutation webhook to create or delete objects. It seems unexpected at best.

I'm in favor of shirking the k8s standard here and doing what makes more common sense, but I suspect I am missing something, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One point of confusion is OldObject has a specific meaning, defined here:

https://github.com/kubernetes/api/blob/4a626d306b987a4096cf0784ec01af1be2f6d67f/admission/v1beta1/types.go#L101-L106

I don't think that changes much, if any, of the above analysis, but has been the source of a bit of confusion.

selector, err := metav1.LabelSelectorAsSelector(match.LabelSelector)
if err != nil {
return false, err
}
if !selector.Matches(labels.Set(meta.GetLabels())) {
return false, nil
}
}

if match.NamespaceSelector != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For MutatingWebhookConfigurations:

  • Cluster scoped resources always match an NS selector
  • If the object itself is an NS, the namespace selector is evaluated against the object's labels
  • Unclear whether the old object + new object matching logic also applies here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The namespace remains the same, so there won't be difference in new vs old.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the first two points and added tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The namespace would remain the same unless the object itself is a namespace.

selector, err := metav1.LabelSelectorAsSelector(match.NamespaceSelector)
if err != nil {
return false, err
}

switch {
case isNamespace(obj): // if the object is a namespace, namespace selector matches against the object
if !selector.Matches(labels.Set(meta.GetLabels())) {
return false, nil
}
case meta.GetNamespace() == "":
// cluster scoped, matches by default
case !selector.Matches(labels.Set(ns.Labels)):
return false, nil
}
}

return true, nil
}

// AppliesTo checks if any item the given slice of ApplyTo applies to the given object
func AppliesTo(applyTo []mutationsv1.ApplyTo, obj *unstructured.Unstructured) bool {
for _, apply := range applyTo {
matchesGroup := false
matchesVersion := false
matchesKind := false

gvk := obj.GroupVersionKind()
for _, g := range apply.Groups {
if g == gvk.Group {
matchesGroup = true
break
}
}
for _, g := range apply.Versions {
if g == gvk.Version {
matchesVersion = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we handle when there are multiple versions of the same group kind in the cluster? this here would resolve to mismatch but the apiserver would consider it a match.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applyTo should be a strict check, because mutations create implicit schema.

Note that this check is for the mutation.spec.applyTo field, not spec.match.kinds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I got the point.
ApplyTo allows to specify multiple versions, which would then match (or, apply) to different versions of the same group.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Let's make sure we document this. To support multiple versions of the same schema, user needs to add another gvk to the applyTo list in the mutation object.

break
}
}
for _, g := range apply.Kinds {
if g == gvk.Kind {
matchesKind = true
break
}
}
if matchesGroup &&
matchesVersion &&
matchesKind {
return true
}
}
return false
}

func isNamespace(obj runtime.Object) bool {
return obj.GetObjectKind().GroupVersionKind().Kind == "Namespace" &&
obj.GetObjectKind().GroupVersionKind().Group == ""
}
Loading