Fix: remove last-applied-config annotation for configmap and secret (#3942)

Signed-off-by: Jianbo Sun <jianbo.sjb@alibaba-inc.com>
(cherry picked from commit 4789fa8833)

Co-authored-by: Jianbo Sun <jianbo.sjb@alibaba-inc.com>

charts/vela-core/templates/defwithtemplate/command.yaml

@@ -106,7 +106,7 @@ spec:
         		}]
         	}
         }
-        parameter: #PatchParams | close({
+        parameter: *#PatchParams | close({
         	// +usage=Specify the commands for multiple containers
         	containers: [...#PatchParams]
         })

charts/vela-core/templates/defwithtemplate/container-image.yaml

@@ -69,7 +69,7 @@ spec:
         		}]
         	}
         }
-        parameter: #PatchParams | close({
+        parameter: *#PatchParams | close({
         	// +usage=Specify the container image for multiple containers
         	containers: [...#PatchParams]
         })

charts/vela-core/templates/defwithtemplate/env.yaml

@@ -96,7 +96,7 @@ spec:
         		}]
         	}
         }
-        parameter: #PatchParams | close({
+        parameter: *#PatchParams | close({
         	// +usage=Specify the environment variables for multiple containers
         	containers: [...#PatchParams]
         })

charts/vela-minimal/templates/defwithtemplate/command.yaml

@@ -106,7 +106,7 @@ spec:
         		}]
         	}
         }
-        parameter: #PatchParams | close({
+        parameter: *#PatchParams | close({
         	// +usage=Specify the commands for multiple containers
         	containers: [...#PatchParams]
         })

charts/vela-minimal/templates/defwithtemplate/container-image.yaml

@@ -69,7 +69,7 @@ spec:
         		}]
         	}
         }
-        parameter: #PatchParams | close({
+        parameter: *#PatchParams | close({
         	// +usage=Specify the container image for multiple containers
         	containers: [...#PatchParams]
         })

charts/vela-minimal/templates/defwithtemplate/env.yaml

@@ -96,7 +96,7 @@ spec:
         		}]
         	}
         }
-        parameter: #PatchParams | close({
+        parameter: *#PatchParams | close({
         	// +usage=Specify the environment variables for multiple containers
         	containers: [...#PatchParams]
         })

pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go

@@ -3897,7 +3897,7 @@ spec:
         		}]
         	}
         }
-        parameter: #PatchParams | close({
+        parameter: *#PatchParams | close({
         	// +usage=Specify the environment variables for multiple containers
         	containers: [...#PatchParams]
         })

pkg/oam/labels.go

@@ -117,7 +117,7 @@ const (
 	AnnotationAppGeneration = "app.oam.dev/generation"
 
 	// AnnotationLastAppliedConfig records the previous configuration of a
-	// resource for use in a three way diff during a patching apply
+	// resource for use in a three-way diff during a patching apply
 	AnnotationLastAppliedConfig = "app.oam.dev/last-applied-configuration"
 
 	// AnnotationLastAppliedTime indicates the last applied time

pkg/utils/apply/apply.go

@@ -20,19 +20,19 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1"
-	"github.com/oam-dev/kubevela/pkg/controller/utils"
-	"github.com/oam-dev/kubevela/pkg/oam/util"
-
-	"github.com/oam-dev/kubevela/pkg/oam"
-
 	"github.com/pkg/errors"
+	corev1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1"
+	"github.com/oam-dev/kubevela/pkg/controller/utils"
+	"github.com/oam-dev/kubevela/pkg/oam"
+	"github.com/oam-dev/kubevela/pkg/oam/util"
 )
 
 const (
@@ -107,13 +107,42 @@ func loggingApply(msg string, desired client.Object) {
 	klog.InfoS(msg, "name", d.GetName(), "resource", desired.GetObjectKind().GroupVersionKind().String())
 }
 
+// filterRecordForSpecial will filter special object that can reduce the record for "app.oam.dev/last-applied-configuration" annotation.
+func filterRecordForSpecial(desired client.Object) bool {
+	if desired == nil {
+		return false
+	}
+	gvk := desired.GetObjectKind().GroupVersionKind()
+	gp, kd := gvk.Group, gvk.Kind
+	if gp == "" {
+		// group is empty means it's Kubernetes core API, we won't record annotation for Secret and Configmap
+		if kd == "Secret" || kd == "ConfigMap" {
+			return false
+		}
+		if _, ok := desired.(*corev1.ConfigMap); ok {
+			return false
+		}
+		if _, ok := desired.(*corev1.Secret); ok {
+			return false
+		}
+	}
+	ann := desired.GetAnnotations()
+	if ann != nil {
+		lac := ann[oam.AnnotationLastAppliedConfig]
+		if lac == "-" || lac == "skip" {
+			return false
+		}
+	}
+	return true
+}
+
 // Apply applies new state to an object or create it if not exist
 func (a *APIApplicator) Apply(ctx context.Context, desired client.Object, ao ...ApplyOption) error {
 	_, err := generateRenderHash(desired)
 	if err != nil {
 		return err
 	}
-	applyAct := &applyAction{updateAnnotation: true}
+	applyAct := &applyAction{updateAnnotation: filterRecordForSpecial(desired)}
 	existing, err := a.createOrGetExisting(ctx, applyAct, a.c, desired, ao...)
 	if err != nil {
 		return err

pkg/utils/apply/apply_test.go

@@ -23,8 +23,10 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/test"
 	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -423,3 +425,19 @@ func TestMustBeControlledByApp(t *testing.T) {
 		})
 	}
 }
