Adds Envoy DaemonSet Support #44

danehans · 2020-10-13T16:18:14Z

Adds support for managing Envoy through a DaemonSet resource.

Fixes #40

/assign @jpeach @stevesloka @skriss
/cc @Miciah

stevesloka

Overall looks good to me, just a question on the source for the images.

Also, would be nice to get the license headers in here now for all new code (and use #11 to follow up on previous files).

stevesloka · 2020-10-15T19:51:24Z

cmd/contour-operator.go

 metricsAddr string
 enableLeaderElection bool
 )

 func main() {
- flag.StringVar(&image, "image", "docker.io/projectcontour/contour:latest", "The image used for the managed Contour.")
+ flag.StringVar(&contourImage, "contour-image", "docker.io/projectcontour/contour:main",


So does this define the default for the operator? Meaning, if I create a new "contour" CRD, then this is what is used if not specified?

@stevesloka that is correct.

stevesloka · 2020-10-15T19:52:25Z

cmd/contour-operator.go

- flag.StringVar(&image, "image", "docker.io/projectcontour/contour:latest", "The image used for the managed Contour.")
+ flag.StringVar(&contourImage, "contour-image", "docker.io/projectcontour/contour:main",
+ "The container image used for the managed Contour.")
+ flag.StringVar(&envoyImage, "envoy-image", "docker.io/envoyproxy/envoy:v1.15.1",


Same here as previous question with Contour image.

Same answer as #44 (comment).

stevesloka · 2020-10-15T19:53:59Z

controller/contour/controller_test.go

+func checkContainerHasImage(t *testing.T, container *corev1.Container, image string) {
+ t.Helper()
+
+ if container.Image == image {


This checks against the image defined in the flag right? So the version of the image defined in the flag changes, would this then cause an immediate upgrade?

As mentioned above, the contour/envoy container images are set from the operator CLI. If the operator was restarted with new values for these flags, the images would be differ causing the Envoy DamonSet and the Contour Deployment to trigger a rolling update.

Maybe it's a docs issue then but that behavior needs to be super clear to users. I would have expected that upgrades to the instances would be done via the CRD that manages that instance.

+1 to @stevesloka's comment, I think ideally you'd manage the versions of Contour and Envoy on the CRD so they could be controlled declaratively and independently of one another if multiple Contours are being managed. Let's file an issue to track.

danehans · 2020-10-15T23:33:36Z

Commit 862a8e3 adds a label/labelSelector that identifies pods of the DaemonSet as being owned by the DaemonSet. The DaemonSet continues to be labeled as being owned by the Contour.

jpeach · 2020-10-16T01:38:30Z

controller/contour/daemonset.go

+ owningContourLabel: contour.Name,
+ }
+
+ pointerTo := func(ios intstr.IntOrString) *intstr.IntOrString { return &ios }


You might as well do this:

pointerTo := func(i int) *intstr.IntOrString { val := intstr.IntOrString{IntVal: int32(i)} return &val }

With your suggested changes:

$ make test /Users/daneyonhansen/code/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..." go fmt ./... go vet ./... # github.com/projectcontour/contour-operator/controller/contour controller/contour/controller.go:35:41: cannot convert i (type intstr.IntOrString) to type int32 # github.com/projectcontour/contour-operator/controller/contour vet: controller/contour/controller.go:35:42: cannot convert i (variable of type intstr.IntOrString) to int32 make: *** [vet] Error 2

I did move the func up in scope to remove duplication with deployment.go.

Well clearly you'd want to update the call sites too 😂

controller/contour/daemonset.go

util/equality/equality_test.go

controller/contour/daemonset.go

jpeach

This looks OK to me. I marked a few nits, and it needs to be rebased. I'll hold off any final merge until @stevesloka completes his review as well.

Signed-off-by: Daneyon Hansen <[email protected]>

skriss · 2020-10-19T19:20:46Z

cmd/contour-operator.go

- flag.StringVar(&image, "image", "docker.io/projectcontour/contour:main", "The image used for the managed Contour.")
+ flag.StringVar(&contourImage, "contour-image", "docker.io/projectcontour/contour:main",
+ "The container image used for the managed Contour.")
+ flag.StringVar(&envoyImage, "envoy-image", "docker.io/envoyproxy/envoy:v1.15.1",


FYI, we've updated Contour's main branch to use Envoy 1.16.0.

skriss · 2020-10-19T19:23:30Z

controller/contour/controller_test.go

+func checkContainerHasImage(t *testing.T, container *corev1.Container, image string) {
+ t.Helper()
+
+ if container.Image == image {


+1 to @stevesloka's comment, I think ideally you'd manage the versions of Contour and Envoy on the CRD so they could be controlled declaratively and independently of one another if multiple Contours are being managed. Let's file an issue to track.

danehans force-pushed the ds_impl branch from d28091a to 538644b Compare October 14, 2020 17:08

danehans requested review from jpeach and skriss October 14, 2020 17:08

stevesloka reviewed Oct 15, 2020

View reviewed changes

danehans force-pushed the ds_impl branch from 538644b to 862a8e3 Compare October 15, 2020 23:26

jpeach reviewed Oct 16, 2020

View reviewed changes

controller/contour/daemonset.go Outdated Show resolved Hide resolved

jpeach reviewed Oct 16, 2020

View reviewed changes

util/equality/equality_test.go Show resolved Hide resolved

jpeach reviewed Oct 16, 2020

View reviewed changes

controller/contour/daemonset.go Outdated Show resolved Hide resolved

jpeach approved these changes Oct 16, 2020

View reviewed changes

danehans force-pushed the ds_impl branch from 862a8e3 to 0751c99 Compare October 19, 2020 18:35

Adds Envoy DaemonSet Support

a9c6721

Signed-off-by: Daneyon Hansen <[email protected]>

danehans force-pushed the ds_impl branch from 0751c99 to a9c6721 Compare October 19, 2020 19:10

skriss reviewed Oct 19, 2020

View reviewed changes

This was referenced Oct 19, 2020

contour/envoy container images should come from CRD #55

Closed

Update the default Envoy version to 1.16.0 #56

Closed

stevesloka merged commit 5eedd7e into projectcontour:main Oct 19, 2020

danehans deleted the ds_impl branch November 2, 2020 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Envoy DaemonSet Support #44

Adds Envoy DaemonSet Support #44

danehans commented Oct 13, 2020

stevesloka left a comment

stevesloka Oct 15, 2020

danehans Oct 15, 2020

stevesloka Oct 15, 2020

danehans Oct 15, 2020

stevesloka Oct 15, 2020

danehans Oct 15, 2020

stevesloka Oct 16, 2020

skriss Oct 19, 2020

stevesloka Oct 19, 2020

danehans commented Oct 15, 2020

jpeach Oct 16, 2020

danehans Oct 19, 2020

jpeach Oct 19, 2020

jpeach left a comment

skriss Oct 19, 2020

skriss Oct 19, 2020

Adds Envoy DaemonSet Support #44

Adds Envoy DaemonSet Support #44

Conversation

danehans commented Oct 13, 2020

stevesloka 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

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

danehans commented Oct 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment