Add topology Rest API by yanivn3 · Pull Request #3426 · Azure/azure-rest-api-specs

yanivn3 · 2018-07-17T11:45:45Z

Adding topology Rest API for Azure Security Center.
TFS item: https://msazure.visualstudio.com/One/_workitems/edit/2555019

AutorestCI · 2018-07-17T11:46:08Z

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

AutorestCI · 2018-07-17T11:46:42Z

Automation for azure-sdk-for-go

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-go#3017

AutorestCI · 2018-07-17T11:47:16Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

AutorestCI · 2018-07-17T11:48:01Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

AutorestCI · 2018-07-17T11:48:44Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

chlahav · 2018-07-17T15:28:03Z

                }
            }
+        },
+        "/subscriptions/{subscriptionId}/providers/Microsoft.Security/topology": {


missing example

for all the operations

examples added

chlahav · 2018-07-17T15:29:58Z

+                    "type": "string",
+                    "description": "The UTC time on which topology where calculated"
+                },
+                "Resources": {


change to camel case

chlahav · 2018-07-17T15:30:30Z

+        "TopologyResourceProperties": {
+            "type": "object",
+            "properties": {
+                "calculatedDateTime": {


"format": "date-time",

also add "readOnly": true

chlahav · 2018-07-17T15:31:03Z

+            "properties": {
+                "calculatedDateTime": {
+                    "type": "string",
+                    "description": "The UTC time on which topology where calculated"


"The UTC time on which the topology was calculated

chlahav · 2018-07-17T15:31:29Z

+                },
+                "Resources": {
+                    "type": "array",
+                    "description": "Azure resources which re part of this topology resource",


Azure resources which are part of this topology

chlahav · 2018-07-17T16:13:09Z

+        "TopologyResourceName": {
+            "name": "topologyResourceName",
+            "in": "path",
+            "description": "Name of an topology resource group.",


resource group?
isn't it just a resource?

No. Each topology contains list of Azure resource. For example: topology/vms contains all vms belongs to this topology

the term "resource group" is already in use so it might confuse customers, please change the description

chlahav · 2018-07-17T16:14:15Z

+        "/subscriptions/{subscriptionId}/providers/Microsoft.Security/topology": {
+            "get": {
+                "tags": ["Topology"],
+                "description": "Gets a list that allows to build a toplogy view of subscription.",


Gets a list that allows to build a topology view of a subscription.

chlahav · 2018-07-17T16:15:24Z

+        "/subscriptions/{subscriptionId}/providers/Microsoft.Security/locations/{ascLocation}/topology": {
+            "get": {
+                "tags": ["Topology"],
+                "description": "Gets a list that allows to build a toplogy view of subscription and location.",


Gets a list that allows to build a topology view of a subscription and location.

chlahav · 2018-07-17T16:16:44Z

+            "get": {
+                "tags": ["Topology"],
+                "description": "Gets a list that allows to build a toplogy view of subscription and location.",
+                "operationId": "Toplogy_ListByHomeRegion",


Topology_ListByHomeRegion

chlahav · 2018-07-17T16:17:15Z

+            "get": {
+                "tags": ["Topology"],
+                "description": "Gets a list that allows to build a toplogy view of subscription.",
+                "operationId": "Toplogy_List",


Topology_List

chlahav · 2018-07-19T15:19:47Z

@@ -1254,9 +1254,14 @@
        },
        "/subscriptions/{subscriptionId}/providers/Microsoft.Security/topology": {


type names should be plural (topologies)

This is a different case, since we return single topology.

chlahav · 2018-07-19T15:22:39Z

+                            "resources": [
+                                {
+                                    "resourceId": {
+                                        "ID": "/subscriptions/3eeab341-f466-499c-a8be-85427e154bad/resourceGroups/myservers/providers/Microsoft.Network/virtualNetworks/myvnet"


field names should be camel case (id)

Yes. Example was old version. resourceId is a string, not an object.

chlahav · 2018-07-19T15:23:32Z

+						            "location": "westus",
+						            "children": [
+							            {
+								            "resourceId": "/subscriptions/3eeab341-f466-499c-a8be-85427e154bad/resourceGroups/myservers/providers/Microsoft.Network/virtualNetworks/myvnet/subnets/mysubnet"


resource Id should be an object containing the "id" property you also have a test that failed on that

chlahav · 2018-07-19T15:30:16Z

+                    "type": "boolean",
+                    "description": "Indicates if the resource has direct connectivity to the Internet"
+                },
+                "secureScore": {


talked to PMs,
this field name should be changed

chlahav · 2018-07-19T15:30:48Z

+        "TopologyResourceName": {
+            "name": "topologyResourceName",
+            "in": "path",
+            "description": "Name of an topology resource group.",


the term "resource group" is already in use so it might confuse customers, please change the description

chlahav · 2018-07-19T15:32:57Z

please make the "MODE=model PR_ONLY=true" test pass

chlahav

Please address these minor changes

chlahav · 2018-07-24T13:26:00Z

            ]
        },
+        "TopologyList": {
+            "type": "object",


add description

chlahav · 2018-07-24T13:27:02Z

+            }
+        },
+        "TopologyResource": {
+            "type": "object",


add description

chlahav · 2018-07-24T13:27:33Z

+        },
+        "TopologyResourceProperties": {
+            "type": "object",
+            "properties": {


add description

for all the objects below as well, it will make the documentation look better

chlahav · 2018-07-24T13:31:55Z

+                "topologyScore": {
+                    "type": "integer",
+                    "readOnly": true,
+                    "description": "Security score of the resource based on its severity, siblings security status and location in topology (to be used for clustering similar objects together)"


can you change the term security score in the description as well?
I think that "Score of the resource based on its security severity" is good enough

ravbhatnagar

Looks fine. Just few comments. Can be merged based on the reply to my comments.

ravbhatnagar · 2018-07-27T21:52:31Z

+                    "readOnly": true,
+                    "description": "The UTC time on which the topology was calculated"
+                },
+                "resources": {


please rename this property to something else. It can potentially conflict with the Deep PUT common model that ARM team is working on enabling

Changed to topologyResources

ravbhatnagar · 2018-07-27T21:53:45Z

+                    "readOnly": true,
+                    "description": "Indicates if the resource has security recommendations"
+                },
+                "internetFacing": {


bools are generally not recommended, they are less descriptive and have removes any scope of future expansion in case you need more states to be reflected through that property.

Agree. In the UI we actually show this as "Network Zones", changed to networkZones here as well (InternetFacing is one of the options).

ravbhatnagar · 2018-07-27T21:55:37Z

+                    "readOnly": true,
+                    "description": "Indicates if the resource has direct connectivity to the Internet"
+                },
+                "topologyScore": {


How does a customer interpret an integer value for the topologyScore? Is 5 good or does it indicate a problem? what is the range of supported values. String enum better for this? If you need integer, may be add description. can it be an enum?

The goal of the score is for clustering and filtering in UI. Since we have customers with many resources, by default we show resources with higher score (lower score items will be less visible). also, resources with same score may be clustered into same UI object.
This is private preview concept and we may improve the explanation upon feedback from customers (before going public)

ravbhatnagar · 2018-07-31T02:39:52Z

Looks good

azuresdkci · 2018-07-31T02:39:54Z

Can one of the admins verify this patch?

lmazuel · 2018-09-14T18:23:07Z

@chlahav I'm waiting for the fix on the minor changes you asked. Let me know if there is anything I can do.
Thank you

AutorestCI · 2018-10-09T12:02:58Z

Automation for azure-sdk-for-js

Nothing to generate for azure-sdk-for-js

lmazuel · 2018-10-09T19:04:52Z

Please look at the model job for remaining errors:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/439100392

Thanks,

DanaPeled · 2018-10-11T08:57:41Z

Hi
We fixed the remaining errors
@lmazuel
Thanks!

yanivn3 changed the title ~~Add topology Rest API~~ Add topology Rest API [DoNotMerge] Jul 17, 2018

azuresdkci assigned lmazuel Jul 17, 2018

azuresdkci requested a review from lmazuel July 17, 2018 11:47

chlahav self-requested a review July 17, 2018 15:26

chlahav suggested changes Jul 17, 2018

View reviewed changes

lmazuel added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jul 18, 2018

lmazuel requested a review from ravbhatnagar July 18, 2018 19:04

chlahav added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Jul 19, 2018

yanivn3 changed the title ~~Add topology Rest API [DoNotMerge]~~ Add topology Rest API Jul 19, 2018

chlahav suggested changes Jul 19, 2018

View reviewed changes

chlahav approved these changes Jul 24, 2018

View reviewed changes

ravbhatnagar reviewed Jul 27, 2018

View reviewed changes

AutorestCI mentioned this pull request Jul 28, 2018

[AutoPR security/resource-manager] Add topology Rest API Azure/azure-sdk-for-go#2339

Closed

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jul 31, 2018

Add topology Rest API

578fccd

yanivn3 force-pushed the master branch from 82c3ce0 to 578fccd Compare October 9, 2018 12:02

chlahav removed the DoNotMerge <valid label in PR review process> use to hold merge after approval label Oct 9, 2018

Fixing additionalProperties configuration to match type verification

3f243e4

lmazuel merged commit 98f4435 into Azure:master Oct 15, 2018

AutorestCI mentioned this pull request Oct 15, 2018

[AutoPR security/resource-manager] Add topology Rest API Azure/azure-sdk-for-go#3016

Merged

		@@ -1254,9 +1254,14 @@
		},
		"/subscriptions/{subscriptionId}/providers/Microsoft.Security/topology": {

Conversation

yanivn3 commented Jul 17, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AutorestCI commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

AutorestCI commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-ruby

Uh oh!

AutorestCI commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

AutorestCI commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-java

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chlahav commented Jul 19, 2018

Uh oh!

chlahav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yanivn3 commented Jul 17, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Jul 17, 2018 •

edited

Loading

AutorestCI commented Jul 17, 2018 •

edited

Loading

AutorestCI commented Jul 17, 2018 •

edited

Loading

AutorestCI commented Jul 17, 2018 •

edited

Loading

AutorestCI commented Jul 17, 2018 •

edited

Loading

AutorestCI commented Oct 9, 2018 •

edited

Loading