Allow for Patches to api-server and other changes #13

FayeGibbins · 2019-06-12T11:49:26Z

Update to apline 3.9
1 sec sleep if api-server errs when patching
small refactor
new flag --patchnode (default off) to enable patching
More info logs
Some JSON munging to enable patching
Munge the AWS update from 2 to 1 method
Allow the passing of an options struct through the code base.

…verbose levels

gjtempleton · 2019-06-12T14:53:23Z

Having a look at the changes now, but you'll need to add the new github.com/mattbaird/jsonpatch dependency to Gopkg.toml as the build is currently failing.

FayeGibbins · 2019-06-12T14:57:05Z

Will do, thanks From: Guy Templeton <notifications@github.com> Reply-To: ottoyiu/k8s-ec2-srcdst <reply@reply.github.com> Date: Wednesday, 12 June 2019 at 15:53 To: ottoyiu/k8s-ec2-srcdst <k8s-ec2-srcdst@noreply.github.com> Cc: Faye Gibbins <faye.gibbins@skyscanner.net>, Author <author@noreply.github.com> Subject: Re: [ottoyiu/k8s-ec2-srcdst] Allow for Patches to api-server and other changes (#13) Having a look at the changes now, but you'll need to add the new github.com/mattbaird/jsonpatch dependency to Gopkg.toml as the build is currently failing. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#13?email_source=notifications&email_token=AJHNPDUSAKX3EXDLDUGJEUDP2EEWHA5CNFSM4HXIEREKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXQWHPQ#issuecomment-501310398>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJHNPDU4OSO27KNJC6IOPO3P2EEWHANCNFSM4HXIEREA>.

ottoyiu · 2019-06-12T16:33:46Z

Thanks for the PR! Can you explain the intent for introducing json patch? I seem to be lacking some context for this change by just reading the code.

FayeGibbins · 2019-06-13T07:59:03Z

Hi @ottoyiu-san,
Thank you responding so quickly! I'm afraid I'm very new to Go and this is my first ever PR written in Go. Apologies for my code not being clear.
I was asked to alter your code so that it could using the api-server's PATCH utility. I do not know how to do this so searched the Internet for an example of how it was to be done. I found this: https://github.com/tamalsaha/patch-demo/blob/master/main.go#L113 and so coppied it.
As you can see it uses JSONPatch.
Perhaps there is a better way?
Yours
Faye

gjtempleton · 2019-06-18T08:22:36Z

cmd/k8s-ec2-srcdst/main.go


 	flag.Set("logtostderr", "true")
 	flag.Parse()

+	var my_opts = common.K8sEc2SrcdstOpts{


By go convention this should be camelCase rather than snake_case: https://github.com/golang/go/wiki/CodeReviewComments#variable-names

gjtempleton · 2019-06-18T08:23:20Z

cmd/k8s-ec2-srcdst/main.go

@@ -18,10 +18,19 @@ import (
 func main() {
 	kubeconfig := flag.String("kubeconfig", "", "Path to a kubeconfig file")
 	version := flag.Bool("version", false, "Prints current k8s-ec2-srcdst version")
+	patchnode := flag.Bool("patchnode", false, "By default k8s-ec2-srcdst updates the node opbject, this flag turns on patching instead. This can be useful for when AWS api rate limiting is an issue.")


s/opbject/object/g

gjtempleton · 2019-06-18T08:23:46Z

Dockerfile

@@ -1,4 +1,4 @@
-FROM alpine:3.6
+FROM alpine:3.9


What's the reason for updating the Alpine version?

gjtempleton · 2019-06-18T08:24:30Z

pkg/controller/srcdst_controller.go

@@ -48,8 +53,9 @@ func NewSrcDstController(client kubernetes.Interface, ec2Client *ec2.EC2) *Contr
 		60*time.Second,
 		// Callback Functions to trigger on add/update/delete
 		cache.ResourceEventHandlerFuncs{
-			AddFunc:    c.handler,
-			UpdateFunc: func(old, new interface{}) { c.handler(new) },
+			//AddFunc:    c.handler,


You can remove this commented out code.

gjtempleton · 2019-06-18T08:30:22Z

pkg/common/util.go

@@ -7,6 +7,10 @@ import (
 	"k8s.io/client-go/tools/clientcmd"
 )

+type K8sEc2SrcdstOpts struct {


If you're exporting this it should have a comment for godoc to pick up as with the funcs in this file.

gjtempleton · 2019-06-18T09:12:03Z

pkg/controller/srcdst_controller.go

+				// Sleep for a second and give it one
+				// more try. The sleep also stops us
+				// slamming AWS if api-server is
+				// containtly refusing to allow the


s/containtly/constantly/g

gjtempleton · 2019-06-18T09:15:08Z

pkg/controller/srcdst_controller.go

+			// https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/node.go#L170
+			if _, err := c.client.Core().Nodes().Patch(nodeCopy.Name, types.JSONPatchType, json_patch); err != nil {
+				glog.Errorf("Failed to patch %s annotation: %v", SrcDstCheckDisabledAnnotation, err)
+				// Sleep for a second and give it one


This comment is slightly oddly formatted with so many line breaks. It also doesn't make clear to me in what circumstance the retry after 1 second would handle an initial failure. The reference to AWS is also confusing me here as the calls to AWS' APIs occur outside of this logic loop.

gjtempleton · 2019-06-18T09:16:16Z

pkg/controller/srcdst_controller.go

+			glog.V(5).Info(string(json_patch))
+			// https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/node.go#L170
+			if _, err := c.client.Core().Nodes().Patch(nodeCopy.Name, types.JSONPatchType, json_patch); err != nil {
+				glog.Errorf("Failed to patch %s annotation: %v", SrcDstCheckDisabledAnnotation, err)


Why an Errorf if we're going to give it a second try? Seems like Warningf would make more sense to me as it's not necessarily an error yet. I'd also make clear in this message that it's only the first attempt.

gjtempleton · 2019-06-18T09:18:39Z

pkg/controller/srcdst_controller.go

+				// containtly refusing to allow the
+				// patches.
+				time.Sleep(1 * time.Second)
+				if _, err := c.client.Core().Nodes().Patch(nodeCopy.Name, types.JSONPatchType, json_patch); err != nil {


We don't log that we've managed a successful patch on a second attempt that I can see.

gjtempleton · 2019-06-18T09:20:55Z

pkg/controller/srcdst_controller.go

+			if err != nil {
+				glog.Errorf("Failed to MarshalIndent: %v", err)
+			}
+			glog.V(5).Info(string(json_patch))


Given that this is basically debug info this doesn't feel like it should be displayed at a lower verbosity level than some of the meaningful info messages around which nodes have been successfully patched.

maruina · 2019-06-18T09:34:11Z

Hello @ottoyiu ,
I work with Faye and Guy and I'll try to give you some context about why she's submitting this PR.

We've been using the controller in production for quite some time and it's working well. Unfortunately, we have a very busy AWS account and we often get rate limited. When we have a scaling event and we get rate limited, we see the controller reporting the following errors

E0618 07:31:45.442463 1 srcdst_controller.go:102] Failed to set kubernetes-ec2-srcdst-controller.ottoyiu.com/srcdst-check-disabled annotation: Operation cannot be fulfilled on nodes "ip-XX.XX.XX.XX.eu-west-1.compute.internal": the object has been modified; please apply your changes to the latest version and try again

and getting a lot of 409 responses from the Kubernetes API server.

In this scenario, the node is marked as ready but the src/dst check is still enabled. This means that we drop all the cross-az traffic.

By using a patch method, we think we will be able to solve this issue. We already tested in one of our production cluster where we added ~100 nodes at once and we didn't see any problem or delay in disabling the src/dst check.

Please get in touch with us if you think there is something we're missing or anything else we can do for you.

gjtempleton · 2019-06-18T10:22:22Z

pkg/controller/srcdst_controller_test.go

@@ -48,14 +49,18 @@ func TestDisableSrcDstIfEnabled(t *testing.T) {
 	ec2Client := NewMockEC2Client()
 	kubeClient := fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{*node0, *node1}})

+	var my_opts = common.K8sEc2SrcdstOpts{


It also seems like we should add a test for this new functionality when PatchNode = true

gjtempleton · 2019-06-18T10:22:46Z

pkg/common/util.go

@@ -7,6 +7,10 @@ import (
 	"k8s.io/client-go/tools/clientcmd"
 )

+type K8sEc2SrcdstOpts struct {
+	Patchnode bool


This should be UpperCamelCased

ottoyiu · 2019-06-18T17:13:48Z

@maruina @FayeGibbins @gjtempleton thanks for the contribution, explanation and the code review. This seem to work at first glance as a workaround for the update conflicts.

We have an internal branch for this project that is completely re-written to make use of informers, and indexers that already take care of this issue. We want to ultimately merge that back upstream here. However, I'm not sure where it is in the timeline unfortunately. I will see if I can get an update from the person involved with maintaining this project at my office.

gjtempleton · 2019-06-19T19:19:12Z

Thanks for getting back to us so quickly.

If those internal changes are able to be merged back upstream here that would be great as they sound like they'll also be a solution to the problem we're seeing. We really appreciate the effort you put into maintaining this project.

FayeGibbins added 4 commits June 10, 2019 12:53

ARGO-1922: Boiler plate done

84b05f7

ARGO-1922: Add the lines to tthe Patch

fc54975

ARGO-1922: A small refactor, use sleep to stop AWS DOS and use globs …

597fd18

…verbose levels

ARGO-1922: Tidy up

f9185d0

FayeGibbins mentioned this pull request Jun 12, 2019

Can it patch against the api-server rather than update? #14

Open

FayeGibbins added 2 commits June 12, 2019 15:59

ARGO-1922: Add dependancies

8222ad3

ARGO-1922: Add dependancies

488bb95

gjtempleton suggested changes Jun 18, 2019

View reviewed changes

gjtempleton reviewed Jun 18, 2019

View reviewed changes

gjtempleton mentioned this pull request Dec 6, 2019

Use informers #16

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for Patches to api-server and other changes #13

Allow for Patches to api-server and other changes #13

FayeGibbins commented Jun 12, 2019

gjtempleton commented Jun 12, 2019

FayeGibbins commented Jun 12, 2019 via email

ottoyiu commented Jun 12, 2019

FayeGibbins commented Jun 13, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

maruina commented Jun 18, 2019 •

edited

Loading

gjtempleton Jun 18, 2019

gjtempleton Jun 18, 2019

ottoyiu commented Jun 18, 2019

gjtempleton commented Jun 19, 2019

Allow for Patches to api-server and other changes #13

Are you sure you want to change the base?

Allow for Patches to api-server and other changes #13

Conversation

FayeGibbins commented Jun 12, 2019

gjtempleton commented Jun 12, 2019

FayeGibbins commented Jun 12, 2019 via email

ottoyiu commented Jun 12, 2019

FayeGibbins commented Jun 13, 2019

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

maruina commented Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ottoyiu commented Jun 18, 2019

gjtempleton commented Jun 19, 2019

maruina commented Jun 18, 2019 •

edited

Loading