Add support for ReplicaSet k8s kind #152

anastasia-malysheva · 2022-06-14T04:38:01Z

Signed-off-by: anastasia.malysheva anastasia.malysheva@xored.com

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

anastasia-malysheva · 2022-06-16T16:10:38Z

As part of the task it was found that Job and CronJob K8s resourses can be supported with nsm annotations too, but it is not necessary now so it was not added.

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

anastasia-malysheva · 2022-06-30T06:17:37Z

As part of the task the small research was made about usage of Annotations in service meshes, Istio and Consul-k8s. For Consul annotations are widely used for pods, but also they are used for Services and Service Accounts and there are examples with custom kinds. For Istio annotations used also for Pods, Deployments, Services and other types too.

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

denis-tingaikin · 2022-07-07T22:18:45Z

main.go

+	if isReplicaOwnedByDeployment(in.Kind.Kind, metaPtr) {
+		return "", nil, nil
+	}


@anastasia-malysheva Could you say more about this?

@denis-tingaikin, replicaSet inherits annotations from the deployment it is owned by. As soon as deployment already has annotations and they've been applied before replicaset was created, replicaset tries to apply same annotations again and it causes errors during installation. That's why this check is here.

denis-tingaikin · 2022-07-07T22:20:05Z

main.go

+	return false
+}
+
+func updatePodAnnotations(kind string, metaPtr, podMetaPtr *v1.ObjectMeta) {


Why we cant do this directly in pod branch in the switch case above?

Because the method is already big enough and lint raise errors when I add more code there.

Can we fix the linter?

I used inline function as you proposed and it worked, so there is no need to change linter settings.

denis-tingaikin · 2022-07-07T22:20:31Z

main.go

+			podMetaPtr.Annotations = metaPtr.Annotations
+		}
+		err := errors.New("can't register a sink factory for empty string")
+		panic(err.Error())


Can we don't panic? It is not good practice.

Yes, I'll fix it.

Replaced with logger error.

denis-tingaikin · 2022-07-11T04:11:43Z

I think this is good. But it requires a small refactor. I see two options:

make new methods inline and fix linter
create for each type in switch operator a handle function. Something like

switch t {
case "pod":
handlePod(...)
...
}

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

main.go

denis-tingaikin · 2022-07-11T15:10:17Z

main.go

 		bytes, err := json.Marshal([]jsonpatch.JsonPatchOperation{
 			s.createInitContainerPatch(p, annotation, spec.InitContainers),
 			s.createContainerPatch(p, annotation, spec.Containers),
 			s.createVolumesPatch(p, spec.Volumes),
 			s.createLabelPatch(p, metaPtr.Labels),
 		})
 		if err != nil {
+			s.logger.Info("Some error happened")


Suggested change

s.logger.Info("Some error happened")

denis-tingaikin · 2022-07-11T15:10:24Z

main.go

@@ -95,36 +103,48 @@ func (s *admissionWebhookServer) Review(in *admissionv1.AdmissionRequest) *admis
 	}

 	resp.Allowed = true
+	s.logger.Infof("Response")


Suggested change

s.logger.Infof("Response")

denis-tingaikin · 2022-07-11T15:10:46Z

main.go

 	}
-	return p, metaPtr, podSpec
+
+	func() {


Suggested change

func() {

denis-tingaikin · 2022-07-11T15:10:53Z

main.go

+			}
+			s.logger.Errorf("Malformed specification. Annotations can't be provided in several places.")
+		}
+	}()


Suggested change

}()

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

denis-tingaikin · 2022-07-12T11:07:11Z

@anastasia-malysheva Could you please resolve these comments
#152 (comment)
#152 (comment)

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

denis-tingaikin · 2022-07-12T13:49:25Z

main.go

+	if in.Kind.Kind == replicaSetKind {
+		for _, o := range metaPtr.OwnerReferences {
+			if o.Kind == deploymentKind {
+				return "", nil, nil
+			}
+		}
 	}


Can we move this into replicaSetKind case at line 137?

No, we can't because unmarshal to target is performed after switch.

Could we please use a simple defer?

Fix was added.

denis-tingaikin · 2022-07-12T13:49:56Z

main.go

+	if in.Kind.Kind != podKind && metaPtr.Annotations != nil {
+		if podMetaPtr.Annotations == nil {
+			podMetaPtr.Annotations = metaPtr.Annotations
+		}
+		s.logger.Errorf("Malformed specification. Annotations can't be provided in several places.")
+	}


Can we move this into pod case at line 119?

no, we can't because it's not a pod case

