Skip to content

perf: use metadata client for InventoryPolicyApplyFilter #672

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 10 additions & 8 deletions pkg/apply/applier.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/discovery"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/metadata"
"k8s.io/klog/v2"
"sigs.k8s.io/cli-utils/pkg/apis/actuation"
"sigs.k8s.io/cli-utils/pkg/apply/cache"
Expand Down Expand Up @@ -42,13 +43,14 @@ import (
// parameters and/or the set of resources that needs to be applied to the
// cluster, different sets of tasks might be needed.
type Applier struct {
pruner *prune.Pruner
statusWatcher watcher.StatusWatcher
invClient inventory.Client
client dynamic.Interface
openAPIGetter discovery.OpenAPISchemaInterface
mapper meta.RESTMapper
infoHelper info.Helper
pruner *prune.Pruner
statusWatcher watcher.StatusWatcher
invClient inventory.Client
client dynamic.Interface
metadataClient metadata.Interface
openAPIGetter discovery.OpenAPISchemaInterface
mapper meta.RESTMapper
infoHelper info.Helper
}

// prepareObjects returns the set of objects to apply and to prune or
Expand Down Expand Up @@ -128,7 +130,7 @@ func (a *Applier) Run(ctx context.Context, invInfo inventory.Info, objects objec
// Build list of apply validation filters.
applyFilters := []filter.ValidationFilter{
filter.InventoryPolicyApplyFilter{
Client: a.client,
Client: a.metadataClient,
Mapper: a.mapper,
Inv: invInfo,
InvPolicy: options.InventoryPolicy,
Expand Down
19 changes: 13 additions & 6 deletions pkg/apply/applier_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"k8s.io/cli-runtime/pkg/resource"
"k8s.io/client-go/discovery"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/metadata"
"k8s.io/client-go/rest"
"k8s.io/kubectl/pkg/cmd/util"
"sigs.k8s.io/cli-utils/pkg/apply/info"
Expand Down Expand Up @@ -38,12 +39,13 @@ func (b *ApplierBuilder) Build() (*Applier, error) {
Client: bx.client,
Mapper: bx.mapper,
},
statusWatcher: bx.statusWatcher,
invClient: bx.invClient,
client: bx.client,
openAPIGetter: bx.discoClient,
mapper: bx.mapper,
infoHelper: info.NewHelper(bx.mapper, bx.unstructuredClientForMapping),
statusWatcher: bx.statusWatcher,
invClient: bx.invClient,
client: bx.client,
metadataClient: bx.metadataClient,
openAPIGetter: bx.discoClient,
mapper: bx.mapper,
infoHelper: info.NewHelper(bx.mapper, bx.unstructuredClientForMapping),
}, nil
}

Expand All @@ -62,6 +64,11 @@ func (b *ApplierBuilder) WithDynamicClient(client dynamic.Interface) *ApplierBui
return b
}

func (b *ApplierBuilder) WithMetadataClient(client metadata.Interface) *ApplierBuilder {
b.metadataClient = client
return b
}

func (b *ApplierBuilder) WithDiscoveryClient(discoClient discovery.CachedDiscoveryInterface) *ApplierBuilder {
b.discoClient = discoClient
return b
Expand Down
8 changes: 8 additions & 0 deletions pkg/apply/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"k8s.io/cli-runtime/pkg/resource"
"k8s.io/client-go/discovery"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/metadata"
"k8s.io/client-go/rest"
"k8s.io/kubectl/pkg/cmd/util"
"sigs.k8s.io/cli-utils/pkg/inventory"
Expand All @@ -21,6 +22,7 @@ type commonBuilder struct {
// factory is only used to retrieve things that have not been provided explicitly.
factory util.Factory
invClient inventory.Client
metadataClient metadata.Interface
client dynamic.Interface
discoClient discovery.CachedDiscoveryInterface
mapper meta.RESTMapper
Expand Down Expand Up @@ -72,6 +74,12 @@ func (cb *commonBuilder) finalize() (*commonBuilder, error) {
return nil, fmt.Errorf("error getting rest config: %v", err)
}
}
if cx.metadataClient == nil {
cx.metadataClient, err = metadata.NewForConfig(cx.restConfig)
if err != nil {
return nil, fmt.Errorf("error getting metadata client: %v", err)
}
}
if cx.unstructuredClientForMapping == nil {
if cx.factory == nil {
return nil, fmt.Errorf("a factory must be provided or all other options: %v", err)
Expand Down
61 changes: 50 additions & 11 deletions pkg/apply/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/cli-runtime/pkg/resource"
dynamicfake "k8s.io/client-go/dynamic/fake"
metadatafake "k8s.io/client-go/metadata/fake"
"k8s.io/client-go/rest/fake"
clienttesting "k8s.io/client-go/testing"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -64,18 +65,22 @@ func newTestApplier(
clusterObjs object.UnstructuredSet,
statusWatcher watcher.StatusWatcher,
) *Applier {
tf := newTestFactory(t, invObj, resources, clusterObjs)
objs := newResourceInfo(resources, clusterObjs)
tf := newTestFactory(t, invObj, objs)
defer tf.Cleanup()

infoHelper := &fakeInfoHelper{
factory: tf,
}

invClient := newTestInventory(t, tf)
mapper, err := tf.ToRESTMapper()
require.NoError(t, err)

applier, err := NewApplierBuilder().
WithFactory(tf).
WithInventoryClient(invClient).
WithMetadataClient(fakeMetadataClient(t, mapper, objs)).
WithStatusWatcher(statusWatcher).
Build()
require.NoError(t, err)
Expand All @@ -93,7 +98,8 @@ func newTestDestroyer(
clusterObjs object.UnstructuredSet,
statusWatcher watcher.StatusWatcher,
) *Destroyer {
tf := newTestFactory(t, invObj, object.UnstructuredSet{}, clusterObjs)
objs := newResourceInfo(object.UnstructuredSet{}, clusterObjs)
tf := newTestFactory(t, invObj, objs)
defer tf.Cleanup()

invClient := newTestInventory(t, tf)
Expand All @@ -119,17 +125,10 @@ func newTestInventory(
return invClient
}

func newTestFactory(
t *testing.T,
invObj *unstructured.Unstructured,
func newResourceInfo(
resourceSet object.UnstructuredSet,
clusterObjs object.UnstructuredSet,
) *cmdtesting.TestFactory {
tf := cmdtesting.NewTestFactory().WithNamespace(invObj.GetNamespace())

mapper, err := tf.ToRESTMapper()
require.NoError(t, err)

) []resourceInfo {
objMap := make(map[object.ObjMetadata]resourceInfo)
for _, r := range resourceSet {
objMeta := object.UnstructuredToObjMetadata(r)
Expand All @@ -149,6 +148,18 @@ func newTestFactory(
for _, obj := range objMap {
objs = append(objs, obj)
}
return objs
}

func newTestFactory(
t *testing.T,
invObj *unstructured.Unstructured,
objs []resourceInfo,
) *cmdtesting.TestFactory {
tf := cmdtesting.NewTestFactory().WithNamespace(invObj.GetNamespace())

mapper, err := tf.ToRESTMapper()
require.NoError(t, err)

handlers := []handler{
&nsHandler{},
Expand Down Expand Up @@ -418,6 +429,34 @@ func (f *fakeInfoHelper) getClient(gv schema.GroupVersion) (resource.RESTClient,
return f.factory.Client, nil
}

// fakeMetadataClient returns a fake metadata client.
func fakeMetadataClient(t *testing.T, mapper meta.RESTMapper, objs []resourceInfo) *metadatafake.FakeMetadataClient {
fakeClient := metadatafake.NewSimpleMetadataClient(scheme.Scheme)

for i := range objs {
obj := objs[i]
gvk := obj.resource.GroupVersionKind()
mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
require.NoError(t, err)
r := mapping.Resource.Resource
objMeta := &metav1.PartialObjectMetadata{}
objMeta.APIVersion = obj.resource.GetAPIVersion()
objMeta.Kind = obj.resource.GetKind()
objMeta.Name = obj.resource.GetName()
objMeta.Namespace = obj.resource.GetNamespace()
objMeta.Annotations = obj.resource.GetAnnotations()
objMeta.Labels = obj.resource.GetLabels()
fakeClient.PrependReactor("get", r, func(clienttesting.Action) (bool, runtime.Object, error) {
if obj.exists {
return true, objMeta, nil
}
return false, nil, nil
})
}

return fakeClient
}

// fakeDynamicClient returns a fake dynamic client.
func fakeDynamicClient(t *testing.T, mapper meta.RESTMapper, invObj *unstructured.Unstructured, objs ...resourceInfo) *dynamicfake.FakeDynamicClient {
fakeClient := dynamicfake.NewSimpleDynamicClient(scheme.Scheme)
Expand Down
6 changes: 3 additions & 3 deletions pkg/apply/filter/inventory-policy-apply-filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/metadata"
"sigs.k8s.io/cli-utils/pkg/inventory"
"sigs.k8s.io/cli-utils/pkg/object"
)
Expand All @@ -20,7 +20,7 @@ import (
// if an object should be applied based on the cluster object's inventory id,
// the id for the inventory object, and the inventory policy.
type InventoryPolicyApplyFilter struct {
Client dynamic.Interface
Client metadata.Interface
Mapper meta.RESTMapper
Inv inventory.Info
InvPolicy inventory.Policy
Expand Down Expand Up @@ -55,7 +55,7 @@ func (ipaf InventoryPolicyApplyFilter) Filter(ctx context.Context, obj *unstruct
}

// getObject retrieves the passed object from the cluster, or an error if one occurred.
func (ipaf InventoryPolicyApplyFilter) getObject(ctx context.Context, id object.ObjMetadata) (*unstructured.Unstructured, error) {
func (ipaf InventoryPolicyApplyFilter) getObject(ctx context.Context, id object.ObjMetadata) (*metav1.PartialObjectMetadata, error) {
mapping, err := ipaf.Mapper.RESTMapping(id.GroupKind)
if err != nil {
return nil, err
Expand Down
21 changes: 18 additions & 3 deletions pkg/apply/filter/inventory-policy-apply-filter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import (

"github.com/stretchr/testify/require"
"k8s.io/apimachinery/pkg/api/meta/testrestmapper"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
dynamicfake "k8s.io/client-go/dynamic/fake"
metadatafake "k8s.io/client-go/metadata/fake"
"k8s.io/kubectl/pkg/scheme"
"sigs.k8s.io/cli-utils/pkg/apis/actuation"
"sigs.k8s.io/cli-utils/pkg/common"
Expand All @@ -28,6 +29,17 @@ var invObjTemplate = &unstructured.Unstructured{
},
}

var defaultMetadataObj = &metav1.PartialObjectMetadata{
ObjectMeta: metav1.ObjectMeta{
Name: defaultObj.GetName(),
Namespace: defaultObj.GetNamespace(),
},
TypeMeta: metav1.TypeMeta{
Kind: defaultObj.GetKind(),
APIVersion: defaultObj.GetAPIVersion(),
},
}

func TestInventoryPolicyApplyFilter(t *testing.T) {
tests := map[string]struct {
inventoryID string
Expand Down Expand Up @@ -89,11 +101,14 @@ func TestInventoryPolicyApplyFilter(t *testing.T) {

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
obj := defaultObj.DeepCopy()
objIDAnnotation := map[string]string{
"config.k8s.io/owning-inventory": tc.objInventoryID,
}

obj := defaultObj.DeepCopy()
obj.SetAnnotations(objIDAnnotation)
metadataObj := defaultMetadataObj.DeepCopy()
metadataObj.SetAnnotations(objIDAnnotation)
invIDLabel := map[string]string{
common.InventoryLabel: tc.inventoryID,
}
Expand All @@ -102,7 +117,7 @@ func TestInventoryPolicyApplyFilter(t *testing.T) {
invInfoObj, err := inventory.ConfigMapToInventoryInfo(invObj)
require.NoError(t, err)
filter := InventoryPolicyApplyFilter{
Client: dynamicfake.NewSimpleDynamicClient(scheme.Scheme, obj),
Client: metadatafake.NewSimpleMetadataClient(scheme.Scheme, metadataObj),
Mapper: testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme,
scheme.Scheme.PrioritizedVersionsAllGroups()...),
Inv: invInfoObj,
Expand Down
8 changes: 6 additions & 2 deletions pkg/inventory/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ const (
NoMatch
)

func IDMatch(inv Info, obj *unstructured.Unstructured) IDMatchStatus {
type Annotated interface {
GetAnnotations() map[string]string
}

func IDMatch(inv Info, obj Annotated) IDMatchStatus {
annotations := obj.GetAnnotations()
value, found := annotations[OwningInventoryKey]
if !found {
Expand All @@ -95,7 +99,7 @@ func IDMatch(inv Info, obj *unstructured.Unstructured) IDMatchStatus {
return NoMatch
}

func CanApply(inv Info, obj *unstructured.Unstructured, policy Policy) (bool, error) {
func CanApply(inv Info, obj Annotated, policy Policy) (bool, error) {
matchStatus := IDMatch(inv, obj)
switch matchStatus {
case Empty:
Expand Down