WIP: Add FIPS to MachineConfig#800
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters 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 |
openshift-ci-robot
added
the
approved
| cmd := "/usr/libexec/rhcos-tools/coreos-fips" | ||
| args := []string{arg} | ||
| dn.logSystem("Running %s %v", cmd, args) | ||
| return exec.Command(cmd, args...).Run() |
There was a problem hiding this comment.
This looks like the right command to me. One thing to keep in mind is that, within OCP, the coreos-fips will not default to updating the kernel args and, instead, defer to MCO to own those args.
There was a problem hiding this comment.
Let's debate that in openshift/rhcos-tools#2 since...it seems to me doing it that way would make everything unnecessarily more complex.
There was a problem hiding this comment.
I closed the PR which would offload kernel args to the MCO. This call should be good as is.
|
@cgwalters: PR needs rebase. DetailsInstructions 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
needs-rebase
|
|
||
| // updateFIPS handles changes in FIPS | ||
| func (dn *Daemon) updateFIPS(oldConfig, newConfig *mcfgv1.MachineConfig) error { | ||
| if oldConfig.Spec.FIPS != newConfig.Spec.FIPS { |
There was a problem hiding this comment.
I know this is wip, just leaving also a comment that we still miss a call to updateFIPS 💪
| // It sorts all the configs in increasing order of their name. | ||
| // It uses the Ignition config from first object as base and appends all the rest. | ||
| // Kernel arguments are concatenated. | ||
| // FIPS uses the last specified value. |
There was a problem hiding this comment.
I think this will be confusing for users, especially because unset values will decode into false. Like, the following would have FIPS mode OFF:
metadata:
name: 00_config_controller
spec:
config: ...
---
metadata:
name: 99_config_fips
spec:
FIPS: true
---
metadata:
name: 99_config_master
spec:
osImageURL: thelatestversionThere was a problem hiding this comment.
We might want to go pointer then to differentiate between set/unset as well?
There was a problem hiding this comment.
Yeah, probably. Another option would be to fail closed, i.e. if ANY configs have FIPS=true, it will be turned on.
5 participants
No description provided.