-
Notifications
You must be signed in to change notification settings - Fork 191
ARO Fips Mode Installation #1732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
m1kola
left a 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 good so far. Left few comments which might be useful.
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.
- Do we need to expose this to the customers?
- If the answer to the above is "yes", then we need to update API spec in https://github.com/Azure/azure-rest-api-specs if we are still allowed to do so. Or create a new API version if not
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.
I suspect this will be breaking change, would be good to raise POC PR into repo @m1kola raised and confirm. Tell them not to merge, but ask a question.
This might need to be ARM feature flag for now.
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.
Azure/azure-rest-api-specs#16028 on my todo list to follow up on this to determine what steps are needed for breaking change approval. I know @cadenmarchese is in a similar boat as myself with L series support.
mjudeikis
left a 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.
Overall good start.
Please move CLI too separate PR as that side is not yet ready and is not a blocker to deliver this feature.
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.
I suspect this will be breaking change, would be good to raise POC PR into repo @m1kola raised and confirm. Tell them not to merge, but ask a question.
This might need to be ARM feature flag for now.
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.
Same as Admin API - no bools.
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.
Overall MSFT does not like abbreviation on the API but this might fly... Need POC pr for this too :)
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.
I am not sure what to expect, but here goes nothing Azure/azure-rest-api-specs#16028
eaabc8a to
086b264
Compare
a191438 to
ab7b645
Compare
1252085 to
6aa2c87
Compare
6aa2c87 to
77f691a
Compare
applied changes
nilsanderselde
left a 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 good, just found one typo
77f691a to
7884fbd
Compare
Which issue this PR addresses:
Fixes https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/10901527/
What this PR does / why we need it:
Exposes a Cluster Profile parameter named
FipsValidatedModuleswhich is sent to the installer to determine if FIPS mode is turned on for cluster.Test plan for issue:
Added frontend tests, defaults tests, e2e tests checking database document and machineconfigs for FIPS, and manual verification process:
Setup basic vars and create network:
Next create the SP, and note the values and assign therem
Give network contributor permissions on SP, and generate JSON payload with FipsEnabled in ClusterProfile
data.json:
Send payload to local RP to build cluster
Once cluster is built, we can query for existence of
FipsValidatedModulesin ClusterProfile:Validate adminapi as well:
Now we can manually test that sysctl fips_enabled is true:
Is there any documentation that needs to be updated for this PR?
Will provide document updates when this is released via az cli.