Put router-ca configmap in openshift-config-managed iff needed

[Miciah]

Put router-ca configmap in openshift-config-managed

• manifests/00-cluster-role.yaml: Grant the operator create, get, list, watch, and delete access to configmaps.
• pkg/operator/controller/controller.go (globalMachineSpecifiedConfigNamespace): New constant for the openshift-config-managed namespace.
  (ensureRouterCACertificate): Create the router-ca configmap in openshift-config-managed.
• test/e2e/operator_test.go (TestRouterCACertificate): Get the router-ca configmap from the openshift-config-managed namespace.

Avoid shadowing the errors package

• pkg/operator/controller/controller.go (reconcile): Rename errors variable to errs to avoid shadowing the errors package.

Delete unnecessary caching of the CA certificate

• pkg/operator/controller/controller.go (CACert): Delete.
  (Config): Delete CACert.
  (ensureRouterCACertificate): Delete caching.

Delete router-ca configmap if it is not needed

If there exists no clusteringress that uses the generated default certificate, delete the configmap for the CA certificate.

This change makes it straightforward for other components to incorporate cluster-ingress-operator's CA certificate into their trust bundles exactly when some router has a default certificate signed by the CA certificate: The router-ca configmap will exist in the openshift-config-managed namespace iff the certificate CA should be trusted.

This is a follow-up to #109.

Related to NE-139.

• pkg/operator/controller/controller.go (reconcile): Use the new shouldPublishRouterCA function to check whether the CA certificate should be published in a configmap, and use the new ensureRouterCAIsPublished method create or update it and the new ensureRouterCAIsUnpublished method to delete it as appropriate.
  (ensureRouterForIngress): Use the new ensureDefaultCertificateDeleted method to delete the operator-generated default certificate if .spec.defaultCertificateSecret is set on the clusteringress.
  (ensureDefaultCertificateForIngress): Delete unnecessary comment. Replace ensureRouterCACertificate with getRouterCA.
  (ensureDefaultCertificateDeleted): New function that deletes the operator-generated default certificate.
  (getRouterCA): New function to get the CA, or create it if it does not already exist, using ensureRouterCACertificate.
  (ensureRouterCACertificate): Rename from this...
  (ensureRouterCACertificateSecret): ...to this. Change from returning the CA itself to returning the secret. Delete unnecessary comment. Do not create the configmap, which is now the responsibility of the reconcile function.
  (shouldPublishRouterCA): New function that returns a Boolean value indicating whether there exists any clusteringress that uses the generated default certificate (in which case the CA certificate that was used to generate them should be published).
  (ensureRouterCAIsPublished): New function that creates or updates the configmap as needed.
  (ensureRouterCAIsUnpublished): New function that deletes the configmap.
• pkg/operator/controller/controller_test.go (TestShouldPublishRouterCA): New test for shouldPublishRouterCA.
• test/e2e/operator_test.go (TestClusterIngressUpdate): Make sure that the configmap exists for the default clusteringress, is deleted when the clusteringress has a custom default certificate set, and is recreated when the clusteringress is changed back to using a default certificate generated by the operator.

[Miciah]

We got the following failure:

=== RUN   TestRouterCACertificate
--- FAIL: TestRouterCACertificate (1.23s)
	operator_test.go:671: failed to connect to router at abdc9f8d725a111e9860f0e4a9b0e5d5-1964219959.us-east-1.elb.amazonaws.com:443: x509: certificate signed by unknown authority

Reviewing the events and operator logs, I believe the following is the sequence of events that results in the failure:

1. The operator is running, and it has created the default clusteringress and the default router deployment, which has a volume referencing the router-certs-default secret with the generated default certificate as the volume source.
2. TestClusterIngressUpdate updates the clusteringress by changing .spec.defaultCertificateSecret from its initial setting.
3. The operator updates the deployment to use the new secret name as its volume source. Then the operator deletes the router-certs-default secret and the router-ca configmap.
4. The deployment controller spawns a new pod based on the updated deployment.
5. TestClusterIngressUpdate updates the clusteringress's .spec.defaultCertificateSecret back to its initial setting.
6. The operator updates the deployment to use the original secret name, router-certs-default, as its volume source. Then the operator generates a new CA certificate, creates a new router-ca configmap, generates a new default certificate, and creates the router-certs-default secret containing the new certificate.
7. At this point, the old pod is still present (the deployment controller hasn't deleted it yet), and now the old pod again matches the deployment (it again specifies router-certs-default as the volume source), so the deployment controller deletes the new pod and leaves the old pod running. However, the old pod is still using the old certificate that it read from the old router-certs-default secret.
8. TestRouterCACertificate reads the router-ca configmap.
9. TestRouterCACertificate attempts to connect to the router using the CA certificate it reads from router-ca. This fails with "certificate signed by unknown authority" because TestRouterCACertificate is using the new CA certificate, but the old router pod is using a certificate generated using the old CA certificate.

Questions:

1. Is the deployment controller's behavior correct?
2. For expediency, should we work around the problem in the test? The problem should only occur when .spec.defaultCertificateSecret is rapidly changed to a new value and then reverted back to its previous value.
3. How can we eliminate the race condition?
   a. Put a timestamp or fingerprint in the secret's name to ensure that we don't use the same name for two different certificates? The operator would need to generate the certificate before creating the deployment and update the clusteringress to refer to the generated secret name, and we would need to devise some way to determine during reconciliation whether a default certificate had been generated by the operator or by the administrator.
   b. Put a timestamp or fingerprint in an annotation in the pod spec of the deployment to ensure the deployment controller doesn't re-use an old pod when the certificate changes? Similar problems to the previous idea.
   c. Something elegant?

[Miciah]

/test e2e-aws-operator

[Miciah]

/test e2e-aws

[ironcladlou]

Some stuff I found here that needs looked at... another major thing which I expected to see here is a change to https://github.com/openshift/cluster-ingress-operator/blob/master/pkg/manifests/manifests.go#L216 — as we had discussed, my understanding is that we should not update the clusteringress spec field for the default. Instead, if the clusteringress secret reference is nil, render a default name into the deployment. That way, we can distinguish between our defaulting and user's specification of a secret.

[Miciah]

Hm, should I move GlobalMachineSpecifiedConfigNamespace to perhaps pkg/operator/config/config.go?

[ironcladlou]

@Miciah

Hm, should I move GlobalMachineSpecifiedConfigNamespace to perhaps pkg/operator/config/config.go?

What about rendering it from ManifestFactory like all the other assets?

[Miciah]

What about rendering it from ManifestFactory like all the other assets?

We're not using the manifest factory to create the configmap or the namespace, so that seems like a strange place. Also, manifests currently just exports constants that no other package uses. It's weird.

[ironcladlou]

/lgtm

[ironcladlou]

/lgtm cancel

[ironcladlou]

/retest

[ironcladlou]

/lgtm

[ironcladlou]

/lgtm

[ironcladlou]

/retest

[ironcladlou]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Miciah,ironcladlou]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[enj]

/refresh

[enj]

/test all

[openshift-merge-robot]

/retest

[openshift-merge-robot]

/retest