Inject the trusted-ca-bundle into openshift-apiserver pods #226
Conversation
openshift-ci-robot
added
the
size/M
stlaz
force-pushed
the
add_trusted_ca_bundle
branch
2 times, most recently
from
ca5b3c7
to
642f3c6
Compare
openshift-ci-robot
added
size/L
stlaz
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
stlaz
force-pushed
the
add_trusted_ca_bundle
branch
5 times, most recently
from
75361fa
to
18216dc
Compare
stlaz
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
manifests/0000_30_openshift-apiserver-operator_07_deployment.yaml
Outdated
Show resolved
Hide resolved
pkg/operator/workloadcontroller/workload_controller_openshiftapiserver_v311_00.go
Outdated
Show resolved
Hide resolved
pkg/operator/workloadcontroller/workload_controller_openshiftapiserver_v311_00.go
Outdated
Show resolved
Hide resolved
| @@ -101,6 +134,12 @@ spec: | |||
| - name: serving-cert | |||
| secret: | |||
| secretName: serving-cert | |||
| - name: trusted-ca-bundle | |||
There was a problem hiding this comment.
Yes, given a message from proxy people yesterday, non-optional CMs like this may prolong upgrades since the network operator may start injecting the CMs too late
There was a problem hiding this comment.
Right answer was no :-) We want to require this.
There was a problem hiding this comment.
Apparently yes was still the good answer :)) There's people who want us to have it optional for the reasons above, this is now handled by bash guard in the pod's commands so that you don't destroy your system trust store if the configmap is missing (shouldn't be) or empty
There was a problem hiding this comment.
optional is needed in the case where your operator starts before the cluster network operator that responds to the cm injection request.
stlaz
force-pushed
the
add_trusted_ca_bundle
branch
from
18216dc
to
898de82
Compare
|
/test e2e-aws |
|
/test e2e-aws |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
@deads2k @sttts just so we are clear... this add mechanism that will force all three openshift apiservers to restart at the same time without rolling new revision out (daemonset). I'm fine with it, because this just substitute the dynamic cert reloading. From what I tested manually, the restart is pretty fast, but aggregator might notice it. |
|
The tests passing now only proves that the CM injection is kind of flawed, will add a two step process to make sure the CM always has data not to wedge the cluster by deleting its system trust store when mounting a CM with no data... |
stlaz
force-pushed
the
add_trusted_ca_bundle
branch
2 times, most recently
from
95e0b08
to
d21939c
Compare
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
From openshift-apiserver logs: “Waiting for watchdog........” |
sttts
force-pushed
the
add_trusted_ca_bundle
branch
from
7250e0d
to
514ab70
Compare
sttts
force-pushed
the
add_trusted_ca_bundle
branch
from
514ab70
to
02b78a7
Compare
| args: | ||
| - "--config=/var/run/configmaps/config/config.yaml" | ||
| - | | ||
| echo $$ > /var/run/watchdog/pid |
There was a problem hiding this comment.
You probably noticed, these dollar signs seem to be escaped, so they won’t be interpreted.
I wonder if we could either use realpath of /proc/self or get the value of either /proc/self/stat{,us}. Starts to be a bit hackish.
There was a problem hiding this comment.
they should be escaped and passed to bash, which then interprets them.
There was a problem hiding this comment.
but I agree that W0827 11:29:32.124287 29 cmd.go:211] Unable to parse pid file /var/run/watchdog/pid: strconv.Atoi: parsing "$": invalid syntax is convincing.
There was a problem hiding this comment.
I’m just assuming given the watchdog reports “/var/run/watchdog/pid: strconv.Atoi: parsing "$": invalid syntax”
There was a problem hiding this comment.
$$$$ does the trick. Is that yaml escaping of dollar?
There was a problem hiding this comment.
Hard to tell, I did not think dollar sign has a special meaning for yaml files :/
There was a problem hiding this comment.
try kubectl run --restart=Never -i -t busybox --image busybox -- /bin/sh -ec 'echo $$$$'
sttts
force-pushed
the
add_trusted_ca_bundle
branch
from
02b78a7
to
f65630c
Compare
sttts
force-pushed
the
add_trusted_ca_bundle
branch
from
f65630c
to
b11c3f0
Compare
|
Upgrade flakes seem unrelated. /retest |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/hold @deads2k had opinions about using the watchdog, I think we should just rollout new deployment when proxy change. |
openshift-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mfojtik, soltysh, stlaz 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 |
|
/hold cancel Alright, we will go with this to unblock 4.2, but there should be BZ created with follow up to revert this approach and just use the existing operator mechanism to rollout new version. |
openshift-ci-robot
removed
the
do-not-merge/hold
|
Follow-up bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=1746375 |
cc @sttts @mfojtik