Skip to content
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

[IoT] Add private link and private end point paramaters to IoT Central CLI #22273

Merged
merged 2 commits into from
May 20, 2022

Conversation

hmmorales
Copy link
Member

@hmmorales hmmorales commented May 4, 2022

Description
Changing the existing IoT Central app to use private endpoint connection and private link network features

Testing Guide
azdev test test_iot_central_commands --live --discover
azdev style iot
azdev linter iot
..

History Notes

[IoT] az iot central app private-link-resource list: Add a new command to support listing private link resources
[IoT] az iot central app private-endpoint-connection show: Add a new command to support showing details of a private endpoint connection of the IoT Central app
[IoT] az iot central app private-endpoint-connection approve: Add a new command to support approving a private endpoint connection for the IoT Central app
[IoT] az iot central app private-endpoint-connection reject: Add a new command to support rejecting a private endpoint connection for the IoT Central app
[IoT] az iot central app private-endpoint-connection delete: Add a new command to support deleting a private endpoint connection for the IoT Central app


This checklist is used to make sure that common guidelines for a pull request are followed.

@ghost ghost added the Auto-Assign Auto assign by bot label May 4, 2022
@ghost ghost requested a review from yonzhan May 4, 2022 21:46
@ghost ghost assigned zhoxing-ms May 4, 2022
@ghost ghost added IoT IoT/CLI labels May 4, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented May 5, 2022

IoT

@hmmorales
Copy link
Member Author

Hello, I am trying to include private endpoint connection and private link service into the iot central app cli and am following the PR #12383 as a template. Is this the correct direction to go? I noticed the document https://github.com/Azure/azure-cli/blob/dev/doc/private_endpoint_connection_command_guideline.md which was revised after that linked PR shows another way to include the desired service.

Are the steps taken in the storage PR deprecated?

@hmmorales
Copy link
Member Author

hmmorales commented May 6, 2022

I am currently trying to just test out the command az iot central app private-link-resource list in the current state of my repo but it shows:
image
when i run azdev test test_iot_central_commands --live --discover I am not really sure why this is

Comment on lines 197 to 212
with self.command_group('iot central app private-endpoint-connection', private_endpoint_sdk,
is_preview=True,
resource_type=ResourceType.MGMT_IOTCENTRAL, min_api='2021-11-01-preview') as g: # removed param: custom_command_type=private_endpoint_custom_type,
from ._validators import validate_private_endpoint_connection_id
g.command('delete', 'delete', confirmation=True, validator=validate_private_endpoint_connection_id)
g.show_command('show', 'get', validator=validate_private_endpoint_connection_id)
g.custom_command('approve', 'approve_private_endpoint_connection',
validator=validate_private_endpoint_connection_id)
g.custom_command('reject', 'reject_private_endpoint_connection',
validator=validate_private_endpoint_connection_id)