+
+func TestFilterSpecialAnn(t *testing.T) {
+	var cm = &corev1.ConfigMap{}
+	var sc = &corev1.Secret{}
+	var dp = &appsv1.Deployment{}
+	assert.Equal(t, false, filterRecordForSpecial(cm))
+	assert.Equal(t, false, filterRecordForSpecial(sc))
+	assert.Equal(t, true, filterRecordForSpecial(dp))
+
+	dp.Annotations = map[string]string{oam.AnnotationLastAppliedConfig: "-"}
+	assert.Equal(t, false, filterRecordForSpecial(dp))
+	dp.Annotations = map[string]string{oam.AnnotationLastAppliedConfig: "skip"}
+	assert.Equal(t, false, filterRecordForSpecial(dp))
+	dp.Annotations = map[string]string{oam.AnnotationLastAppliedConfig: "xxx"}
+	assert.Equal(t, true, filterRecordForSpecial(dp))
+}

references/cli/traits_test.go

@@ -17,11 +17,22 @@ limitations under the License.
 package cli
 
 import (
+	"bytes"
+	"context"
+	"os"
+	"strings"
 	"testing"
 
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
 	"github.com/stretchr/testify/assert"
+	"sigs.k8s.io/yaml"
 
+	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1"
 	"github.com/oam-dev/kubevela/apis/types"
+	util2 "github.com/oam-dev/kubevela/pkg/oam/util"
+	common2 "github.com/oam-dev/kubevela/pkg/utils/common"
+	"github.com/oam-dev/kubevela/pkg/utils/util"
 	"github.com/oam-dev/kubevela/references/common"
 )
 
@@ -43,3 +54,170 @@ func TestTraitsAppliedToAllWorkloads(t *testing.T) {
 	}
 	assert.Equal(t, []string{"*"}, common.ConvertApplyTo(trait.AppliesTo, workloads))
 }
