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

Added new built-in Policies into the Initiative #1109

Merged
merged 33 commits into from
Nov 23, 2022
Merged

Added new built-in Policies into the Initiative #1109

merged 33 commits into from
Nov 23, 2022

Conversation

pkorolo
Copy link
Contributor

@pkorolo pkorolo commented Nov 9, 2022

Overview/Summary

This PR brings additional existing built-in DINE Policies into this initiative, as per performed gap analysis

This PR fixes/adds/changes/removes

  1. Adds the majority of the existing DINE Policies, that were not initially added in the Initiative
  2. The "Storage Table" Policies were not included, due to conflict with the "Cosmos DB" (Table API) Policy, which was included
  3. The IoT Central & Bot Service Policies were not included, due to limited or conflicting information in the documentation, regarding the respective required DNS Zone integration

Breaking Changes

  1. Private DNS Zone for "Azure Migrate" ("privatelink.prod.migration.windowsazure.com"), is not included in the RI, hence either the zone must be added as well, or the respective Policy for "Azure Migrate" must be removed

Testing Evidence

Testing URLs

The below URLs can be updated where the placeholders are, look for {YOUR GITHUB BRANCH NAME HERE - Remove Curly Brackets Also} & {YOUR GITHUB BRANCH NAME HERE - Remove Curly Brackets Also}, to allow you to test your portal deployment experience.

Please also replace the curly brackets on the placeholders {}

Azure Public

Deploy To Azure

As part of this Pull Request I have

  • Checked for duplicate Pull Requests
  • Associated it with relevant issues, for tracking and closure.
  • Ensured my code/branch is up-to-date with the latest changes in the main branch
  • Performed testing and provided evidence.
  • Updated relevant and associated documentation.
  • Updated the "What's New?" wiki page (located: /docs/wiki/whats-new.md)

@ghost ghost added the Needs: Triage 🔍 Needs triaging by the team label Nov 9, 2022
@github-actions github-actions bot requested a review from a team as a code owner November 9, 2022 15:19
Copy link
Collaborator

@jtracey93 jtracey93 left a comment

Choose a reason for hiding this comment

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

Nice stuff @pkorolo.

A couple of things:

  1. Can we update the Whats New Wiki page? - https://github.com/Azure/Enterprise-Scale/blob/main/docs/wiki/Whats-new.md
  2. Do we also need to update the assignments?

As for the Azure Migrate piece I think its worth adding this so up to you if you want to wrap into this branch/PR - might be easiest?

Might be worth you also picking up #1073 as part of this.

Let me know your thoughts

Thanks

Jack

@jtracey93 jtracey93 added enhancement New feature or request policy engineering engineering work and removed Needs: Author Feedback Needs: Triage 🔍 Needs triaging by the team labels Nov 11, 2022
@pkorolo
Copy link
Contributor Author

pkorolo commented Nov 11, 2022

Hello @jtracey93

To your points:

I am in very limited availability as we speak, due to my "WAF-Resiliency" assignment. But I will have a clearer picture early next week, if I can follow up with additional tasks or not; but:

  1. if I cannot take this (documentation - "What's new"), I can help the one that will
  2. Assignments definitely need to be updated; again, not sure if I can take that next week - btw, I think (maybe) I should have put default parameters in the Initiative itself for all the "groupIDs", so that the one who will eventually work with this, will have to only supply the "ZoneIDs" (as in the previous iteration), what do you think?
    [If "yes", I can work with this (minor) addition and submit a new PR (?) ]

Last, for the deployment of Azure Migrate Private DNS Zone itself, if I would take that eventually, where should I touch this?

Thank you.

@pkorolo
Copy link
Contributor Author

pkorolo commented Nov 16, 2022

@jtracey93 can you check this conflict please?

Btw, I have updated

  • the Policy Definition with default values in GroupIDs
  • also updated the Azure Public assignments, and
  • added the extra private DNS zone for Azure migrate

please review, thank you!