Could we please use a simple defer?

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

denis-tingaikin · 2022-07-15T15:41:34Z

main.go

+const (
+	deploymentKind string = "Deployment"
+	podKind        string = "Pod"
+	replicaSetKind string = "ReplicaSet"
+)
+


Suggested change

const (

deploymentKind string = "Deployment"

podKind string = "Pod"

replicaSetKind string = "ReplicaSet"

)

@anastasia-malysheva Could you please resolve this comment?

@denis-tingaikin I took a look to the k8s doc and couldn't find constants for this

@denis-tingaikin I changed linter setting and removed constants

denis-tingaikin · 2022-07-15T15:41:47Z

main.go

 	var target interface{}
 	p = "/spec/template"
 	switch in.Kind.Kind {
-	case "Deployment":
+	case deploymentKind:


Suggested change

case deploymentKind:

case "Deployment":

denis-tingaikin · 2022-07-15T15:50:02Z

main.go

 		podSpec = &deployment.Spec.Template.Spec
 		target = &deployment
-	case "Pod":
+	case podKind:


Suggested change

case podKind:

case "Pod":

denis-tingaikin · 2022-07-15T15:57:50Z

main.go

+		defer func() {
+			s.logger.Info("Replicaset Defer method")
+			for _, o := range metaPtr.OwnerReferences {
+				if o.Kind == deploymentKind {
+					p, meta, spec = "", nil, nil
+				}
+			}
+		}()


This is still looking overcomplicated.

I think the main problem here that we are doing things in unmarshal method that actually doesnt related to the operation.

Please move all 'post unmarshal' changes into separate method and use it right afiter unmarshal in the Review function.

I've added changes, please take a look

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

…d-admission-webhook-k8s@main PR link: networkservicemesh/cmd-admission-webhook-k8s#152 Commit: aa7055e Author: Denis Tingaikin Date: 2022-07-20 12:46:24 +0300 Message: - Merge pull request #152 from anastasia-malysheva/support-replicaset Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

…k@main PR link: networkservicemesh/sdk#1630 Commit: 78bedfb Author: Network Service Mesh Bot Date: 2024-08-14 08:41:05 -0500 Message: - Update go.mod and go.sum to latest version from networkservicemesh/api@main (#1630) PR link: networkservicemesh/api#152 Commit: 03f6633 Author: Nikita Skrynnik Date: 2024-08-14 20:39:07 +1100 Message: - Delete unused helper functions (#152) * delete some helper functions * delete more hepler functions * delete more unused helper function * fix linter * return som e helper functions * fix CI --------- Signed-off-by: Nikita Skrynnik <nikita.skrynnik@xored.com> Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

…k@main (#448) PR link: networkservicemesh/sdk#1630 Commit: 78bedfb Author: Network Service Mesh Bot Date: 2024-08-14 08:41:05 -0500 Message: - Update go.mod and go.sum to latest version from networkservicemesh/api@main (#1630) PR link: networkservicemesh/api#152 Commit: 03f6633 Author: Nikita Skrynnik Date: 2024-08-14 20:39:07 +1100 Message: - Delete unused helper functions (#152) * delete some helper functions * delete more hepler functions * delete more unused helper function * fix linter * return som e helper functions * fix CI --------- Signed-off-by: Nikita Skrynnik <nikita.skrynnik@xored.com> Signed-off-by: NSMBot <nsmbot@networkservicmesh.io> Co-authored-by: NSMBot <nsmbot@networkservicmesh.io>

…k-k8s@main PR link: networkservicemesh/sdk-k8s#517 Commit: f9f55b4 Author: Network Service Mesh Bot Date: 2024-08-14 08:46:00 -0500 Message: - Update go.mod and go.sum to latest version from networkservicemesh/sdk@main (#517) PR link: networkservicemesh/sdk#1630 Commit: 78bedfb Author: Network Service Mesh Bot Date: 2024-08-14 08:41:05 -0500 Message: - Update go.mod and go.sum to latest version from networkservicemesh/api@main (#1630) PR link: networkservicemesh/api#152 Commit: 03f6633 Author: Nikita Skrynnik Date: 2024-08-14 20:39:07 +1100 Message: - Delete unused helper functions (#152) * delete some helper functions * delete more hepler functions * delete more unused helper function * fix linter * return som e helper functions * fix CI --------- Signed-off-by: Nikita Skrynnik <nikita.skrynnik@xored.com> Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

…k-k8s@main (#449) PR link: networkservicemesh/sdk-k8s#517 Commit: f9f55b4 Author: Network Service Mesh Bot Date: 2024-08-14 08:46:00 -0500 Message: - Update go.mod and go.sum to latest version from networkservicemesh/sdk@main (#517) PR link: networkservicemesh/sdk#1630 Commit: 78bedfb Author: Network Service Mesh Bot Date: 2024-08-14 08:41:05 -0500 Message: - Update go.mod and go.sum to latest version from networkservicemesh/api@main (#1630) PR link: networkservicemesh/api#152 Commit: 03f6633 Author: Nikita Skrynnik Date: 2024-08-14 20:39:07 +1100 Message: - Delete unused helper functions (#152) * delete some helper functions * delete more hepler functions * delete more unused helper function * fix linter * return som e helper functions * fix CI --------- Signed-off-by: Nikita Skrynnik <nikita.skrynnik@xored.com> Signed-off-by: NSMBot <nsmbot@networkservicmesh.io> Co-authored-by: NSMBot <nsmbot@networkservicmesh.io>

anastasia-malysheva added 5 commits June 10, 2022 21:36

support ReplicaSet

8939c58

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

remove redundant switch

c9ef8c5

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

return switch back and update, update rules for selfRegister

2f22dca

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

Fix processing of deployment resources

0c5016d

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

add import

ce2d6a3

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

add import

9c8733a

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

check tracing disable

3131c88

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

denis-tingaikin requested changes Jul 7, 2022

View reviewed changes

anastasia-malysheva added 2 commits July 11, 2022 19:38

Merge method back in one

ddf76f5

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

import fix

2dccc62

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

github-advanced-security bot found potential problems Jul 11, 2022

View reviewed changes

main.go Fixed Show fixed Hide fixed

main.go Fixed Show fixed Hide fixed

main.go Fixed Show fixed Hide fixed

main.go Fixed Show fixed Hide fixed

denis-tingaikin reviewed Jul 11, 2022

View reviewed changes

remove redundant logs.

951d0e4

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

fixes after comments

ead9483

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

denis-tingaikin reviewed Jul 12, 2022

View reviewed changes

fixes after comments

d7709e4

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

denis-tingaikin requested changes Jul 15, 2022

View reviewed changes

anastasia-malysheva added 2 commits July 18, 2022 20:56

fixes after comments

d3b2f29

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

fixes after comments

c5c4f45

Signed-off-by: anastasia.malysheva <anastasia.malysheva@xored.com>

denis-tingaikin approved these changes Jul 20, 2022

View reviewed changes

denis-tingaikin merged commit aa7055e into networkservicemesh:main Jul 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ReplicaSet k8s kind #152

Add support for ReplicaSet k8s kind #152

anastasia-malysheva commented Jun 14, 2022

anastasia-malysheva commented Jun 16, 2022

anastasia-malysheva commented Jun 30, 2022

denis-tingaikin Jul 7, 2022

anastasia-malysheva Jul 8, 2022

denis-tingaikin Jul 7, 2022

anastasia-malysheva Jul 8, 2022

denis-tingaikin Jul 11, 2022

anastasia-malysheva Jul 11, 2022

denis-tingaikin Jul 7, 2022

anastasia-malysheva Jul 8, 2022

anastasia-malysheva Jul 11, 2022

denis-tingaikin commented Jul 11, 2022

denis-tingaikin Jul 11, 2022

denis-tingaikin Jul 11, 2022

denis-tingaikin Jul 11, 2022

denis-tingaikin Jul 11, 2022

denis-tingaikin commented Jul 12, 2022

denis-tingaikin Jul 12, 2022

anastasia-malysheva Jul 12, 2022

denis-tingaikin Jul 12, 2022

anastasia-malysheva Jul 14, 2022

denis-tingaikin Jul 12, 2022

anastasia-malysheva Jul 12, 2022

denis-tingaikin Jul 12, 2022

denis-tingaikin Jul 15, 2022

denis-tingaikin Jul 19, 2022

anastasia-malysheva Jul 19, 2022

anastasia-malysheva Jul 19, 2022

denis-tingaikin Jul 15, 2022

denis-tingaikin Jul 15, 2022

denis-tingaikin Jul 15, 2022

anastasia-malysheva Jul 19, 2022

Add support for ReplicaSet k8s kind #152

Add support for ReplicaSet k8s kind #152

Conversation

anastasia-malysheva commented Jun 14, 2022

anastasia-malysheva commented Jun 16, 2022

anastasia-malysheva commented Jun 30, 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

denis-tingaikin commented 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

denis-tingaikin commented Jul 12, 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

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