+
+var _ = Describe("Test trait cli", func() {
+
+	When("there are container-image and configmap traits", func() {
+		BeforeEach(func() {
+			// Install trait locally
+			containerImage := v1beta1.TraitDefinition{}
+			Expect(yaml.Unmarshal([]byte(containerImageYaml), &containerImage)).Should(BeNil())
+			Expect(k8sClient.Create(context.Background(), &containerImage)).Should(SatisfyAny(BeNil(), util2.AlreadyExistMatcher{}))
+
+			configMap := v1beta1.TraitDefinition{}
+			Expect(yaml.Unmarshal([]byte(configmapYaml), &configMap)).Should(BeNil())
+			Expect(k8sClient.Create(context.Background(), &configMap)).Should(SatisfyAny(BeNil(), util2.AlreadyExistMatcher{}))
+		})
+
+		It("should not have any err", func() {
+			arg := common2.Args{}
+			arg.SetClient(k8sClient)
+			buffer := bytes.NewBuffer(nil)
+			ioStreams := util.IOStreams{In: os.Stdin, Out: buffer, ErrOut: buffer}
+			cmd := NewTraitCommand(arg, ioStreams)
+			Expect(cmd.Execute()).Should(BeNil())
+			buf, ok := ioStreams.Out.(*bytes.Buffer)
+			Expect(ok).Should(BeTrue())
+			Expect(strings.Contains(buf.String(), "error")).Should(BeFalse())
+		})
+	})
+})
+
+const (
+	containerImageYaml = `apiVersion: core.oam.dev/v1beta1
+kind: TraitDefinition
+metadata:
+  annotations:
+    definition.oam.dev/description: Set the image of the container.
+  name: container-image
+  namespace: vela-system
+spec:
+  appliesToWorkloads:
+    - '*'
+  podDisruptive: true
+  schematic:
+    cue:
+      template: |
+        #PatchParams: {
+        	// +usage=Specify the name of the target container, if not set, use the component name
+        	containerName: *"" | string
+        	// +usage=Specify the image of the container
+        	image: string
+        	// +usage=Specify the image pull policy of the container
+        	imagePullPolicy: *"" | "IfNotPresent" | "Always" | "Never"
+        }
+        PatchContainer: {
+        	_params:         #PatchParams
+        	name:            _params.containerName
+        	_baseContainers: context.output.spec.template.spec.containers
+        	_matchContainers_: [ for _container_ in _baseContainers if _container_.name == name {_container_}]
+        	_baseContainer: *_|_ | {...}
+        	if len(_matchContainers_) == 0 {
+        		err: "container \(name) not found"
+        	}
+        	if len(_matchContainers_) > 0 {
+        		// +patchStrategy=retainKeys
+        		image: _params.image
+
+        		if _params.imagePullPolicy != "" {
+        			// +patchStrategy=retainKeys
+        			imagePullPolicy: _params.imagePullPolicy
+        		}
+        	}
+        }
+        patch: spec: template: spec: {
+        	if parameter.containers == _|_ {
+        		// +patchKey=name
+        		containers: [{
+        			PatchContainer & {_params: {
+        				if parameter.containerName == "" {
+        					containerName: context.name
+        				}
+        				if parameter.containerName != "" {
+        					containerName: parameter.containerName
+        				}
+        				image:           parameter.image
+        				imagePullPolicy: parameter.imagePullPolicy
+        			}}
+        		}]
+        	}
+        	if parameter.containers != _|_ {
+        		// +patchKey=name
+        		containers: [ for c in parameter.containers {
+        			if c.containerName == "" {
+        				err: "containerName must be set for containers"
+        			}
+        			if c.containerName != "" {
+        				PatchContainer & {_params: c}
+        			}
+        		}]
+        	}
+        }
+        parameter: *#PatchParams | close({
+        	// +usage=Specify the container image for multiple containers
+        	containers: [...#PatchParams]
+        })
+        errs: [ for c in patch.spec.template.spec.containers if c.err != _|_ {c.err}]
+`
+	configmapYaml = `apiVersion: core.oam.dev/v1beta1
+kind: TraitDefinition
+metadata:
+  annotations:
+    definition.oam.dev/description: Create/Attach configmaps on K8s pod for your workload which follows the pod spec in path 'spec.template'. This definition is DEPRECATED, please specify configmap in 'storage' instead.
+  labels:
+    custom.definition.oam.dev/deprecated: "true"
+  name: configmap
+  namespace: vela-system
+spec:
+  appliesToWorkloads:
+    - '*'
+  podDisruptive: true
+  schematic:
+    cue:
+      template: |
+        patch: spec: template: spec: {
+        	containers: [{
+        		// +patchKey=name
+        		volumeMounts: [
+        			for v in parameter.volumes {
+        				{
+        					name:      "volume-\(v.name)"
+        					mountPath: v.mountPath
+        					readOnly:  v.readOnly
+        				}
+        			},
+        		]
+        	}, ...]
+        	// +patchKey=name
+        	volumes: [
+        		for v in parameter.volumes {
+        			{
+        				name: "volume-\(v.name)"
+        				configMap: name: v.name
+        			}
+        		},
+        	]
+        }
+        outputs: {
+        	for v in parameter.volumes {
+        		if v.data != _|_ {
+        			"\(v.name)": {
+        				apiVersion: "v1"
+        				kind:       "ConfigMap"
+        				metadata: name: v.name
+        				data: v.data
+        			}
+        		}
+        	}
+        }
+        parameter: {
+        	// +usage=Specify mounted configmap names and their mount paths in the container
+        	volumes: [...{
+        		name:      string
+        		mountPath: string
+        		readOnly:  *false | bool
+        		data?: [string]: string
+        	}]
+        }
+`
+)

vela-templates/definitions/internal/trait/command.cue

@@ -100,7 +100,7 @@ template: {
 		}
 	}
 
-	parameter: #PatchParams | close({
+	parameter: *#PatchParams | close({
 		// +usage=Specify the commands for multiple containers
 		containers: [...#PatchParams]
 	})

vela-templates/definitions/internal/trait/container-image.cue

@@ -65,7 +65,7 @@ template: {
 		}
 	}
 
-	parameter: #PatchParams | close({
+	parameter: *#PatchParams | close({
 		// +usage=Specify the container image for multiple containers
 		containers: [...#PatchParams]
 	})

vela-templates/definitions/internal/trait/env.cue

@@ -90,7 +90,7 @@ template: {
 		}
 	}
 
-	parameter: #PatchParams | close({
+	parameter: *#PatchParams | close({
 		// +usage=Specify the environment variables for multiple containers
 		containers: [...#PatchParams]
 	})