[Network-2018-02-01] DDoS Protection Plan resource #2567

murilogr · 2018-03-01T01:58:54Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-03-01T02:22:27Z

Automation for azure-sdk-for-go

Encountered an unknown error: (azure-sdk-for-go)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/github_tools.py", line 32, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 167, in rest_handle_action
    return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 182, in rest_pull_close
    rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 142, in rest_pr_management
    sdk_tag=sdk_tag
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 283, in generate_sdk_from_git_object
    sdk_repo.git.push('origin', base_branch, set_upstream=True)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 551, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1010, in _call_process
    return self.execute(call, **exec_kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 821, in execute
    raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(1)
  cmdline: git push --set-upstream origin restapi_auto_Network-2018-02-01
  stderr: 'To https://AutorestCI:58ab395c311d1bd75b3e1db1cc8adaf9acc42afe@github.com/Azure/azure-sdk-for-go.git
 ! [rejected]        restapi_auto_Network-2018-02-01 -> restapi_auto_Network-2018-02-01 (fetch first)
error: failed to push some refs to 'https://AutorestCI:[email protected]/Azure/azure-sdk-for-go.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.'

AutorestCI · 2018-03-01T02:22:39Z

Automation for azure-sdk-for-python

A PR has been created for you:
Azure/azure-sdk-for-python#2141

murilogr · 2018-03-01T02:28:51Z

Design document:

https://microsoft.sharepoint.com/:w:/r/teams/WAG/AzureNetworking/Engineering%20Q%20%20Z/RNM/Design%20Specs/Calcium/[NRP]%20DDoS%20Protection%20Plan%20-%20Design.docx?d=wac94c6540b854ea482e69b6093f916b7&csf=1&e=DhD5Bm

salameer · 2018-03-09T00:59:16Z

@johanste and @devigned would appreciate comment from API review perspective here

salameer · 2018-03-09T00:59:40Z

@ravbhatnagar please review for ARM

johanste · 2018-03-09T01:07:22Z

specification/network/resource-manager/Microsoft.Network/stable/2018-02-01/virtualNetwork.json

-          "description": "Indicates if Vm protection is enabled for all the subnets in a Virtual Network." 
+        "enableDdosProtection": {
+          "type": "boolean",
+          "description": "Indicates if DDoS protection is enabled for all the protected resources in the virtual network. It requires a DDoS protection plan associated with the resource."


Is there a default value?

Yes, it's false by default

johanste · 2018-03-09T01:07:32Z

specification/network/resource-manager/Microsoft.Network/stable/2018-02-01/virtualNetwork.json

+          "description": "Indicates if DDoS protection is enabled for all the protected resources in the virtual network. It requires a DDoS protection plan associated with the resource."
+        },
+        "enableVmProtection": {
+          "type": "boolean",


Is there a default value?

Also false by default

...it would be a good idea to add the default value to the swagger. This way you will get correct documentation.

salameer · 2018-03-09T01:36:44Z

"type": "Error",

"code": "RequiredPropertiesMissingInResourceModel",

"message": "Model definition 'DdosProtectionPlan' must have the properties 'name', 'id' and 'type' in its hierarchy and these properties must be marked as readonly.",

"id": "R2020",

"validationCategory": "RPCViolation",

"providerNamespace": null,

"resourceType": null,

"sources": [

"file:///home/travis/build/Azure/azure-rest-api-specs/specification/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json:260:4 ($.definitions.DdosProtectionPlan)"

],

"jsonref": "file:///home/travis/build/Azure/azure-rest-api-specs/specification/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json:260:4 ($.definitions.DdosProtectionPlan)",

"json-path": "file:///home/travis/build/Azure/azure-rest-api-specs/specification/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json:260:4 ($.definitions.DdosProtectionPlan)"

}

salameer · 2018-03-09T01:37:03Z

I see this error that's related to your changes

anuchandy · 2018-03-09T01:33:17Z

...ication/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json

+        "tags": [
+          "DdosProtectionPlans"
+        ],
+        "operationId": "DdosProtectionPlans_List",


Can we rename method to DdosProtectionPlans_ListByResourceGroup? we are trying to have this consistent name for the the generated method that retrieve resources in a subscription.

Fixed, see /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/ddosProtectionPlans

anuchandy · 2018-03-09T01:33:54Z

...ication/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json

+        "tags": [
+          "DdosProtectionPlans"
+        ],
+        "operationId": "DdosProtectionPlans_ListAll",


Can we rename method to DdosProtectionPlans_List? this is for consistency.

anuchandy · 2018-03-09T01:40:52Z

...ication/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json

+          "$ref": "./network.json#/definitions/Resource"
+        }
+      ],
+      "description": "A DDoS protection plan in a resource group."


Seems only properties we can set on DdosProtectionPlan are location and name and tags. Just trying confirm my understanding.

That's correct

AutorestCI · 2018-03-09T01:56:19Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

ravbhatnagar · 2018-03-09T06:51:46Z

Looks good!

johanste · 2018-03-09T16:45:52Z

...ication/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json

+  },
+  "paths": {
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/ddosProtectionPlans/{ddosProtectionPlanName}": {
+      "delete": {


What is the response if one or more vnets are referencing ddpp? If they are referencing the dopp, but the enableddpp property is set to false?

@ravbhatnagar, when setting the ddpp property of a vnet to reference a ddpp considered a write operation on the ddpp from a RBAC perspective?

Multiple VNETs can be associated with a DdosProtectionPlan. EnableDdosProtection does not affect the relationship, it only depends on the plan to exist.

You can write only from the VNET side and you need to have permissions to write to the DdosProtectionPlan (you have if it's in the same subscription).

avijitgupta

Looks good!

anuchandy

There are some linter errors reported, but not due to swagger file (ddosProtectionPlan) introduced in this PR but errors from old swagger files such as application gateway, express route.

This reverts commit 9a32d84.

AutorestCI · 2018-03-12T17:30:15Z

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

anuchandy · 2018-03-12T21:02:02Z

Note: There are three breaking changes reported by CI, PR adds default value (as false) for enableDdosProtection and enableVmProtection properties in virtualNetwork model and introduces ddosProtectionPlan property in virtualNetwork. diff here. These are not breaking changes in the server - if these properties does not present in the payload then service assumes false, false and null respectively. Checked and confirmed with @murilogr.

johanste · 2018-03-13T17:29:01Z

I have no further questions.

… 2018 (#2642) * Add support for Express Route Circuit Connection resource as a child of Express Route Circuit Peering in 2018-02-01 API (#2358) * copy all files from 2018-01-01 to 2018-02-01 * update api version to 2018 in all files * add express route circuit connection as child of express route bgp peering * update readme.md * add examples * fix travis build error * fix travis build error * fix indentation errors * update reference to circuit connections in peering * fix indentation * Merge Express Route Circuit Connection to Networking2018-02-01 (#2370) * copy all files from 2018-01-01 to 2018-02-01 * update api version to 2018 in all files * add express route circuit connection as child of express route bgp peering * update readme.md * add examples * fix travis build error * fix travis build error * fix indentation errors * update reference to circuit connections in peering * fix indentation * fix indentation * fix indentation' * get latest file * Fix indentation * fix key * fix indentation and example * fix indentation * fix indentation * make circuit con singular * add fix as PR #2364 (#2376) * Network feature: Setting custom ipsec policy for Virtual Network Gateway P2S clients. (#2521) * 1443089: Network feature: Setting custom ipsec policy for Virtual Network Gateway P2S clients. * 1443089:Fix network ReadMe file. * 1443089:Fix network ReadMe file. * Temporary bug fix * Add support for Express Route Provider APIs (#2532) * Add support for Express Route Provider APIs A new top level NRP resource ExpressRouteCrossConnection is being introduced with this change. This resource shadows the customer ExpressRouteCircuit resouce in the customer subscription and enables the provider to perform CRUD operations on some of the sub-resources, such as peerings on the ExpressRoute Circuit * remove wrong enum values from circuit connection status and fix enum … (#2572) * remove wrong enum values from circuit connection status and fix enum name * Update ExpressRouteCrossConnection Route Table Summary Record (#2574) * Update CrossConnection Route Table Summary record * [Network-2018-02-01] DDoS Protection Plan resource (#2567) * Initial version * Add eol * Add extra property in example * Add default values for fields * Rename operation IDs * Make 'id' read-only for all network resources * Revert "Make 'id' read-only for all network resources" * Fix reference * making peer express route circuit peering and express route circuit peering as references (#2696) * Express Route Cross Connection Swagger changes as per PM requirements (#2644) * Fixed operationIds for non-CRUD operations in ExpressRouteCrossConnections (#2720) * Making travis run > 10 mins (#2739)

murilogr added 3 commits February 28, 2018 17:35

Initial version

1a8f014

Add eol

57ee8c0

Add extra property in example

917a892

salameer added Reassign WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. labels Mar 9, 2018

johanste reviewed Mar 9, 2018

View reviewed changes

anuchandy self-assigned this Mar 9, 2018

anuchandy removed the Reassign label Mar 9, 2018

anuchandy reviewed Mar 9, 2018

View reviewed changes

Add default values for fields

a38173e

murilogr added 2 commits March 8, 2018 18:02

Rename operation IDs

b224ae6

Make 'id' read-only for all network resources

9a32d84

This was referenced Mar 9, 2018

[AutoPR network/resource-manager] [Network-2018-02-01] DDoS Protection Plan resource Azure/azure-sdk-for-go#1224

Closed

[AutoPR network/resource-manager] [Network-2018-02-01] DDoS Protection Plan resource Azure/azure-sdk-for-python#2141

Merged

ravbhatnagar approved these changes Mar 9, 2018

View reviewed changes

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 Mar 9, 2018

johanste reviewed Mar 9, 2018

View reviewed changes

avijitgupta approved these changes Mar 10, 2018

View reviewed changes

anuchandy approved these changes Mar 12, 2018

View reviewed changes

Revert "Make 'id' read-only for all network resources"

4fb4bf5

This reverts commit 9a32d84.

johanste removed the APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. label Mar 13, 2018

murilogr mentioned this pull request Mar 13, 2018

Merge Network-2018-02-01 to Master Networking Swagger Changes for Feb 2018 #2642

Merged

10 tasks

anuchandy merged commit 15604e2 into Azure:Network-2018-02-01 Mar 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Network-2018-02-01] DDoS Protection Plan resource #2567

[Network-2018-02-01] DDoS Protection Plan resource #2567

murilogr commented Mar 1, 2018

AutorestCI commented Mar 1, 2018 •

edited

Loading

AutorestCI commented Mar 1, 2018 •

edited

Loading

murilogr commented Mar 1, 2018

salameer commented Mar 9, 2018

salameer commented Mar 9, 2018

johanste Mar 9, 2018

murilogr Mar 9, 2018

johanste Mar 9, 2018

murilogr Mar 9, 2018

johanste Mar 9, 2018

salameer commented Mar 9, 2018

salameer commented Mar 9, 2018 •

edited

Loading

anuchandy Mar 9, 2018

murilogr Mar 9, 2018

anuchandy Mar 9, 2018

anuchandy Mar 9, 2018 •

edited

Loading

murilogr Mar 9, 2018

AutorestCI commented Mar 9, 2018 •

edited

Loading

ravbhatnagar commented Mar 9, 2018

johanste Mar 9, 2018

murilogr Mar 9, 2018 •

edited

Loading

avijitgupta left a comment

anuchandy left a comment

AutorestCI commented Mar 12, 2018 •

edited

Loading

anuchandy commented Mar 12, 2018 •

edited

Loading

johanste commented Mar 13, 2018 •

edited

Loading

[Network-2018-02-01] DDoS Protection Plan resource #2567

[Network-2018-02-01] DDoS Protection Plan resource #2567

Conversation

murilogr commented Mar 1, 2018

PR information

Quality of Swagger

AutorestCI commented Mar 1, 2018 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Mar 1, 2018 • edited Loading

Automation for azure-sdk-for-python

murilogr commented Mar 1, 2018

salameer commented Mar 9, 2018

salameer commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salameer commented Mar 9, 2018

salameer commented Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuchandy Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AutorestCI commented Mar 9, 2018 • edited Loading

Automation for azure-sdk-for-node

ravbhatnagar commented Mar 9, 2018

Choose a reason for hiding this comment

murilogr Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

avijitgupta left a comment

Choose a reason for hiding this comment

anuchandy left a comment

Choose a reason for hiding this comment

AutorestCI commented Mar 12, 2018 • edited Loading

Automation for azure-libraries-for-java

anuchandy commented Mar 12, 2018 • edited Loading

johanste commented Mar 13, 2018 • edited Loading

AutorestCI commented Mar 1, 2018 •

edited

Loading

AutorestCI commented Mar 1, 2018 •

edited

Loading

salameer commented Mar 9, 2018 •

edited

Loading

anuchandy Mar 9, 2018 •

edited

Loading

AutorestCI commented Mar 9, 2018 •

edited

Loading

murilogr Mar 9, 2018 •

edited

Loading

AutorestCI commented Mar 12, 2018 •

edited

Loading

anuchandy commented Mar 12, 2018 •

edited

Loading

johanste commented Mar 13, 2018 •

edited

Loading