Azure / CanadaPubSecALZ

This reference implementation is based on Cloud Adoption Framework for Azure and provides an opinionated implementation that enables ITSG-33 regulatory compliance by using NIST SP 800-53 Rev. 4 and Canada Federal PBMM Regulatory Compliance Policy Sets.
MIT License
124 stars 86 forks source link

Disable BGP Route Propagation to Spokes #368

Closed Jamalzkr closed 7 months ago

Jamalzkr commented 1 year ago

Is your feature request related to a problem? Please describe. A clear and concise description of what the problem is. E.g. I'm always frustrated when [...]

When you allow "use remote gateways" option on route table to force traffic to hub firewall. "Virtual network gateway route propagation" is enabled by default, advertising all the learned BGP routes to the spokes, conflicting with UDR's. A switch in the archetype json files should be added to disable this.

Describe the solution you'd like A clear and concise description of what you want to happen.

Add true / false switch in config/subscriptions json files to control this functionality. See my fork.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

None, this is a simple solution that works.

Additional context Add any other context or screenshots about the feature request here.

https://github.com/Azure/CanadaPubSecALZ/compare/main...Jamalzkr:CanadaPubSecALZ:main

skeeler commented 1 year ago

@Jamalzkr & @tredell , I did a quick code review of the proposed changes and have two comments:

  1. For the line, e.g.:

disableBgpRoutePropagation: network.disableBgpRoutesFromVPNGateway

Any reason why the configuration variable name does not match the resource property? If there is that is fine, but if not it would be easier to find all matching references based on using the same name for config element as resource property.

  1. We'll need additional code changes to handle config schema. Two options: optional vs. required. Either way, the config schema will need to be updated (and up-versioned). If we make the new property Required then that will be a breaking change (to existing configurations) and will necessitate a major version increment for the project (e.g. 2.0.0). If we make the new property Optional then we can introduce as a minor version change (e.g. 1.3.0, 1.4.0, etc.), will still need to adjust the config schema (showing it is available but not required), and will also need to adjust the Bicep code changes here to handle both use cases - it has been specified & it has not been specified, in the config.
Jamalzkr commented 1 year ago

There is no good reason, it should be changed to match the name for the config element.

I think optional would be fine, but not sure I fully understand the difference.

On Mon, Jul 24, 2023 at 1:34 PM Steve Keeler @.***> wrote:

@Jamalzkr https://github.com/Jamalzkr & @tredell https://github.com/tredell , I did a quick code review of the proposed changes and have two comments:

  1. For the line, e.g.:

disableBgpRoutePropagation: network.disableBgpRoutesFromVPNGateway

Any reason why the configuration variable name does not match the resource property? If there is that is fine, but if not it would be easier to find all matching references based on using the same name for config element as resource property.

  1. We'll need additional code changes to handle config schema. Two options: optional vs. required. Either way, the config schema will need to be updated (and up-versioned). If we make the new property Required then that will be a breaking change (to existing configurations) and will necessitate a major version increment for the project (e.g. 2.0.0). If we make the new property Optional then we can introduce as a minor version change (e.g. 1.3.0, 1.4.0, etc.), will still need to adjust the config schema (showing it is available but not required), and will also need to adjust the Bicep code changes here to handle both use cases - it has been specified & it has not been specified, in the config.

— Reply to this email directly, view it on GitHub https://github.com/Azure/CanadaPubSecALZ/issues/368#issuecomment-1648326772, or unsubscribe https://github.com/notifications/unsubscribe-auth/A76MJUXD2FILTJELSRNQBOTXR2WZXANCNFSM6AAAAAAYG5XHS4 . You are receiving this because you were mentioned.Message ID: @.***>

github-actions[bot] commented 11 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions[bot] commented 7 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.