OCPBUGS-19625: Multus per-node certificate request #2009
Conversation
|
/hold |
openshift-ci
bot
added
do-not-merge/work-in-progress
openshift-ci
bot
added
the
approved
dougbtv
force-pushed
the
csr-request-update
branch
from
5b82043 to
53e6b84
Compare
|
Hi
|
dougbtv
force-pushed
the
csr-request-update
branch
6 times, most recently
from
141349b to
8c17e36
Compare
|
/retitle Multus per-node certificate request |
openshift-ci
bot
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
dougbtv
force-pushed
the
csr-request-update
branch
2 times, most recently
from
76b1988 to
c579770
Compare
|
/retitle OCPBUGS-19625: Multus per-node certificate request |
openshift-ci
bot
changed the title
openshift-ci-robot
added
jira/severity-critical
|
@dougbtv: This pull request references Jira Issue OCPBUGS-19625, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest-required |
1 similar comment
|
/retest-required |
bindata/network/multus/multus.yaml
Outdated
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: spec.nodeName | ||
| - name: MULTUS_NODE_NAME |
There was a problem hiding this comment.
CI failure:
level=error msg=Cluster operator network Degraded is True with ApplyOperatorConfig: Error while updating operator configuration: could not apply (apps/v1, Kind=DaemonSet) openshift-multus/multus: failed to apply / update (apps/v1, Kind=DaemonSet) openshift-multus/multus: failed to create typed patch object (openshift-multus/multus; apps/v1, Kind=DaemonSet): .spec.template.spec.containers[name="kube-multus"].env: duplicate entries for key [name="MULTUS_NODE_NAME"]
It might be related to the fact that Dan already added this variable here: #2020
There was a problem hiding this comment.
ok, yep, I think it needs a rebase likely. good eye
bindata/network/multus/002-rbac.yaml
Outdated
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: multus-proper |
There was a problem hiding this comment.
This seems to be enabled unconditionally but we should allow for the fallback option that would disable the per-node cert/webhook.
See here: https://github.com/openshift/cluster-network-operator/pull/1983/files#diff-cd68b4caec2e8a6afb4c120d980469834c62524b90a300f2adf90b6d6bec0c55R49-R58
And: https://github.com/openshift/cluster-network-operator/pull/1983/files#diff-13e60e30d72f7b5c263fabc48ae9533c32531df3983d7e70d8a4e1a0a1c1a97dR25-R28
There was a problem hiding this comment.
Good call. Added conditions for RBAC + serviceaccount + serviceaccountname, as well as multus configuration to disable cert per node mode.
| @@ -299,6 +313,9 @@ spec: | |||
| items: | |||
| - key: daemon-config.json | |||
| path: daemon-config.json | |||
| - name: host-run-multus-certs | |||
| hostPath: | |||
| path: /run/multus_certs | |||
There was a problem hiding this comment.
This will be removed if a node reboots right?
I don't think we want to wipe the certs if they are still valid.
There was a problem hiding this comment.
Great observation. Turns out that we'll need a modification to ensure that we re-use it on the Multus end instead of re-request. This will certainly be an improvement, but we'll address it as a follow-on item.
| [{ | ||
| "commonNamePrefix":"system:multus", | ||
| "allowedPodAnnotations": ["k8s.v1.cni.cncf.io/network-status"] | ||
| }] |
| @@ -121,6 +121,11 @@ data: | |||
| "logToStderr": true, | |||
| "logLevel": "verbose", | |||
| "binDir": "{{ .CNIBinDir }}", | |||
| "perNodeCertificate": { | |||
There was a problem hiding this comment.
Looks like currently multus hard-codes cert-duration to 10min: https://github.com/k8snetworkplumbingwg/multus-cni/pull/1159/files#diff-562bab31c287044ff5fd9a5ba21041103225e129ca4373634b389356bdce0070R101
I think that the cert rotation shouldn't happen that often in a production environment.
In comparison, kubelet and ovn-kube use 24h.
There was a problem hiding this comment.
Chatted with @knobunc and we're going to submit this as a follow on due to the dependent change in Multus. Great eye, though, this is necessary. 💯
dougbtv
force-pushed
the
csr-request-update
branch
5 times, most recently
from
2c8bbb4 to
0fbefec
Compare
|
/lgtm |
|
/test ci/prow/e2e-aws-sdn-multi |
|
@abhat: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
|
/test e2e-aws-sdn-multi |
dougbtv
force-pushed
the
csr-request-update
branch
from
0fbefec to
741c68a
Compare
|
/lgtm |
This introduces configurations for multus per-node certification.
dougbtv
force-pushed
the
csr-request-update
branch
from
741c68a to
5a97ce5
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dougbtv, jcaamano, knobunc 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 |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@dougbtv: The following tests failed, say
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. |
|
@dougbtv: Jira Issue OCPBUGS-19625: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged: These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-19625 has not 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. |
|
/retest-required |
|
Fix included in accepted release 4.15.0-0.nightly-2023-09-27-073353 |
No description provided.