Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AzExt - Adding support for the v2021-09-01-preview API #1739
AzExt - Adding support for the v2021-09-01-preview API #1739
Changes from 3 commits
0463556
6d6c922
417dad3
fd22ba2
b6451f3
d8d13bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We discussed with @Makdaam that encryption parameters need a clear documentation on docs.microsoft.com.
disk_encryption_set
is providing customer keys and makes customer responsible for managing them. Without this option set, disks will get encrypted with Azure provided and rotated keys.This will be documented later on.
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.
is there reason we use
--master-enc-at-host
and not--master-encryption-at-host
? Matches original flag nameThere 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.
+1 on using full name.
Also I see that
az vm create
uses--encryption-at-host
argument as a boolean flag (either present or not, no value). If I read this code correctly our command is going to be--master-enc-at-host Enabled
.Maybe we also need to use it as boolean for simplicity? If present - send
Enabled
to the API, if not - sendDisabled
.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 had master-encryption-at-host, but that doesn't pass validation since the parameter name is too long.
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.
Ok, found the linter rule, and at least one version of the parameter has to be <22 characters (not counting -- or - in front). I'm wondering if I should shorten the short versions even more.
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.
To sum up:
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.
API Accepts separate disk encryption sets for master and workers, should we not add 2 options here 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.
@mjudeikis API does accept two valeus, but we decided to force worker encrpyion set id match master one for now (see code below).
ARO-RP/pkg/api/v20210901preview/openshiftcluster_validatestatic.go
Lines 292 to 294 in 1af1f2b
I don't worry much about implementation: I'm ok with
aro_create
accepting two separate params.But if we are talking about exposing separate CLI params to the customers - I would be in favour of keeping 1 param for now to avoid confusion. If we decide to relax this validation - we will add a second param (
--disk-encryption-set
and new--master-disk-encryption-set
) or rename existing and add new one (so we have, for example,--worker-disk-encryption-set
and--master-disk-encryption-set
).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.
If we decide to relax I'd leave the old --disk-encryption-set, and add 2 new (one for master, one for worker). Then throw an error if the old one is specified at the same time as one of the new ones. This will keep backwards compatibility if someone uses az aro in a script.
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.
After a discussion, the set() is used to keep code consistency even when there will be one object. This makes code more streamlined.