-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
This pull request should close this issue: #62 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty solid. I have a few requested changes to keep this from breaking older installs.
@@ -25,7 +25,8 @@ def __init__(self, path, footer, rtr_interfaces): | |||
]) | |||
self.cluster = ParameterCollection('cluster', 'Cluster Configuration', [ | |||
PasswordParameter('ADMIN_PASSWORD', 'Adminstrator Password'), | |||
PasswordParameter('PULL_SECRET', 'Pull Secret') | |||
PasswordParameter('PULL_SECRET', 'Pull Secret'), | |||
Parameter('FIPS_MODE', 'Fips Mode') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter should be of BooleanParameter type. Also Fips should be FIPS as it is an acronym.
BooleanParameter('FIPS_MODE', 'FIPS Mode', 'False')
Note that False is of string type. This will set the default value to False.
app/inventory.py
Outdated
@@ -210,6 +210,7 @@ def main(config, ipam, inv): | |||
extra_nodes=extra_nodes, | |||
ignored_macs=config['IGNORE_MACS'], | |||
dns_forwarders=[item['server'] for item in json.loads(config.get('DNS_FORWARDERS', '[]'))], | |||
fips_mode=config['FIPS_MODE'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.get('FIPS_MODE', 'False') == 'True'
This will account for legacy configs without a FIPS_MODE parameter.
@rmkraus The requested changes have been committed. Thanks. |
Do you think this should also enable FIPS mode on the bastion node? Obviously, that's tricky because a restart is required. Alternatively, this could just check if FIPS is enabled and warn if it is not. Or we just make a not in the docs that this switch does not change the FIPS status of the bastion. I'm not really sure what is best. What do you think? |
I'm still fairly new to this project. Is there other existing code that makes changes to the bastion node? If so, then we could open another issue to work on enabling FIPS there and add a note stating that the current code base does not configure FIPS on bastion node. If there is no existing code that makes changes to the bastion node, then in my mind it makes less sense to add an issue to work on that. Mainly because it expands the scope of this project. Alternatively, we could make an opinionated choice on some other hardening project and point folks that way if they desire a hardened or FIPS enabled bastion node? |
There is code that makes changes to the bastion node (installs packages, sets up DNS, etc), but I also think we should keep the scope on this code change small. We need this specific change sooner rather than later, but we could follow up to add a FIPS on bastion node option later. I vote for leaving the scope of this change as the cluster only and writing another issue for FIPS on bastion. |
👍 We'll have to make sure (when we document) to mention that this option is for the cluster only and FIPS on the bastion should be done at OS install time. That should suffice. |
We're adding a toggle for FIPS mode installation of OCP clusters. I believe we got all the places necessary to add the prompt to the CLI and ensure it results in a
fips: true
orfips: false
within theinstall-config.yaml
.Please let us know if there is something that was missed or a test we should run. We will update the documentation after if this functionality is correct.