From f83c32f77611339bba4cf3b107284b46b73b197d Mon Sep 17 00:00:00 2001 From: veophi Date: Mon, 20 Dec 2021 09:24:13 +0800 Subject: [PATCH] fix bug for rd: remove forbidden ns; fix dry-run; merge old&new metadata; improve update logic; watch resource changes Signed-off-by: veophi --- Makefile | 2 +- .../v1alpha1/resourcedistribution_types.go | 9 +- .../apps.kruise.io_resourcedistributions.yaml | 17 ++- .../resourcedistribution_controller.go | 97 ++++++++++---- .../resourcedistribution_controller_test.go | 32 +++-- pkg/controller/resourcedistribution/utils.go | 88 +++++++++---- ...ourcedistribution_create_update_handler.go | 38 +++--- .../resourcedistribution_validating_test.go | 2 +- .../resourcedistribution/validating/utils.go | 35 ++---- test/e2e/apps/resourcedistribution.go | 118 +++++++++++------- .../framework/resourcedistribution_utils.go | 33 +++-- 11 files changed, 301 insertions(+), 170 deletions(-) diff --git a/Makefile b/Makefile index b879b489f6..14bd4a9a28 100644 --- a/Makefile +++ b/Makefile @@ -49,7 +49,7 @@ test: generate fmt vet manifests ## Run tests ##@ Build -build: manifests generate fmt vet ## Build manager binary. +build: generate fmt vet manifests ## Build manager binary. go build -o bin/manager main.go run: manifests generate fmt vet ## Run a controller from your host. diff --git a/apis/apps/v1alpha1/resourcedistribution_types.go b/apis/apps/v1alpha1/resourcedistribution_types.go index 4c997b1228..2c572ab8fa 100644 --- a/apis/apps/v1alpha1/resourcedistribution_types.go +++ b/apis/apps/v1alpha1/resourcedistribution_types.go @@ -159,9 +159,12 @@ const ( // +genclient // +genclient:nonNamespaced // +k8s:openapi-gen=true -//+kubebuilder:object:root=true -//+kubebuilder:subresource:status -//+kubebuilder:resource:scope=Cluster +// +kubebuilder:object:root=true +// +kubebuilder:subresource:status +// +kubebuilder:resource:scope=Cluster,shortName=distributor +// +kubebuilder:printcolumn:name="TOTAL",type="integer",JSONPath=".status.desired",description="The desired number of desired distribution and syncs." +// +kubebuilder:printcolumn:name="SUCCEED",type="integer",JSONPath=".status.succeeded",description="The number of successful distribution and syncs." +// +kubebuilder:printcolumn:name="FAILED",type="integer",JSONPath=".status.failed",description="The number of failed distributions and syncs." // ResourceDistribution is the Schema for the resourcedistributions API. type ResourceDistribution struct { diff --git a/config/crd/bases/apps.kruise.io_resourcedistributions.yaml b/config/crd/bases/apps.kruise.io_resourcedistributions.yaml index 7a341779ee..1f1f17e63a 100644 --- a/config/crd/bases/apps.kruise.io_resourcedistributions.yaml +++ b/config/crd/bases/apps.kruise.io_resourcedistributions.yaml @@ -13,10 +13,25 @@ spec: kind: ResourceDistribution listKind: ResourceDistributionList plural: resourcedistributions + shortNames: + - distributor singular: resourcedistribution scope: Cluster versions: - - name: v1alpha1 + - additionalPrinterColumns: + - description: The desired number of desired distribution and syncs. + jsonPath: .status.desired + name: TOTAL + type: integer + - description: The number of successful distribution and syncs. + jsonPath: .status.succeeded + name: SUCCEED + type: integer + - description: The number of failed distributions and syncs. + jsonPath: .status.failed + name: FAILED + type: integer + name: v1alpha1 schema: openAPIV3Schema: description: ResourceDistribution is the Schema for the resourcedistributions diff --git a/pkg/controller/resourcedistribution/resourcedistribution_controller.go b/pkg/controller/resourcedistribution/resourcedistribution_controller.go index bc23bbcaf4..49d3d6247e 100644 --- a/pkg/controller/resourcedistribution/resourcedistribution_controller.go +++ b/pkg/controller/resourcedistribution/resourcedistribution_controller.go @@ -106,6 +106,46 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return err } + // Watch for changes to Secrets + err = c.Watch(&source.Kind{Type: &corev1.Secret{}}, &handler.EnqueueRequestForOwner{ + IsController: true, OwnerType: &appsv1alpha1.ResourceDistribution{}, + }, predicate.Funcs{ + CreateFunc: func(createEvent event.CreateEvent) bool { + return false + }, + UpdateFunc: func(updateEvent event.UpdateEvent) bool { + oldObject, oldOK := updateEvent.ObjectOld.(*corev1.Secret) + newObject, newOK := updateEvent.ObjectNew.(*corev1.Secret) + if !oldOK || !newOK { + return false + } + return !reflect.DeepEqual(oldObject.Data, newObject.Data) || !reflect.DeepEqual(oldObject.StringData, newObject.StringData) + }, + }) + if err != nil { + return err + } + + // Watch for changes to ConfigMap + err = c.Watch(&source.Kind{Type: &corev1.ConfigMap{}}, &handler.EnqueueRequestForOwner{ + IsController: true, OwnerType: &appsv1alpha1.ResourceDistribution{}, + }, predicate.Funcs{ + CreateFunc: func(createEvent event.CreateEvent) bool { + return false + }, + UpdateFunc: func(updateEvent event.UpdateEvent) bool { + oldObject, oldOK := updateEvent.ObjectOld.(*corev1.ConfigMap) + newObject, newOK := updateEvent.ObjectNew.(*corev1.ConfigMap) + if !oldOK || !newOK { + return false + } + return !reflect.DeepEqual(oldObject.Data, newObject.Data) || !reflect.DeepEqual(oldObject.BinaryData, newObject.BinaryData) + }, + }) + if err != nil { + return err + } + return nil } @@ -170,7 +210,7 @@ func (r *ReconcileResourceDistribution) doReconcile(distributor *appsv1alpha1.Re _, cleanErrList := r.cleanResource(distributor, unmatchedNamespaces, resource) // 3. process all errors about resource distribution and cleanup - conditions, errList := r.handleErrors(distributor.Name, distributeErrList, cleanErrList) + conditions, errList := r.handleErrors(distributeErrList, cleanErrList) // 4. update distributor status newStatus := calculateNewStatus(distributor, conditions, int32(len(matchedNamespaces)), succeeded) @@ -185,9 +225,18 @@ func (r *ReconcileResourceDistribution) distributeResource(distributor *appsv1al resourceName := utils.ConvertToUnstructured(resource).GetName() resourceKind := resource.GetObjectKind().GroupVersionKind().Kind - newResourceHashCode := hashResource(distributor.Spec.Resource) - + resourceHashCode := hashResource(distributor.Spec.Resource) return syncItSlowly(matchedNamespaces, 1, func(namespace string) *UnexpectedError { + ns := &corev1.Namespace{} + getNSErr := r.Client.Get(context.TODO(), types.NamespacedName{Name: namespace}, ns) + if errors.IsNotFound(getNSErr) || (getNSErr == nil && ns.DeletionTimestamp != nil) { + return &UnexpectedError{ + err: fmt.Errorf("namespace not found or is terminating"), + namespace: namespace, + conditionID: NotExistConditionID, + } + } + // 1. try to fetch existing old resource oldResource := &unstructured.Unstructured{} oldResource.SetGroupVersionKind(resource.GetObjectKind().GroupVersionKind()) @@ -202,17 +251,10 @@ func (r *ReconcileResourceDistribution) distributeResource(distributor *appsv1al } // 2. if resource doesn't exist, create resource; - newResource := makeResourceObject(distributor, namespace, resource, newResourceHashCode).(client.Object) if getErr != nil && errors.IsNotFound(getErr) { - if createErr := r.Client.Create(context.TODO(), newResource); createErr != nil { + newResource := makeResourceObject(distributor, namespace, resource, resourceHashCode, nil) + if createErr := r.Client.Create(context.TODO(), newResource.(client.Object)); createErr != nil { klog.Errorf("Error occurred when creating resource in namespace %s, err： %v, name: %s", namespace, createErr, distributor.Name) - if errors.IsNotFound(createErr) { - return &UnexpectedError{ - err: fmt.Errorf("namespace not found"), - namespace: namespace, - conditionID: NotExistConditionID, - } - } return &UnexpectedError{ err: createErr, namespace: namespace, @@ -223,19 +265,20 @@ func (r *ReconcileResourceDistribution) distributeResource(distributor *appsv1al return nil } - // 3. if resource exits - annotations := oldResource.GetAnnotations() - if annotations == nil || annotations[utils.SourceResourceDistributionOfResource] != distributor.Name { - // if conflict occurred + // 3. check conflict + if !isControlledByDistributor(oldResource, distributor) { klog.Errorf("Conflict with existing resource(%s/%s) in namespaces %s, name: %s", resourceKind, resourceName, namespace, distributor.Name) return &UnexpectedError{ - err: fmt.Errorf("conflict with existing resources because of the same namespace and name"), + err: fmt.Errorf("conflict with existing resources because of the same namespace, group, version, kind and name"), namespace: namespace, conditionID: ConflictConditionID, } - } else if annotations[utils.ResourceHashCodeAnnotation] != newResourceHashCode { - // else if the resource needs to update - if updateErr := r.Client.Update(context.TODO(), newResource); updateErr != nil { + } + + // 4. check whether resource need to update + if needToUpdate(oldResource, utils.ConvertToUnstructured(resource)) { + newResource := makeResourceObject(distributor, namespace, resource, resourceHashCode, oldResource) + if updateErr := r.Client.Update(context.TODO(), newResource.(client.Object)); updateErr != nil { klog.Errorf("Error occurred when updating resource in namespace %s, err： %v, name: %s", namespace, updateErr, distributor.Name) return &UnexpectedError{ err: updateErr, @@ -255,6 +298,12 @@ func (r *ReconcileResourceDistribution) cleanResource(distributor *appsv1alpha1. resourceName := utils.ConvertToUnstructured(resource).GetName() resourceKind := resource.GetObjectKind().GroupVersionKind().Kind return syncItSlowly(unmatchedNamespaces, 1, func(namespace string) *UnexpectedError { + ns := &corev1.Namespace{} + getNSErr := r.Client.Get(context.TODO(), types.NamespacedName{Name: namespace}, ns) + if errors.IsNotFound(getNSErr) || (getNSErr == nil && ns.DeletionTimestamp != nil) { + return nil + } + // 1. try to fetch existing old resource oldResource := &unstructured.Unstructured{} oldResource.SetGroupVersionKind(resource.GetObjectKind().GroupVersionKind()) @@ -270,11 +319,11 @@ func (r *ReconcileResourceDistribution) cleanResource(distributor *appsv1alpha1. } } - // 2. just return if the owner of the oldResource is not this distributor - annotations := oldResource.GetAnnotations() - if annotations == nil || annotations[utils.SourceResourceDistributionOfResource] != distributor.Name { + // 2. if the owner of the oldResource is not this distributor, just return + if !isControlledByDistributor(oldResource, distributor) { return nil } + // 3. else clean the resource if deleteErr := r.Client.Delete(context.TODO(), oldResource); deleteErr != nil && !errors.IsNotFound(deleteErr) { klog.Errorf("Error occurred when deleting resource in namespace %s from client, err： %v, name: %s", namespace, deleteErr, distributor.Name) @@ -290,7 +339,7 @@ func (r *ReconcileResourceDistribution) cleanResource(distributor *appsv1alpha1. } // handlerErrors process all errors about resource distribution and clean, and record them to conditions -func (r *ReconcileResourceDistribution) handleErrors(distributorName string, errLists ...[]*UnexpectedError) ([]appsv1alpha1.ResourceDistributionCondition, field.ErrorList) { +func (r *ReconcileResourceDistribution) handleErrors(errLists ...[]*UnexpectedError) ([]appsv1alpha1.ResourceDistributionCondition, field.ErrorList) { // init a status.conditions conditions := make([]appsv1alpha1.ResourceDistributionCondition, NumberOfConditionTypes) initConditionType(conditions) diff --git a/pkg/controller/resourcedistribution/resourcedistribution_controller_test.go b/pkg/controller/resourcedistribution/resourcedistribution_controller_test.go index 3aadafa7c2..8c2a003209 100644 --- a/pkg/controller/resourcedistribution/resourcedistribution_controller_test.go +++ b/pkg/controller/resourcedistribution/resourcedistribution_controller_test.go @@ -79,8 +79,10 @@ func TestDoReconcile(t *testing.T) { t.Fatalf("failed to get resource(%s.%s) from fake client, err %v", namespace, "test-secret-1", err) } // check resource source and version - if distributor.Name != resource.Annotations[utils.SourceResourceDistributionOfResource] || - resourceVersion != resource.Annotations[utils.ResourceHashCodeAnnotation] { + if !isControlledByDistributor(resource, distributor) { + t.Fatalf("failed to sync resource(%s) for namespace (%s), owner set error", resource.Name, namespace) + } + if resourceVersion != resource.Annotations[utils.ResourceHashCodeAnnotation] { t.Fatalf("failed to sync resource(%s) for namespace (%s) from %s, \nexpected version:%x \nactual version:%x", resource.Name, namespace, resource.Annotations[utils.SourceResourceDistributionOfResource], resourceVersion, resource.Annotations[utils.ResourceHashCodeAnnotation]) } } @@ -112,7 +114,13 @@ func buildResourceDistributionWithSecret() *appsv1alpha1.ResourceDistribution { func buildResourceDistribution(raw runtime.RawExtension) *appsv1alpha1.ResourceDistribution { return &appsv1alpha1.ResourceDistribution{ - ObjectMeta: metav1.ObjectMeta{Name: "test-resource-distribution"}, + TypeMeta: metav1.TypeMeta{ + APIVersion: appsv1alpha1.GroupVersion.String(), + Kind: "ResourceDistribution", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-resource-distribution", + }, Spec: appsv1alpha1.ResourceDistributionSpec{ Resource: raw, Targets: appsv1alpha1.ResourceDistributionTargets{ @@ -147,6 +155,7 @@ func buildResourceDistribution(raw runtime.RawExtension) *appsv1alpha1.ResourceD } func makeEnvironment() []runtime.Object { + distributor := buildResourceDistributionWithSecret() return []runtime.Object{ &corev1.Namespace{ // for create ObjectMeta: metav1.ObjectMeta{ @@ -193,21 +202,16 @@ func makeEnvironment() []runtime.Object { }, }, }, - &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "kube-system", - Labels: map[string]string{ - "group": "two", - "environment": "test", - }, - }, - }, &corev1.Secret{ // for update ObjectMeta: metav1.ObjectMeta{ Name: "test-secret-1", Namespace: "ns-1", Annotations: map[string]string{ utils.SourceResourceDistributionOfResource: "test-resource-distribution", + utils.ResourceHashCodeAnnotation: hashResource(distributor.Spec.Resource), + }, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(distributor, distributor.GroupVersionKind()), }, }, }, @@ -217,6 +221,10 @@ func makeEnvironment() []runtime.Object { Namespace: "ns-4", Annotations: map[string]string{ utils.SourceResourceDistributionOfResource: "test-resource-distribution", + utils.ResourceHashCodeAnnotation: hashResource(distributor.Spec.Resource), + }, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(distributor, distributor.GroupVersionKind()), }, }, }, diff --git a/pkg/controller/resourcedistribution/utils.go b/pkg/controller/resourcedistribution/utils.go index 23472c2d70..5de7c4a871 100644 --- a/pkg/controller/resourcedistribution/utils.go +++ b/pkg/controller/resourcedistribution/utils.go @@ -20,6 +20,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "reflect" "sync" "time" @@ -29,6 +30,7 @@ import ( 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/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -175,17 +177,50 @@ func calculateNewStatus(distributor *appsv1alpha1.ResourceDistribution, newCondi return status } +// mergeMetadata will merge labels/annotations/finalizers +func mergeMetadata(newResource, oldResource *unstructured.Unstructured) { + if newResource.GetLabels() == nil { + newResource.SetLabels(make(map[string]string)) + } + + if newResource.GetAnnotations() == nil { + newResource.SetAnnotations(make(map[string]string)) + } + + for k, v := range oldResource.GetLabels() { + newLabels := newResource.GetLabels() + if _, ok := newLabels[k]; !ok { + newLabels[k] = v + } + newResource.SetLabels(newLabels) + } + + for k, v := range oldResource.GetAnnotations() { + newAnnotations := newResource.GetAnnotations() + if _, ok := newAnnotations[k]; !ok { + newAnnotations[k] = v + } + newResource.SetAnnotations(newAnnotations) + } + + newResource.SetFinalizers(sets.NewString(newResource.GetFinalizers()...). + Union(sets.NewString(oldResource.GetFinalizers()...)).List()) +} + // makeResourceObject set some necessary information for resource before updating and creating -func makeResourceObject(distributor *appsv1alpha1.ResourceDistribution, namespace string, resource runtime.Object, hashCode string) runtime.Object { +func makeResourceObject(distributor *appsv1alpha1.ResourceDistribution, namespace string, resource runtime.Object, hashCode string, oldResource *unstructured.Unstructured) runtime.Object { // convert to unstructured - resource = resource.DeepCopyObject() - resourceOperation := utils.ConvertToUnstructured(resource) + newResource := utils.ConvertToUnstructured(resource.DeepCopyObject()) + if oldResource != nil { + mergeMetadata(newResource, oldResource) + } + // 1. set namespace - resourceOperation.SetNamespace(namespace) + newResource.SetNamespace(namespace) // 2. set ownerReference for cascading deletion found := false - owners := resourceOperation.GetOwnerReferences() + owners := newResource.GetOwnerReferences() for i := range owners { if owners[i].UID == distributor.UID { found = true @@ -193,26 +228,19 @@ func makeResourceObject(distributor *appsv1alpha1.ResourceDistribution, namespac } } if !found { - owners = append(owners, metav1.OwnerReference{ - APIVersion: distributor.APIVersion, - Kind: distributor.Kind, - Name: distributor.Name, - UID: distributor.UID, - }) - resourceOperation.SetOwnerReferences(owners) + newResource.SetOwnerReferences(append(owners, *metav1.NewControllerRef(distributor, distributor.GroupVersionKind()))) } // 3. set resource annotations - annotations := resourceOperation.GetAnnotations() + annotations := newResource.GetAnnotations() if annotations == nil { annotations = make(map[string]string) } annotations[utils.ResourceHashCodeAnnotation] = hashCode - annotations[utils.ResourceDistributedTimestamp] = time.Now().String() annotations[utils.SourceResourceDistributionOfResource] = distributor.Name - resourceOperation.SetAnnotations(annotations) + newResource.SetAnnotations(annotations) - return resource + return newResource } func syncItSlowly(namespaces []string, initialBatchSize int, fn func(namespace string) *UnexpectedError) (int32, []*UnexpectedError) { @@ -255,6 +283,7 @@ func listNamespacesForDistributor(handlerClient client.Client, targets *appsv1al if err := handlerClient.List(context.TODO(), namespacesList); err != nil { return nil, nil, err } + for _, namespace := range namespacesList.Items { unmatchedSet.Insert(namespace.Name) } @@ -294,11 +323,26 @@ func listNamespacesForDistributor(handlerClient client.Client, targets *appsv1al unmatchedSet.Delete(matched) } - // 6. exclude forbidden namespaces - for _, forbiddenNamespace := range utils.ForbiddenNamespaces { - matchedSet.Delete(forbiddenNamespace) - unmatchedSet.Delete(forbiddenNamespace) - } - return matchedSet.List(), unmatchedSet.List(), nil } + +func needToUpdate(old, new *unstructured.Unstructured) bool { + oldObject := old.DeepCopy().Object + newObject := new.DeepCopy().Object + oldObject["metadata"] = nil + newObject["metadata"] = nil + oldObject["status"] = nil + newObject["status"] = nil + return !reflect.DeepEqual(oldObject, newObject) +} + +func isControlledByDistributor(resource metav1.Object, distributor *appsv1alpha1.ResourceDistribution) bool { + controller := metav1.GetControllerOf(resource) + if controller != nil && distributor != nil && + distributor.APIVersion == controller.APIVersion && + distributor.Kind == controller.Kind && + distributor.Name == controller.Name { + return true + } + return false +} diff --git a/pkg/webhook/resourcedistribution/validating/resourcedistribution_create_update_handler.go b/pkg/webhook/resourcedistribution/validating/resourcedistribution_create_update_handler.go index 92ab8b54a1..f1616147c8 100755 --- a/pkg/webhook/resourcedistribution/validating/resourcedistribution_create_update_handler.go +++ b/pkg/webhook/resourcedistribution/validating/resourcedistribution_create_update_handler.go @@ -19,11 +19,14 @@ import ( "net/http" appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" + webhookutil "github.com/openkruise/kruise/pkg/webhook/util" admissionv1 "k8s.io/api/admission/v1" + "k8s.io/apimachinery/pkg/api/errors" apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/klog/v2" coreval "k8s.io/kubernetes/pkg/apis/core/validation" @@ -60,7 +63,7 @@ func (h *ResourceDistributionCreateUpdateHandler) validateResourceDistributionSp allErrs = append(allErrs, errs...) } // 1. validate resource - allErrs = append(allErrs, h.validateResourceDistributionResource(resource, oldResource, fldPath.Child("resource"))...) + allErrs = append(allErrs, h.validateResourceDistributionSpecResource(resource, oldResource, fldPath.Child("resource"))...) // 2. validate targets allErrs = append(allErrs, h.validateResourceDistributionSpecTargets(&obj.Spec.Targets, fldPath.Child("targets"))...) return @@ -70,20 +73,21 @@ func (h *ResourceDistributionCreateUpdateHandler) validateResourceDistributionSp // (1). check whether type of the resource is supported // (2). detect updating conflict, i.e., GK and name cannot be modified // (3). dry run to check whether resource can be created -func (h *ResourceDistributionCreateUpdateHandler) validateResourceDistributionResource(resource, oldResource runtime.Object, fldPath *field.Path) (allErrs field.ErrorList) { +func (h *ResourceDistributionCreateUpdateHandler) validateResourceDistributionSpecResource(resource, oldResource runtime.Object, fldPath *field.Path) (allErrs field.ErrorList) { // 1. check whether the GK of the resource is in supportedGKList if !isSupportedGK(resource) { - return append(allErrs, field.Invalid(fldPath, resource, fmt.Sprintf("unknown or unsupported resource GroupVersionKind, only support %v", supportedGKList))) + return append(allErrs, field.Invalid(fldPath, resource, fmt.Sprintf("unknown or unsupported resource GroupKind, only support %v", supportedGKList))) } // 2. validate resource group, kind and name when updating - if oldResource != nil && !haveSameGKAndName(resource, oldResource) { + if oldResource != nil && !haveSameGVKAndName(resource, oldResource) { return append(allErrs, field.Invalid(fldPath, nil, "resource apiVersion, kind, and name are immutable")) } - // 3. dry run to check resource - mice := resource.DeepCopyObject() - ConvertToUnstructured(mice).SetNamespace(DefaultNamespace) - if err := h.Client.Create(context.TODO(), mice.(client.Object), &client.CreateOptions{DryRun: []string{metav1.DryRunAll}}); err != nil { - return append(allErrs, field.InternalError(fldPath, fmt.Errorf("dry-run to validate resource failed, err: %v", err))) + // 3. dry run to check the resource + mice := resource.DeepCopyObject().(client.Object) + ConvertToUnstructured(mice).SetNamespace(webhookutil.GetNamespace()) + err := h.Client.Create(context.TODO(), mice, &client.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + if err != nil && !errors.IsAlreadyExists(err) { + return append(allErrs, field.InternalError(fldPath, fmt.Errorf("failed to dry-run to validate spec.resource, error: %v", err))) } return } @@ -93,19 +97,14 @@ func (h *ResourceDistributionCreateUpdateHandler) validateResourceDistributionRe // (2). validate conflict between existing resources func (h *ResourceDistributionCreateUpdateHandler) validateResourceDistributionSpecTargets(targets *appsv1alpha1.ResourceDistributionTargets, fldPath *field.Path) (allErrs field.ErrorList) { // 1. validate namespace of IncludedNamespaces.List and ExcludedNamespaces.List - forbidden := make([]string, 0) conflicted := make([]string, 0) - included := make(map[string]struct{}) + includedNS := sets.NewString() for _, namespace := range targets.IncludedNamespaces.List { - included[namespace.Name] = struct{}{} + includedNS.Insert(namespace.Name) // validate namespace name for _, msg := range coreval.ValidateNamespaceName(namespace.Name, false) { allErrs = append(allErrs, field.Invalid(fldPath.Child("includedNamespaces"), targets.IncludedNamespaces, msg)) } - // validate whether namespace is forbidden - if isForbiddenNamespace(namespace.Name) { - forbidden = append(forbidden, namespace.Name) - } } for _, namespace := range targets.ExcludedNamespaces.List { // validate namespace name @@ -113,20 +112,17 @@ func (h *ResourceDistributionCreateUpdateHandler) validateResourceDistributionSp allErrs = append(allErrs, field.Invalid(fldPath.Child("excludedNamespaces"), targets.ExcludedNamespaces, msg)) } // validate conflict between IncludedNamespaces and ExcludedNamespaces - if _, ok := included[namespace.Name]; ok { + if includedNS.Has(namespace.Name) { conflicted = append(conflicted, namespace.Name) } } if len(conflicted) != 0 { allErrs = append(allErrs, field.Invalid(fldPath, targets, fmt.Sprintf("ambiguous targets because namespace %v is in both IncludedNamespaces.List and ExcludedNamesapces.List", conflicted))) } - if len(forbidden) != 0 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("includedNamespaces"), targets.IncludedNamespaces, fmt.Sprintf("cannot distribute rsource to forbidden namespaces %v", ForbiddenNamespaces))) - } // 2. validate targets.NamespaceLabelSelector if _, err := metav1.LabelSelectorAsSelector(&targets.NamespaceLabelSelector); err != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("namespaceLabelSelector"), targets.IncludedNamespaces, fmt.Sprintf("labelSelectorAsSelector error: %v", err))) + allErrs = append(allErrs, field.Invalid(fldPath.Child("namespaceLabelSelector"), targets.NamespaceLabelSelector, fmt.Sprintf("labelSelectorAsSelector error: %v", err))) } return diff --git a/pkg/webhook/resourcedistribution/validating/resourcedistribution_validating_test.go b/pkg/webhook/resourcedistribution/validating/resourcedistribution_validating_test.go index 0712425358..6b6e2a543f 100644 --- a/pkg/webhook/resourcedistribution/validating/resourcedistribution_validating_test.go +++ b/pkg/webhook/resourcedistribution/validating/resourcedistribution_validating_test.go @@ -109,7 +109,7 @@ func TestValidateResourceDistributionTargets(t *testing.T) { // error 2 IncludedNamespaces: appsv1alpha1.ResourceDistributionTargetNamespaces{ List: []appsv1alpha1.ResourceDistributionNamespace{ - {Name: "ns-1"}, {Name: "ns-2"}, {Name: "kube-system"}, {Name: "kube-public"}, + {Name: "ns-1"}, {Name: "ns-2"}, {Name: "kube-system"}, {Name: ""}, }, }, // error 3 diff --git a/pkg/webhook/resourcedistribution/validating/utils.go b/pkg/webhook/resourcedistribution/validating/utils.go index e2b75fe602..712a1b836e 100644 --- a/pkg/webhook/resourcedistribution/validating/utils.go +++ b/pkg/webhook/resourcedistribution/validating/utils.go @@ -24,9 +24,7 @@ import ( ) const ( - DefaultNamespace = "default" ResourceHashCodeAnnotation = "kruise.io/resourcedistribution.resource.hashcode" - ResourceDistributedTimestamp = "kruise.io/resourcedistribution.resource.distributed.timestamp" SourceResourceDistributionOfResource = "kruise.io/resourcedistribution.resource.from" ) @@ -38,15 +36,6 @@ var ( {Group: "", Kind: "Secret"}, {Group: "", Kind: "ConfigMap"}, } - - // ForbiddenNamespaces is a list that contains all forbidden namespaces - // Resources will never be distributed to these namespaces - // reused by controller - /* ADD NEW FORBIDDEN NAMESPACE HERE*/ - ForbiddenNamespaces = []string{ - "kube-system", - "kube-public", - } ) // isSupportedGVK check whether object is supported by ResourceDistribution @@ -54,30 +43,20 @@ func isSupportedGK(object runtime.Object) bool { if object == nil { return false } - objGVK := object.GetObjectKind().GroupVersionKind().GroupKind() - for _, gvk := range supportedGKList { - if reflect.DeepEqual(gvk, objGVK) { - return true - } - } - return false -} - -// isForbiddenNamespace check whether the namespace is forbidden -func isForbiddenNamespace(namespace string) bool { - for _, forbiddenNamespace := range ForbiddenNamespaces { - if namespace == forbiddenNamespace { + objGK := object.GetObjectKind().GroupVersionKind().GroupKind() + for _, gk := range supportedGKList { + if reflect.DeepEqual(gk, objGK) { return true } } return false } -// haveSameGKAndName return true if two resources have the same group, kind and name -func haveSameGKAndName(resource, otherResource runtime.Object) bool { +// haveSameGVKAndName return true if two resources have the same group, version, kind and name +func haveSameGVKAndName(resource, otherResource runtime.Object) bool { Name, anotherName := ConvertToUnstructured(resource).GetName(), ConvertToUnstructured(otherResource).GetName() - GK, anotherGK := resource.GetObjectKind().GroupVersionKind().GroupKind(), otherResource.GetObjectKind().GroupVersionKind().GroupKind() - return Name == anotherName && reflect.DeepEqual(GK, anotherGK) + GVK, anotherGVK := resource.GetObjectKind().GroupVersionKind(), otherResource.GetObjectKind().GroupVersionKind() + return Name == anotherName && reflect.DeepEqual(GVK, anotherGVK) } // ConvertToUnstructured receive runtime.Object, return *unstructured.Unstructured diff --git a/test/e2e/apps/resourcedistribution.go b/test/e2e/apps/resourcedistribution.go index 1231229d11..4a4706a297 100644 --- a/test/e2e/apps/resourcedistribution.go +++ b/test/e2e/apps/resourcedistribution.go @@ -18,7 +18,6 @@ package apps import ( "crypto/sha256" - "encoding/base64" "encoding/hex" "fmt" "strings" @@ -63,7 +62,7 @@ var _ = SIGDescribe("ResourceDistribution", func() { }) ginkgo.It("namespace event checker", func() { - prefix := "resourcedistribution-e2e-test2" + prefix := "resourcedistribution-e2e-test1" // clean resource to avoid conflict tester.DeleteResourceDistributions(prefix) tester.DeleteNamespaces(prefix) @@ -100,7 +99,7 @@ var _ = SIGDescribe("ResourceDistribution", func() { matchedNamespaces, _, err := tester.GetNamespaceForDistributor(&resourceDistribution.Spec.Targets) gomega.Expect(err).NotTo(gomega.HaveOccurred()) return matchedNamespaces.Len() - }, 10*time.Second, time.Second).Should(gomega.Equal(2)) + }, time.Minute, time.Second).Should(gomega.Equal(2)) ginkgo.By("checking created secret...") matchedNamespaces, _, err := tester.GetNamespaceForDistributor(&resourceDistribution.Spec.Targets) @@ -109,7 +108,7 @@ var _ = SIGDescribe("ResourceDistribution", func() { gomega.Eventually(func() error { _, err := tester.GetSecret(namespace, secretName, true) return err - }, 10*time.Second, time.Second).ShouldNot(gomega.HaveOccurred()) + }, time.Minute, time.Second).ShouldNot(gomega.HaveOccurred()) } //clear all resources in cluster @@ -118,17 +117,75 @@ var _ = SIGDescribe("ResourceDistribution", func() { ginkgo.By("Done!") }) - ginkgo.It("resourcedistribution functionality checker", func() { + ginkgo.It("resource event checker", func() { prefix := "resourcedistribution-e2e-test2" // clean resource to avoid conflict tester.DeleteResourceDistributions(prefix) tester.DeleteNamespaces(prefix) + namespaces := []*corev1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: prefix + "-1", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: prefix + "-2", + }, + }, + } + tester.CreateNamespaces(namespaces...) + + // create ResourceDistribution + resourceDistribution := tester.NewBaseResourceDistribution(prefix) + tester.CreateResourceDistribution(resourceDistribution) + + var err error + var secret *corev1.Secret + gomega.Eventually(func() error { + secret, err = tester.GetSecret(namespaces[0].Name, secretName, true) + return err + }, time.Minute, time.Second).Should(gomega.BeNil()) + + // If resource was modified directly, resourceDistribution should modify it back + ginkgo.By("update resource directly...") + secret.StringData = map[string]string{ + "updated": "yes", + } + err = tester.UpdateSecret(secret) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Eventually(func() int { + secret, err = tester.GetSecret(namespaces[0].Name, secretName, true) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + return len(secret.StringData) + }, time.Minute, time.Second).Should(gomega.Equal(0)) + + // If resource was deleted directly, resourceDistribution should create it again + ginkgo.By("delete resource directly...") + err = tester.DeleteSecret(secret.Namespace, secret.Name) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Eventually(func() error { + secret, err = tester.GetSecret(namespaces[0].Name, secretName, true) + return err + }, time.Minute, time.Second).Should(gomega.BeNil()) + + //clear all resources in cluster + tester.DeleteResourceDistributions(prefix) + tester.DeleteNamespaces(prefix) + ginkgo.By("Done!") + }) + + ginkgo.It("resourcedistribution functionality checker", func() { + prefix := "resourcedistribution-e2e-test3" + // clean resource to avoid conflict + tester.DeleteResourceDistributions(prefix) + tester.DeleteNamespaces(prefix) + // build ResourceDistribution object resourceDistribution := tester.NewBaseResourceDistribution(prefix) cases := []struct { name string - getResources func() []*corev1.Secret getNamespaces func() []*corev1.Namespace }{ { @@ -182,37 +239,6 @@ var _ = SIGDescribe("ResourceDistribution", func() { }, } }, - getResources: func() []*corev1.Secret { - secretContent := []byte(base64.StdEncoding.EncodeToString([]byte("myUsername:myPassword"))) - return []*corev1.Secret{ - { // for update - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: prefix + "-1", - Annotations: map[string]string{ - utils.SourceResourceDistributionOfResource: resourceDistribution.Name, - }, - }, - Type: "Opaque", - Data: map[string][]byte{ - ".dockerconfigjson": secretContent, - }, - }, - { // for delete - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: prefix + "-5", - Annotations: map[string]string{ - utils.SourceResourceDistributionOfResource: resourceDistribution.Name, - }, - }, - Type: "Opaque", - Data: map[string][]byte{ - ".dockerconfigjson": secretContent, - }, - }, - } - }, }, } @@ -222,10 +248,6 @@ var _ = SIGDescribe("ResourceDistribution", func() { ginkgo.By(fmt.Sprintf("creating namespaces")) tester.CreateNamespaces(allNamespaces...) - resources := cs.getResources() - ginkgo.By("creating resources") - tester.CreateSecretResources(resources...) - ginkgo.By(fmt.Sprintf("Creating ResourceDistribution %s", resourceDistribution.Name)) tester.CreateResourceDistribution(resourceDistribution) @@ -237,13 +259,13 @@ var _ = SIGDescribe("ResourceDistribution", func() { matchedNamespaces, unmatchedNamespaces, err = tester.GetNamespaceForDistributor(&resourceDistribution.Spec.Targets) gomega.Expect(err).NotTo(gomega.HaveOccurred()) return matchedNamespaces.Len() - }, 10*time.Second, time.Second).Should(gomega.Equal(4)) + }, time.Minute, time.Second).Should(gomega.Equal(4)) // ensure all desired resources have been created gomega.Eventually(func() int32 { resourceDistribution, err = tester.GetResourceDistribution(resourceDistribution.Name, true) gomega.Expect(err).NotTo(gomega.HaveOccurred()) return resourceDistribution.Status.Succeeded - }, 20*time.Second, time.Second).Should(gomega.Equal(int32(len(matchedNamespaces)))) + }, time.Minute, time.Second).Should(gomega.Equal(int32(len(matchedNamespaces)))) gomega.Expect(resourceDistribution.Status.Desired).Should(gomega.Equal(resourceDistribution.Status.Succeeded)) // checking created and updated resources @@ -287,7 +309,7 @@ var _ = SIGDescribe("ResourceDistribution", func() { mice, err := tester.GetResourceDistribution(resourceDistribution.Name, true) gomega.Expect(err).NotTo(gomega.HaveOccurred()) return len(mice.Status.Conditions[5].FailedNamespaces) - }, 10*time.Second, time.Second).Should(gomega.Equal(1)) + }, time.Minute, time.Second).Should(gomega.Equal(1)) gomega.Expect(resourceDistribution.Status.Desired).Should(gomega.Equal(resourceDistribution.Status.Succeeded)) // checking after updating spec.targets @@ -298,7 +320,7 @@ var _ = SIGDescribe("ResourceDistribution", func() { mice, err := tester.GetResourceDistribution(resourceDistribution.Name, true) gomega.Expect(err).NotTo(gomega.HaveOccurred()) return mice.Status.Succeeded - }, 20*time.Second, time.Second).Should(gomega.Equal(int32(2))) + }, time.Minute, time.Second).Should(gomega.Equal(int32(2))) matchedNamespaces, unmatchedNamespaces, err = tester.GetNamespaceForDistributor(&resourceDistribution.Spec.Targets) gomega.Expect(err).NotTo(gomega.HaveOccurred()) @@ -318,7 +340,7 @@ var _ = SIGDescribe("ResourceDistribution", func() { if !strings.HasPrefix(namespace, prefix) { continue } - object, err := tester.GetSecret(namespace, resources[0].Name, false) + object, err := tester.GetSecret(namespace, secretName, false) gomega.Expect(errors.IsNotFound(err)).Should(gomega.BeTrue()) gomega.Expect(object).Should(gomega.BeNil()) } @@ -330,9 +352,9 @@ var _ = SIGDescribe("ResourceDistribution", func() { continue } gomega.Eventually(func() bool { - _, err := tester.GetSecret(namespace, resources[0].Name, false) + _, err := tester.GetSecret(namespace, secretName, false) return errors.IsNotFound(err) - }, 20*time.Second, time.Second).Should(gomega.BeTrue()) + }, time.Minute, time.Second).Should(gomega.BeTrue()) } ginkgo.By("Done!") tester.DeleteNamespaces(prefix) diff --git a/test/e2e/framework/resourcedistribution_utils.go b/test/e2e/framework/resourcedistribution_utils.go index 400179539e..b79fdb4209 100644 --- a/test/e2e/framework/resourcedistribution_utils.go +++ b/test/e2e/framework/resourcedistribution_utils.go @@ -21,12 +21,10 @@ import ( "strings" "time" + "github.com/onsi/gomega" appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1" kruiseclientset "github.com/openkruise/kruise/pkg/client/clientset/versioned" "github.com/openkruise/kruise/pkg/util" - utils "github.com/openkruise/kruise/pkg/webhook/resourcedistribution/validating" - - "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -151,7 +149,7 @@ func (s *ResourceDistributionTester) UpdateNamespace(namespace *corev1.Namespace if updateErr == nil { return nil } - namespaceClone, _ = s.c.CoreV1().Namespaces().Update(context.TODO(), namespaceClone, metav1.UpdateOptions{}) + namespaceClone, _ = s.c.CoreV1().Namespaces().Get(context.TODO(), namespace.Name, metav1.GetOptions{}) return updateErr }) gomega.Expect(err).NotTo(gomega.HaveOccurred()) @@ -197,6 +195,28 @@ func (s *ResourceDistributionTester) GetSecret(namespace, name string, mustExist return secret, nil } +func (s *ResourceDistributionTester) UpdateSecret(secret *corev1.Secret) error { + Logf("update secret(%s/%s)", secret.Namespace, secret.Name) + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + secretClone, getErr := s.c.CoreV1().Secrets(secret.Namespace).Get(context.TODO(), secret.Name, metav1.GetOptions{}) + if getErr != nil { + return getErr + } + metadata := &secretClone.ObjectMeta + secretClone = secret.DeepCopy() + secretClone.ObjectMeta = *metadata + _, updateErr := s.c.CoreV1().Secrets(secret.Namespace).Update(context.TODO(), secretClone, metav1.UpdateOptions{}) + if updateErr == nil { + return nil + } + return updateErr + }) +} + +func (s *ResourceDistributionTester) DeleteSecret(namespace, name string) error { + return s.c.CoreV1().Secrets(namespace).Delete(context.TODO(), name, metav1.DeleteOptions{}) +} + func (s *ResourceDistributionTester) GetResourceDistribution(name string, mustExistAssertion bool) (*appsv1alpha1.ResourceDistribution, error) { if mustExistAssertion { s.WaitForResourceDistributionCreated(name, 2*time.Minute) @@ -387,10 +407,5 @@ func (s *ResourceDistributionTester) GetNamespaceForDistributor(targets *appsv1a unmatchedNamespaces.Delete(matched) } - // 6. exclude forbidden namespaces - for _, forbiddenNamespace := range utils.ForbiddenNamespaces { - matchedNamespaces.Delete(forbiddenNamespace) - unmatchedNamespaces.Delete(forbiddenNamespace) - } return }