with self.command_group('iot central app private-link-resource', private_link_resource_sdk,
resource_type=ResourceType.MGMT_IOTCENTRAL) as g:
from azure.cli.core.commands.transform import gen_dict_to_list_transform
g.command('list', 'list_private_link_resources', is_preview=True, min_api='2021-11-01-preview',
transform=gen_dict_to_list_transform(key="value"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At present, this code has the following errors, resulting in iot central app private-endpoint-connection and iot central app private-link-resource can not being loaded normally:

  1. The python SDK package azure-mgmt-iotcentral corresponding to ResourceType.MGMT_IOTCENTRAL does not support multiple api-version folders, so please remove the declaration of min_api='2021-11-01-preview' and the declaration of ResourceType.MGMT_IOTCENTRAL: '2018-09-01' in the _shared.py file

  2. There is no method list_private_link_resources() in _private_links_operations.py, but only method list()
    code link

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented May 7, 2022

I noticed the document https://github.com/Azure/azure-cli/blob/dev/doc/private_endpoint_connection_command_guideline.md which was revised after that linked PR shows another way to include the desired service.

As for private endpoint connection related question, @necusjz could you please help have a look?

@necusjz
Copy link
Member

necusjz commented May 9, 2022

@hmmorales, IMO, you could follow PR #12383 as a template. And there is no need to modify the network module, usually it's just to make the development easier within network module (reference: #19908).

@hmmorales
Copy link
Member Author

I neglected to run the command azdev setup -c after I had updated the files requirements.py3.*.txt and setup.py. Now when I run azdev test test_iot_central_commands --live --discover I am seeing
image

I am not sure how to address this error, I assume that since I am using an updated python sdk I have to run the azdev setup -c command but that seemed to cause this decoding error

@zhoxing-ms
Copy link
Contributor

@hmmorales This problem seems to be caused by you modifying the default output format. Our tests parse the result value of the commands in JSON format. If the default output format is modified to other formats(such as table), such problems in the screenshot will occur.
Please use the az config set core.output=json command to change the output format back to json, and then try the test again

@hmmorales
Copy link
Member Author

@hmmorales This problem seems to be caused by you modifying the default output format. Our tests parse the result value of the commands in JSON format. If the default output format is modified to other formats(such as table), such problems in the screenshot will occur. Please use the az config set core.output=json command to change the output format back to json, and then try the test again

I am receiving this response when I run the command az config set core.output=json
image

and after running the test script again I receive the same error response printed earlier

@zhoxing-ms
Copy link
Contributor

@hmmorales Could you confirm that the return result of iot central app update command is in JSON format?

@hmmorales
Copy link
Member Author

@zhoxing-ms I included a small print statement in azure-cli-testsdk/azure/cli/testsdk/base.py to print what is causing the error image
and it seems like the type that is printed is None
image

@zhoxing-ms
Copy link
Contributor

@hmmorales I guess there may be a problem with the output format of the command itself. Could you execute the command separately in the dev environment and then show me the output of this command?

@hmmorales
Copy link
Member Author

hmmorales commented May 11, 2022

@zhoxing-ms

I tested two things

  • latest Dev Branch with older version of azure-mgmt-iotcentral=9.0.0
  1. I checked out the latest dev branch (again this branch does not have the most recent azure-mgmt-iotcentral, so below test is using version 9.0.0)
  2. ran azdev setup -c
  3. ran azdev test test_iot_central_commands --live --discover

and received a passed
image

  • latest Dev Branch with updated version of azure-mgmt-iotcentral=10.0.0b1
  1. In the latest dev branch I updated azure-mgmt-iotcentral in requirements.py.Darwin.txt, requirements.py.Linux.txt, requirements.py.windows.txt, and setup.py to azure-mgmt-iotcentral=10.0.0b1
  2. ran azdev setup -c
  3. ran azdev test test_iot_central_commands --live --discover

and received the same decoding error response in az iot central update as earlier
image

I find it very odd how updating azure-mgmt-iotcentral creates this error... I think I may be updating the listed txt files or the setup file incorrectly. I did a somewhat unrelated test as a sanity check with a few iot central commands to ensure they are working as shown below
image
Then I ran az iot central app update -n my-sample-app123 -g myResourceGroup --set displayName=my-sample-app123updated subdomain=my-sample-app123updated and printed the resource
image

Not sure how to move past this, any help would be greatly appreciated

@hmmorales
Copy link
Member Author

hmmorales commented May 11, 2022

@zhoxing-ms I wanted to provide some additional information with the problem I am seeing. I found that within the funtion def _in_process_execute(...) in the file src/azure-cli/azure-cli-testsdk/azure/cli/testsdk/base.py there is an stdout_buf that gets populated which then updates self.output which is then used to load the decoder in def get_output_in_json(...) For some reason when I update the version to azure-mgmt-iotcentral=10.0.0b1 the return value for stdout_buf.getvalue() is equal to None despite the variable command being equal to some string value such as iot central app update -n iotc-cli-testv3nr5dj7r7c-template -g clitest.rgmrkj4o4cw7woyqzb2peamrb3cq3f3jegpjzsz2wyfun4z4u2toxmkvz25j3mi2eui --set displayName=iotc-cli-testv3nr5dj7r7cupdate subdomain=iotc-cli-testv3nr5dj7r7cupdate sku.name=ST2 I included an image of the function for reference with some print statements I used to verify these values
image

The print statements when the first create command is tested:
image

The print statements when the update command is tested
image

The line *+*+*+*+*+*+*+ The value of self.output within in_process_execute is: in both images shows what self.output is and you can see it is a json object during the create command but it is None in the update command. I hope this additional information is helpful in diagnosing the issue.

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented May 12, 2022

@hmmorales This problem should be caused by the breaking change of the SDK package azure-mgmt-iotcentral.

In the SDK version 9.0.0, the get_long_running_output() method of _apps_operations.begin_create_or_update() will return deserialized by default. code link

But in the SDK version 10.0.0b1, the get_long_running_output() method of _apps_operations.begin_create_or_update() will not return deserialized anymore. code link

As for Python SDK related issue, you can ask @msyyc for help

@zhoxing-ms
Copy link
Contributor

@hmmorales Could you please address those comments?

@zhoxing-ms zhoxing-ms changed the title [IoT] Add private link and private end point paramaters to IoT Central CLI [IoT] Add private link and private end point paramaters to IoT Central CLI May 17, 2022
@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented May 17, 2022

@hmmorales Please refer to submitting-pull-requests to modify the PR description to add the specific information of new commands and parameters. such as
Screenshot 2022-05-17 205133

@hmmorales
Copy link
Member Author

hmmorales commented May 17, 2022

Disregard, the 'Reverting merge' part of the last commit. I just had to pull in the most recent edits to dev. Additionally after I address the remaining PR comments I may need some more time for my team to verify the --help message text before the PR can be merged

@wangzelin007
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@zhoxing-ms
Copy link
Contributor

Disregard, the 'Reverting merge' part of the last commit. I just had to pull in the most recent edits to dev. Additionally after I address the remaining PR comments I may need some more time for my team to verify the --help message text before the PR can be merged

@hmmorales May I ask when can we merge this PR?

@@ -1351,7 +1351,7 @@ def iot_central_app_list(client, resource_group_name=None):


def iot_central_app_update(client, app_name, parameters, resource_group_name):
return client.apps.begin_update(resource_group_name, app_name, parameters)
return client.apps.begin_create_or_update(resource_group_name, app_name, parameters)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for switching this over to the CreateOrUpdate API?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_long_running_output here in the updated iotcentral api doesn't return a deserialized json which caused decoding issues as mentioned earlier. The get_long_running_output here in begin_create_or_update does return a deserialzied json thus why I went with it. I suspect this is a bug in the new api.

"Type must be Microsoft.IoTCentral/iotApps")
elif connection_id:
id_list = connection_id.split('/')
if id_list[id_list.index('providers') + 1].lower() != 'microsoft.iotcentral':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there not some helper methods that parse the resourceId for us?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I can find..

src/azure-cli/azure/cli/command_modules/iot/custom.py Outdated Show resolved Hide resolved
@hmmorales
Copy link
Member Author

Disregard, the 'Reverting merge' part of the last commit. I just had to pull in the most recent edits to dev. Additionally after I address the remaining PR comments I may need some more time for my team to verify the --help message text before the PR can be merged

@hmmorales May I ask when can we merge this PR?

I am having a few other devs on my team review the PR, I will send a follow up when it is ready to merge

@wangzelin007
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@zhoxing-ms
Copy link
Contributor

@hmmorales Could you please address those conflicts and CI issues? Please note that if you do not address them in time by tomorrow, this PR will not catch up with the release of this sprint, and we will have to postpone it to the release of next sprint (07-05)

@zhoxing-ms
Copy link
Contributor

As for the CI issue, the test_iot_central_private_link_and_private_endpoint is a new test introduced by this PR. I believe this test needs to be re-recorded after upgrading the api-version of Network #22487.
@hmmorales Please re-record this test on live mode and then push the updated yaml file

@hmmorales hmmorales force-pushed the update-cli-for-private-link branch from f67f147 to fde5920 Compare May 19, 2022 21:05
@hmmorales
Copy link
Member Author

hmmorales commented May 19, 2022

@zhoxing-ms It looks like all checks passed, PR should be good to merge 😄

@zhoxing-ms zhoxing-ms merged commit 52ceb02 into Azure:dev May 20, 2022
@hmmorales hmmorales deleted the update-cli-for-private-link branch May 20, 2022 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants