sidecarset support inject&upgrade pod annotations #992

zmberg · 2022-06-14T11:42:50Z

Signed-off-by: liheng.zms liheng.zms@alibaba-inc.com

Ⅰ. Describe what this PR does

sidecarSet support inject or upgrade pod annotations, as follows:

# sidecarset.yaml
apiVersion: apps.kruise.io/v1alpha1
kind: SidecarSet
metadata:
  name: test-sidecarset
spec:
  patchPodMetadata:
  - patchPolicy: MergePatchJson
    annotations:
      key1: {"log-agent":1}

codecov-commenter · 2022-07-06T11:35:25Z

Codecov Report

Merging #992 (b877b92) into master (5380c57) will decrease coverage by 0.03%.
The diff coverage is 41.75%.

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
- Coverage   49.64%   49.60%   -0.04%     
==========================================
  Files         124      124              
  Lines       12078    12156      +78     
==========================================
+ Hits         5996     6030      +34     
- Misses       5166     5198      +32     
- Partials      916      928      +12

Flag	Coverage Δ
unittests	`49.60% <41.75%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...roller/workloadspread/workloadspread_controller.go	`61.11% <0.00%> (ø)`
pkg/util/tools.go	`61.64% <0.00%> (-2.65%)`	⬇️
pkg/webhook/sidecarset/mutating/hash.go	`64.70% <ø> (ø)`
pkg/webhook/pod/mutating/sidecarset.go	`73.56% <25.00%> (-1.15%)`	⬇️
pkg/controller/sidecarset/sidecarset_processor.go	`70.04% <33.33%> (+0.04%)`	⬆️
pkg/control/sidecarcontrol/util.go	`45.36% <34.00%> (-3.67%)`	⬇️
...set/validating/sidecarset_create_update_handler.go	`54.08% <70.83%> (+2.91%)`	⬆️
pkg/controller/daemonset/daemonset_update.go	`54.86% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5380c57...b877b92. Read the comment docs.

sonatype-lift · 2022-07-06T11:42:43Z

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

+				annotationsInOthers[key] = fmt.Sprintf("%s#%s", set.Name, patch.PatchPolicy)
+			}
+		}
+		if set.Spec.PatchPodMetadata != nil {


SA9003: empty branch

Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-07-06T11:42:44Z

pkg/controller/workloadspread/workloadspread_controller.go

@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"


goimports: File is not goimports-ed

Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

furykerry · 2022-07-06T11:48:10Z

pkg/control/sidecarcontrol/util.go

+}
+
+func ValidateSidecarSetPatchMetadataWhitelist(sidecarSet *appsv1alpha1.SidecarSet) error {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.SidecarSetPatchPodMetadataWhitelistGate) ||


i think SidecarSetPatchPodMetadataWhitelistGate should be enabled by default

pkg/util/settings/types.go

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

pkg/control/sidecarcontrol/util_test.go

furykerry · 2022-07-08T06:39:43Z

/lgtm

sonatype-lift · 2022-07-11T10:27:34Z

pkg/webhook/sidecarset/validating/sidecarset_validating_test.go

@@ -2,6 +2,8 @@ package validating

 import (
 	"fmt"


goimports: File is not goimports-ed

Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

FillZpp · 2022-07-22T03:47:13Z

apis/apps/v1alpha1/sidecarset_types.go

+	PatchPolicy PatchPolicyType `json:"patchPolicy,omitempty"`
+}
+
+type PatchPodFields struct {


PatchPodMetadataFields as it is a sub structure in PatchPodMetadata

FillZpp · 2022-07-22T03:59:47Z

apis/apps/v1alpha1/sidecarset_types.go

+	PatchPodMetadata []PatchPodMetadata `json:"patchPodMetadata,omitempty"`
+}
+
+type PatchPodMetadata struct {


PatchPodMetadata, PatchPodFields, PatchPolicyType, OverwritePatchPolicy, ..., all these definitions should have SidecarSet prefix to avoid conflict with other resources.

FillZpp · 2022-07-22T04:12:06Z

pkg/util/settings/settings.go

+
+const (
+	// kruise configmap name
+	KruiseConfigmapName = "kruise-cm"


kruise-config or kruise-configuration, so do the package and variable names

There is no need to init or set up the single Informer. Just GetSidecarSetPatchMetadataWhiteList(r client.Reader) to get ConfigMap from cache.

FillZpp · 2022-07-22T07:03:36Z

pkg/controller/workloadspread/workloadspread_controller.go

@@ -110,7 +111,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 	}

 	// Watch WorkloadSpread
-	err = c.Watch(&source.Kind{Type: &appsv1alpha1.WorkloadSpread{}}, &handler.EnqueueRequestForObject{})
+	err = c.Watch(&source.Kind{Type: &appsv1alpha1.WorkloadSpread{}}, &handler.EnqueueRequestForObject{}, predicate.GenerationChangedPredicate{})


Should we use GenerationChangedPredicate here? If webhook modified the workloadspread status, doesn't controller need to reconcile it?

FillZpp · 2022-07-22T07:03:57Z

pkg/features/kruise_features.go

+	PodWebhook:                              {Default: true, PreRelease: featuregate.Beta},
+	KruiseDaemon:                            {Default: true, PreRelease: featuregate.Beta},
+	DaemonWatchingPod:                       {Default: true, PreRelease: featuregate.Beta},
+	SidecarSetPatchPodMetadataWhitelistGate: {Default: true, PreRelease: featuregate.Alpha},


Alpha featureGate should be false by default.

FillZpp · 2022-07-22T07:11:57Z

pkg/util/settings/types.go

+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+const (
+	SidecarSetPatchPodMetadataWhiteListKey = "kruise.sidecarset.patch.pod.metadata.whitelist"


I'm not sure, does K8s have any configuration stored in ConfigMap that gives us examples of the key format?

Is sidecarset-patchpodmetadata-writelist or SidecarSet_PatchPodMetadata_WriteList better?

FillZpp · 2022-07-22T07:15:09Z

pkg/util/settings/types.go

+	// If selector is nil, assume that the rules should apply for every sidecarSets
+	Selector *metav1.LabelSelector `json:"selector,omitempty"`
+	// Support for regular expressions
+	AnnotationKeyExprs []string `json:"annotationKeyExprs"`


AllowedAnnotationKeyExprs is better to understand, and also in case we will add a DeniedAnnotationKeyExprs in future.

FillZpp · 2022-07-22T07:20:26Z

pkg/util/tools.go

@@ -189,3 +190,10 @@ func IsReferenceEqual(ref1, ref2 appsv1alpha1.TargetReference) bool {
 	}
 	return gv1.Group == gv2.Group && ref1.Kind == ref2.Kind && ref1.Name == ref2.Name
 }
+
+func GetKruiseNamespace() string {


It's weird to put this in tools.go, how about a new meta.go file?

FillZpp · 2022-07-22T07:21:35Z

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

@@ -132,7 +132,30 @@ func validateSidecarSetSpec(obj *appsv1alpha1.SidecarSet, fldPath *field.Path) f
 	} else {
 		allErrs = append(allErrs, validateContainersForSidecarSet(spec.InitContainers, spec.Containers, vols, fldPath.Root())...)
 	}
+	// validating metadata
+	annotations := sets.NewString()


annotationKeys

FillZpp · 2022-07-22T07:23:56Z

pkg/webhook/sidecarset/validating/sidecarset_create_update_handler.go

+			allErrs = append(allErrs, field.Required(fldPath.Child("patchPodMetadata"), "no annotations defined for SidecarSet"))
+		} else {
+			metadata := metav1.ObjectMeta{Annotations: patch.Annotations, Name: "fake-name"}
+			allErrs = append(allErrs, genericvalidation.ValidateObjectMeta(&metadata, false, validateSidecarSetName, field.NewPath("metadata"))...)


fldPath.Child("patchPodMetadata")..Child("annotations")

sonatype-lift · 2022-07-22T09:30:21Z

pkg/control/sidecarcontrol/util.go

 	"github.com/openkruise/kruise/pkg/util"
-
+	utilfeature "github.com/openkruise/kruise/pkg/util/feature"
+	"github.com/openkruise/kruise/pkg/util/kruise-configuration"


goimports: File is not goimports-ed

Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-07-22T09:30:23Z

pkg/control/sidecarcontrol/util_test.go

 	"testing"

 	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
-
+	"github.com/openkruise/kruise/pkg/util"
+	"github.com/openkruise/kruise/pkg/util/kruise-configuration"


goimports: File is not goimports-ed

Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

FillZpp · 2022-07-25T05:33:39Z

pkg/util/kruise-configuration/kruise-configuration.go

+limitations under the License.
+*/
+
+package kruise_configuration


Usually we don't recommend to put - or _ into go package path. How about just configuration?

FillZpp · 2022-07-25T05:39:05Z

pkg/util/kruise-configuration/types.go

+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+const (
+	SidecarSetPatchPodMetadataWhiteListKey = "SidecarSet_PatchPodMetadata_WriteList"


SidecarSet_PatchPodMetadata_WhiteList, must be my typo in #992 (comment) ...

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com>

FillZpp

/lgtm

kruise-bot · 2022-07-25T08:28:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FillZpp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FillZpp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com> Signed-off-by: Liu Zhenwei <zwliu@thoughtworks.com>

kruise-bot requested review from Fei-Guo and FillZpp June 14, 2022 11:42

zmberg requested review from furykerry and removed request for Fei-Guo June 14, 2022 11:43

kruise-bot added the size/L size/L: 100-499 label Jun 14, 2022

zmberg force-pushed the sidecarset-annotations branch from 072c35a to 6b66c7b Compare July 6, 2022 11:07

kruise-bot added needs-rebase size/XXL and removed size/L size/L: 100-499 labels Jul 6, 2022

zmberg force-pushed the sidecarset-annotations branch from 6b66c7b to b877b92 Compare July 6, 2022 11:25

kruise-bot removed the needs-rebase label Jul 6, 2022

sonatype-lift bot reviewed Jul 6, 2022

View reviewed changes

furykerry reviewed Jul 6, 2022

View reviewed changes

pkg/control/sidecarcontrol/util_test.go Show resolved Hide resolved

zmberg force-pushed the sidecarset-annotations branch from b877b92 to 3caf9bb Compare July 8, 2022 06:08

kruise-bot assigned furykerry Jul 8, 2022

kruise-bot added the lgtm label Jul 8, 2022

zmberg force-pushed the sidecarset-annotations branch from 3caf9bb to 922698b Compare July 11, 2022 10:10

kruise-bot removed the lgtm label Jul 11, 2022

sonatype-lift bot reviewed Jul 11, 2022

View reviewed changes

zmberg force-pushed the sidecarset-annotations branch 6 times, most recently from 82f0a2d to eba9b58 Compare July 14, 2022 12:25

FillZpp added this to the v1.3.0 milestone Jul 21, 2022

FillZpp reviewed Jul 22, 2022

View reviewed changes

zmberg force-pushed the sidecarset-annotations branch from eba9b58 to ddc0646 Compare July 22, 2022 09:15

kruise-bot added the needs-rebase label Jul 22, 2022

sonatype-lift bot reviewed Jul 22, 2022

View reviewed changes

zmberg force-pushed the sidecarset-annotations branch from ddc0646 to d82913d Compare July 22, 2022 09:54

kruise-bot removed the needs-rebase label Jul 22, 2022

zmberg force-pushed the sidecarset-annotations branch from d82913d to ec4d8ba Compare July 25, 2022 03:09

FillZpp reviewed Jul 25, 2022

View reviewed changes

zmberg force-pushed the sidecarset-annotations branch 2 times, most recently from 5f1f35d to 92f1041 Compare July 25, 2022 07:50

sidecarset support inject&upgrade pod annotations

71e638e

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com>

zmberg force-pushed the sidecarset-annotations branch from 92f1041 to 71e638e Compare July 25, 2022 07:55

FillZpp approved these changes Jul 25, 2022

View reviewed changes

kruise-bot assigned FillZpp Jul 25, 2022

kruise-bot added the lgtm label Jul 25, 2022

kruise-bot added the approved label Jul 25, 2022

kruise-bot merged commit 2867926 into openkruise:master Jul 25, 2022

zmberg deleted the sidecarset-annotations branch July 25, 2022 08:34

diannaowa pushed a commit to diannaowa/kruise that referenced this pull request Sep 14, 2022

sidecarset support inject&upgrade pod annotations (openkruise#992)

e8266dd

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com> Signed-off-by: Liu Zhenwei <zwliu@thoughtworks.com>

ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024

sidecarset support inject&upgrade pod annotations (openkruise#992)

4d24933

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sidecarset support inject&upgrade pod annotations #992

sidecarset support inject&upgrade pod annotations #992

zmberg commented Jun 14, 2022 •

edited

Loading

codecov-commenter commented Jul 6, 2022

sonatype-lift bot Jul 6, 2022

sonatype-lift bot Jul 6, 2022

furykerry Jul 6, 2022

furykerry commented Jul 8, 2022

sonatype-lift bot Jul 11, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

FillZpp Jul 22, 2022

sonatype-lift bot Jul 22, 2022

sonatype-lift bot Jul 22, 2022

FillZpp Jul 25, 2022

FillZpp Jul 25, 2022

FillZpp left a comment

kruise-bot commented Jul 25, 2022

sidecarset support inject&upgrade pod annotations #992

sidecarset support inject&upgrade pod annotations #992

Conversation

zmberg commented Jun 14, 2022 • edited Loading

Ⅰ. Describe what this PR does

codecov-commenter commented Jul 6, 2022

Codecov Report

sonatype-lift bot Jul 6, 2022

Choose a reason for hiding this comment

sonatype-lift bot Jul 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furykerry commented Jul 8, 2022

sonatype-lift bot Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Jul 22, 2022

Choose a reason for hiding this comment

sonatype-lift bot Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FillZpp left a comment

Choose a reason for hiding this comment

kruise-bot commented Jul 25, 2022

zmberg commented Jun 14, 2022 •

edited

Loading