[release-4.11] OCPBUGS-853: certificate-publisher: Don't publish extraneous certificates #824
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@Miciah: This pull request references Jira Issue OCPBUGS-853, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Miciah: This pull request references Jira Issue OCPBUGS-853, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Miciah
force-pushed
the
OCPBUGS-853-certificate-publisher-do-not-publish-extra-certs
branch
from
7dd8e0f
to
1b88f04
Compare
|
@Miciah: This pull request references Jira Issue OCPBUGS-853, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
|
|
#826 merged. |
Miciah
force-pushed
the
OCPBUGS-853-certificate-publisher-do-not-publish-extra-certs
branch
2 times, most recently
from
87fe114
to
8fd69d3
Compare
|
/assign @suleymanakbas91 |
| data = bytes.Join([][]byte{ | ||
| s1.Data["tls.crt"], | ||
| s1.Data["tls.key"], | ||
| defaultCIExplicitDefaultDefaultCert = newIngressController("default", "router-certs-default", "apps.my.devcluster.openshift.com") |
There was a problem hiding this comment.
I suppose this is a typo.
| defaultCIExplicitDefaultDefaultCert = newIngressController("default", "router-certs-default", "apps.my.devcluster.openshift.com") | |
| defaultCIExplicitDefaultCert = newIngressController("default", "router-certs-default", "apps.my.devcluster.openshift.com") |
There was a problem hiding this comment.
It actually isn't; it's just confusing. * grin *. I've added a comment to explain:
// defaultCIExplicitDefaultDefaultCert is an ingresscontroller
// named "default" that specifies an explicit reference for
// default certificate secret, where that secret is the same
// default one that the operator generates when none is
// specified (the "default default certificate").
Is the comment sufficient, or would it still be less confusing if I renamed the variable?
There was a problem hiding this comment.
It is clear now with the comment.
There was a problem hiding this comment.
Maybe renaming the variable would further aid clarity because I first wondered whether the run-on of DefaultDefault was intentional. And CI is Custom Ingress? Or Cluster Ingress? Maybe we could choose names like "production", or "staging", or "dev"...?
There was a problem hiding this comment.
I've renamed the variables: defaultCIWithDefaultCertUnspecified, defaultCIWithDefaultCertSetToDefault, and defaultCIWithDefaultCertSetToCustom.
There was a problem hiding this comment.
Hmm, I suppose while I'm renaming variables, I can get rid of the archaic "CI" initialism... Done.
test/e2e/operator_test.go
Outdated
|
|
||
| // Verify that the router-certs secret gets updated. | ||
| previousRouterCertsSecret = routerCertsSecret.DeepCopy() | ||
| wait.PollImmediate(1*time.Second, 20*time.Second, func() (bool, error) { |
There was a problem hiding this comment.
Timeout values look arbitrary (e.g. 10s, 15s, 20s). Would it make sense to use the same timeout values for consistency or to write a brief explanation of why a particular value is preferred?
There was a problem hiding this comment.
There was probably some off-the-cuff reasoning about how long it might take the operator to update a resource, or assumptions that if resource A has been updated, then resource B should have been updated too. However, in lieu of detailed explanations, I've updated the polling loops all to use timeout for consistency. This increases the timeouts for some polling loops; at worst, it will take a little longer for the test to fail, and at best, this can make the test less flaky.
Delete the secretIsInUse predicate for the watch on secrets. The predicate is superfluous because the map func maps the event to an empty slice anyway if no ingresscontroller uses the secret. * pkg/operator/controller/certificate-publisher/controller.go (New): Delete the predicates on the watch on secrets. (secretIsInUse): Delete function.
Refactor the secretToIngressController map func and the ingressControllersWithSecret helper. The helper is no longer used by any other functions besides secretToIngressController, so the helper can be inlined into secretToIngressController. Remove selflink from log messages since OpenShift 4.8 turned off selflinks. * pkg/operator/controller/certificate-publisher/controller.go (secretToIngressController): Inline ingressControllersWithSecret. (ingressControllersWithSecret): Delete function.
* pkg/operator/controller/certificate/controller.go (Reconcile): Log the reconcile request.
Miciah
force-pushed
the
OCPBUGS-853-certificate-publisher-do-not-publish-extra-certs
branch
from
8fd69d3
to
6a905c2
Compare
|
Rebased and addressed Suleyman's comments. |
|
/lgtm Failures look unrelated.. |
| return true | ||
| } | ||
| ic := o.(*operatorv1.IngressController) | ||
| return ic.Status.Domain == ingressConfig.Spec.Domain |
There was a problem hiding this comment.
Should this be a case insensitive comparison?
There was a problem hiding this comment.
When cluster-authentication-operator reads the secret, it uses (effectively) secret.Data[ingressConfig.Spec.Domain]: https://github.com/openshift/cluster-authentication-operator/blob/686e2897e0d6c4a0b2d68ef0cb8e28e56a6d190d/pkg/controllers/routercerts/controller.go#L137-L159
When cluster-ingress-operator publishes the secret, the old logic uses ic.Status.Domain as the key in the secret's data:
So we could normalize the key in the new logic, but not doing so doesn't break anything that would have worked before.
There was a problem hiding this comment.
Got it, it's fine the way it is. Thanks for looking into this.
| for _, ic := range controllers { | ||
| log.Info("queueing ingresscontroller", "name", ic.Name, "related", o.GetSelfLink()) | ||
| for _, ic := range list.Items { | ||
| if ic.Status.Domain != ingressConfig.Spec.Domain { |
There was a problem hiding this comment.
Should this be a case insensitive comparison?
There was a problem hiding this comment.
The analysis in #824 (comment) applies here. If the ingresscontroller's domain doesn't match the cluster ingress config's domain in a case-sensitive match, the end result is the same with either the old logic or the new logic (namely that cluster-authentication-operator won't find the certificate in the secret).
| var defaultIngressController *operatorv1.IngressController | ||
| for i := range controllers.Items { | ||
| ic := &controllers.Items[i] | ||
| if ic.Namespace == controller.DefaultOperatorNamespace && ic.Name == manifests.DefaultIngressControllerName { |
There was a problem hiding this comment.
Could you just reuse isDefaultIngressController(ic) here?
There was a problem hiding this comment.
It turns out I can! ic satisfies client.Object. I'll update that.
| // ingresscontrollers could cause the secret's size to exceed | ||
| // the maximum secret size of 1 mebibyte. See | ||
| // <https://issues.redhat.com/browse/OCPBUGS-853>. | ||
| if ingresses[i].Status.Domain != clusterIngressDomain { |
There was a problem hiding this comment.
Does this need to be case insensitive?
There was a problem hiding this comment.
The analysis in #824 (comment) applies here.
| // defaultCertName can be the same as customCertName, so | ||
| // both of the following conditions can be true; see | ||
| // <https://bugzilla.redhat.com/show_bug.cgi?id=1912922>. | ||
| if defaultCertName.Name == secrets[i].Name { |
There was a problem hiding this comment.
I've stared at this for a while and still it's not really making sense. We didn't fix https://bugzilla.redhat.com/show_bug.cgi?id=1912922 in this PR right? It was already fixed? How'd did the old code handle this?
There was a problem hiding this comment.
Right, 00f17ef fixed https://bugzilla.redhat.com/show_bug.cgi?id=1912922 but didn't require changes in this code to do so. In this code, the old logic was fine, but it is a little subtle, so I figured it would be helpful to add a comment to explain why I wrote the logic this way.
The old logic went something like this:
- Build
nameToSecret. - For each ingresscontroller,
a. Ifingress.Spec.DefaultCertificateis set, add the secret namedingress.Spec.DefaultCertificatetoglobalSecret.
b. Otherwise add the secret namedcontroller.RouterOperatorGeneratedDefaultCertificateSecretName(&ingress, operandNamespace)toglobalSecret. - Publish
globalSecret.
The new logic, because it only cares about the default ingresscontroller, inverts this logic:
- For each secret,
a. If the secret's name matchescontroller.RouterOperatorGeneratedDefaultCertificateSecretName(&ingress[i], operandNamespace)where&ingresses[i]is the default ingresscontroller, setcertto the secret.
b. If the secret's name matchesingresses[i].Spec.DefaultCertificatewhere&ingresses[i]is the default ingresscontroller, setcertto the secret`. - If
certis set, publish it inglobalSecret.
Now suppose controller.RouterOperatorGeneratedDefaultCertificateSecretName(&ingresses[i], operandNamespace) is "router-certs-default" and ingresses[i].Spec.DefaultCertificate is "foo" and that both the "router-certs-default" secret and the "foo" secret exist. Then the following logic could do the wrong thing:
for i := range secrets {
if defaultCertName.Name == secrets[i].Name {
cert = &secrets[i]
}
if customCertName != nil && customCertName.Name == secrets[i].Name {
cert = &secrets[i]
}
}The following test case (added to TestDesiredRouterCertsGlobalSecret) fails with the faulty logic:
{
description: "custom certificate, with secrets having the custom one and then default one",
inputs: testInputs{
[]operatorv1.IngressController{defaultCICustomDefaultCert},
[]corev1.Secret{customDefaultCert, defaultCert},
},
output: testOutputs{
&corev1.Secret{
Data: map[string][]byte{"apps.my.devcluster.openshift.com": customDefaultCertData},
},
},
},If secrets has "router-certs-default" after "foo", then cert would get assigned the wrong value.
Having worked through this example, I'll add the aforementioned test case to the PR. Does my explanation make the logic clearer?
There was a problem hiding this comment.
Yea thanks this helps - it was definitely subtle in the existing code. I think your code is a bit more verbose, which is good.
Test case makes sense.
There was a problem hiding this comment.
I went ahead and refactored the tricky logic into a new function, getDefaultCertificateSecretForIngressController, with more comments.
There was a problem hiding this comment.
The new variables names makes it MUCH more readable in my opinion, thanks!
Move the reconciliation of the "default-ingress-cert" configmap from the "certificate" controller to the "certificate-publisher" controller in order to ensure that the configmap gets updated when the default-certificate secret is updated. Before this commit, the certificate-publisher controller already published the "router-certs" secret in the "openshift-config-managed" namespace. The router-certs secret is supposed to have the certificate and key of the ingresscontroller that has the cluster ingress domain, so the controller watches secrets as well as ingresscontrollers in order to update the router-certs secret when the ingresscontroller is updated to reference a different secret or when the content of the referenced secret is updated. In contrast, the certificate controller was originally written to generate a self-signed CA and generate default certificates for ingresscontrollers using this CA, and for these purposes, the controller only needs to watch ingresscontrollers and not the secrets themselves. Commit 9640767 added reconciliation of the default-ingress-cert configmap to the certificate controller. This configmap has the default certificate of the "default" ingresscontroller. The configmap should be updated when the secret reference or content is updated. Moreover, it makes sense conceptually for the certificate-publisher controller to handle both publication tasks: both the router-certs secret as well as the default-ingress-certs configmap. * pkg/operator/controller/certificate/controller.go (Reconcile): Move the call to ensureDefaultIngressCertConfigMap from here... * pkg/operator/controller/certificate-publisher/controller.go (Reconcile): ...to here. * pkg/operator/controller/certificate/publish_ca.go: Rename... * pkg/operator/controller/certificate-publisher/publish_ca.go: ...to this.
Miciah
force-pushed
the
OCPBUGS-853-certificate-publisher-do-not-publish-extra-certs
branch
from
6a905c2
to
43e9469
Compare
|
/assign @gcs278 |
Publish the default certificate and key of only whichever ingresscontroller has the cluster ingress domain. Before this commit, the certificate-publisher controller published the certificates and keys of all ingresscontrollers, in the "router-certs" secret. However, the only component that uses the router-certs secret is the authentication operator, which only needs the certificate and key for the cluster ingress domain. Moreover, collecting the certificates and keys for all ingresscontrollers can produce a result that exceeds the maximum secret size of 1 mebibyte, causing the certificate-publisher controller to fail to create or update the router-certs secret. This commit changes the certificate-publisher controller not to publish the extraneous certificates and keys. This commit fixes OCPBUGS-853. https://issues.redhat.com/browse/OCPBUGS-853 * pkg/operator/controller/certificate-publisher/controller.go: Update comments to reflect that the controller only publishes the certificate and key of the ingresscontroller for the cluster ingress domain in the "router-certs" secret. (New): Add a new predicate to the watch on ingresscontrollers, using the new hasClusterIngressDomain method and isDefaultIngressController function. (secretToIngressController): Skip ingresscontrollers for domains other than the cluster ingress domain. (hasClusterIngressDomain): New method. Get the cluster ingress config and return true iff the given ingresscontroller has the same domain. (isDefaultIngressController): New function. Return true iff the given ingresscontroller is the "default" ingresscontroller. (Reconcile): Get the cluster ingress config, and pass it to ensureRouterCertsGlobalSecret. * pkg/operator/controller/certificate-publisher/publish_certs.go (ensureRouterCertsGlobalSecret): Add a parameter for the ingress config, and pass the domain from the ingress config to desiredRouterCertsGlobalSecret. (desiredRouterCertsGlobalSecret): Add a parameter for the cluster ingress domain, use it to filter out extraneous ingresscontrollers, and publish the certificate and key for only the ingresscontroller that has the cluster ingress domain. (getDefaultCertificateSecretForIngressController): New function used by desiredRouterCertsGlobalSecret. * pkg/operator/controller/certificate-publisher/publish_certs_test.go (newSecret): Delete the example PEM data and put the secret's name in the "tls.crt" and "tls.key" data fields of the returned secret so that the data fields differ for different secrets. (TestDesiredRouterCertsGlobalSecret): Modify test cases so that they verify that the router-cert secret's data fields have the expected values. Add a "default certificate, explicit" test case. Add "custom certificate" and "missing custom certificate" test cases for the default ingresscontroller with a custom default certificate. Add a "custom ingresscontroller with the cluster ingress domain" test case. Change the "no secrets" test case to use the default ingresscontroller so that the test case would expect a router-cert secret but for the missing secret for the ingresscontroller. Update the "missing secret", "extra secret", and "perfect match" test cases not to expect the router-cert secret to include data for other ingresscontrollers. * test/e2e/all_test.go (TestAll): Update the lists of tests. * test/e2e/certificate_publisher_test.go: Delete file. This deletes the TestCreateIngressControllerThenSecret and TestCreateSecretThenIngressController tests. * test/e2e/operator_test.go (TestUpdateDefaultIngressController): Rename... (TestUpdateDefaultIngressControllerSecret): ...to this. Expand the test to verify that the operator updates both the router-certs secret as well as the default-ingress-cert configmap correctly, as well as verifying that the operator *does not* update the router-certs secret and the configmap if the ingresscontroller is updated to reference a non-existent secret. Update polling loops to use the same timeout value for consistency.
* pkg/operator/controller/certificate-publisher/publish_certs_test.go (TestDesiredRouterCertsGlobalSecret): Use t.Run. Co-authored-by: Andrew McDermott <aim@frobware.com>
Miciah
force-pushed
the
OCPBUGS-853-certificate-publisher-do-not-publish-extra-certs
branch
from
26cc90a
to
c07c9bb
Compare
|
variable naming changes and test updates looks good to me |
|
/test e2e-azure-operator |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
e2e-azure-operator failed because a bunch of image pulls failed; /test e2e-azure-operator |
|
/test all |
Move the isAdmitted function from the ingress controller package to a new util package that can be imported into other controllers. * pkg/operator/controller/ingress/controller.go (isAdmitted): Move from here... * pkg/util/ingresscontroller/ingresscontroller.go (IsAdmitted): ...to here. New file for IngressController-related util functions.
When determining the desired "router-certs" secret, ignore ingresscontrollers that haven't been admitted. Multiple ingresscontrollers can all have the cluster ingress domain, but only one can be admitted at a given time. Any ingresscontrollers that haven't been admitted should be ignored so that the default certificate of any one that has been admitted is published. * pkg/operator/controller/certificate-publisher/publish_certs.go (desiredRouterCertsGlobalSecret): Ignore ingresscontrollers that haven't been admitted. * pkg/operator/controller/certificate-publisher/publish_certs_test.go (newIngressController): Add an "admitted" parameter for specifying the status of the "Admitted" status condition. (TestDesiredRouterCertsGlobalSecret): Add a test case for "default certificate and custom ingresscontroller that conflicts on domain".
|
/test e2e-azure-operator |
|
@Miciah: This pull request references Jira Issue OCPBUGS-853, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
|
e2e-aws-ovn-single-node failed because |
|
e2e-aws-operator failed because kube-apiserver reported |
|
/override ci/prow/e2e-aws-operator |
|
@Miciah: Overrode contexts on behalf of Miciah: ci/prow/e2e-aws-operator, ci/prow/e2e-aws-ovn-single-node, ci/prow/e2e-gcp-ovn-serial In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@Miciah: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/tide refresh |
|
@Miciah: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-853 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.11 |
|
@Miciah: #824 failed to apply on top of branch "release-4.11": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Miciah
changed the title
certificate-publisher: Delete
secretIsInUseDelete the
secretIsInUsepredicate for the watch on secrets. The predicate is superfluous because the map func maps the event to an empty slice anyway if no ingresscontroller uses the secret.pkg/operator/controller/certificate-publisher/controller.go(New): Delete the predicates on the watch on secrets.(
secretIsInUse): Delete function.certificate-publisher: Simplify the map func
Refactor the
secretToIngressControllermap func and theingressControllersWithSecrethelper. The helper is no longer used by any other functions besidessecretToIngressController, so the helper can be inlined intosecretToIngressController.Remove selflink from log messages since OpenShift 4.8 turned off selflinks.
pkg/operator/controller/certificate-publisher/controller.go(secretToIngressController): InlineingressControllersWithSecret.(
ingressControllersWithSecret): Delete function.certificate: Log reconcile requests.
pkg/operator/controller/certificate/controller.go(Reconcile): Log the reconcile request.Move "default-ingress-cert" configmap publishing
Move the reconciliation of the "default-ingress-cert" configmap from the "certificate" controller to the "certificate-publisher" controller in order to ensure that the configmap gets updated when the default-certificate secret is updated.
Before this change, the certificate-publisher controller already published the "router-certs" secret in the "openshift-config-managed" namespace. The router-certs secret is supposed to have the certificate and key of the ingresscontroller that has the cluster ingress domain, so the controller watches secrets as well as ingresscontrollers in order to update the router-certs secret when the ingresscontroller is updated to reference a different secret or when the content of the referenced secret is updated.
In contrast, the certificate controller was originally written to generate a self-signed CA and generate default certificates for ingresscontrollers using this CA, and for these purposes, the controller only needs to watch ingresscontrollers and not the secrets themselves.
#331 added reconciliation of the default-ingress-cert configmap to the certificate controller. This configmap has the default certificate of the "default" ingresscontroller. The configmap should be updated when the secret reference or content is updated. Moreover, it makes sense conceptually for the certificate-publisher controller to handle both publication tasks: both the router-certs secret as well as the default-ingress-certs configmap.
pkg/operator/controller/certificate/controller.go(Reconcile): Move the call toensureDefaultIngressCertConfigMapfrom here...pkg/operator/controller/certificate-publisher/controller.go(Reconcile): ...to here.pkg/operator/controller/certificate/publish_ca.go: Rename...pkg/operator/controller/certificate-publisher/publish_ca.go: ...to this.certificate-publisher: Don't publish extra certs
Publish the default certificate and key of only whichever ingresscontroller has the cluster ingress domain.
Before this change, the certificate-publisher controller published the certificates and keys of all ingresscontrollers, in the "router-certs" secret. However, the only component that uses the router-certs secret is the authentication operator, which only needs the certificate and key for the cluster ingress domain. Moreover, collecting the certificates and keys for all ingresscontrollers can produce a result that exceeds the maximum secret size of 1 mebibyte, causing the certificate-publisher controller to fail to create or update the router-certs secret. This PR changes the certificate-publisher controller not to publish the extraneous certificates and keys.
pkg/operator/controller/certificate-publisher/controller.go: Update comments to reflect that the controller only publishes the certificate and key of the ingresscontroller for the cluster ingress domain in the "router-certs" secret.(
New): Add a new predicate to the watch on ingresscontrollers, using the newhasClusterIngressDomainmethod andisDefaultIngressControllerfunction.(
secretToIngressController): Skip ingresscontrollers for domains other than the cluster ingress domain.(
hasClusterIngressDomain): New method. Get the cluster ingress config and return true iff the given ingresscontroller has the same domain.(
isDefaultIngressController): New function. Return true iff the given ingresscontroller is the "default" ingresscontroller.(
Reconcile): Get the cluster ingress config, and pass it toensureRouterCertsGlobalSecret.pkg/operator/controller/certificate-publisher/publish_certs.go(ensureRouterCertsGlobalSecret): Add a parameter for the ingress config, and pass the domain from the ingress config todesiredRouterCertsGlobalSecret.(
desiredRouterCertsGlobalSecret): Add a parameter for the cluster ingress domain, use it to filter out extraneous ingresscontrollers, and publish the certificate and key for only the ingresscontroller that has the cluster ingress domain.(
getDefaultCertificateSecretForIngressController): New function used bydesiredRouterCertsGlobalSecret.pkg/operator/controller/certificate-publisher/publish_certs_test.go(newSecret): Delete the example PEM data and put the secret's name in the "tls.crt" and "tls.key" data fields of the returned secret so that the data fields differ for different secrets.(
TestDesiredRouterCertsGlobalSecret): Modify test cases so that they verify that the router-cert secret's data fields have the expected values. Add a "default certificate, explicit" test case. Add "custom certificate" and "missing custom certificate" test cases for the default ingresscontroller with a custom default certificate. Add a "custom ingresscontroller with the cluster ingress domain" test case. Change the "no secrets" test case to use the default ingresscontroller so that the test case would expect a router-cert secret but for the missing secret for the ingresscontroller. Update the "missing secret", "extra secret", and "perfect match" test cases not to expect the router-cert secret to include data for other ingresscontrollers.test/e2e/all_test.go(TestAll): Update the lists of tests.test/e2e/certificate_publisher_test.go: Delete file. This deletes theTestCreateIngressControllerThenSecretandTestCreateSecretThenIngressControllertests.test/e2e/operator_test.go(TestUpdateDefaultIngressController): Rename...(
TestUpdateDefaultIngressControllerSecret): ...to this. Expand the test to verify that the operator updates both the router-certs secret as well as the default-ingress-cert configmap correctly, as well as verifying that the operator does not update the router-certs secret and the configmap if the ingresscontroller is updated to reference a non-existent secret.TestDesiredRouterCertsGlobalSecret: Uset.Runpkg/operator/controller/certificate-publisher/publish_certs_test.go(TestDesiredRouterCertsGlobalSecret): Uset.Run.Move
IsAdmittedto new util packageMove the
isAdmittedfunction from the ingress controller package to a new util package that can be imported into other controllers.pkg/operator/controller/ingress/controller.go(isAdmitted): Move from here...pkg/util/ingresscontroller/ingresscontroller.go(IsAdmitted): ...to here. New file for IngressController-related util functions.certificate-publisher: Ignore not-admitted ingresscontrollers
When determining the desired "router-certs" secret, ignore ingresscontrollers that haven't been admitted. Multiple ingresscontrollers can all have the cluster ingress domain, but only one can be admitted at a given time. Any ingresscontrollers that haven't been admitted should be ignored so that the default certificate of any one that has been admitted is published.
pkg/operator/controller/certificate-publisher/publish_certs.go(desiredRouterCertsGlobalSecret): Ignore ingresscontrollers that haven't been admitted.pkg/operator/controller/certificate-publisher/publish_certs_test.go(newIngressController): Add an "admitted" parameter for specifying the status of the "Admitted" status condition.(
TestDesiredRouterCertsGlobalSecret): Add a test case for "default certificate and custom ingresscontroller that conflicts on domain".