Bug 1813894: Add configmap for service CA bundle #399
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
@Miciah: This pull request references Bugzilla bug 1813894, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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. |
openshift-ci-robot
added
the
bugzilla/valid-bug
openshift-ci-robot
added
the
approved
Miciah
force-pushed
the
BZ1813894-add-configmap-for-service-CA-bundle
branch
2 times, most recently
from
aed81db to
34bb971
Compare
|
/test e2e-aws-operator /retitle Bug 1813894: Add configmap for service CA bundle @openshift/openshift-team-network-edge, this is ready for review. |
openshift-ci-robot
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
| // Returns a Boolean indicating whether the configmap was updated, and an error | ||
| // value. | ||
| func (r *reconciler) updateServiceCAConfigMap(current, desired *corev1.ConfigMap) (bool, error) { | ||
| if current.Annotations["service.beta.openshift.io/inject-cabundle"] == "true" { |
There was a problem hiding this comment.
This acts as a gate? Once present we never update - is that the implied semantics? If so, maybe the check should occur by callers of updateServiceCAConfigMap so that update always means update.
There was a problem hiding this comment.
That is pretty much right: cluster-ingress-operator manages the configmap's lifecycle and annotation, and the serving cert signer manages the data. If the configmap's annotation were deleted, cluster-ingress-operator would restore it, but otherwise it should not update the configmap. Would it be best to fold updateServiceCAConfigMap into ensureServiceCAConfigMap?
There was a problem hiding this comment.
For me this was mostly about the semantics. A function called update... doesn't always update, but only if you peek at its implementation. If it is only called from one place them maybe folding would help.
There was a problem hiding this comment.
Here, "update" means "update if needed", and in this particular case, "if needed" means "if the critical annotation is not set properly". It is kind of a degenerate case of a pattern we use elsewhere in the operator, where ensureFoo calls createFoo, deleteFoo, and updateFoo, and updateFoo determines whether update is needed.
There was a problem hiding this comment.
OK, fine for me. Thanks for explaining the pattern.
Create a configmap that is annotated to request injection of the service CA bundle, and configure ingress controller deployments to mount this configmap and use it instead of using the service account token to get the service CA. See bug 1813894. https://bugzilla.redhat.com/show_bug.cgi?id=1813894 * assets/router/deployment.yaml: Add a volume and volume mount for the new configmap, and set the DEFAULT_DESTINATION_CA_PATH environment variable to configure openshift-router to use the bundle from the configmap. * pkg/manifests/bindata.go: Regenerate. * pkg/operator/controller/ingress/controller.go (ensureIngressController): Call ensureServiceCAConfigMap once the "openshift-ingress" namespace has been created and before creating the deployment. * pkg/operator/controller/ingress/serviceca_configmap.go: New file. (ensureServiceCAConfigMap): New method. Ensure the configmap for the service CA bundle exists. (desiredServiceCAConfigMap): New function. Return the desired configmap for the service CA bundle. (currentServiceCAConfigMap): New method. Return the current configmap for the service CA bundle. (updateServiceCAConfigMap): New method. Update the service CA configmap bundle if the expected annotation is missing. * pkg/operator/controller/names.go (ServiceCAConfigMapName): New function. Return the name of the configmap for the service CA bundle.
Miciah
force-pushed
the
BZ1813894-add-configmap-for-service-CA-bundle
branch
from
34bb971 to
fa2873f
Compare
|
/test e2e-aws-upgrade |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, Miciah 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@Miciah: Some pull requests linked via external trackers have merged: . The following pull requests linked via external trackers have not merged:
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. |
6 participants
Create a configmap that is annotated to request injection of the service CA bundle, and configure ingress controller deployments to mount this configmap and use it instead of using the service account token to get the service CA.
assets/router/deployment.yaml: Add a volume and volume mount for the new configmap, and set theDEFAULT_DESTINATION_CA_PATHenvironment variable to configure openshift-router to use the bundle from the configmap.pkg/manifests/bindata.go: Regenerate.pkg/operator/controller/ingress/controller.go(ensureIngressController): CallensureServiceCAConfigMaponce theopenshift-ingressnamespace has been created and before creating the deployment.pkg/operator/controller/ingress/serviceca_configmap.go: New file.(
ensureServiceCAConfigMap): New method. Ensure the configmap for the service CA bundle exists.(
desiredServiceCAConfigMap): New function. Return the desired configmap for the service CA bundle.(
currentServiceCAConfigMap): New method. Return the current configmap for the service CA bundle.(
updateServiceCAConfigMap): New method. Update the service CA configmap bundle if the expected annotation is missing.pkg/operator/controller/names.go(ServiceCAConfigMapName): New function. Return the name of the configmap for the service CA bundle.