-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Azure IoT CA functionality #4804
Conversation
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 LGTM. A couple comments. Also, are you the new POC for IoT commands?
.gitignore
Outdated
*.key | ||
*.cer | ||
*.pem | ||
*.pvk |
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.
Why are these added?
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 is so that test certs that are created in the repo during tests aren't accidentally checked in if they fail to get deleted. I can simplify this to only 2 types instead of 4.
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.
There are cer files and pem files checked in with batch and keyvault tests, which they treat as test resource. Better not to exclude these file at the global level. You can add .gitignore
to iot folder to narrow the scope.
23:56 $ find . -name '*.cer'
./src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/tests/data/batchtest.cer
./src/command_modules/azure-cli-network/azure/cli/command_modules/network/tests/test-root-cert.cer
23:56 $ find . -name '*.pem'
./env/lib/python3.5/site-packages/certifi/cacert.pem
./env/lib/python3.5/site-packages/certifi/old_root.pem
./env/lib/python3.5/site-packages/certifi/weak.pem
./env/lib/python3.5/site-packages/pip/_vendor/requests/cacert.pem
./src/azure-cli-core/azure/cli/core/tests/sp_cert.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/import_pem_encrypted_pwd_123.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/import_pem_encrypted_pwd_1234.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/import_pem_plain.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/mydomain.test.encrypted.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/mydomain.test.pem
./src/command_modules/azure-cli-role/azure/cli/command_modules/role/tests/cert.pem
return certificate | ||
|
||
|
||
def _create_test_cert(cert_file, key_file, subject, valid_days, serial_number): |
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.
What is this used for? If only for tests, consider moving it into your test directory so that it doesn't find its way into a command some day.
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.
Yes, this specific function is only used in tests, I'll separate these out.
custom_path.format('iot_hub_certificate_gen_code'), certificate_factory) | ||
cli_command(__name__, 'iot hub certificate verify', custom_path.format('iot_hub_certificate_verify'), | ||
certificate_factory) | ||
cli_command(__name__, 'iot hub certificate update', custom_path.format('iot_hub_certificate_update'), |
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 should be a generic update.
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.
What do you mean by "generic update"?
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.
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.
Gotcha, is this a valid scenario for this generic update command? In this case, the Certificate object is created by providing a friendly name and a certificate string. Once it's created the ONLY thing that can be updated is the entire certificate (meaning uploading an entirely new certificate string). The name cannot be 'updated' nor can any of the properties. Please correct me if I'm wrong, but does the generic update command simply provide a way to craft your update to an object before writing it back to the server?
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.
It wasn't completely clear to me from the description in the doc.
def iot_hub_certificate_factory(_): | ||
from azure.cli.core.commands.client_factory import get_mgmt_service_client | ||
from azure.mgmt.iothub.iot_hub_client import IotHubClient | ||
return (get_mgmt_service_client(IotHubClient).iot_hub_resource, get_mgmt_service_client(IotHubClient).certificates) |
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.
You can simply return get_mgmt_service_client(IotHubClient)
and then based on what you need in your commands, reference .iot_hub_resource
or .certificates
as needed. It would make your custom commands much easier to read.
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.
Okay, let me investigate this!
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.
You need to change the version in setup.py. CI will fail because of this.
cert_list = client[1].list_by_iot_hub(resource_group_name, hub_name) | ||
for cert in cert_list.value: | ||
if cert.name == certificate_name: | ||
raise CLIError('Certificate \'{0}\' already exists. Use \'iot hub certificate update\' to update an existing certificate.'.format(certificate_name)) |
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.
Would just suggest using double quotes CLIError("...")
then you don't need to escape the single quotes and it's clearer.
0.1.15 | ||
++++++ | ||
* Adds support for certificate authorities (CA) and certificate chains. | ||
|
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.
No need to update the version, only 0.1.13 has been released.
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.
Ah yes so move the changelog to 0.1.14.
try: | ||
certificate = certificate.decode("utf-8") | ||
except UnicodeError: | ||
certificate = base64.b64encode(certificate).decode("utf-8") |
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.
Just to confirm, we don't want to encode the certificate with base64 if all the characters are valid unicode?
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.
Yes, that's correct.
Also, include the recordings for your tests please. |
@tjprescott I would note my manager @iluican as the POC for IoT commands going forward as I won't be the only one contributing from our team. Working on addressing your comments now. |
.gitignore
Outdated
*.key | ||
*.cer | ||
*.pem | ||
*.pvk |
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.
There are cer files and pem files checked in with batch and keyvault tests, which they treat as test resource. Better not to exclude these file at the global level. You can add .gitignore
to iot folder to narrow the scope.
23:56 $ find . -name '*.cer'
./src/command_modules/azure-cli-batch/azure/cli/command_modules/batch/tests/data/batchtest.cer
./src/command_modules/azure-cli-network/azure/cli/command_modules/network/tests/test-root-cert.cer
23:56 $ find . -name '*.pem'
./env/lib/python3.5/site-packages/certifi/cacert.pem
./env/lib/python3.5/site-packages/certifi/old_root.pem
./env/lib/python3.5/site-packages/certifi/weak.pem
./env/lib/python3.5/site-packages/pip/_vendor/requests/cacert.pem
./src/azure-cli-core/azure/cli/core/tests/sp_cert.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/import_pem_encrypted_pwd_123.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/import_pem_encrypted_pwd_1234.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/import_pem_plain.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/mydomain.test.encrypted.pem
./src/command_modules/azure-cli-keyvault/azure/cli/command_modules/keyvault/tests/mydomain.test.pem
./src/command_modules/azure-cli-role/azure/cli/command_modules/role/tests/cert.pem
@@ -5,6 +5,9 @@ | |||
|
|||
from azure.cli.core.help_files import helps | |||
|
|||
certificate_help = ('For a detailed explanation of CA certificates in Azure IoT Hub, ' | |||
'see https://docs.microsoft.com/en-us/azure/iot-hub/iot-hub-x509ca-overview') |
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.
You don't need the parenthesis.
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.
Couldn't get this to work without them
@@ -23,16 +23,31 @@ def get_device_id_completion_list(prefix, action, parsed_args, | |||
completer=get_resource_name_completion_list('Microsoft.Devices/IotHubs'), | |||
help='IoT Hub name.') | |||
|
|||
etag_type = CliArgumentType( | |||
None, | |||
help='Entity Tag (etag) of the object.') |
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.
Key this on the same line to make code condense.
None, | ||
help='Entity Tag (etag) of the object.') | ||
|
||
|
||
register_cli_argument('iot hub', 'hub_name', hub_name_type, options_list=('--name', '-n'), | ||
id_part='name') |
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 above. The line limitation is 120.
|
||
helps['iot hub certificate create'] = """ | ||
type: command | ||
short-summary: Create/upload an Azure IoT Hub certificate. |
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.
Why is this command iot hub certificate create
when --path
to file containing the certificate is required?
Looks like this should be iot hub certificate upload
?
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.
It is a bit strange. The command creates a Certificate object in a users IoT Hub. One of the parameters for creation is a Unicode string that represents the certificate. It's not truly uploading a file, but rather we are extracting the file contents if it's valid Unicode and using that as the "certificate" parameter, or base64 encoding the contents and using that string as the param. Additionally, we were trying to keep command name parity with the other operations we own.
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.
Okay I understand.
register_cli_argument('iot hub {}'.format(subgroup), 'hub_name', options_list=('--hub-name',)) | ||
|
||
register_cli_argument('iot device', 'hub_name', hub_name_type) | ||
|
||
register_cli_argument('iot', 'device_id', options_list=('--device-id', '-d'), help='Device Id.', | ||
completer=get_device_id_completion_list) | ||
|
||
# Arguments for 'iot hub certificate' group | ||
register_cli_argument('iot hub certificate', 'certificate_path', options_list=('--path', '-p'), | ||
help='The path to the file containing the certificate.') |
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.
Would be good to add a File completer to this.
@derekbekoe is the version in setup.py supposed to match the most recently released version or the highest one in HISTORY.rst? |
0.1.13 is the latest released right now so in HISTORY.rst, the top entry should be |
7ed0817
to
ce4ca58
Compare
@williexu @troydai @derekbekoe seems like 9097.3 failure is unrelated to this work. However. 9097.4 seems to be a linting failure that I'm not able to repro locally on python 3.6. |
I've restarted 9097.3. I think it's a flaky test. |
I suspect the error for .3 is due to one of the changes in your help.py as it happened again. |
After doing the rebase I'm no longer able to run tests. <clone_root>/src/scripts no longer contains the run_tests and check_style scripts. Seems like they now live in env/scripts. I made the style changes and the linter passes, however many tests seem to be failing suddenly. Using the setup instructions here which had previously been working for me: https://github.com/Azure/azure-cli/blob/dev/doc/configuring_your_machine.md |
Got the tests to pass by going to an earlier python version. Seems like the scripts have been moved though. |
ce4ca58
to
90ba64c
Compare
@andrew-buckley That should be fine. The scripts are added to your python environment upon running |
After pulling the latest changes, run python scripts/dev_setup.py again to get the latest dependencies. |
90ba64c
to
6d578c8
Compare
Hm, I ran through those steps (https://github.com/Azure/azure-cli/blob/dev/doc/configuring_your_machine.md) a few times and again after going down to 3.5.4 but still didn't have any luck. |
Seems like *.3 is still failing with the same error. I tried removing the single quotes from the help text but it doesn't seem like that helped. Is there a better way to narrow down this problem? Maybe locally, so that I don't have to push updates and wait for a build job to run in order to do trial and error? |
@andrew-buckley our doc is a bit behind. sorry for that. To run the style check locally:
|
@andrew-buckley after the steps @troydai listed, run |
@williexu thanks for that. I tried those steps but they're passing for me. Although I'm running python 3.5, I'll try on 2.7 as it seems that's the version it's failing for. |
Having a bit of trouble getting setup going with python 2.7 following the instructions in the doc as well as those described above by @troydai. |
Which part? did you create and activate a virtual environment using python 2? |
I'm rather new to python so apologies if I'm clearly doing something wrong here. I'm having trouble creating the virtual environment. I tried uninstalling python 3.5 from my machine:
|
FYI I'm attempting to install and use virtualenv as it seems like venv only ships with python 3.3+ |
the |
…mands.py. Moves test utilities. Adds a file completer.
6d578c8
to
3a1d149
Compare
2fbb7d3
to
0260755
Compare
0260755
to
f29a815
Compare
@williexu please take a look when you can. All checks have passed. |
LGTM |
vmss: support basic tier of vms (Azure#4847) Azure IoT CA functionality (Azure#4804) * Adds support for X.509 Certificates in IoT Hub. * Modifies release notes and changes help link. * Cleans up .gitignore, and HISTORY.rst. Passes a single factory in commands.py. Moves test utilities. Adds a file completer. * Updates style after rebase based on additional linter restrictions. Reserved Instance cli public PR (Azure#4838) Handle dashes in extension names better (Azure#4839) Fix `az aks get-credentials` on Windows (Azure#4762) * Fix `az aks get-credentials` on Windows * Move k8s file merge logic to helper function appservice: support assign managed service identity to webapp/functionapp (Azure#4837) Extension examples small improvement (Azure#4852) Fixing appservice list-locations (Azure#4846) Add extension name to telemetry and UA header (Azure#4854) Fix Azure#4825. (Azure#4853) Extensions `az extension add` --yes param as flag (Azure#4858) Fix issue Azure#2752. (Azure#4859)
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Command Guidelines
(see Authoring Command Modules)