METAL-1105: support specifying CA to verify BMC connections #10072
Conversation
Signed-off-by: Zou Yu <[email protected]>
openshift-ci-robot
added
the
jira/valid-reference
|
@dtantsur: This pull request references METAL-1105 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @zaneb |
data/data/bootstrap/baremetal/files/etc/containers/systemd/ironic.container.template
Outdated
Show resolved
Hide resolved
data/data/bootstrap/baremetal/files/usr/local/bin/build-ironic-env.sh
Outdated
Show resolved
Hide resolved
data/data/bootstrap/baremetal/files/usr/local/bin/build-ironic-env.sh
Outdated
Show resolved
Hide resolved
Signed-off-by: Dmitry Tantsur <[email protected]>
Signed-off-by: Dmitry Tantsur <[email protected]>
Signed-off-by: Dmitry Tantsur <[email protected]>
|
/retest |
|
/retest OFCIR migration |
|
/verified by @dtantsur I have tested #10072 + openshift/cluster-baremetal-operator#527 + openshift/ironic-image#691 with the help of openshift-metal3/dev-scripts#1812 on a local environment. Neither install-config nor BMH definitions contain |
openshift-ci-robot
added
the
verified
|
@dtantsur: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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 |
openshift-ci
bot
added
the
approved
zaneb
left a comment
zaneb
left a comment
There was a problem hiding this comment.
This looks correct, but can be simplified so that you only have to deal with the data in 2 places instead of 3.
|
|
||
| # This is required since the progress service (https://github.com/openshift/installer/blob/dd9047c4c119e942331f702a4b7da85c60042da5/data/data/bootstrap/files/usr/local/bin/report-progress.sh#L22-L33), | ||
| # usually dedicated to creating the bootstrap ConfigMap, will fail to create this configmap in case of bootstrap-in-place single node deployment, | ||
| # usually dedicated to creating the bootstrap ConfigMap, will fail to create this configmap in case of bootstrap-in-place single node deployment, |
There was a problem hiding this comment.
nit: unintentional whitespace change?
| &tls.ServiceAccountKeyPair{}, | ||
| &tls.JournalCertKey{}, | ||
| &tls.IronicTLSCert{}, | ||
| &tls.BMCVerifyCA{}, |
There was a problem hiding this comment.
Since you already have BMCVerifyCA in the PlatformData, instead of doing this you could just create a file at data/data/bootstrap/baremetal/files/opt/openshift/bmc-ca/verify_ca.crt.template with the content:
{{ .PlatformData.BareMetal.BMCVerifyCA }}
Then you wouldn't need the BMCVerifyCA asset at all, and its one line of code could be rolled straight into the BMCVerifyCAConfigMap asset.
There was a problem hiding this comment.
Hmm, I need to digest this. I guess one issue is that we'll get an empty file by default, which may confuse Ironic. Can I make a file creation conditional?
There was a problem hiding this comment.
You already made mounting the volume conditional.
|
@zaneb if the general logic is okay, can we continue this discussion in a follow-up work? Testing each change is a bit cumbersome, and we're so close to FF. |
openshift-merge-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
verified
openshift-merge-robot
removed
the
needs-rebase
|
/lgtm |
|
/test e2e-aws-ovn |
|
/test e2e-aws-ovn Repo problems |
|
/verified later @dtantsur I don't have reasons to believe that a rebase caused the feature to get broken, but I cannot verify it now because building images in dev-scripts is currently broken |
openshift-ci-robot
added
verified-later
verified
|
@dtantsur: This PR has been marked to be verified later by 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-ovn I see improvements locally, so hopefully the repos will work this time. |
|
/test e2e-aws-ovn |
|
@dtantsur: 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-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit e22009c
into
openshift:main
6 participants
Implements the most important parts of https://github.com/openshift/enhancements/blob/master/enhancements/security/bmc-ca-certificate-support.md
by creating a new asset that conditionally generates a ConfigMap in the openshift-machine-api namespace.
Co-Authored-By: Zou Yu [email protected]
Assisted-By: Claude Code (commercial license)