Merge pull request #118942 from justinsb/switch_kubectl_prune_annotation

kubectl prune v2: switch to contains-group-kinds annotation

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

@@ -2403,7 +2403,7 @@ kind: Secret
 metadata:
   annotations:
     applyset.kubernetes.io/additional-namespaces: ""
-    applyset.kubernetes.io/contains-group-resources: replicationcontrollers
+    applyset.kubernetes.io/contains-group-kinds: ReplicationController
     applyset.kubernetes.io/tooling: kubectl/v0.0.0-master+$Format:%H$
   creationTimestamp: null
   labels:
@@ -2437,7 +2437,7 @@ kind: Secret
 metadata:
   annotations:
     applyset.kubernetes.io/additional-namespaces: ""
-    applyset.kubernetes.io/contains-group-resources: replicationcontrollers,services
+    applyset.kubernetes.io/contains-group-kinds: ReplicationController,Service
     applyset.kubernetes.io/tooling: kubectl/v0.0.0-master+$Format:%H$
   creationTimestamp: null
   labels:
@@ -2472,7 +2472,7 @@ kind: Secret
 metadata:
   annotations:
     applyset.kubernetes.io/additional-namespaces: ""
-    applyset.kubernetes.io/contains-group-resources: replicationcontrollers,services
+    applyset.kubernetes.io/contains-group-kinds: ReplicationController,Service
     applyset.kubernetes.io/tooling: kubectl/v0.0.0-master+$Format:%H$
   creationTimestamp: null
   labels:
@@ -2507,7 +2507,7 @@ kind: Secret
 metadata:
   annotations:
     applyset.kubernetes.io/additional-namespaces: ""
-    applyset.kubernetes.io/contains-group-resources: services
+    applyset.kubernetes.io/contains-group-kinds: Service
     applyset.kubernetes.io/tooling: kubectl/v0.0.0-master+$Format:%H$
   creationTimestamp: null
   labels:
