-
Notifications
You must be signed in to change notification settings - Fork 220
OCPBUGS-8000: certificate-publisher: Don't publish extraneous certificates #894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-8000: certificate-publisher: Don't publish extraneous certificates #894
Conversation
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.
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.
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 <[email protected]>
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".
|
@Miciah: No Bugzilla bug is referenced in the title of this pull request. DetailsIn 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: This pull request references Jira Issue OCPBUGS-8000, which is valid. The bug has been moved to the POST state. 6 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
From QE side, tested with the 4.11.0-0.ci.test-2023-02-28-074757-ci-ln-5h62p9k-latest: after created 180 ingresscontrollers with specified defaultCertificate, only one data in the router-certs secret and the log was correct, too.
% oc -n openshift-ingress get secret | grep custom-certs-180 |
|
/assign @rfredette |
|
/retest required |
|
@candita: The
The following commands are available to trigger optional jobs:
Use
DetailsIn 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. |
|
/retest-required |
|
e2e-gcp-serial failure: /test e2e-gcp-serial |
|
e2e-aws-single-node failed on /test e2e-aws-single-node e2e-gcp-serial also failed on /test e2e-gcp-serial |
|
@Miciah: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
e2e-aws-single-node failed again with the same failure; I filed https://issues.redhat.com/browse/OCPBUGS-11313 to track the issue, but as the job is optional and the failure seems to be unrelated to the changes in this PR, I will not rerun the tests. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/label backport-risk-assessed |
|
/label cherry-pick-approved |
|
@Miciah: Jira Issue OCPBUGS-8000: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-8000 has been moved to the MODIFIED state. DetailsIn 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. |
|
Fix included in accepted release 4.11.0-0.nightly-2023-04-25-182720 |
This is a manual cherry-pick of #824. #834 introduced a conflict that required resolution.
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".