Bug 1703941: use image-registry-certificates for system and user certificates #272
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
size/L
was this a recent change? we had tested this previously right? @adambkaplan does this affect the configmap that the buildcontroller creates for builds, also? |
|
/hold @bparees this affects builds as well. Observing this in my testing. @openshift/sig-auth is the API for the annotation frozen? If possible we shouldn't ship GA code with beta APIs. |
openshift-ci-robot
added
the
do-not-merge/hold
| } | ||
|
|
||
| cm.Data[internalHostname] = cert | ||
| } else { |
There was a problem hiding this comment.
+1 - @dmage this will also help us drop the port on the internal registry's service.
|
This looks like the right direction, thanks @dmage
(the node-ca pods will pick up the updated value from (2) automatically since they just reread it periodically) |
|
/hold cancel Annotation will be using |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@bparees yes
|
if we miss the event(for example, because we were down at the time), will we also detect the change on a relist? everything else sounds good. |
|
@bparees yes, we will. |
dmage
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
This PR removes support of |
|
/retest |
|
/assign @adambkaplan |
yeah let's not rock the boat if we don't have to. |
| else | ||
| rm /etc/docker/certs.d/image-registry.openshift-image-registry.svc.cluster.local:5000/service-ca.crt | ||
| rm /etc/docker/certs.d/${internalRegistryHostname}/service-ca.crt | ||
| fi |
There was a problem hiding this comment.
@dmage If I read the PR correctly - we don't need this any more because the ServiceCA ConfigMap is created first, and then the certificate data is copied into the registry's "global" CA keyed on the internal registry hostname.
There was a problem hiding this comment.
right, but we don't put image-registry.openshift-image-registry.svc.cluster.local:5000 to this configmap.
There was a problem hiding this comment.
per @bparees - let's restore this for now. We can revisit in 4.2 when we hopefully are able to drop the port number.
|
@dmage I assume we already have logic that forces a rollout of the nodeca dameon when the ConfigMap containing the CAs is updated? |
|
@adambkaplan afaik kubelet will update pods filesystem and we do not need to restart it. |
|
@dmage is correct. the node-ca run-script just re-reads the filesystem every minute or so. And kube will update the filesystem when the configmap changes. |
|
@dmage so we're only worried about the registry pods, which per your previous comment is already taken care of. Latest update adds the local service name to the mounted ConfigMap. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, dmage 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 |
dmage
changed the title
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
New changes are detected. LGTM label has been removed. |
|
/retest |
A config map with the annotation
service.beta.openshift.io/inject-cabundlecan contain onlyservice-ca.crt. We need to combine user-provided certificates with this service-ca.crt save the result asimage-registry-certificates.https://bugzilla.redhat.com/show_bug.cgi?id=1703941