Feedback and linter

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

@@ -176,6 +176,8 @@ var (
 	warningMigrationReapplyFailed        = "Warning: failed to re-apply configuration after performing Server-Side Apply migration. This is non-fatal and will be retried next time you apply. Error: %[1]s\n"
 )
 
+var ApplySetToolVersion = version.Get().GitVersion
+
 // NewApplyFlags returns a default ApplyFlags
 func NewApplyFlags(streams genericclioptions.IOStreams) *ApplyFlags {
 	return &ApplyFlags{
@@ -312,7 +314,7 @@ func (flags *ApplyFlags) ToOptions(f cmdutil.Factory, cmd *cobra.Command, baseNa
 			parent.Namespace = namespace
 		}
 		// TODO: is version.Get() the right thing? Does it work for non-kubectl package consumers?
-		tooling := ApplySetTooling{name: baseName, version: version.Get().String()}
+		tooling := ApplySetTooling{name: baseName, version: ApplySetToolVersion}
 		restClient, err := f.ClientForMapping(parent.RESTMapping)
 		if err != nil || restClient == nil {
 			return nil, fmt.Errorf("failed to initialize RESTClient for ApplySet: %w", err)
@@ -511,6 +513,18 @@ func (o *ApplyOptions) Run() error {
 		if err := o.ApplySet.FetchParent(); err != nil {
 			return err
 		}
+		// Update the live parent object to the superset of the current and previous resources.
+		// Doing this before the actual apply and prune operations improves behavior by ensuring
+		// the live object contains the superset on failure. This may cause the next pruning
+		// operation to make a larger number of GET requests than strictly necessary, but it prevents
+		// object leakage from the set. The superset will automatically be reduced to the correct
+		// set by the next successful operation.
+		for _, info := range infos {
+			o.ApplySet.AddResource(info.ResourceMapping(), info.Namespace)
+		}
+		if err := o.ApplySet.UpdateParent(UpdateToSuperset, o.DryRunStrategy, o.ValidationDirective); err != nil {
+			return err
+		}
 	}
 	// Iterate through all objects, applying each one.
 	for _, info := range infos {
@@ -997,10 +1011,6 @@ func (o *ApplyOptions) MarkObjectVisited(info *resource.Info) error {
 		return err
 	}
 	o.VisitedUids.Insert(metadata.GetUID())
-
-	if o.ApplySet != nil {
-		o.ApplySet.MarkObjectVisited(info.Mapping, info.Namespace)
-	}
 	return nil
 }
 
@@ -1021,10 +1031,12 @@ func (o *ApplyOptions) PrintAndPrunePostProcessor() func() error {
 			if cmdutil.ApplySet.IsEnabled() && o.ApplySet != nil {
 				pruner := newApplySetPruner(o)
 				if err := pruner.pruneAll(ctx, o.ApplySet); err != nil {
-					applySetErr := o.ApplySet.UpdateParent(SetUpdateIncomplete, o.DryRunStrategy, o.ValidationDirective)
+					// Do not update the ApplySet. If pruning failed, we want to keep the superset
-					return utilerrors.NewAggregate([]error{err, applySetErr})
+					// of the previous and current resources in the ApplySet, so that the pruning
+					// step of the next apply will be able to clean up the set correctly.
+					return err
 				}
-				if err := o.ApplySet.UpdateParent(SetUpdateSuccessful, o.DryRunStrategy, o.ValidationDirective); err != nil {
+				if err := o.ApplySet.UpdateParent(UpdateToLatestSet, o.DryRunStrategy, o.ValidationDirective); err != nil {
 					return fmt.Errorf("apply and prune succeeded, but ApplySet update failed: %w", err)
 				}
 			} else {

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go

@@ -335,20 +335,6 @@ func readReplicationController(t *testing.T, filenameRC string) (string, []byte)
 	return metaAccessor.GetName(), rcBytes
 }
 
-func readService(t *testing.T, filenameSVC string) (string, []byte) {
-	svcObj := readServiceFromFile(t, filenameSVC)
-	metaAccessor, err := meta.Accessor(svcObj)
-	if err != nil {
-		t.Fatal(err)
-	}
-	svcBytes, err := runtime.Encode(codec, svcObj)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	return metaAccessor.GetName(), svcBytes
-}
-
 func readReplicationControllerFromFile(t *testing.T, filename string) *corev1.ReplicationController {
 	data := readBytesFromFile(t, filename)
 	rc := corev1.ReplicationController{}
@@ -2205,8 +2191,8 @@ func TestApplySetParentValidation(t *testing.T) {
 					return
 				}
 
-				assert.Equal(t, expectedParentNs, o.ApplySet.parent.Namespace)
+				assert.Equal(t, expectedParentNs, o.ApplySet.parentRef.Namespace)
-				assert.Equal(t, test.expectParentKind, o.ApplySet.parent.GroupVersionKind.Kind)
+				assert.Equal(t, test.expectParentKind, o.ApplySet.parentRef.GroupVersionKind.Kind)
 
 				err = o.Validate()
 				if test.expectErr != "" {
@@ -2486,19 +2472,19 @@ func TestApplySetInvalidLiveParent(t *testing.T) {
 			grsAnnotation:     validGrsAnnotation,
 			toolingAnnotation: "helm/v3",
 			idLabel:           validIDLabel,
-			expectErr:         "error: ApplySet parent object \"secrets./mySet\" already exists and is managed by tooling helm instead of kubectl",
+			expectErr:         "error: ApplySet parent object \"secrets./mySet\" already exists and is managed by tooling \"helm\" instead of \"kubectl\"",
 		},
 		"tooling annotation with invalid prefix with one segment can be parsed": {
 			grsAnnotation:     validGrsAnnotation,
 			toolingAnnotation: "helm",
 			idLabel:           validIDLabel,
-			expectErr:         "error: ApplySet parent object \"secrets./mySet\" already exists and is managed by tooling helm instead of kubectl",
+			expectErr:         "error: ApplySet parent object \"secrets./mySet\" already exists and is managed by tooling \"helm\" instead of \"kubectl\"",
 		},
 		"tooling annotation with invalid prefix with many segments can be parsed": {
 			grsAnnotation:     validGrsAnnotation,
 			toolingAnnotation: "example.com/tool/why/v1",
 			idLabel:           validIDLabel,
-			expectErr:         "error: ApplySet parent object \"secrets./mySet\" already exists and is managed by tooling example.com/tool/why instead of kubectl",
+			expectErr:         "error: ApplySet parent object \"secrets./mySet\" already exists and is managed by tooling \"example.com/tool/why\" instead of \"kubectl\"",
 		},
 		"ID label is required": {
 			grsAnnotation:     validGrsAnnotation,

staging/src/k8s.io/kubectl/pkg/cmd/apply/applyset.go

@@ -72,8 +72,8 @@ var defaultApplySetParentGVR = schema.GroupVersionResource{Version: "v1", Resour
 
 // ApplySet tracks the information about an applyset apply/prune
 type ApplySet struct {
-	// parent is the reference to the parent object that is used to track the applyset.
+	// parentRef is a reference to the parent object that is used to track the applyset.
-	parent *ApplySetParent
+	parentRef *ApplySetParentRef
 
 	// toolingID is the value to be used and validated in the applyset.k8s.io/tooling annotation.
 	toolingID ApplySetTooling
@@ -92,7 +92,7 @@ type ApplySet struct {
 
 	restMapper meta.RESTMapper
 
-	// client is a client specific to the parent's type
+	// client is a client specific to the ApplySet parent object's type
 	client resource.RESTClient
 }
 
@@ -101,17 +101,23 @@ var builtinApplySetParentGVRs = map[schema.GroupVersionResource]bool{
 	{Version: "v1", Resource: "configmaps"}: true,
 }
 
-// ApplySetParent stores object and type meta for the parent object that is used to track the applyset.
+// ApplySetParentRef stores object and type meta for the parent object that is used to track the applyset.
-type ApplySetParent struct {
+type ApplySetParentRef struct {
 	Name      string
 	Namespace string
 	*meta.RESTMapping
 }
 
-func (p ApplySetParent) IsNamespaced() bool {
+func (p ApplySetParentRef) IsNamespaced() bool {
 	return p.Scope.Name() == meta.RESTScopeNameNamespace
 }
 
+// String returns the string representation of the parent object using the same format
+// that we expect to receive in the --applyset flag on the CLI.
+func (p ApplySetParentRef) String() string {
+	return fmt.Sprintf("%s.%s/%s", p.Resource.Resource, p.Resource.Group, p.Name)
+}
+
 type ApplySetTooling struct {
 	name    string
 	version string
@@ -122,25 +128,19 @@ func (t ApplySetTooling) String() string {
 }
 
 // NewApplySet creates a new ApplySet object tracked by the given parent object.
-func NewApplySet(parent *ApplySetParent, tooling ApplySetTooling, mapper meta.RESTMapper, client resource.RESTClient) *ApplySet {
+func NewApplySet(parent *ApplySetParentRef, tooling ApplySetTooling, mapper meta.RESTMapper, client resource.RESTClient) *ApplySet {
 	return &ApplySet{
 		currentResources:  make(map[schema.GroupVersionResource]*meta.RESTMapping),
 		currentNamespaces: make(sets.Set[string]),
 		updatedResources:  make(map[schema.GroupVersionResource]*meta.RESTMapping),
 		updatedNamespaces: make(sets.Set[string]),
-		parent:            parent,
+		parentRef:         parent,
 		toolingID:         tooling,
 		restMapper:        mapper,
 		client:            client,
 	}
 }
 
-// ParentRef returns the string representation of the parent object using the same format
-// that we expect to receive in the --applyset flag on the CLI.
-func (a *ApplySet) ParentRef() string {
-	return fmt.Sprintf("%s.%s/%s", a.parent.Resource.Resource, a.parent.Resource.Group, a.parent.Name)
-}
-
 // ID is the label value that we are using to identify this applyset.
 func (a ApplySet) ID() string {
 	// TODO: base64(sha256(gknn))
@@ -151,10 +151,10 @@ func (a ApplySet) ID() string {
 func (a ApplySet) Validate() error {
 	var errors []error
 	// TODO: permit CRDs that have the annotation required by the ApplySet specification
-	if !builtinApplySetParentGVRs[a.parent.Resource] {
+	if !builtinApplySetParentGVRs[a.parentRef.Resource] {
-		errors = append(errors, fmt.Errorf("resource %q is not permitted as an ApplySet parent", a.parent.Resource))
+		errors = append(errors, fmt.Errorf("resource %q is not permitted as an ApplySet parent", a.parentRef.Resource))
 	}
-	if a.parent.IsNamespaced() && a.parent.Namespace == "" {
+	if a.parentRef.IsNamespaced() && a.parentRef.Namespace == "" {
 		errors = append(errors, fmt.Errorf("namespace is required to use namespace-scoped ApplySet"))
 	}
 	return utilerrors.NewAggregate(errors)
@@ -190,67 +190,55 @@ func (a *ApplySet) addLabels(objects []*resource.Info) error {
 	return nil
 }
 
-// MarkObjectVisited keeps track of the applied objects, so that we can update the list of in-use namespaces
-// and resouce kinds, and so that we can prune objects not applied.
-func (a *ApplySet) MarkObjectVisited(resource *meta.RESTMapping, namespace string) {
-	a.updatedResources[resource.Resource] = resource
-	if namespace != "" {
-		a.updatedNamespaces.Insert(namespace)
-	}
-}
-
 func (a *ApplySet) FetchParent() error {
-	helper := resource.NewHelper(a.client, a.parent.RESTMapping)
+	helper := resource.NewHelper(a.client, a.parentRef.RESTMapping)
-	obj, err := helper.Get(a.parent.Namespace, a.parent.Name)
+	obj, err := helper.Get(a.parentRef.Namespace, a.parentRef.Name)
 	if errors.IsNotFound(err) {
 		return nil
 	} else if err != nil {
-		return fmt.Errorf("failed to fetch ApplySet parent object %q from server: %w", a.ParentRef(), err)
+		return fmt.Errorf("failed to fetch ApplySet parent object %q from server: %w", a.parentRef, err)
 	} else if obj == nil {
-		return fmt.Errorf("failed to fetch ApplySet parent object %q from server", a.ParentRef())
+		return fmt.Errorf("failed to fetch ApplySet parent object %q from server", a.parentRef)
 	}
 
-	labels, annotations := getLabelsAndAnnotations(obj)
+	labels, annotations, err := getLabelsAndAnnotations(obj)
+	if err != nil {
+		return fmt.Errorf("getting metadata from parent object %q: %w", a.parentRef, err)
+	}
 
 	toolAnnotation, hasToolAnno := annotations[ApplySetToolingAnnotation]
 	if !hasToolAnno {
-		return fmt.Errorf("ApplySet parent object %q already exists and is missing required annotation %q", a.ParentRef(), ApplySetToolingAnnotation)
+		return fmt.Errorf("ApplySet parent object %q already exists and is missing required annotation %q", a.parentRef, ApplySetToolingAnnotation)
 	}
 	if managedBy := toolingBaseName(toolAnnotation); managedBy != a.toolingID.name {
-		return fmt.Errorf("ApplySet parent object %q already exists and is managed by tooling %s instead of %s", a.ParentRef(), managedBy, a.toolingID.name)
+		return fmt.Errorf("ApplySet parent object %q already exists and is managed by tooling %q instead of %q", a.parentRef, managedBy, a.toolingID.name)
 	}
 
 	idLabel, hasIDLabel := labels[ApplySetParentIDLabel]
 	if !hasIDLabel {
-		return fmt.Errorf("ApplySet parent object %q exists and does not have required label %s", a.ParentRef(), ApplySetParentIDLabel)
+		return fmt.Errorf("ApplySet parent object %q exists and does not have required label %s", a.parentRef, ApplySetParentIDLabel)
 	}
 	if idLabel != a.ID() {
-		return fmt.Errorf("ApplySet parent object %q exists and has incorrect value for label %q (got: %s, want: %s)", a.ParentRef(), ApplySetParentIDLabel, idLabel, a.ID())
+		return fmt.Errorf("ApplySet parent object %q exists and has incorrect value for label %q (got: %s, want: %s)", a.parentRef, ApplySetParentIDLabel, idLabel, a.ID())
 	}
 
 	if a.currentResources, err = parseResourcesAnnotation(annotations, a.restMapper); err != nil {
 		// TODO: handle GVRs for now-deleted CRDs
-		return fmt.Errorf("parsing ApplySet annotation on %q: %w", a.ParentRef(), err)
+		return fmt.Errorf("parsing ApplySet annotation on %q: %w", a.parentRef, err)
 	}
 	a.currentNamespaces = parseNamespacesAnnotation(annotations)
-	if a.parent.IsNamespaced() {
+	if a.parentRef.IsNamespaced() {
-		a.currentNamespaces.Insert(a.parent.Namespace)
+		a.currentNamespaces.Insert(a.parentRef.Namespace)
 	}
 	return nil
 }
 
-func getLabelsAndAnnotations(obj runtime.Object) (map[string]string, map[string]string) {
+func getLabelsAndAnnotations(obj runtime.Object) (map[string]string, map[string]string, error) {
-	accessor := meta.NewAccessor()
+	accessor, err := meta.Accessor(obj)
-	annotations := make(map[string]string)
+	if err != nil {
-	labels := make(map[string]string)
+		return nil, nil, err
-
-	if data, err := accessor.Annotations(obj); err == nil {
-		annotations = data
 	}
-	if data, err := accessor.Labels(obj); err == nil {
+	return accessor.GetLabels(), accessor.GetAnnotations(), nil
-		labels = data
-	}
-	return labels, annotations
 }
 
 func toolingBaseName(toolAnnotation string) string {
@@ -285,21 +273,26 @@ func parseResourcesAnnotation(annotations map[string]string, mapper meta.RESTMap
 }
 
 func parseNamespacesAnnotation(annotations map[string]string) sets.Set[string] {
-	namespaces := make(sets.Set[string])
 	annotation, ok := annotations[ApplySetAdditionalNamespacesAnnotation]
 	if !ok { // this annotation is completely optional
-		return namespaces
+		return sets.Set[string]{}
 	}
-	for _, ns := range strings.Split(annotation, ",") {
+	return sets.New(strings.Split(annotation, ",")...)
-		namespaces.Insert(ns)
+}
+
+// AddResource registers the given resource and namespace as being part of the updated set of
+// resources being applied by the current operation.
+func (a *ApplySet) AddResource(resource *meta.RESTMapping, namespace string) {
+	a.updatedResources[resource.Resource] = resource
+	if resource.Scope == meta.RESTScopeNamespace && namespace != "" {
+		a.updatedNamespaces.Insert(namespace)
 	}
-	return namespaces
 }
 
 type ApplySetUpdateMode string
 
-var SetUpdateSuccessful ApplySetUpdateMode = "latest"
+var UpdateToLatestSet ApplySetUpdateMode = "latest"
-var SetUpdateIncomplete ApplySetUpdateMode = "superset"
+var UpdateToSuperset ApplySetUpdateMode = "superset"
 
 func (a *ApplySet) UpdateParent(mode ApplySetUpdateMode, dryRun cmdutil.DryRunStrategy, validation string) error {
 	data, err := json.Marshal(a.buildParentPatch(mode))
@@ -322,7 +315,7 @@ func serverSideApplyRequest(a *ApplySet, data []byte, dryRun cmdutil.DryRunStrat
 	if dryRun == cmdutil.DryRunClient {
 		return nil
 	}
-	helper := resource.NewHelper(a.client, a.parent.RESTMapping).
+	helper := resource.NewHelper(a.client, a.parentRef.RESTMapping).
 		DryRun(dryRun == cmdutil.DryRunServer).
 		WithFieldManager(a.FieldManager()).
 		WithFieldValidation(validation)
@@ -331,8 +324,8 @@ func serverSideApplyRequest(a *ApplySet, data []byte, dryRun cmdutil.DryRunStrat
 		Force: &forceConficts,
 	}
 	_, err := helper.Patch(
-		a.parent.Namespace,
+		a.parentRef.Namespace,
-		a.parent.Name,
+		a.parentRef.Name,
 		types.ApplyPatchType,
 		data,
 		&options,
@@ -343,32 +336,26 @@ func serverSideApplyRequest(a *ApplySet, data []byte, dryRun cmdutil.DryRunStrat
 func (a *ApplySet) buildParentPatch(mode ApplySetUpdateMode) *metav1.PartialObjectMetadata {
 	var newGRsAnnotation, newNsAnnotation string
 	switch mode {
-	case SetUpdateIncomplete:
+	case UpdateToSuperset:
 		// If the apply succeeded but pruning failed, the set of group resources that
 		// the ApplySet should track is the superset of the previous and current resources.
 		// This ensures that the resources that failed to be pruned are not orphaned from the set.
-		grSuperset := make(map[schema.GroupVersionResource]*meta.RESTMapping)
+		grSuperset := sets.KeySet(a.currentResources).Union(sets.KeySet(a.updatedResources))
-		for versionResource, mapping := range a.currentResources {
-			grSuperset[versionResource] = mapping
-		}
-		for versionResource, mapping := range a.updatedResources {
-			grSuperset[versionResource] = mapping
-		}
 		newGRsAnnotation = generateResourcesAnnotation(grSuperset)
-		newNsAnnotation = generateNamespacesAnnotation(a.currentNamespaces.Union(a.updatedNamespaces), a.parent.Namespace)
+		newNsAnnotation = generateNamespacesAnnotation(a.currentNamespaces.Union(a.updatedNamespaces), a.parentRef.Namespace)
-	case SetUpdateSuccessful:
+	case UpdateToLatestSet:
-		newGRsAnnotation = generateResourcesAnnotation(a.updatedResources)
+		newGRsAnnotation = generateResourcesAnnotation(sets.KeySet(a.updatedResources))
-		newNsAnnotation = generateNamespacesAnnotation(a.updatedNamespaces, a.parent.Namespace)
+		newNsAnnotation = generateNamespacesAnnotation(a.updatedNamespaces, a.parentRef.Namespace)
 	}
 
 	return &metav1.PartialObjectMetadata{
 		TypeMeta: metav1.TypeMeta{
-			Kind:       a.parent.GroupVersionKind.Kind,
+			Kind:       a.parentRef.GroupVersionKind.Kind,
-			APIVersion: a.parent.GroupVersionKind.GroupVersion().String(),
+			APIVersion: a.parentRef.GroupVersionKind.GroupVersion().String(),
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      a.parent.Name,
+			Name:      a.parentRef.Name,
-			Namespace: a.parent.Namespace,
+			Namespace: a.parentRef.Namespace,
 			Annotations: map[string]string{
 				ApplySetToolingAnnotation:              a.toolingID.String(),
 				ApplySetGRsAnnotation:                  newGRsAnnotation,
@@ -382,17 +369,12 @@ func (a *ApplySet) buildParentPatch(mode ApplySetUpdateMode) *metav1.PartialObje
 }
 
 func generateNamespacesAnnotation(namespaces sets.Set[string], skip string) string {
-	var ns []string
+	nsList := namespaces.Clone().Delete(skip).UnsortedList()
-	for n := range namespaces {
+	sort.Strings(nsList)
-		if n != "" && n != skip {
+	return strings.Join(nsList, ",")
-			ns = append(ns, n)
-		}
-	}
-	sort.Strings(ns)
-	return strings.Join(ns, ",")
 }
 
-func generateResourcesAnnotation(resources map[schema.GroupVersionResource]*meta.RESTMapping) string {
+func generateResourcesAnnotation(resources sets.Set[schema.GroupVersionResource]) string {
 	var grs []string
 	for gvr := range resources {
 		grs = append(grs, gvr.GroupResource().String())
@@ -406,7 +388,7 @@ func (a ApplySet) FieldManager() string {
 }
 
 // ParseApplySetParentRef creates a new ApplySetParentRef from a parent reference in the format [RESOURCE][.GROUP]/NAME
-func ParseApplySetParentRef(parentRefStr string, mapper meta.RESTMapper) (*ApplySetParent, error) {
+func ParseApplySetParentRef(parentRefStr string, mapper meta.RESTMapper) (*ApplySetParentRef, error) {
 	var gvr schema.GroupVersionResource
 	var name string
 
@@ -430,5 +412,5 @@ func ParseApplySetParentRef(parentRefStr string, mapper meta.RESTMapper) (*Apply
 	if err != nil {
 		return nil, err
 	}
-	return &ApplySetParent{Name: name, RESTMapping: mapping}, nil
+	return &ApplySetParentRef{Name: name, RESTMapping: mapping}, nil
 }

staging/src/k8s.io/kubectl/pkg/cmd/apply/applyset_pruner.go

@@ -28,6 +28,6 @@ func newApplySetPruner(_ *ApplyOptions) *applySetPruner {
 	return &applySetPruner{}
 }
 
-func (p *applySetPruner) pruneAll(_ context.Context, _ *ApplySet) error {
+func (p *applySetPruner) pruneAll(context.Context, *ApplySet) error {
 	return fmt.Errorf("ApplySet-based pruning is not yet implemented")
 }