"azureMonitorPrivateDnsZoneId2": "[concat(variables('baseId'), 'privatelink.oms.opinsights.azure.com')]",
"azureMonitorPrivateDnsZoneId3": "[concat(variables('baseId'), 'privatelink.ods.opinsights.azure.com')]",
"azureMonitorPrivateDnsZoneId4": "[concat(variables('baseId'), 'privatelink.agentsvc.azure-automation.net')]",
"azureMonitorPrivateDnsZoneId5": "[concat(variables('baseId'), 'privatelink.blob.core.windows.net')]",
"azureWebPrivateDnsZoneId": "[concat(variables('baseId'), 'privatelink.webpubsub.azure.com')]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realised that this has probably been wrong since creation, as this is the namespace for Azure SignalR WebPubSub, but the parameter name (in my opinion) leads me to believe this was intended for Azure Web Apps.

The policy mapped to this parameter is indeed Azure SignalR WebPubSub so it technically maps correctly, but this doesn't feel correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, if I would make it from the beginning, naming would be different for cases like this one, but I did not touch the originals

"azureWebPrivateDnsZoneId": "[concat(variables('baseId'), 'privatelink.webpubsub.azure.com')]",
"azureBatchPrivateDnsZoneId": "[concat(variables('baseId'), 'privatelink.', parameters('location'), '.batch.azure.com')]",
"azureBatchPrivateDnsZoneId": "[concat(variables('baseId'), 'privatelink.batch.azure.com')]",
"azureAppPrivateDnsZoneId": "[concat(variables('baseId'), 'privatelink.azconfig.io')]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, this appears to be the namespace for App Configuration Stores - making this an equally ambiguous parameter name. Would probably be a breaking change to reverse some of this though unfortunately.

"azureAppPrivateDnsZoneId": "[concat(variables('baseId'), 'privatelink.azconfig.io')]",
"azureAsrPrivateDnsZoneId": "[concat(variables('baseId'), '.privatelink.siterecovery.windowsazure.com')]",
"azureAsrPrivateDnsZoneId": "[concat(variables('baseId'), 'privatelink.siterecovery.windowsazure.com')]",
"azureIotPrivateDnsZoneId": "[concat(variables('baseId'), 'privatelink.azure-devices-provisioning.net')]",
Copy link
Contributor

Choose a reason for hiding this comment

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

And another where the name is misleading given the presence of IoT hubs, although this one may be easier to switch around if needed.

krowlandson
krowlandson previously approved these changes Nov 23, 2022
Copy link
Contributor

@krowlandson krowlandson left a comment

Choose a reason for hiding this comment

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

LGTM 🔥 Thank you

@krowlandson krowlandson marked this pull request as draft November 23, 2022 15:46
@krowlandson krowlandson marked this pull request as ready for review November 23, 2022 15:46
@krowlandson krowlandson marked this pull request as draft November 23, 2022 19:44
@krowlandson krowlandson marked this pull request as ready for review November 23, 2022 19:44
krowlandson
krowlandson previously approved these changes Nov 23, 2022
Copy link
Contributor

@krowlandson krowlandson left a comment

Choose a reason for hiding this comment

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

LGTM

Kevin Rowlandson added 2 commits November 23, 2022 19:52
@krowlandson krowlandson temporarily deployed to csu-rw November 23, 2022 20:06 Inactive
Copy link
Contributor

@krowlandson krowlandson left a comment

Choose a reason for hiding this comment

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

LGTM

@krowlandson krowlandson merged commit 4f23cea into Azure:main Nov 23, 2022
@pkorolo
Copy link
Contributor Author

pkorolo commented Nov 24, 2022

Branch to be deleted

@pkorolo pkorolo deleted the Update-Custom-PrivateDNSInitiative branch November 24, 2022 17:00
@jtracey93 jtracey93 linked an issue Dec 13, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering engineering work enhancement New feature or request policy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Built-in policies DINE policies for DNSZoneGroup registration missing
3 participants