@@ -2524,60 +2524,60 @@ func TestApplySetInvalidLiveParent(t *testing.T) {
 	defer tf.Cleanup()
 
 	type testCase struct {
-		grsAnnotation     string
+		gksAnnotation     string
 		toolingAnnotation string
 		idLabel           string
 		expectErr         string
 	}
 	validIDLabel := "applyset-0eFHV8ySqp7XoShsGvyWFQD3s96yqwHmzc4e0HR1dsY-v1"
 	validToolingAnnotation := "kubectl/v1.27.0"
-	validGrsAnnotation := "deployments.apps,namespaces,secrets"
+	validGksAnnotation := "Deployment.apps,Namespace,Secret"
 
 	for name, test := range map[string]testCase{
 		"group-resources annotation is required": {
-			grsAnnotation:     "",
+			gksAnnotation:     "",
 			toolingAnnotation: validToolingAnnotation,
 			idLabel:           validIDLabel,
-			expectErr:         "error: parsing ApplySet annotation on \"secrets./my-set\": kubectl requires the \"applyset.kubernetes.io/contains-group-resources\" annotation to be set on all ApplySet parent objects",
+			expectErr:         "error: parsing ApplySet annotation on \"secrets./my-set\": kubectl requires the \"applyset.kubernetes.io/contains-group-kinds\" annotation to be set on all ApplySet parent objects",
 		},
 		"group-resources annotation should not contain invalid resources": {
-			grsAnnotation:     "does-not-exist",
+			gksAnnotation:     "does-not-exist",
 			toolingAnnotation: validToolingAnnotation,
 			idLabel:           validIDLabel,
-			expectErr:         "error: parsing ApplySet annotation on \"secrets./my-set\": invalid group resource in \"applyset.kubernetes.io/contains-group-resources\" annotation: no matches for /, Resource=does-not-exist",
+			expectErr:         "error: parsing ApplySet annotation on \"secrets./my-set\": could not find mapping for kind in \"applyset.kubernetes.io/contains-group-kinds\" annotation: no matches for kind \"does-not-exist\" in group \"\"",
 		},
 		"tooling annotation is required": {
-			grsAnnotation:     validGrsAnnotation,
+			gksAnnotation:     validGksAnnotation,
 			toolingAnnotation: "",
 			idLabel:           validIDLabel,
 			expectErr:         "error: ApplySet parent object \"secrets./my-set\" already exists and is missing required annotation \"applyset.kubernetes.io/tooling\"",
 		},
 		"tooling annotation must have kubectl prefix": {
-			grsAnnotation:     validGrsAnnotation,
+			gksAnnotation:     validGksAnnotation,
 			toolingAnnotation: "helm/v3",
 			idLabel:           validIDLabel,
 			expectErr:         "error: ApplySet parent object \"secrets./my-set\" already exists and is managed by tooling \"helm\" instead of \"kubectl\"",
 		},
 		"tooling annotation with invalid prefix with one segment can be parsed": {
-			grsAnnotation:     validGrsAnnotation,
+			gksAnnotation:     validGksAnnotation,
 			toolingAnnotation: "helm",
 			idLabel:           validIDLabel,
 			expectErr:         "error: ApplySet parent object \"secrets./my-set\" already exists and is managed by tooling \"helm\" instead of \"kubectl\"",
 		},
 		"tooling annotation with invalid prefix with many segments can be parsed": {
-			grsAnnotation:     validGrsAnnotation,
+			gksAnnotation:     validGksAnnotation,
 			toolingAnnotation: "example.com/tool/why/v1",
 			idLabel:           validIDLabel,
 			expectErr:         "error: ApplySet parent object \"secrets./my-set\" already exists and is managed by tooling \"example.com/tool/why\" instead of \"kubectl\"",
 		},
 		"ID label is required": {
-			grsAnnotation:     validGrsAnnotation,
+			gksAnnotation:     validGksAnnotation,
 			toolingAnnotation: validToolingAnnotation,
 			idLabel:           "",
 			expectErr:         "error: ApplySet parent object \"secrets./my-set\" exists and does not have required label applyset.kubernetes.io/id",
 		},
 		"ID label must match the ApplySet's real ID": {
-			grsAnnotation:     validGrsAnnotation,
+			gksAnnotation:     validGksAnnotation,
 			toolingAnnotation: validToolingAnnotation,
 			idLabel:           "somethingelse",
 			expectErr:         fmt.Sprintf("error: ApplySet parent object \"secrets./my-set\" exists and has incorrect value for label \"applyset.kubernetes.io/id\" (got: somethingelse, want: %s)", validIDLabel),
@@ -2596,8 +2596,8 @@ func TestApplySetInvalidLiveParent(t *testing.T) {
 			secret.SetNamespace("test")
 			annotations := make(map[string]string)
 			labels := make(map[string]string)
-			if test.grsAnnotation != "" {
+			if test.gksAnnotation != "" {
-				annotations[ApplySetGRsAnnotation] = test.grsAnnotation
+				annotations[ApplySetGKsAnnotation] = test.gksAnnotation
 			}
 			if test.toolingAnnotation != "" {
 				annotations[ApplySetToolingAnnotation] = test.toolingAnnotation
@@ -2670,7 +2670,7 @@ kind: ApplySet
 metadata:
   annotations:
     applyset.kubernetes.io/additional-namespaces: test
-    applyset.kubernetes.io/contains-group-resources: replicationcontrollers
+    applyset.kubernetes.io/contains-group-kinds: ReplicationController
     applyset.kubernetes.io/tooling: kubectl/v0.0.0-master+$Format:%H$
   creationTimestamp: null
   labels:

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

@@ -54,13 +54,22 @@ const (
 	// Example value: "kube-system,ns1,ns2".
 	ApplySetAdditionalNamespacesAnnotation = "applyset.kubernetes.io/additional-namespaces"
 
-	// ApplySetGRsAnnotation is a list of group-resources used to optimize listing of ApplySet member objects.
+	// Deprecated: ApplySetGRsAnnotation is a list of group-resources used to optimize listing of ApplySet member objects.
+	// It is optional in the ApplySet specification, as tools can perform discovery or use a different optimization.
+	// However, it is currently required in kubectl.
+	// When present, the value of this annotation must be a comma separated list of the group-resources,
+	// in the fully-qualified name format, i.e. <resourcename>.<group>.
+	// Example value: "certificates.cert-manager.io,configmaps,deployments.apps,secrets,services"
+	// Deprecated and replaced by ApplySetGKsAnnotation, support for this can be removed in applyset beta or GA.
+	DeprecatedApplySetGRsAnnotation = "applyset.kubernetes.io/contains-group-resources"
+
+	// ApplySetGKsAnnotation is a list of group-kinds used to optimize listing of ApplySet member objects.
 	// It is optional in the ApplySet specification, as tools can perform discovery or use a different optimization.
 	// However, it is currently required in kubectl.
 	// When present, the value of this annotation must be a comma separated list of the group-kinds,
-	// in the fully-qualified name format, i.e. <resourcename>.<group>.
+	// in the fully-qualified name format, i.e. <kind>.<group>.
-	// Example value: "certificates.cert-manager.io,configmaps,deployments.apps,secrets,services"
+	// Example value: "Certificate.cert-manager.io,ConfigMap,deployments.apps,Secret,Service"
-	ApplySetGRsAnnotation = "applyset.kubernetes.io/contains-group-resources"
+	ApplySetGKsAnnotation = "applyset.kubernetes.io/contains-group-kinds"
 
 	// ApplySetParentIDLabel is the key of the label that makes object an ApplySet parent object.
 	// Its value MUST use the format specified in V1ApplySetIdFormat below
@@ -92,13 +101,13 @@ type ApplySet struct {
 	toolingID ApplySetTooling
 
 	// currentResources is the set of resources that are part of the sever-side set as of when the current operation started.
-	currentResources map[schema.GroupVersionResource]*meta.RESTMapping
+	currentResources map[schema.GroupKind]*kindInfo
 
 	// currentNamespaces is the set of namespaces that contain objects in this applyset as of when the current operation started.
 	currentNamespaces sets.Set[string]
 
 	// updatedResources is the set of resources that will be part of the set as of when the current operation completes.
-	updatedResources map[schema.GroupVersionResource]*meta.RESTMapping
+	updatedResources map[schema.GroupKind]*kindInfo
 
 	// updatedNamespaces is the set of namespaces that will contain objects in this applyset as of when the current operation completes.
 	updatedNamespaces sets.Set[string]
@@ -143,9 +152,9 @@ func (t ApplySetTooling) String() string {
 // NewApplySet creates a new ApplySet object tracked by the given parent object.
 func NewApplySet(parent *ApplySetParentRef, tooling ApplySetTooling, mapper meta.RESTMapper, client resource.RESTClient) *ApplySet {
 	return &ApplySet{
-		currentResources:  make(map[schema.GroupVersionResource]*meta.RESTMapping),
+		currentResources:  make(map[schema.GroupKind]*kindInfo),
 		currentNamespaces: make(sets.Set[string]),
-		updatedResources:  make(map[schema.GroupVersionResource]*meta.RESTMapping),
+		updatedResources:  make(map[schema.GroupKind]*kindInfo),
 		updatedNamespaces: make(sets.Set[string]),
 		parentRef:         parent,
 		toolingID:         tooling,
@@ -284,7 +293,7 @@ func (a *ApplySet) fetchParent() error {
 		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 {
+	if a.currentResources, err = parseKindAnnotation(annotations, a.restMapper); err != nil {
 		// TODO: handle GVRs for now-deleted CRDs
 		return fmt.Errorf("parsing ApplySet annotation on %q: %w", a.parentRef, err)
 	}
@@ -302,8 +311,8 @@ func (a *ApplySet) LabelSelectorForMembers() string {
 
 // AllPrunableResources returns the list of all resources that should be considered for pruning.
 // This is potentially a superset of the resources types that actually contain resources.
-func (a *ApplySet) AllPrunableResources() []*meta.RESTMapping {
+func (a *ApplySet) AllPrunableResources() []*kindInfo {
-	var ret []*meta.RESTMapping
+	var ret []*kindInfo
 	for _, m := range a.currentResources {
 		ret = append(ret, m)
 	}
@@ -336,14 +345,43 @@ func toolingBaseName(toolAnnotation string) string {
 	return toolAnnotation
 }
 
-func parseResourcesAnnotation(annotations map[string]string, mapper meta.RESTMapper) (map[schema.GroupVersionResource]*meta.RESTMapping, error) {
+// kindInfo holds type information about a particular resource type.
-	annotation, ok := annotations[ApplySetGRsAnnotation]
+type kindInfo struct {
+	restMapping *meta.RESTMapping
+}
+
+func parseKindAnnotation(annotations map[string]string, mapper meta.RESTMapper) (map[schema.GroupKind]*kindInfo, error) {
+	annotation, ok := annotations[ApplySetGKsAnnotation]
 	if !ok {
+		if annotations[DeprecatedApplySetGRsAnnotation] != "" {
+			return parseDeprecatedResourceAnnotation(annotations[DeprecatedApplySetGRsAnnotation], mapper)
+		}
+
 		// The spec does not require this annotation. However, 'missing' means 'perform discovery'.
 		// We return an error because we do not currently support dynamic discovery in kubectl apply.
-		return nil, fmt.Errorf("kubectl requires the %q annotation to be set on all ApplySet parent objects", ApplySetGRsAnnotation)
+		return nil, fmt.Errorf("kubectl requires the %q annotation to be set on all ApplySet parent objects", ApplySetGKsAnnotation)
 	}
-	mappings := make(map[schema.GroupVersionResource]*meta.RESTMapping)
+	mappings := make(map[schema.GroupKind]*kindInfo)
+	// Annotation present but empty means that this is currently an empty set.
+	if annotation == "" {
+		return mappings, nil
+	}
+	for _, gkString := range strings.Split(annotation, ",") {
+		gk := schema.ParseGroupKind(gkString)
+		restMapping, err := mapper.RESTMapping(gk)
+		if err != nil {
+			return nil, fmt.Errorf("could not find mapping for kind in %q annotation: %w", ApplySetGKsAnnotation, err)
+		}
+		mappings[gk] = &kindInfo{
+			restMapping: restMapping,
+		}
+	}
+
+	return mappings, nil
+}
+
+func parseDeprecatedResourceAnnotation(annotation string, mapper meta.RESTMapper) (map[schema.GroupKind]*kindInfo, error) {
+	mappings := make(map[schema.GroupKind]*kindInfo)
 	// Annotation present but empty means that this is currently an empty set.
 	if annotation == "" {
 		return mappings, nil
@@ -352,13 +390,15 @@ func parseResourcesAnnotation(annotations map[string]string, mapper meta.RESTMap
 		gr := schema.ParseGroupResource(grString)
 		gvk, err := mapper.KindFor(gr.WithVersion(""))
 		if err != nil {
-			return nil, fmt.Errorf("invalid group resource in %q annotation: %w", ApplySetGRsAnnotation, err)
+			return nil, fmt.Errorf("invalid group resource in %q annotation: %w", DeprecatedApplySetGRsAnnotation, err)
 		}
-		mapping, err := mapper.RESTMapping(gvk.GroupKind())
+		restMapping, err := mapper.RESTMapping(gvk.GroupKind())
 		if err != nil {
-			return nil, fmt.Errorf("could not find kind for resource in %q annotation: %w", ApplySetGRsAnnotation, err)
+			return nil, fmt.Errorf("could not find kind for resource in %q annotation: %w", DeprecatedApplySetGRsAnnotation, err)
+		}
+		mappings[gvk.GroupKind()] = &kindInfo{
+			restMapping: restMapping,
 		}
-		mappings[mapping.Resource] = mapping
 	}
 	return mappings, nil
 }
@@ -377,9 +417,14 @@ func parseNamespacesAnnotation(annotations map[string]string) sets.Set[string] {
 
 // 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) {
+func (a *ApplySet) addResource(restMapping *meta.RESTMapping, namespace string) {
-	a.updatedResources[resource.Resource] = resource
+	gk := restMapping.GroupVersionKind.GroupKind()
-	if resource.Scope == meta.RESTScopeNamespace && namespace != "" {
+	if _, found := a.updatedResources[gk]; !found {
+		a.updatedResources[gk] = &kindInfo{
+			restMapping: restMapping,
+		}
+	}
+	if restMapping.Scope == meta.RESTScopeNamespace && namespace != "" {
 		a.updatedNamespaces.Insert(namespace)
 	}
 }
@@ -394,6 +439,8 @@ func (a *ApplySet) updateParent(mode ApplySetUpdateMode, dryRun cmdutil.DryRunSt
 	if err != nil {
 		return fmt.Errorf("failed to encode patch for ApplySet parent: %w", err)
 	}
+	// Note that because we are using SSA, we will remove any annotations we don't specify,
+	// which is how we remove the deprecated contains-group-resources annotation.
 	err = serverSideApplyRequest(a, data, dryRun, validation, false)
 	if err != nil && errors.IsConflict(err) {
 		// Try again with conflicts forced
@@ -429,17 +476,17 @@ func serverSideApplyRequest(a *ApplySet, data []byte, dryRun cmdutil.DryRunStrat
 }
 
 func (a *ApplySet) buildParentPatch(mode ApplySetUpdateMode) *metav1.PartialObjectMetadata {
-	var newGRsAnnotation, newNsAnnotation string
+	var newGKsAnnotation, newNsAnnotation string
 	switch mode {
 	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 := sets.KeySet(a.currentResources).Union(sets.KeySet(a.updatedResources))
-		newGRsAnnotation = generateResourcesAnnotation(grSuperset)
+		newGKsAnnotation = generateKindsAnnotation(grSuperset)
 		newNsAnnotation = generateNamespacesAnnotation(a.currentNamespaces.Union(a.updatedNamespaces), a.parentRef.Namespace)
 	case updateToLatestSet:
-		newGRsAnnotation = generateResourcesAnnotation(sets.KeySet(a.updatedResources))
+		newGKsAnnotation = generateKindsAnnotation(sets.KeySet(a.updatedResources))
 		newNsAnnotation = generateNamespacesAnnotation(a.updatedNamespaces, a.parentRef.Namespace)
 	}
 
@@ -453,7 +500,7 @@ func (a *ApplySet) buildParentPatch(mode ApplySetUpdateMode) *metav1.PartialObje
 			Namespace: a.parentRef.Namespace,
 			Annotations: map[string]string{
 				ApplySetToolingAnnotation:              a.toolingID.String(),
-				ApplySetGRsAnnotation:                  newGRsAnnotation,
+				ApplySetGKsAnnotation:                  newGKsAnnotation,
 				ApplySetAdditionalNamespacesAnnotation: newNsAnnotation,
 			},
 			Labels: map[string]string{
@@ -469,13 +516,13 @@ func generateNamespacesAnnotation(namespaces sets.Set[string], skip string) stri
 	return strings.Join(nsList, ",")
 }
 
-func generateResourcesAnnotation(resources sets.Set[schema.GroupVersionResource]) string {
+func generateKindsAnnotation(resources sets.Set[schema.GroupKind]) string {
-	var grs []string
+	var gks []string
-	for gvr := range resources {
+	for gk := range resources {
-		grs = append(grs, gvr.GroupResource().String())
+		gks = append(gks, gk.String())
 	}
-	sort.Strings(grs)
+	sort.Strings(gks)
-	return strings.Join(grs, ",")
+	return strings.Join(gks, ",")
 }
 
 func (a ApplySet) FieldManager() string {

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

@@ -77,27 +77,29 @@ func (a *ApplySet) FindAllObjectsToPrune(ctx context.Context, dynamicClient dyna
 
 	// We run discovery in parallel, in as many goroutines as priority and fairness will allow
 	// (We don't expect many requests in real-world scenarios - maybe tens, unlikely to be hundreds)
-	for _, restMapping := range a.AllPrunableResources() {
+	for gvk, resource := range a.AllPrunableResources() {
-		switch restMapping.Scope.Name() {
+		scope := resource.restMapping.Scope
+
+		switch scope.Name() {
 		case meta.RESTScopeNameNamespace:
 			for _, namespace := range a.AllPrunableNamespaces() {
 				if namespace == "" {
 					// Just double-check because otherwise we get cryptic error messages
-					return nil, fmt.Errorf("unexpectedly encountered empty namespace during prune of namespace-scoped resource %v", restMapping.GroupVersionKind)
+					return nil, fmt.Errorf("unexpectedly encountered empty namespace during prune of namespace-scoped resource %v", gvk)
 				}
 				tasks = append(tasks, &task{
 					namespace:   namespace,
-					restMapping: restMapping,
+					restMapping: resource.restMapping,
 				})
 			}
 
 		case meta.RESTScopeNameRoot:
 			tasks = append(tasks, &task{
-				restMapping: restMapping,
+				restMapping: resource.restMapping,
 			})
 
 		default:
-			return nil, fmt.Errorf("unhandled scope %q", restMapping.Scope.Name())
+			return nil, fmt.Errorf("unhandled scope %q", scope.Name())
 		}
 	}
 

staging/src/k8s.io/kubectl/testdata/apply/applyset-cr.yaml

@@ -4,6 +4,6 @@ metadata:
   name: my-set
   annotations:
     applyset.kubernetes.io/tooling: kubectl/v0.0.0
-    applyset.kubernetes.io/contains-group-resources: ""
+    applyset.kubernetes.io/contains-group-kinds: ""
   labels:
     applyset.kubernetes.io/id: applyset-rhp1a-HVAVT_dFgyEygyA1BEB82HPp2o10UiFTpqtAs-v1