From 402fdb65ab60ead49aae544378b683c652b27dae Mon Sep 17 00:00:00 2001 From: natasha41575 Date: Tue, 31 Jan 2023 15:45:56 -0600 Subject: [PATCH 1/4] packagevariantset controller: support arbitrary object selectors --- .../api/v1alpha1/packagevariantset_types.go | 16 +++ .../packagevariantsets/config/rbac/role.yaml | 3 + .../config/samples/pvs.yaml | 11 +- .../packagevariantset/fake_client.go | 104 ++++++++++++++++++ .../packagevariantset_controller.go | 83 ++++++++++++-- .../packagevariantset_controller_test.go | 77 +++++++++++++ 6 files changed, 286 insertions(+), 8 deletions(-) create mode 100644 porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go diff --git a/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go b/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go index 8441b56181..0be7604a0a 100644 --- a/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go +++ b/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go @@ -15,6 +15,7 @@ package v1alpha1 import ( + kptfilev1 "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1" pkgvarapi "github.com/GoogleContainerTools/kpt/porch/controllers/packagevariants/api/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -101,6 +102,21 @@ type Selector struct { Annotations map[string]string `yaml:"annotations,omitempty" json:"annotations,omitempty"` } +func (s *Selector) ToKptfileSelector() kptfilev1.Selector { + var labels map[string]string + if s.Labels != nil { + labels = s.Labels.MatchLabels + } + return kptfilev1.Selector{ + APIVersion: s.APIVersion, + Kind: s.Kind, + Name: s.Name, + Namespace: s.Namespace, + Labels: labels, + Annotations: s.Annotations, + } +} + type PackageName struct { Name *ValueOrFromField `json:"baseName,omitempty"` diff --git a/porch/controllers/packagevariantsets/config/rbac/role.yaml b/porch/controllers/packagevariantsets/config/rbac/role.yaml index dad1da23ee..74eb4f40fd 100644 --- a/porch/controllers/packagevariantsets/config/rbac/role.yaml +++ b/porch/controllers/packagevariantsets/config/rbac/role.yaml @@ -46,3 +46,6 @@ rules: - get - patch - update +- apiGroups: ["*"] + resources: ["*"] + verbs: ["list"] diff --git a/porch/controllers/packagevariantsets/config/samples/pvs.yaml b/porch/controllers/packagevariantsets/config/samples/pvs.yaml index ecf877856c..fa7a99096c 100644 --- a/porch/controllers/packagevariantsets/config/samples/pvs.yaml +++ b/porch/controllers/packagevariantsets/config/samples/pvs.yaml @@ -33,4 +33,13 @@ spec: packageName: baseName: value: beta - + - objects: + selectors: + - apiVersion: v1 + kind: Pod + name: my-pod + repoName: + value: blueprints + packageName: + baseName: + value: gamma diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go new file mode 100644 index 0000000000..8114b30c18 --- /dev/null +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go @@ -0,0 +1,104 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package packagevariantset + +import ( + "context" + "fmt" + + pkgvarapi "github.com/GoogleContainerTools/kpt/porch/controllers/packagevariants/api/v1alpha1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/yaml" +) + +type fakeClient struct { + output []string + client.Client +} + +var _ client.Client = &fakeClient{} + +func (f *fakeClient) Create(_ context.Context, obj client.Object, _ ...client.CreateOption) error { + f.output = append(f.output, fmt.Sprintf("creating object: %s", obj.GetName())) + return nil +} + +func (f *fakeClient) Delete(_ context.Context, obj client.Object, _ ...client.DeleteOption) error { + f.output = append(f.output, fmt.Sprintf("deleting object: %s", obj.GetName())) + return nil +} + +func (f *fakeClient) List(_ context.Context, obj client.ObjectList, _ ...client.ListOption) error { + f.output = append(f.output, fmt.Sprintf("listing objects")) + podList := `apiVersion: v1 +kind: PodList +metadata: + name: my-pod-list +items: +- apiVersion: v1 + kind: Pod + metadata: + name: my-pod-1 + labels: + foo: bar + abc: def +- apiVersion: v1 + kind: Pod + metadata: + name: my-pod-2 + labels: + abc: def + efg: hij` + + pvList := `apiVersion: config.porch.kpt.dev +kind: PackageVariantList +metadata: + name: my-pv-list +items: +- apiVersion: config.porch.kpt.dev + kind: PackageVariant + metadata: + name: my-pv-1 + spec: + upstream: + repo: up + package: up + revision: up + downstream: + repo: dn-1 + package: dn-1 +- apiVersion: config.porch.kpt.dev + kind: PackageVariant + metadata: + name: my-pv-2 + spec: + upstream: + repo: up + package: up + revision: up + downstream: + repo: dn-2 + package: dn-2` + + switch v := obj.(type) { + case *unstructured.UnstructuredList: + return yaml.Unmarshal([]byte(podList), v) + case *pkgvarapi.PackageVariantList: + return yaml.Unmarshal([]byte(pvList), v) + default: + return nil + } +} diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go index 674033c8f5..c3362dfa9c 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go @@ -29,8 +29,11 @@ import ( pkgvarapi "github.com/GoogleContainerTools/kpt/porch/controllers/packagevariants/api/v1alpha1" api "github.com/GoogleContainerTools/kpt/porch/controllers/packagevariantsets/api/v1alpha1" + "github.com/GoogleContainerTools/kpt/internal/fnruntime" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" @@ -40,6 +43,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" + "sigs.k8s.io/kustomize/kyaml/resid" kyamlutils "sigs.k8s.io/kustomize/kyaml/utils" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -161,6 +165,14 @@ func validatePackageVariantSet(pvs *api.PackageVariantSet) []error { if target.Objects.RepoName == nil { allErrs = append(allErrs, fmt.Errorf("spec.targets[%d].objects must specify `repoName` field", i)) } + for j, selector := range target.Objects.Selectors { + if selector.APIVersion == "" { + allErrs = append(allErrs, fmt.Errorf("spec.targets[%d].objects.selectors[%d] must specify 'apiVersion'", i, j)) + } + if selector.Kind == "" { + allErrs = append(allErrs, fmt.Errorf("spec.targets[%d].objects.selectors[%d] must specify 'kind'", i, j)) + } + } count++ } if count != 1 { @@ -245,7 +257,11 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex case target.Objects != nil: // a selector against a set of arbitrary objects - pkgs, err := r.objectSet(ctx, &target, upstreamPackageName) + selectedObjects, err := r.getSelectedObjects(ctx, target.Objects.Selectors) + if err != nil { + return nil, err + } + pkgs, err := r.objectSet(&target, upstreamPackageName, selectedObjects) if err != nil { return nil, fmt.Errorf("error when selecting object set: %v", err) } @@ -271,7 +287,6 @@ func (r *PackageVariantSetReconciler) repositorySet( target *api.Target, upstreamPackageName string, repoList *configapi.RepositoryList) ([]*pkgvarapi.Downstream, error) { - var result []*pkgvarapi.Downstream for _, repo := range repoList.Items { repoAsRNode, err := r.convertObjectToRNode(&repo) @@ -291,11 +306,65 @@ func (r *PackageVariantSetReconciler) repositorySet( return result, nil } -func (r *PackageVariantSetReconciler) objectSet(ctx context.Context, - target *api.Target, - upstreamPackageName string) ([]*pkgvarapi.Downstream, error) { - // TODO: Implement this - return nil, fmt.Errorf("specifying a set of objects in the target is not yet supported") +func (r *PackageVariantSetReconciler) objectSet(target *api.Target, + upstreamPackageName string, + selectedObjects map[resid.ResId]*yaml.RNode) ([]*pkgvarapi.Downstream, error) { + var result []*pkgvarapi.Downstream + for _, obj := range selectedObjects { + downstreamPackageName, err := r.getDownstreamPackageName(target.PackageName, + upstreamPackageName, obj) + if err != nil { + return nil, err + } + repo, err := r.fetchValue(target.Objects.RepoName, obj) + if err != nil { + return nil, err + } + if repo == "" { + return nil, fmt.Errorf("error evaluating repo name: received empty string") + } + result = append(result, &pkgvarapi.Downstream{ + Package: downstreamPackageName, + Repo: repo, + }) + } + return result, nil +} + +func (r *PackageVariantSetReconciler) getSelectedObjects(ctx context.Context, selectors []api.Selector) (map[resid.ResId]*yaml.RNode, error) { + selectedObjects := make(map[resid.ResId]*yaml.RNode) // this is a map to prevent duplicates + + for _, selector := range selectors { + uList := &unstructured.UnstructuredList{} + group, version := resid.ParseGroupVersion(selector.APIVersion) + uList.SetGroupVersionKind(schema.GroupVersionKind{ + Group: group, + Version: version, + Kind: selector.Kind, + }) + + labelSelector, err := metav1.LabelSelectorAsSelector(selector.Labels) + if err != nil { + return nil, err + } + + if err := r.Client.List(ctx, uList, + client.InNamespace(selector.Namespace), + client.MatchingLabelsSelector{Selector: labelSelector}); err != nil { + return nil, fmt.Errorf("unable to list objects in cluster: %v", err) + } + + for _, u := range uList.Items { + objAsRNode, err := r.convertObjectToRNode(&u) + if err != nil { + return nil, fmt.Errorf("error converting unstructured object to RNode: %v", err) + } + if fnruntime.IsMatch(objAsRNode, selector.ToKptfileSelector()) { + selectedObjects[resid.FromRNode(objAsRNode)] = objAsRNode + } + } + } + return selectedObjects, nil } func (r *PackageVariantSetReconciler) getDownstreamPackageName(targetName *api.PackageName, diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go index 53f8e45eda..dd659ccb5d 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go @@ -15,6 +15,7 @@ package packagevariantset import ( + "context" "testing" configapi "github.com/GoogleContainerTools/kpt/porch/api/porchconfig/v1alpha1" @@ -22,7 +23,9 @@ import ( api "github.com/GoogleContainerTools/kpt/porch/controllers/packagevariantsets/api/v1alpha1" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/serializer/json" + "sigs.k8s.io/kustomize/kyaml/resid" kyaml "sigs.k8s.io/kustomize/kyaml/yaml" "sigs.k8s.io/yaml" ) @@ -286,3 +289,77 @@ packageName: }, }, result) } + +func TestGetSelectedObjects(t *testing.T) { + selectors := []api.Selector{{ + APIVersion: "v1", + Kind: "Pod", + Labels: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + }} + reconciler := &PackageVariantSetReconciler{ + Client: new(fakeClient), + serializer: json.NewSerializerWithOptions(json.DefaultMetaFactory, nil, nil, json.SerializerOptions{Yaml: true}), + } + selectedObjects, err := reconciler.getSelectedObjects(context.Background(), selectors) + require.NoError(t, err) + require.Equal(t, 1, len(selectedObjects)) + + expectedResId := resid.NewResIdWithNamespace(resid.NewGvk("", "v1", "Pod"), "my-pod-1", "") + obj, found := selectedObjects[expectedResId] + require.True(t, found) + require.Equal(t, `apiVersion: v1 +kind: Pod +metadata: + labels: + abc: def + foo: bar + name: my-pod-1 +`, obj.MustString()) +} + +func TestObjectSet(t *testing.T) { + selectedObjects := map[resid.ResId]*kyaml.RNode{ + resid.NewResIdWithNamespace(resid.NewGvk("", "v1", "Pod"), "my-pod-1", ""): kyaml.MustParse(`apiVersion: v1 +kind: Pod +metadata: + labels: + repo: my-repo + name: downstream +`), + } + + target := &api.Target{ + PackageName: &api.PackageName{ + Name: &api.ValueOrFromField{FromField: "metadata.name"}, + }, + Objects: &api.ObjectSelector{ + RepoName: &api.ValueOrFromField{FromField: "metadata.labels.repo"}, + }, + } + + pvs := &PackageVariantSetReconciler{} + objectSet, err := pvs.objectSet(target, "upstream", selectedObjects) + require.NoError(t, err) + require.Equal(t, len(objectSet), 1) + require.Equal(t, pkgvarapi.Downstream{ + Repo: "my-repo", + Package: "downstream", + }, *objectSet[0]) +} + +func TestEnsurePackageVariants(t *testing.T) { + upstream := &pkgvarapi.Upstream{Repo: "up", Package: "up", Revision: "up"} + downstreams := []*pkgvarapi.Downstream{ + {Repo: "dn-1", Package: "dn-1"}, + {Repo: "dn-3", Package: "dn-3"}, + } + pvs := &api.PackageVariantSet{ObjectMeta: metav1.ObjectMeta{Name: "my-pvs"}} + + fc := &fakeClient{} + reconciler := &PackageVariantSetReconciler{Client: fc} + require.NoError(t, reconciler.ensurePackageVariants(context.Background(), upstream, downstreams, pvs)) + require.Equal(t, []string{"listing objects", + "deleting object: my-pv-2", + "creating object: my-pvs-59bfcf77a6b032a656d78b14e2d92fa8c1e978a3", + }, fc.output) +} From 431eb4d4068935253e33232c332337a79307ba2c Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Fri, 17 Feb 2023 19:54:35 +0000 Subject: [PATCH 2/4] code review --- ...nfig.porch.kpt.dev_packagevariantsets.yaml | 71 ++++++++++++++++++- .../api/v1alpha1/packagevariantset_types.go | 4 +- .../api/v1alpha1/zz_generated.deepcopy.go | 10 +-- .../packagevariantset/fake_client.go | 27 +++++-- .../packagevariantset_controller.go | 54 ++++++++++---- .../packagevariantset_controller_test.go | 12 ++-- 6 files changed, 142 insertions(+), 36 deletions(-) diff --git a/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml b/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml index 76f9f57df8..1757dcc0dc 100644 --- a/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml +++ b/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml @@ -245,10 +245,75 @@ spec: status: description: PackageVariantSetStatus defines the observed state of PackageVariantSet properties: - validationErrors: - description: 'TODO: Move this to conditions.' + conditions: + description: Conditions describes the reconciliation state of the + object. items: - type: string + description: "Condition contains details for one aspect of the current + state of this API Resource. --- This struct is intended for direct + use as an array at the field path .status.conditions. For example, + \n type FooStatus struct{ // Represents the observations of a + foo's current state. // Known .status.conditions.type are: \"Available\", + \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge + // +listType=map // +listMapKey=type Conditions []metav1.Condition + `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" + protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }" + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another. This should be when + the underlying condition changed. If that is not known, then + using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: message is a human readable message indicating + details about the transition. This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: observedGeneration represents the .metadata.generation + that the condition was set based upon. For instance, if .metadata.generation + is currently 12, but the .status.conditions[x].observedGeneration + is 9, the condition is out of date with respect to the current + state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: reason contains a programmatic identifier indicating + the reason for the condition's last transition. Producers + of specific condition types may define expected values and + meanings for this field, and whether the values are considered + a guaranteed API. The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + --- Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object type: array type: object type: object diff --git a/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go b/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go index 0be7604a0a..45260bcd35 100644 --- a/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go +++ b/porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go @@ -132,8 +132,8 @@ type ValueOrFromField struct { // PackageVariantSetStatus defines the observed state of PackageVariantSet type PackageVariantSetStatus struct { - // TODO: Move this to conditions. - ValidationErrors []string `json:"validationErrors,omitempty"` + // Conditions describes the reconciliation state of the object. + Conditions []metav1.Condition `json:"conditions,omitempty"` } //+kubebuilder:object:root=true diff --git a/porch/controllers/packagevariantsets/api/v1alpha1/zz_generated.deepcopy.go b/porch/controllers/packagevariantsets/api/v1alpha1/zz_generated.deepcopy.go index 785b7ec9f0..b880c46947 100644 --- a/porch/controllers/packagevariantsets/api/v1alpha1/zz_generated.deepcopy.go +++ b/porch/controllers/packagevariantsets/api/v1alpha1/zz_generated.deepcopy.go @@ -199,10 +199,12 @@ func (in *PackageVariantSetSpec) DeepCopy() *PackageVariantSetSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PackageVariantSetStatus) DeepCopyInto(out *PackageVariantSetStatus) { *out = *in - if in.ValidationErrors != nil { - in, out := &in.ValidationErrors, &out.ValidationErrors - *out = make([]string, len(*in)) - copy(*out, *in) + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]v1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } } diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go index 8114b30c18..4ea508b9b8 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go @@ -25,24 +25,29 @@ import ( ) type fakeClient struct { - output []string + objects []client.Object client.Client } var _ client.Client = &fakeClient{} func (f *fakeClient) Create(_ context.Context, obj client.Object, _ ...client.CreateOption) error { - f.output = append(f.output, fmt.Sprintf("creating object: %s", obj.GetName())) + f.objects = append(f.objects, obj) return nil } func (f *fakeClient) Delete(_ context.Context, obj client.Object, _ ...client.DeleteOption) error { - f.output = append(f.output, fmt.Sprintf("deleting object: %s", obj.GetName())) + var newObjects []client.Object + for _, old := range f.objects { + if obj.GetName() != old.GetName() { + newObjects = append(newObjects, old) + } + } + f.objects = newObjects return nil } func (f *fakeClient) List(_ context.Context, obj client.ObjectList, _ ...client.ListOption) error { - f.output = append(f.output, fmt.Sprintf("listing objects")) podList := `apiVersion: v1 kind: PodList metadata: @@ -93,12 +98,20 @@ items: repo: dn-2 package: dn-2` + var err error switch v := obj.(type) { case *unstructured.UnstructuredList: - return yaml.Unmarshal([]byte(podList), v) + err = yaml.Unmarshal([]byte(podList), v) + for _, o := range v.Items { + f.objects = append(f.objects, o.DeepCopy()) + } case *pkgvarapi.PackageVariantList: - return yaml.Unmarshal([]byte(pvList), v) + err = yaml.Unmarshal([]byte(pvList), v) + for _, o := range v.Items { + f.objects = append(f.objects, o.DeepCopy()) + } default: - return nil + return fmt.Errorf("unsupported type") } + return err } diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go index c3362dfa9c..9d122661cb 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -80,13 +81,21 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, nil } + var conditions []metav1.Condition + if errs := validatePackageVariantSet(pvs); len(errs) > 0 { - pvs.Status.ValidationErrors = nil for _, validationErr := range errs { if validationErr.Error() != "" { - pvs.Status.ValidationErrors = append(pvs.Status.ValidationErrors, validationErr.Error()) + conditions = append(conditions, metav1.Condition{ + Type: "Valid", + Status: "False", + Reason: "ValidationFailed", + Message: validationErr.Error(), + LastTransitionTime: metav1.Now(), + }) } } + pvs.Status.Conditions = conditions statusUpdateErr := r.Client.Status().Update(ctx, pvs) return ctrl.Result{}, statusUpdateErr } @@ -102,7 +111,20 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re downstreams, err := r.unrollDownstreamTargets(ctx, pvs, upstream.Package) if err != nil { - return ctrl.Result{}, err + if strings.Contains(err.Error(), "unable to list specified objects in cluster") { + conditions = append(conditions, metav1.Condition{ + Type: "ObjectsListed", + Status: "False", + Reason: "Error", + Message: err.Error(), + LastTransitionTime: metav1.Now(), + }) + pvs.Status.Conditions = conditions + statusUpdateErr := r.Client.Status().Update(ctx, pvs) + return ctrl.Result{}, statusUpdateErr + } else { + return ctrl.Result{}, err + } } if err := r.ensurePackageVariants(ctx, upstream, downstreams, pvs); err != nil { @@ -231,7 +253,7 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex pvs *api.PackageVariantSet, upstreamPackageName string) ([]*pkgvarapi.Downstream, error) { var result []*pkgvarapi.Downstream - for _, target := range pvs.Spec.Targets { + for i, target := range pvs.Spec.Targets { switch { case target.Package != nil: // an explicit repo/package name pair @@ -249,6 +271,9 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex client.MatchingLabelsSelector{Selector: selector}); err != nil { return nil, err } + if len(repoList.Items) == 0 { + klog.Warningf("no repositories selected by spec.targets[%d]", i) + } pkgs, err := r.repositorySet(&target, upstreamPackageName, &repoList) if err != nil { return nil, fmt.Errorf("error when selecting repository set: %v", err) @@ -261,6 +286,9 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex if err != nil { return nil, err } + if len(selectedObjects) == 0 { + klog.Warningf("no objects selected by spec.targets[%d]", i) + } pkgs, err := r.objectSet(&target, upstreamPackageName, selectedObjects) if err != nil { return nil, fmt.Errorf("error when selecting object set: %v", err) @@ -343,15 +371,16 @@ func (r *PackageVariantSetReconciler) getSelectedObjects(ctx context.Context, se Kind: selector.Kind, }) - labelSelector, err := metav1.LabelSelectorAsSelector(selector.Labels) - if err != nil { - return nil, err + opts := []client.ListOption{client.InNamespace(selector.Namespace)} + if selector.Labels != nil { + labelSelector, err := metav1.LabelSelectorAsSelector(selector.Labels) + if err != nil { + return nil, err + } + opts = append(opts, client.MatchingLabelsSelector{Selector: labelSelector}) } - - if err := r.Client.List(ctx, uList, - client.InNamespace(selector.Namespace), - client.MatchingLabelsSelector{Selector: labelSelector}); err != nil { - return nil, fmt.Errorf("unable to list objects in cluster: %v", err) + if err := r.Client.List(ctx, uList, opts...); err != nil { + return nil, fmt.Errorf("unable to list specified objects in cluster: %v", err) } for _, u := range uList.Items { @@ -446,7 +475,6 @@ func (r *PackageVariantSetReconciler) fetchValue(value *api.ValueOrFromField, func (r *PackageVariantSetReconciler) ensurePackageVariants(ctx context.Context, upstream *pkgvarapi.Upstream, downstreams []*pkgvarapi.Downstream, pvs *api.PackageVariantSet) error { - var pvList pkgvarapi.PackageVariantList if err := r.Client.List(ctx, &pvList, client.InNamespace(pvs.Namespace), diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go index dd659ccb5d..aea9de00fc 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go @@ -353,13 +353,11 @@ func TestEnsurePackageVariants(t *testing.T) { {Repo: "dn-1", Package: "dn-1"}, {Repo: "dn-3", Package: "dn-3"}, } - pvs := &api.PackageVariantSet{ObjectMeta: metav1.ObjectMeta{Name: "my-pvs"}} - fc := &fakeClient{} reconciler := &PackageVariantSetReconciler{Client: fc} - require.NoError(t, reconciler.ensurePackageVariants(context.Background(), upstream, downstreams, pvs)) - require.Equal(t, []string{"listing objects", - "deleting object: my-pv-2", - "creating object: my-pvs-59bfcf77a6b032a656d78b14e2d92fa8c1e978a3", - }, fc.output) + require.NoError(t, reconciler.ensurePackageVariants(context.Background(), upstream, downstreams, + &api.PackageVariantSet{ObjectMeta: metav1.ObjectMeta{Name: "my-pvs"}})) + require.Equal(t, 2, len(fc.objects)) + require.Equal(t, "my-pv-1", fc.objects[0].GetName()) + require.Equal(t, "my-pvs-28ace69e71f644931cd8cc1e8e9388f4de486901", fc.objects[1].GetName()) } From 30e2e1d69a1a0fbb7aa322072b5a038ca8ba9417 Mon Sep 17 00:00:00 2001 From: natasha41575 Date: Thu, 23 Feb 2023 21:56:05 +0000 Subject: [PATCH 3/4] code review --- .../packagevariantset_controller.go | 82 ++++++++++--------- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go index 9d122661cb..3b91378bc6 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go @@ -30,6 +30,7 @@ import ( api "github.com/GoogleContainerTools/kpt/porch/controllers/packagevariantsets/api/v1alpha1" "github.com/GoogleContainerTools/kpt/internal/fnruntime" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -81,23 +82,26 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, nil } - var conditions []metav1.Condition + defer func() { + r.Client.Status().Update(ctx, pvs) + }() if errs := validatePackageVariantSet(pvs); len(errs) > 0 { - for _, validationErr := range errs { - if validationErr.Error() != "" { - conditions = append(conditions, metav1.Condition{ - Type: "Valid", - Status: "False", - Reason: "ValidationFailed", - Message: validationErr.Error(), - LastTransitionTime: metav1.Now(), - }) - } - } - pvs.Status.Conditions = conditions - statusUpdateErr := r.Client.Status().Update(ctx, pvs) - return ctrl.Result{}, statusUpdateErr + validationErr := combineErrors(errs) + meta.SetStatusCondition(&pvs.Status.Conditions, metav1.Condition{ + Type: "Valid", + Status: "False", + Reason: "Invalid", + Message: validationErr, + }) + return ctrl.Result{}, fmt.Errorf(validationErr) + } else { + meta.SetStatusCondition(&pvs.Status.Conditions, metav1.Condition{ + Type: "Valid", + Status: "True", + Reason: "Valid", + Message: "all validation checks passed", + }) } upstream, err := r.getUpstreamPR(pvs.Spec.Upstream) @@ -105,33 +109,25 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, err } if upstream == nil { + // Currently, this code will never be reached, because the upstream.Tag option + // is not yet implemented. return ctrl.Result{}, fmt.Errorf("could not find specified upstream") } - downstreams, err := r.unrollDownstreamTargets(ctx, pvs, - upstream.Package) + downstreams, err := r.unrollDownstreamTargets(ctx, pvs, upstream.Package) if err != nil { - if strings.Contains(err.Error(), "unable to list specified objects in cluster") { - conditions = append(conditions, metav1.Condition{ - Type: "ObjectsListed", - Status: "False", - Reason: "Error", - Message: err.Error(), - LastTransitionTime: metav1.Now(), + if meta.IsNoMatchError(err) { + meta.SetStatusCondition(&pvs.Status.Conditions, metav1.Condition{ + Type: "Valid", + Status: "False", + Reason: "Invalid", + Message: err.Error(), }) - pvs.Status.Conditions = conditions - statusUpdateErr := r.Client.Status().Update(ctx, pvs) - return ctrl.Result{}, statusUpdateErr - } else { - return ctrl.Result{}, err } - } - - if err := r.ensurePackageVariants(ctx, upstream, downstreams, pvs); err != nil { return ctrl.Result{}, err } - return ctrl.Result{}, nil + return ctrl.Result{}, r.ensurePackageVariants(ctx, upstream, downstreams, pvs) } func (r *PackageVariantSetReconciler) init(ctx context.Context, req ctrl.Request) (*api.PackageVariantSet, error) { @@ -145,16 +141,16 @@ func (r *PackageVariantSetReconciler) init(ctx context.Context, req ctrl.Request func validatePackageVariantSet(pvs *api.PackageVariantSet) []error { var allErrs []error if pvs.Spec.Upstream == nil { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "upstream"), "{}", "missing required field")) + allErrs = append(allErrs, fmt.Errorf("spec.upstream is a required field")) } else { if pvs.Spec.Upstream.Package == nil { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "upstream", "package"), "{}", "missing required field")) + allErrs = append(allErrs, fmt.Errorf("spec.upstream.package is a required field")) } else { if pvs.Spec.Upstream.Package.Name == "" { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "upstream", "package", "name"), "", "missing required field")) + allErrs = append(allErrs, fmt.Errorf("spec.upstream.package.name is a required field")) } if pvs.Spec.Upstream.Package.Repo == "" { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "upstream", "package", "repo"), "", "missing required field")) + allErrs = append(allErrs, fmt.Errorf("spec.upstream.package.repo is a required field")) } } if (pvs.Spec.Upstream.Tag == "" && pvs.Spec.Upstream.Revision == "") || @@ -223,6 +219,16 @@ func validatePackageVariantSet(pvs *api.PackageVariantSet) []error { return allErrs } +func combineErrors(errs []error) string { + var errMsgs []string + for _, e := range errs { + if e.Error() != "" { + errMsgs = append(errMsgs, e.Error()) + } + } + return strings.Join(errMsgs, "; ") +} + func (r *PackageVariantSetReconciler) getUpstreamPR( upstream *api.Upstream) (*pkgvarapi.Upstream, error) { @@ -380,7 +386,7 @@ func (r *PackageVariantSetReconciler) getSelectedObjects(ctx context.Context, se opts = append(opts, client.MatchingLabelsSelector{Selector: labelSelector}) } if err := r.Client.List(ctx, uList, opts...); err != nil { - return nil, fmt.Errorf("unable to list specified objects in cluster: %v", err) + return nil, err } for _, u := range uList.Items { From 57057d0d95685e17c27d1e299ccbfa2bf4309e2c Mon Sep 17 00:00:00 2001 From: natasha41575 Date: Tue, 28 Feb 2023 11:16:17 -0600 Subject: [PATCH 4/4] nits --- .../packagevariantset_controller.go | 5 ++++- .../packagevariantset_controller_test.go | 14 +++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go index 3b91378bc6..563e83a4ab 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go @@ -83,7 +83,9 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re } defer func() { - r.Client.Status().Update(ctx, pvs) + if err := r.Client.Status().Update(ctx, pvs); err != nil { + klog.Errorf("could not update status: %w\n", err) + } }() if errs := validatePackageVariantSet(pvs); len(errs) > 0 { @@ -123,6 +125,7 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re Reason: "Invalid", Message: err.Error(), }) + return ctrl.Result{}, nil } return ctrl.Result{}, err } diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go index aea9de00fc..865432010f 100644 --- a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go +++ b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go @@ -42,7 +42,7 @@ metadata: }{ "empty spec": { packageVariant: packageVariantHeader, - expectedErrs: []string{"spec.upstream: Invalid value: \"{}\": missing required field", + expectedErrs: []string{"spec.upstream is a required field", "must specify at least one item in spec.targets", }, }, @@ -52,7 +52,7 @@ spec: upstream: revision: v1 tag: main`, - expectedErrs: []string{"spec.upstream.package: Invalid value: \"{}\": missing required field", + expectedErrs: []string{"spec.upstream.package is a required field", "must specify at least one item in spec.targets", }, }, @@ -62,7 +62,7 @@ spec: upstream: package: name: foo`, - expectedErrs: []string{"spec.upstream.package.repo: Invalid value: \"\": missing required field", + expectedErrs: []string{"spec.upstream.package.repo is a required field", "must have one of spec.upstream.revision and spec.upstream.tag", "must specify at least one item in spec.targets", }, @@ -73,7 +73,7 @@ spec: upstream: package: repo: foo`, - expectedErrs: []string{"spec.upstream.package.name: Invalid value: \"\": missing required field", + expectedErrs: []string{"spec.upstream.package.name is a required field", "must have one of spec.upstream.revision and spec.upstream.tag", "must specify at least one item in spec.targets", }, @@ -98,7 +98,7 @@ spec: repoName: value: foo `, - expectedErrs: []string{"spec.upstream: Invalid value: \"{}\": missing required field", + expectedErrs: []string{"spec.upstream is a required field", "spec.targets[0] cannot specify both fields `packageName` and `package`", "spec.targets[0].package.repo cannot be empty when using `package`", "spec.targets[1] must specify one of `package`, `repositories`, or `objects`", @@ -112,7 +112,7 @@ spec: adoptionPolicy: invalid deletionPolicy: invalid `, - expectedErrs: []string{"spec.upstream: Invalid value: \"{}\": missing required field", + expectedErrs: []string{"spec.upstream is a required field", "must specify at least one item in spec.targets", "spec.adoptionPolicy: Invalid value: \"invalid\": field can only be \"adoptNone\" or \"adoptExisting\"", "spec.deletionPolicy: Invalid value: \"invalid\": field can only be \"orphan\" or \"delete\"", @@ -124,7 +124,7 @@ spec: adoptionPolicy: adoptExisting deletionPolicy: orphan `, - expectedErrs: []string{"spec.upstream: Invalid value: \"{}\": missing required field", + expectedErrs: []string{"spec.upstream is a required field", "must specify at least one item in spec.targets", }, },