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

Add Allowed Connections REST API #4106

Merged
merged 14 commits into from
Nov 12, 2018
Merged

Add Allowed Connections REST API #4106

merged 14 commits into from
Nov 12, 2018

Conversation

snhaphil
Copy link
Contributor

@snhaphil snhaphil commented Oct 9, 2018

Reopening the previously closed PR:
#3626

Adding allowed connections Rest API for Azure Security Center.
TFS item: https://msazure.visualstudio.com/One/_workitems/edit/2745880

@AutorestCI
Copy link

AutorestCI commented Oct 9, 2018

Automation for azure-sdk-for-js

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-js#471

@AutorestCI
Copy link

AutorestCI commented Oct 9, 2018

Automation for azure-sdk-for-python

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-python#3782

@AutorestCI
Copy link

AutorestCI commented Oct 9, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@azuresdkci azuresdkci requested a review from anuchandy October 9, 2018 07:13
@snhaphil snhaphil changed the title [DoNotMerge] Add Allowed Connections REST API Add Allowed Connections REST API Oct 9, 2018
@AutorestCI
Copy link

AutorestCI commented Oct 9, 2018

Automation for azure-sdk-for-node

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-node#4079

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Oct 9, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@chlahav chlahav self-requested a review October 9, 2018 10:26
}
}
},
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Security/locations/{ascLocation}/allowedConnections/{connectionsType}": {
Copy link
Contributor

Choose a reason for hiding this comment

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

please change connectionsType parameter name to connectionType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@snhaphil @yanivid3 was not resolved yet

}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing "list by resource group" path and "list by resource group and home region" path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Chen, list by RG is implicitly defined when we have list by Susbcription, ins't ? We do not have in other API's list by RG (although supported)

Copy link
Contributor

Choose a reason for hiding this comment

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

@yanivn3 if it's not defined in the swagger it will not be documented and will not be available to any generated client. if the path is supported then it should be available in the swagger.

"inboundConnectedResources": {
"type": "array",
"readOnly": true,
"description": "The list of azure resources that the resource has inbound allowed connection from",
Copy link
Contributor

Choose a reason for hiding this comment

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

please replace "azure" with "Azure" in all occurrences

Copy link
Contributor

Choose a reason for hiding this comment

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

@snhaphil reminder

"name": "connectionsType",
"type": "string",
"in": "path",
"description": "The type of allowed connections.",
Copy link
Contributor

Choose a reason for hiding this comment

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

please add allowed values in the description as well since it is not visible in the documentation yet

"in": "path",
"description": "The type of allowed connections.",
"required": true,
"enum": [
Copy link
Contributor

Choose a reason for hiding this comment

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

add the "x-ms-enum" properties like the other enums in this file

"responses": {
"200": {
"body": {
"value": [
Copy link
Contributor

Choose a reason for hiding this comment

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

value field should not be present - this is a get single

@chlahav chlahav added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required DoNotMerge <valid label in PR review process> use to hold merge after approval labels Oct 9, 2018
@AutorestCI
Copy link

AutorestCI commented Oct 16, 2018

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#3274

@@ -1437,6 +1437,132 @@
}
}
}
},
"/subscriptions/{subscriptionId}/providers/Microsoft.Security/allowedConnections": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make sense to allow a $filter property to filter the results at this scope?

@@ -1437,6 +1437,132 @@
}
}
}
},
"/subscriptions/{subscriptionId}/providers/Microsoft.Security/allowedConnections": {
"get": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these resources system-generated? The user can't update/delete them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. It is a read only collection. User get only see the outcome of the computation we perform.

Copy link
Contributor

Choose a reason for hiding this comment

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

@KrisBash , This API was reviewed by Gaurav more than 6m ago and it is already deployed. We issued the PR here in order to add it to ASC SDK and to update documentation.

Copy link
Contributor

@KrisBash KrisBash left a comment

Choose a reason for hiding this comment

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

Signing off, but please review comments. In the future, please schedule a review with the ARM API review alias for new resource types.

@KrisBash KrisBash 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 Oct 17, 2018
@yanivn3
Copy link
Contributor

yanivn3 commented Oct 18, 2018

@chlahav , Can you approve the PR as well and remove the DoNoMerge ?

@anuchandy
Copy link
Member

@snhaphil, @yanivn3, @chlahav - are we good to merge this? any more changes?

Copy link
Contributor

@chlahav chlahav left a comment

Choose a reason for hiding this comment

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

please address the comments

@chlahav chlahav removed the DoNotMerge <valid label in PR review process> use to hold merge after approval label Nov 11, 2018
@anuchandy anuchandy merged commit 2389da3 into Azure:master Nov 12, 2018
lmazuel added a commit that referenced this pull request Nov 15, 2018
lmazuel added a commit that referenced this pull request Nov 15, 2018
* Revert "Add maximum chunk size to data lake store docs. (#4464)"

This reverts commit ab9c6ff.

* Revert "Add latest authorization tag to java config (#4406)"

This reverts commit 7f0fd16.

* Revert "Mdb 20181029 statepropdescriptions (#4452)"

This reverts commit 90db24c.

* Revert "fixing invalid location in examples (#4415)"

This reverts commit ffaf595.

* Revert "/identity endpoint added (#4460)"

This reverts commit 4f93dee.

* Revert "Added missing PATCH operation to Api Issue resource (#4440)"

This reverts commit 7df0ce5.

* Revert "Adds storage account quick failover API (#4457)"

This reverts commit 10ec74f.

* Revert "DataFactory: add table name in dataset (#4424)"

This reverts commit f8465de.

* Revert "Update README.md (#4421)"

This reverts commit fa6a584.

* Revert "Move Ruby SDK config to its own config file (#4451)"

This reverts commit 2ca50c8.

* Revert "[CognitiveServices] enable Bing Search/CustomSearch products to specify Endpoint (#4401)"

This reverts commit dc782b3.

* Revert "Add GeoRegion property to DeletedSite model (#4448)"

This reverts commit a079367.

* Revert "  Azure Container Instance: Add start container group API (#4423)"

This reverts commit 2ea3e9e.

* Revert "add guidance for example names (#4432)"

This reverts commit 6780e8a.

* Revert "Remove security contact phone field mandatory fields (#4417)"

This reverts commit eaf464e.

* Revert "Add Allowed Connections REST API (#4106)"

This reverts commit 2389da3.

* Revert "Make NIC VM readOnly (#4444)"

This reverts commit 2d83450.

* Revert "Remove readonly attribute from identities collection in users (#4442)"

This reverts commit 7577df9.

* Revert "Add spec for alert repair/remediate (#4084)"

This reverts commit 3f568b4.

* Revert "Py conf in its own file (#4439)"

This reverts commit 3373760.

* Revert "Split Python conf in it's own file (#4438)"

This reverts commit 77cf607.

* Revert "Adding ManagedInstanceVulnerabilityAssessments and ServerVulnerabilityAssessments (#4416)"

This reverts commit dec7549.

* Revert "WebApps: Snapshots API for DR stamps (#4291)"

This reverts commit 916ff2e.

* Revert "Swagger closure for Azure Migration Hub Operations (#4330)"

This reverts commit badd1de.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants