Preserve generated cookie secret on the reconciliation process #883

rubenvp8510 · 2020-02-04T06:59:12Z

This is another patch for prevent multiple creations of deployments when a jaeger resource is created.

I only tested with all-in-one strategy, I'll test tomorrow with-cassandra yaml file

jpkrohling · 2020-02-04T14:45:21Z

pkg/inject/oauth_proxy.go

+containerLoop:
+	for index, container := range specSrc.Template.Spec.Containers {
+		if container.Name == "oauth-proxy" {
+			for argIndex, arg := range container.Args {


You can probably use something like this function:

jaeger-operator/pkg/util/util.go

Lines 169 to 177 in d169d79

func FindItem(prefix string, args []string) string {

for _, v := range args {

if strings.HasPrefix(v, prefix) {

return v

}

}

return ""

}

jpkrohling · 2020-02-04T14:45:56Z

pkg/inject/oauth_proxy.go

+	destinationLoop:
+		for index, container := range specDst.Template.Spec.Containers {
+			if container.Name == "oauth-proxy" {
+				for argIndex, arg := range container.Args {


Same here, use util.FindItem

I modified the above code to use util.FindItem, but here util.FindItem only return the value if found it, in this case I'm interested on modify the value. I don't want to modify util.FindItem to return index, as is used in other parts of the code.

jpkrohling · 2020-02-04T15:02:40Z

pkg/inject/oauth_proxy.go

@@ -97,3 +98,35 @@ func getOAuthProxyContainer(jaeger *v1.Jaeger) corev1.Container {
 		Resources: commonSpec.Resources,
 	}
 }
+
+//PreserveOauthCookieSecret preserve the generated oauth cookie across multiple reconciliations
+func PreserveOauthCookieSecret(specSrc, specDst *appsv1.DeploymentSpec) {


This could have a better name. Perhaps PropagateOAuthCookieSecret? Or just HandleOAuthCookieSecret?

In recent changes, I've been trying to avoid those pointers, returning the resulting spec. You could also do it, so that your caller would look like this:

tp.Spec = inject.PropagateOAuthCookieSecret(&t.Spec, &tp.Spec)

Sounds reasonable. I would prefer to not modify funcion parameters , make it pure function.

About the names, I'll rename it. I just put a the first name came to my mind just to test if this fix the issue.

Thanks.

jpkrohling · 2020-02-04T16:30:48Z

By the way, the fix itself looks correct, as @kevinearls confirmed during his tests. I could also confirm that the several deployment versions were caused by this:

$ kubectl rollout history deployment/with-cassandra --revision=1 > /tmp/deployment-r1.yaml
$ kubectl rollout history deployment/with-cassandra --revision=2 > /tmp/deployment-r2.yaml
$ diff /tmp/deployment-r*
1c1
< deployment.extensions/with-cassandra with revision #1
---
> deployment.extensions/with-cassandra with revision #2
9c9
< 	pod-template-hash=59c7df5c79
---
> 	pod-template-hash=57865b948b
38c38
<       --cookie-secret=rcoiF6NixkQzKbhJFTJcEw==
---
>       --cookie-secret=yCD2nT9xw64DYqFKGxUYJw==

kevinearls · 2020-02-04T17:24:55Z

I ran the full set of E2E tests on this on OCP 4.2 and they all passed.

rubenvp8510 · 2020-02-04T19:09:47Z

I've already address comments. If no other comment or observation I think this is ready to merge.

jpkrohling

Needs tests for the new util function, plus a small improvement can be made at the inventory routine. Other than that, LGTM.

jpkrohling · 2020-02-05T10:08:56Z

pkg/inventory/deployment.go

@@ -28,6 +29,7 @@ func ForDeployments(existing []appsv1.Deployment, desired []appsv1.Deployment) D

 			// we can't blindly DeepCopyInto, so, we select what we bring from the new to the old object
 			tp.Spec = v.Spec
+			tp.Spec = inject.PropagateOAuthCookieSecret(&t.Spec, &tp.Spec)


You can have both lines 31 and 32 done on the same line:

tp.Spec = inject.PropagateOAuthCookieSecret(&t.Spec, v.Spec)

You'll need to change your arguments to accept values instead of pointers, which is in line with what we discussed in a previous review :)

jpkrohling · 2020-02-05T10:10:22Z

pkg/util/util.go

@@ -176,6 +176,16 @@ func FindItem(prefix string, args []string) string {
 	return ""
 }

+// ReplaceArgument replace argument value with given value.
+func ReplaceArgument(prefix string, newValue string, args []string) {
+	for argIndex, arg := range args {


What happens if prefix isn't found? And when multiple prefixes exist? You'll need tests to assert those cases.

This method could return a bool, signaling whether there was a replacement.

jpkrohling

Just a couple of minor comments, but LGTM in general.

jpkrohling · 2020-02-06T08:54:41Z

pkg/inject/oauth_proxy.go

+			break
+		}
+	}
+	// Found the cooke secretArg parameter, replace argument.


s/cooke/cookie

jpkrohling · 2020-02-06T08:55:16Z

pkg/inject/oauth_proxy_test.go

@@ -5,13 +5,15 @@ import (
 	"sort"
 	"testing"

+	v1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"


Something is odd with the formatter

mmm.. it is.

Not sure why but this happens when I do a make format.

I changed this line and make check don't pass. :(

jpkrohling · 2020-02-06T09:00:04Z

pkg/util/util.go

@@ -176,6 +176,21 @@ func FindItem(prefix string, args []string) string {
 	return ""
 }

+// ReplaceArgument replace argument value with given value.
+func ReplaceArgument(prefix string, newValue string, args []string, firstOccurrence bool) int {


I appreciate the idea behind the firstOccurrence, but there's no need for it, at least for the moment. If you decide to keep this type of feature, you can follow the idea behind Go's strings.Replace, where the last parameter is the max number of times the replace should be applied, with a negative meaning "replace all occurrences".

https://golang.org/src/strings/strings.go?s=23551:23597#L918

I'll remove the feature, as you said is not necessary for now.

rubenvp8510 · 2020-02-06T14:07:29Z

Done with observations.
Thanks

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 requested a review from jpkrohling February 4, 2020 06:59

jpkrohling reviewed Feb 4, 2020

View reviewed changes

rubenvp8510 force-pushed the cookie-secret-multiple-deps branch from d47b7ac to 33108b6 Compare February 4, 2020 18:40

jpkrohling reviewed Feb 5, 2020

View reviewed changes

rubenvp8510 force-pushed the cookie-secret-multiple-deps branch 3 times, most recently from f6852d5 to a074fff Compare February 6, 2020 07:04

jpkrohling approved these changes Feb 6, 2020

View reviewed changes

rubenvp8510 force-pushed the cookie-secret-multiple-deps branch from f62c5d4 to 673246d Compare February 6, 2020 14:16

jpkrohling approved these changes Feb 6, 2020

View reviewed changes

rubenvp8510 added 4 commits February 7, 2020 08:48

Preserve generated cookie secret on the reconciliation process

d212e33

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Address requested changes

688ebcf

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Tests on cookies propagation

360350f

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Removed firstOccurrence from util.ReplaceArgument and other minor format

c044c13

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the cookie-secret-multiple-deps branch from 673246d to c044c13 Compare February 7, 2020 14:48

jpkrohling merged commit f158ce5 into jaegertracing:master Feb 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve generated cookie secret on the reconciliation process #883

Preserve generated cookie secret on the reconciliation process #883

rubenvp8510 commented Feb 4, 2020 •

edited

Loading

jpkrohling Feb 4, 2020

jpkrohling Feb 4, 2020

rubenvp8510 Feb 4, 2020 •

edited

Loading

jpkrohling Feb 4, 2020

jpkrohling Feb 4, 2020

rubenvp8510 Feb 4, 2020

jpkrohling commented Feb 4, 2020

kevinearls commented Feb 4, 2020

rubenvp8510 commented Feb 4, 2020

jpkrohling left a comment

jpkrohling Feb 5, 2020

jpkrohling Feb 5, 2020

jpkrohling left a comment

jpkrohling Feb 6, 2020

jpkrohling Feb 6, 2020

rubenvp8510 Feb 6, 2020

rubenvp8510 Feb 6, 2020 •

edited

Loading

jpkrohling Feb 6, 2020

rubenvp8510 Feb 6, 2020

rubenvp8510 commented Feb 6, 2020

	func FindItem(prefix string, args []string) string {
	for _, v := range args {
	if strings.HasPrefix(v, prefix) {
	return v
	}
	}

	return ""
	}

Preserve generated cookie secret on the reconciliation process #883

Preserve generated cookie secret on the reconciliation process #883

Conversation

rubenvp8510 commented Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Feb 4, 2020 • edited Loading

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

jpkrohling commented Feb 4, 2020

kevinearls commented Feb 4, 2020

rubenvp8510 commented Feb 4, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a 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

rubenvp8510 Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 commented Feb 6, 2020

rubenvp8510 commented Feb 4, 2020 •

edited

Loading

rubenvp8510 Feb 4, 2020 •

edited

Loading

rubenvp8510 Feb 6, 2020 •

edited

Loading