Adds Contour Deployment Support

[danehans]

Adds support for managing the Contour Deployment based on this reference.

Fixes #37

/cc @jpeach @skriss @stevesloka @Miciah

[Miciah]

Can you have two contours with the same name in distinct namespaces? Perhaps the value be not just the name but the namespaced name (<namespace>/<name>).

[danehans]

Can you have two contours with the same name in distinct namespaces?

@Miciah I don't see why not. Let's say I want a "default" contour for test/dev/prod namespaces. Each namespace will have a Contour named "default" and a Deployment labeled "contour.operator.projectcontour.io/deployment-contour". What do you see as the benefit of the name being a ns/name for namespace-scoped resources?

[Miciah]

My point was that if the value of the contour.operator.projectcontour.io/deployment-contour label were just the name of the contour, then to take your example, you would have 3 contours with contour.operator.projectcontour.io/deployment-contour=default. I am suggesting that you change the label value to the namespaced name of the contour, so you get contour.operator.projectcontour.io/deployment-contour=test/default, contour.operator.projectcontour.io/deployment-contour=dev/default, and contour.operator.projectcontour.io/deployment-contour=prod/default.

[Miciah]

I'm not sure how standard the "app" label is. https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ has some recommended labels. Following that reference, we could use something like the following:

	labels := map[string]string{
		"app.kubernetes.io/name": "contour",
		"app.kubernetes.io/instance": contour.Name,
		"app.kubernetes.io/version": contourVersion,
		"app.kubernetes.io/component": "ingress-controller",
		"app.kubernetes.io/part-of": "project-contour",
		"app.kubernetes.io/managed-by": "contour-operator",
	}

[danehans]

@Miciah I was trying to be consistent with projectcontour/contour: https://github.com/projectcontour/contour/blob/main/examples/contour/03-contour.yaml.

projectcontour/contour#1821 appears to track the issue that you mention. I'll update the labels accordingly.

[jpeach]

Happy to drop the "app" label in favor of well-known standards :)

[Miciah]

Don't you want anti-affinity?

I don't think affinity policy matters much anyway for the controller (Contour). It would matter more for the proxy (Envoy), except I expect we'll use a daemonset for that.

[danehans]

Same as #33 (comment).

[danehans]

@Miciah xref projectcontour/contour#2997 for anti-affinity.

[Miciah]

Hm, but https://github.com/projectcontour/contour/blob/93aed23549fc87ad59689079da5cf0cd5bfaa3d2/examples/contour/03-contour.yaml#L27-L36 does specify podAntiAffinity. So (a) I think this PR should specify podAntiAffinity as well, and (b) it isn't clear to me what #2997 is suggesting be changed.

[jpeach]

This definitely should be anti-affinity, but we can fix it in a follow-up PR.

[stevesloka]

@danehans I assume this is just your first pass to get it up and running, but many of the values should be able to be customized via the CRD, like image, pull policy, args to contour, etc.

[danehans]

@danehans I assume this is just your first pass to get it up and running, but many of the values should be able to be customized via the CRD, like image, pull policy, args to contour, etc.

@stevesloka that is correct. The first iteration of the operator will implement the reference example.

[danehans]

@Miciah I've updated the PR based on your comments, PTAL when you have a moment.

[danehans]

@jpeach I addressed your review feedback above and in commit 31d2afe, PTAL when you have a moment.

[stevesloka]

There really isn't a need to expose this port.

[danehans]

@stevesloka the port is being exposed in https://raw.githubusercontent.com/projectcontour/contour/release-1.9/examples/render/contour.yaml, so I'd like to keep it for consistency. I created #46 to capture conditionally exposing the debug port.

[stevesloka]

/lgtm

[Miciah]

Can you add a test case like this? https://github.com/openshift/cluster-ingress-operator/blob/c6b66d029f59e71b8d5e345c39beaa819a54d204/pkg/operator/controller/ingress/deployment_test.go#L846-L861

The reason I ask for that test case is kind of subtle, so I'll try to explain: DesiredDeployment defines probes like this:

		LivenessProbe: &corev1.Probe{
			Handler: corev1.Handler{
				HTTPGet: &corev1.HTTPGetAction{
					Path: "/healthz",
					Port: intstr.IntOrString{IntVal: int32(8000)},
				},
			},
		},

When the operator creates the resource, the API sets default values for the unspecified fields (TimeoutSeconds, PeriodSeconds, SuccessThreshold, and FailureThreshold).

Later the operator gets the deployment and checks the desired and actual deployments. The API has set values for TimeoutSeconds etc., and these values aren't in the deployment that DesiredDeployment builds, so DeploymentConfigChanged returns true, which causes updateDeploymentIfNeeded to try to update to the desired deployment, every time the reconciliation loop runs. At least this is what I suspect will happen in contour-operator because we have had similar things happen in other operators. Possible solutions are (a) to define the comparison to treat omitted values the same as the corresponding default values or (b) to define explicit values for any field for which the API sets a default value.

[danehans]

@Miciah thanks for catching this. I missed it in my testing, but commit 4f3c821 now fixes the issue by setting the defaults in the desired Deployment resource.

[stevesloka]

/lgtm

[danehans]

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

[jpeach]

LGTM. We can address remaining comments in followup PRs.