From 840b0781093cc736b1bdfa9ab9d52406276f8311 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Mon, 24 Apr 2023 18:13:22 -0500 Subject: [PATCH] porch: packagevariant controller conditions (#3898) * porch: packagevariantcontroller conditions * code review * change valid condition to stalled * remove lifecycle from status * code review --- .../config.porch.kpt.dev_packagevariants.yaml | 80 +++++++++- .../api/v1alpha1/packagevariant_types.go | 12 +- .../api/v1alpha1/zz_generated.deepcopy.go | 29 +++- .../packagevariant_controller.go | 141 ++++++++++++------ .../packagevariant_controller_test.go | 14 +- 5 files changed, 216 insertions(+), 60 deletions(-) diff --git a/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariants.yaml b/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariants.yaml index 39f402df6..6bb1de9a9 100644 --- a/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariants.yaml +++ b/porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariants.yaml @@ -97,10 +97,84 @@ spec: status: description: PackageVariantStatus defines the observed state of PackageVariant 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 + downstreamTargets: + description: DownstreamTargets contains the downstream targets that + the PackageVariant either created or adopted. + items: + properties: + name: + type: string + type: object type: array type: object type: object diff --git a/porch/controllers/packagevariants/api/v1alpha1/packagevariant_types.go b/porch/controllers/packagevariants/api/v1alpha1/packagevariant_types.go index 59994da0a..dc32d936a 100644 --- a/porch/controllers/packagevariants/api/v1alpha1/packagevariant_types.go +++ b/porch/controllers/packagevariants/api/v1alpha1/packagevariant_types.go @@ -86,8 +86,16 @@ type PackageContext struct { // PackageVariantStatus defines the observed state of PackageVariant type PackageVariantStatus 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"` + + // DownstreamTargets contains the downstream targets that the PackageVariant + // either created or adopted. + DownstreamTargets []DownstreamTarget `json:"downstreamTargets,omitempty"` +} + +type DownstreamTarget struct { + Name string `json:"name,omitempty"` } //+kubebuilder:object:root=true diff --git a/porch/controllers/packagevariants/api/v1alpha1/zz_generated.deepcopy.go b/porch/controllers/packagevariants/api/v1alpha1/zz_generated.deepcopy.go index 45170d538..47e289241 100644 --- a/porch/controllers/packagevariants/api/v1alpha1/zz_generated.deepcopy.go +++ b/porch/controllers/packagevariants/api/v1alpha1/zz_generated.deepcopy.go @@ -20,6 +20,7 @@ package v1alpha1 import ( + "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" ) @@ -38,6 +39,21 @@ func (in *Downstream) DeepCopy() *Downstream { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DownstreamTarget) DeepCopyInto(out *DownstreamTarget) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DownstreamTarget. +func (in *DownstreamTarget) DeepCopy() *DownstreamTarget { + if in == nil { + return nil + } + out := new(DownstreamTarget) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PackageContext) DeepCopyInto(out *PackageContext) { *out = *in @@ -171,9 +187,16 @@ func (in *PackageVariantSpec) DeepCopy() *PackageVariantSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PackageVariantStatus) DeepCopyInto(out *PackageVariantStatus) { *out = *in - if in.ValidationErrors != nil { - in, out := &in.ValidationErrors, &out.ValidationErrors - *out = make([]string, len(*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]) + } + } + if in.DownstreamTargets != nil { + in, out := &in.DownstreamTargets, &out.DownstreamTargets + *out = make([]DownstreamTarget, len(*in)) copy(*out, *in) } } diff --git a/porch/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go b/porch/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go index 08c1457cc..088a2da9a 100644 --- a/porch/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go +++ b/porch/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go @@ -28,6 +28,7 @@ import ( "github.com/GoogleContainerTools/kpt-functions-sdk/go/fn" "golang.org/x/mod/semver" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" @@ -53,6 +54,9 @@ type PackageVariantReconciler struct { const ( workspaceNamePrefix = "packagevariant-" + + ConditionTypeStalled = "Stalled" // whether or not the packagevariant object is making progress or not + ConditionTypeReady = "Ready" // whether or notthe reconciliation succeded ) //go:generate go run sigs.k8s.io/controller-tools/cmd/controller-gen@v0.8.0 rbac:roleName=porch-controllers-packagevariants webhook paths="." output:rbac:artifacts:config=../../../config/rbac @@ -73,6 +77,12 @@ func (r *PackageVariantReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } + defer func() { + if err := r.Client.Status().Update(ctx, pv); err != nil { + klog.Errorf("could not update status: %s\n", err.Error()) + } + }() + if !pv.ObjectMeta.DeletionTimestamp.IsZero() { // This object is being deleted, so we need to make sure the packagerevisions owned by this object // are deleted. Normally, garbage collection can handle this, but we have a special case here because @@ -105,24 +115,23 @@ func (r *PackageVariantReconciler) Reconcile(ctx context.Context, req ctrl.Reque } if errs := validatePackageVariant(pv); len(errs) > 0 { - pv.Status.ValidationErrors = nil - for _, validationErr := range errs { - if validationErr.Error() != "" { - pv.Status.ValidationErrors = append(pv.Status.ValidationErrors, validationErr.Error()) - } - } - statusUpdateErr := r.Client.Status().Update(ctx, pv) - return ctrl.Result{}, statusUpdateErr - } - - upstream := r.getUpstreamPR(pv.Spec.Upstream, prList) - if upstream == nil { - return ctrl.Result{}, fmt.Errorf("could not find upstream package revision") + setStalledConditionsToTrue(pv, combineErrors(errs)) + return ctrl.Result{}, nil } - - if err := r.ensurePackageVariant(ctx, pv, upstream, prList); err != nil { + upstream, err := r.getUpstreamPR(pv.Spec.Upstream, prList) + if err != nil { + setStalledConditionsToTrue(pv, err.Error()) return ctrl.Result{}, err } + meta.SetStatusCondition(&pv.Status.Conditions, metav1.Condition{ + Type: ConditionTypeStalled, + Status: "False", + Reason: "Valid", + Message: "all validation checks passed", + }) + + targets, err := r.ensurePackageVariant(ctx, pv, upstream, prList) + setTargetStatusConditions(pv, targets, err) return ctrl.Result{}, nil } @@ -142,29 +151,29 @@ func (r *PackageVariantReconciler) init(ctx context.Context, return &pv, &prList, nil } -func validatePackageVariant(pv *api.PackageVariant) field.ErrorList { - allErrs := field.ErrorList{} +func validatePackageVariant(pv *api.PackageVariant) []string { + var allErrs []string if pv.Spec.Upstream == nil { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "upstream"), "{}", "missing required field")) + allErrs = append(allErrs, "missing required field spec.upstream") } else { if pv.Spec.Upstream.Repo == "" { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "upstream", "repo"), pv.Spec.Upstream.Repo, "missing required field")) + allErrs = append(allErrs, "missing required field spec.upstream.repo") } if pv.Spec.Upstream.Package == "" { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "upstream", "package"), pv.Spec.Upstream.Package, "missing required field")) + allErrs = append(allErrs, "missing required field spec.upstream.package") } if pv.Spec.Upstream.Revision == "" { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "upstream", "revision"), pv.Spec.Upstream.Revision, "missing required field")) + allErrs = append(allErrs, "missing required field spec.upstream.revision") } } if pv.Spec.Downstream == nil { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "downstream"), "{}", "missing required field")) + allErrs = append(allErrs, "missing required field spec.downstream") } else { if pv.Spec.Downstream.Repo == "" { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "downstream", "repo"), pv.Spec.Downstream.Repo, "missing required field")) + allErrs = append(allErrs, "missing required field spec.downstream.repo") } if pv.Spec.Downstream.Package == "" { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "downstream", "package"), pv.Spec.Downstream.Package, "missing required field")) + allErrs = append(allErrs, "missing required field spec.downstream.package") } } if pv.Spec.AdoptionPolicy == "" { @@ -174,16 +183,12 @@ func validatePackageVariant(pv *api.PackageVariant) field.ErrorList { pv.Spec.DeletionPolicy = api.DeletionPolicyDelete } if pv.Spec.AdoptionPolicy != api.AdoptionPolicyAdoptNone && pv.Spec.AdoptionPolicy != api.AdoptionPolicyAdoptExisting { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec", "adoptionPolicy"), pv.Spec.AdoptionPolicy, - fmt.Sprintf("field can only be %q or %q", - api.AdoptionPolicyAdoptNone, api.AdoptionPolicyAdoptExisting))) + allErrs = append(allErrs, fmt.Sprintf("spec.adoptionPolicy field can only be %q or %q", + api.AdoptionPolicyAdoptNone, api.AdoptionPolicyAdoptExisting)) } if pv.Spec.DeletionPolicy != api.DeletionPolicyOrphan && pv.Spec.DeletionPolicy != api.DeletionPolicyDelete { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec", "deletionPolicy"), pv.Spec.DeletionPolicy, - fmt.Sprintf("field can only be %q or %q", - api.DeletionPolicyOrphan, api.DeletionPolicyDelete))) + allErrs = append(allErrs, fmt.Sprintf("spec.deletionPolicy can only be %q or %q", + api.DeletionPolicyOrphan, api.DeletionPolicyDelete)) } if pc := pv.Spec.PackageContext; pc != nil { invalidKeys := []string{"name", "package-path"} @@ -193,7 +198,7 @@ func validatePackageVariant(pv *api.PackageVariant) field.ErrorList { allErrs = append(allErrs, field.Invalid( field.NewPath("spec", "packageContext", "data"), pv.Spec.PackageContext.Data, - fmt.Sprintf("must not contain the key %q", invalid))) + fmt.Sprintf("must not contain the key %q", invalid)).Error()) } } if len(pc.RemoveKeys) > 0 { @@ -202,7 +207,7 @@ func validatePackageVariant(pv *api.PackageVariant) field.ErrorList { allErrs = append(allErrs, field.Invalid( field.NewPath("spec", "packageContext", "removeKeys"), pv.Spec.PackageContext.RemoveKeys, - fmt.Sprintf("must not contain the key %q", invalid))) + fmt.Sprintf("must not contain the key %q", invalid)).Error()) } } } @@ -211,16 +216,42 @@ func validatePackageVariant(pv *api.PackageVariant) field.ErrorList { return allErrs } +func combineErrors(errs []string) string { + var errMsgs []string + for _, e := range errs { + if e != "" { + errMsgs = append(errMsgs, e) + } + } + return strings.Join(errMsgs, "; ") +} + func (r *PackageVariantReconciler) getUpstreamPR(upstream *api.Upstream, - prList *porchapi.PackageRevisionList) *porchapi.PackageRevision { + prList *porchapi.PackageRevisionList) (*porchapi.PackageRevision, error) { for _, pr := range prList.Items { if pr.Spec.RepositoryName == upstream.Repo && pr.Spec.PackageName == upstream.Package && pr.Spec.Revision == upstream.Revision { - return &pr + return &pr, nil } } - return nil + return nil, fmt.Errorf("could not find upstream package revision '%s/%s' in repo '%s'", + upstream.Package, upstream.Revision, upstream.Repo) +} + +func setStalledConditionsToTrue(pv *api.PackageVariant, message string) { + meta.SetStatusCondition(&pv.Status.Conditions, metav1.Condition{ + Type: ConditionTypeStalled, + Status: "True", + Reason: "ValidationError", + Message: message, + }) + meta.SetStatusCondition(&pv.Status.Conditions, metav1.Condition{ + Type: ConditionTypeReady, + Status: "False", + Reason: "Error", + Message: "invalid packagevariant object", + }) } // ensurePackageVariant needs to: @@ -236,13 +267,13 @@ func (r *PackageVariantReconciler) getUpstreamPR(upstream *api.Upstream, func (r *PackageVariantReconciler) ensurePackageVariant(ctx context.Context, pv *api.PackageVariant, upstream *porchapi.PackageRevision, - prList *porchapi.PackageRevisionList) error { + prList *porchapi.PackageRevisionList) ([]*porchapi.PackageRevision, error) { existing, err := r.findAndUpdateExistingRevisions(ctx, pv, upstream, prList) if err != nil { - return err + return nil, err } if existing != nil { - return nil + return existing, nil } // No downstream package created by this controller exists. Create one. @@ -277,14 +308,14 @@ func (r *PackageVariantReconciler) ensurePackageVariant(ctx context.Context, } if err := r.Client.Create(ctx, newPR); err != nil { - return err + return nil, err } klog.Infoln(fmt.Sprintf("package variant %q created package revision %q", pv.Name, newPR.Name)) if err := r.applyMutations(ctx, pv, newPR); err != nil { - return err + return nil, err } - return nil + return []*porchapi.PackageRevision{newPR}, nil } func (r *PackageVariantReconciler) findAndUpdateExistingRevisions(ctx context.Context, @@ -591,6 +622,30 @@ func (r *PackageVariantReconciler) updateDraft(ctx context.Context, return draft, nil } +func setTargetStatusConditions(pv *api.PackageVariant, targets []*porchapi.PackageRevision, err error) { + pv.Status.DownstreamTargets = nil + if err != nil { + meta.SetStatusCondition(&pv.Status.Conditions, metav1.Condition{ + Type: ConditionTypeReady, + Status: "False", + Reason: "Error", + Message: err.Error(), + }) + return + } + for _, t := range targets { + pv.Status.DownstreamTargets = append(pv.Status.DownstreamTargets, api.DownstreamTarget{ + Name: t.GetName(), + }) + } + meta.SetStatusCondition(&pv.Status.Conditions, metav1.Condition{ + Type: ConditionTypeReady, + Status: "True", + Reason: "NoErrors", + Message: "successfully ensured downstream package variant", + }) +} + // SetupWithManager sets up the controller with the Manager. func (r *PackageVariantReconciler) SetupWithManager(mgr ctrl.Manager) error { if err := api.AddToScheme(mgr.GetScheme()); err != nil { diff --git a/porch/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller_test.go b/porch/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller_test.go index 0e7aaaf27..1a77f74ad 100644 --- a/porch/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller_test.go +++ b/porch/controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller_test.go @@ -37,7 +37,7 @@ metadata: }{ "empty spec": { packageVariant: packageVariantHeader, - expectedErr: "[spec.upstream: Invalid value: \"{}\": missing required field, spec.downstream: Invalid value: \"{}\": missing required field]", + expectedErr: "missing required field spec.upstream; missing required field spec.downstream", }, "missing package names": { @@ -49,7 +49,7 @@ spec: downstream: repo: deployments `, - expectedErr: "[spec.upstream.package: Invalid value: \"\": missing required field, spec.downstream.package: Invalid value: \"\": missing required field]", + expectedErr: "missing required field spec.upstream.package; missing required field spec.downstream.package", }, "empty adoption and deletion policies": { @@ -78,7 +78,7 @@ spec: adoptionPolicy: invalid deletionPolicy: invalid `, - expectedErr: "[spec.adoptionPolicy: Invalid value: \"invalid\": field can only be \"adoptNone\" or \"adoptExisting\", spec.deletionPolicy: Invalid value: \"invalid\": field can only be \"orphan\" or \"delete\"]", + expectedErr: "spec.adoptionPolicy field can only be \"adoptNone\" or \"adoptExisting\"; spec.deletionPolicy can only be \"orphan\" or \"delete\"", }, "valid adoption and deletion policies": { @@ -189,12 +189,8 @@ spec: t.Run(tn, func(t *testing.T) { var pv api.PackageVariant require.NoError(t, yaml.Unmarshal([]byte(tc.packageVariant), &pv)) - actualErr := validatePackageVariant(&pv).ToAggregate() - if tc.expectedErr == "" { - require.NoError(t, actualErr) - } else { - require.EqualError(t, actualErr, tc.expectedErr) - } + actualErr := combineErrors(validatePackageVariant(&pv)) + require.Equal(t, tc.expectedErr, actualErr) }) } }