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 ManagedServerSecurityAlertPolicy and ManagedDatabaseSecurityAlert… #4521

Merged
merged 3 commits into from
Dec 13, 2018
Merged

Conversation

talhers
Copy link
Member

@talhers talhers commented Nov 20, 2018

…Policies

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

@openapi-portal-comment
Copy link

If you're a MSFT employee, click this link
to view this PR's validation status on our new OpenAPI Hub spec management tool.

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#3769

@AutorestCI
Copy link

AutorestCI commented Nov 20, 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#570

@AutorestCI
Copy link

AutorestCI commented Nov 20, 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#4181

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-ruby

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-ruby#1871

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-java

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-java#2138

@AutorestCI
Copy link

AutorestCI commented Nov 20, 2018

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@talhers
Copy link
Member Author

talhers commented Nov 20, 2018

@yaakoviyun @yoavfr

@yaakoviyun
Copy link
Member

@ayeletshpigelman

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@praries880
Copy link

@talhers Have the new APIs been through an API review?

@praries880 praries880 added the APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. label Nov 20, 2018
@KrisBash KrisBash added ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Nov 21, 2018
@talhers
Copy link
Member Author

talhers commented Nov 21, 2018

@talhers Have the new APIs been through an API review?

These APIs are similar to the regular server and database APIs:

https://github.com/Azure/azure-rest-api-specs/blob/master/specification/sql/resource-manager/Microsoft.Sql/stable/2014-04-01/databaseSecurityAlertPolicies.json

https://github.com/Azure/azure-rest-api-specs/blob/master/specification/sql/resource-manager/Microsoft.Sql/preview/2017-03-01-preview/serverSecurityAlertPolicies.json

The difference is that they apply to managed instance and managed database (except for that they are identical)

@praries880 praries880 added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required ARMReviewInProgress and removed ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. labels Dec 5, 2018
@KrisBash KrisBash added the ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review label Dec 7, 2018
Copy link
Contributor

@ravbhatnagar ravbhatnagar left a comment

Choose a reason for hiding this comment

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

@talhers - some comments. please take a look.

"application/json"
],
"paths": {
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/managedInstances/{managedInstanceName}/databases/{databaseName}/securityAlertPolicies/{securityAlertPolicyName}": {
Copy link
Contributor

Choose a reason for hiding this comment

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

LIST api to get all securityAlertPolicies for a database missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will open a work item for this

"produces": [
"application/json"
],
"paths": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it may be helpful to have a PATCH APi for this. For example to update the state of a policy, doing a PUT may be an overkill. PATCH will be much more convenient.

Copy link
Member Author

Choose a reason for hiding this comment

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

After thinking about it we don't think that a patch is needed in our customer scenarios

"description": "Specifies the blob storage endpoint (e.g. https://MyAccount.blob.core.windows.net). This blob storage will hold all Threat Detection audit logs.",
"type": "string"
},
"storageAccountAccessKey": {
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be returned in the response of a Get or PUT. A better model may be to fetch it on behalf of the user using s2S auth.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will open a work item for this

"produces": [
"application/json"
],
"paths": {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comments as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will open a work item for this

@talhers
Copy link
Member Author

talhers commented Dec 12, 2018

We will open work items on the comments above and fix them in our next API version.
Can you take another look and merge the PR if everything is fine? This is a blocker for cmdlet implementation.
Thanks!

@ravbhatnagar
Copy link
Contributor

Couple of pending issues Tracked here #4910
Signing off from ARM side.

@ravbhatnagar
Copy link
Contributor

#4910

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Dec 13, 2018
@praries880 praries880 merged commit 2f6d951 into Azure:master Dec 13, 2018
TalluriAnusha pushed a commit to AsrOneSdk/azure-rest-api-specs that referenced this pull request Feb 6, 2019
Azure#4521)

* Add ManagedServerSecurityAlertPolicy and ManagedDatabaseSecurityAlertPolicies

* fix incompatible values in swagger

* Fix ManagedDatabaseSecurityAlertCreateMin example file
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