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.

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

feat: cache namespaces in targethandler #1908

Merged
merged 31 commits into from
Mar 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6dbd7ff
Cache namespaces in targethandler
davis-haba Mar 10, 2022
c886a62
chore: bump actions/checkout from 2 to 3 (#1888)
dependabot[bot] Mar 2, 2022
899cb49
fix uninstall version typo (#1890)
avinashdesireddy Mar 3, 2022
ffdca05
chore: Remove unneeded spaces in helm chart (#1885)
mrueg Mar 3, 2022
ff170ba
chore: bump @docusaurus/core from 2.0.0-beta.16 to 2.0.0-beta.17 in /…
dependabot[bot] Mar 4, 2022
1158dc1
chore: bump @docusaurus/preset-classic from 2.0.0-beta.16 to 2.0.0-be…
dependabot[bot] Mar 4, 2022
9a96157
docs: add instructions on how to use tilt for development (#1895)
chewong Mar 8, 2022
b4a4191
test: Fix `BenchmarkValidationHandler` was broken (#1896)
mozillazg Mar 8, 2022
93c2615
Integration test for referential data in `gator test` (#1899)
julianKatz Mar 9, 2022
57d00a9
test: Fix `BenchmarkModifySetMutator_Mutate` was broken (#1897)
mozillazg Mar 9, 2022
5f68b9c
gofmt target.go
davis-haba Mar 10, 2022
abc709d
Add type assertion when pulling namespace from cache
davis-haba Mar 10, 2022
90c17c1
Do not use pointers for nsCache and RWLock when not necessary
davis-haba Mar 10, 2022
b6f6af6
return error when unsuccesful reading from nscache
davis-haba Mar 10, 2022
9569bf2
Revert "return error when unsuccesful reading from nscache"
davis-haba Mar 10, 2022
3d4e906
Add tests for nsCache
davis-haba Mar 10, 2022
eb94380
remove unused helpers in target_test.go
davis-haba Mar 10, 2022
6d91bba
remove commented code
davis-haba Mar 10, 2022
fe7db44
gofumpt target.go and target_test.go
davis-haba Mar 10, 2022
4a26e30
re-add accidently deleted test
davis-haba Mar 10, 2022
dd3c192
add helpers to original code location to clean up diff
davis-haba Mar 10, 2022
bfa17e8
gofumpt target.go
davis-haba Mar 10, 2022
cb8914c
Add remove cache tests. Change nsCache.Get/Add API to take a namespac…
davis-haba Mar 10, 2022
e125c91
namespace cache tests verifies extranious elements do not exist
davis-haba Mar 10, 2022
9d7fd83
increment with ++ instead of +=
davis-haba Mar 10, 2022
10b1213
Replace deprecated Ingress with new Ingress (#1906)
Mar 14, 2022
7e5fda8
do a audit run when we deploy (#1901)
grosser Mar 14, 2022
4163d8c
Merge branch 'master' into cache-namespaces
davis-haba Mar 14, 2022
650b65a
Do not add to cache within targethandler
davis-haba Mar 14, 2022
b2d0a58
use gkReq.Namespace as key when writing to cache
davis-haba Mar 15, 2022
43ea0fc
Merge branch 'master' into cache-namespaces
Mar 16, 2022
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
13 changes: 8 additions & 5 deletions pkg/target/errors.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package target

import "errors"
import (
"errors"
)

var (
ErrCreatingMather = errors.New("unable to create matcher")
ErrReviewFormat = errors.New("unsupported request review")
ErrRequestObject = errors.New("invalid request object")
ErrMatching = errors.New("error matching the requested object")
ErrCreatingMatcher = errors.New("unable to create matcher")
ErrReviewFormat = errors.New("unsupported request review")
ErrRequestObject = errors.New("invalid request object")
ErrMatching = errors.New("error matching the requested object")
ErrCachingType = errors.New("cannot cache non-namespace type")
)
98 changes: 79 additions & 19 deletions pkg/target/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"net/url"
"path"
"sync"
"text/template"

"github.com/open-policy-agent/frameworks/constraint/pkg/core/constraints"
Expand Down Expand Up @@ -47,12 +48,26 @@ const wildcardNSPattern = `^(\*|\*-)?[a-z0-9]([-a-z0-9]*[a-z0-9])?$|^[a-z0-9]([-

var _ handler.TargetHandler = &K8sValidationTarget{}

type K8sValidationTarget struct{}
type K8sValidationTarget struct {
cache nsCache
}

func (h *K8sValidationTarget) GetName() string {
return "admission.k8s.gatekeeper.sh"
}

func (h *K8sValidationTarget) Add(key string, object interface{}) error {
ns, ok := object.(*corev1.Namespace)
if !ok {
return fmt.Errorf("%w: cannot cache type %T", ErrCachingType, object)
}
return h.cache.Add(key, ns)
}

func (h *K8sValidationTarget) Remove(key string) {
h.cache.Remove(key)
}

var libTempl = template.Must(template.New("library").Parse(templSrc))

func (h *K8sValidationTarget) Library() *template.Template {
Expand Down Expand Up @@ -84,7 +99,7 @@ type unstable struct {
Namespace *corev1.Namespace `json:"namespace,omitempty"`
}

func processUnstructured(o *unstructured.Unstructured) (bool, string, interface{}, error) {
func (h *K8sValidationTarget) processUnstructured(o *unstructured.Unstructured) (bool, string, interface{}, error) {
// Namespace will be "" for cluster objects
gvk := o.GetObjectKind().GroupVersionKind()
if gvk.Version == "" {
Expand All @@ -103,9 +118,9 @@ func processUnstructured(o *unstructured.Unstructured) (bool, string, interface{
func (h *K8sValidationTarget) ProcessData(obj interface{}) (bool, string, interface{}, error) {
switch data := obj.(type) {
case unstructured.Unstructured:
return processUnstructured(&data)
return h.processUnstructured(&data)
case *unstructured.Unstructured:
return processUnstructured(data)
return h.processUnstructured(data)
case WipeData, *WipeData:
return processWipeData()
default:
Expand Down Expand Up @@ -444,41 +459,83 @@ func convertToMatch(object map[string]interface{}) (*match.Match, error) {
func (h *K8sValidationTarget) ToMatcher(u *unstructured.Unstructured) (constraints.Matcher, error) {
obj, found, err := unstructured.NestedMap(u.Object, "spec", "match")
if err != nil {
return nil, fmt.Errorf("%w: %v", ErrCreatingMather, err)
return nil, fmt.Errorf("%w: %v", ErrCreatingMatcher, err)
}
if found && obj != nil {
match, err := convertToMatch(obj)
if err != nil {
return nil, fmt.Errorf("%w: %v", ErrCreatingMather, err)
return nil, fmt.Errorf("%w: %v", ErrCreatingMatcher, err)
}
return &Matcher{match}, nil
return &Matcher{match, &h.cache}, nil
}
return nil, fmt.Errorf("%w: %s %s has no match found", ErrCreatingMather, u.GetKind(), u.GetName())
return nil, fmt.Errorf("%w: %s %s has no match found", ErrCreatingMatcher, u.GetKind(), u.GetName())
}

// Matcher implements constraint.Matcher.
// TODO: add cache when handler.Cache interface is available.
type Matcher struct {
match *match.Match
cache *nsCache
}

type nsCache struct {
Copy link
Member

@sozercan sozercan Mar 15, 2022

Choose a reason for hiding this comment

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

do we want to make this generic and replace

type nsCache struct {

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "make this generic"? Indicate that we should be able to cache other objects?

If so, we could, but we probably don't want to do that currently since we'd be duplicating OPA's cache. It's something we may want to consider as possible future work if we needed wider access to the cache, though.

Copy link
Member

Choose a reason for hiding this comment

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

I think Sertaç is talking about that we have a different nsCache in audit/manager.go. I had thought about that initially when looking at Davis's PR, but I think that deduplication would be better suited in a different PR. Audit's nsCache has slightly different uses, so changing that in this PR would make this change a bit larger (e.g. we'd have to figure out where to export the type so it's available from both locations). It's work that is easy to have as a follow-up

Copy link
Member

Choose a reason for hiding this comment

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

*follow-up = after merging in the frameworks changes

Copy link
Member

Choose a reason for hiding this comment

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

SGTM as a follow-up

lock sync.RWMutex
cache map[string]*corev1.Namespace
}

func (nc *nsCache) Add(key string, ns *corev1.Namespace) error {
nc.lock.Lock()
defer nc.lock.Unlock()

if nc.cache == nil {
nc.cache = make(map[string]*corev1.Namespace)
}

nc.cache[key] = ns
return nil
}

func (nc *nsCache) Get(key string) (*corev1.Namespace, error) {
nc.lock.RLock()
defer nc.lock.RUnlock()

ns, ok := nc.cache[key]
if !ok {
return nil, nil
}
return ns, nil
}

func (nc *nsCache) Remove(key string) {
nc.lock.Lock()
defer nc.lock.Unlock()
delete(nc.cache, key)
}

func (m *Matcher) Match(review interface{}) (bool, error) {
var gkReq *gkReview
Copy link
Contributor

Choose a reason for hiding this comment

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

@willbeason Will your follow-on PR make it so HandleReview() always returns a gkReview?

Side note... with these changes, we will also be able to drop that Unstable field when passing to Rego, since Rego no longer has need for an injected namespace

Copy link
Member

Choose a reason for hiding this comment

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

On the first note: sure, I'll make sure of that. On the second, it's an optimization we can look at later.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

switch req := review.(type) {
case *gkReview:
obj, oldObj, ns, err := gkReviewToObject(req)
if err != nil {
return false, err
}
return matchAny(m, ns, &obj, &oldObj)
gkReq = req
case gkReview:
obj, oldObj, ns, err := gkReviewToObject(&req)
gkReq = &req
default:
return false, fmt.Errorf("%w: expect %T, got %T", ErrReviewFormat, gkReview{}, review)
}

obj, oldObj, ns, err := gkReviewToObject(gkReq)
if err != nil {
return false, err
}

if ns == nil {
cachedNs, err := m.cache.Get(gkReq.Namespace)
if err != nil {
return false, err
}
return matchAny(m, ns, &obj, &oldObj)
default:
return false, fmt.Errorf("%w: expect gkReview, got %T", ErrReviewFormat, review)
ns = cachedNs
}

return matchAny(m, ns, &obj, &oldObj)
}

func matchAny(m *Matcher, ns *corev1.Namespace, objs ...*unstructured.Unstructured) (bool, error) {
Expand Down Expand Up @@ -518,4 +575,7 @@ func gkReviewToObject(req *gkReview) (obj, oldObj unstructured.Unstructured, ns
return obj, oldObj, req.Unstable.Namespace, nil
}

var _ constraints.Matcher = &Matcher{}
var (
_ constraints.Matcher = &Matcher{}
_ handler.Cache = &K8sValidationTarget{}
)
Loading