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

Feature/optional dns resolver #5

Merged
merged 12 commits into from
Apr 9, 2024

Conversation

QBY-ChristianHartmann
Copy link
Contributor

@QBY-ChristianHartmann QBY-ChristianHartmann commented Mar 12, 2024

Description

As we are not deploying private dns resolver or azure as a standard and not always azure dcs, both network collection groups are now optional. There is also a new option to add rules for azure bastion.

Change overview (tick true):

  • This introduces backward incompatible changes
  • This adds a new backward compatible Feature
  • This fixes a Bug

Version information:

  • Previous Version: 1.1.0

How Has This Been Tested?

  • Apply of all examples was successful

Checklist:

  • I have run tests and documented them above
  • I have performed a self-review of my own code
  • I have updated the documentation
  • I have updated the CHANGELOG

Copy link
Member

@QBY-MarkusMaring QBY-MarkusMaring left a comment

Choose a reason for hiding this comment

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

It works when the optional parameters are empty. But when I put something in it seems that no network rule collections will be deployed at all and the apply fails.

Can you add an example of more than just required IP groups? And take a look whats happening there. I don't think you can combine static and dynamic network_rule_collection blocks like that. I remember it being a one or the other kind of thing. So either 100% dynamic or 100% static. But I can't find documentation for that :/

See issue: hashicorp/terraform#34933

examples/advanced/main.tf Outdated Show resolved Hide resolved
examples/basic/main.tf Outdated Show resolved Hide resolved
@QBY-ChristianHartmann QBY-ChristianHartmann merged commit d5e0447 into main Apr 9, 2024
@QBY-ChristianHartmann QBY-ChristianHartmann deleted the feature/optional-dns-resolver branch April 9, 2024 11:40
Comment on lines +68 to +69
ipg_rdp_access_ids: If rdp access is needed, provide vm ip-groups in this variable. Every ip-group provided in this list, will be accessible by bastion.
ipg_ssh_access_ids: If ssh access is needed, provide vm ip-groups in this variable. Every ip-group provided in this list, will be accessible by bastion.
Copy link

Choose a reason for hiding this comment

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

I don't get it :(

Copy link

Choose a reason for hiding this comment

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

oooh, I think I got it.

Suggested change
ipg_rdp_access_ids: If rdp access is needed, provide vm ip-groups in this variable. Every ip-group provided in this list, will be accessible by bastion.
ipg_ssh_access_ids: If ssh access is needed, provide vm ip-groups in this variable. Every ip-group provided in this list, will be accessible by bastion.
ipg_rdp_access_ids: If rdp access is needed, provide vm ip-groups in this variable. Every ip-group provided in this list, will be accessible via rdp by bastion.
ipg_ssh_access_ids: If ssh access is needed, provide vm ip-groups in this variable. Every ip-group provided in this list, will be accessible via ssh by bastion.

network_rule_collection {
name = "rc-internet_outbound-${var.stage}"
priority = 120
Copy link

Choose a reason for hiding this comment

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

Change of priority on purpose?
This would make this a breaking change, if someone already uses priority 100

Copy link
Member

Choose a reason for hiding this comment

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

Priority 100 is reserved by platform. So no collisions there.

Rules were re-ordered because all network rules need to be in front of application rules. Previously we didn't leave any free space so it was not possible to insert a new rule if we wanted to stick to a 10 diff.

We did a 5 diff to allow for more RCs in total and left space between the end of network rules and the start of application rules.

This change causes no downtime.

Comment on lines +78 to +81
protocols = ["Any"]
source_ip_groups = var.ipg_onpremise_dc_id != null ? [var.ipg_azure_dc_id, var.ipg_onpremise_dc_id] : [var.ipg_azure_dc_id]
destination_ip_groups = [var.ipg_dnsprivateresolver_id]
destination_ports = ["*"]
Copy link

Choose a reason for hiding this comment

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

Why do we need any port to DNS resolver?

Suggested change
protocols = ["Any"]
source_ip_groups = var.ipg_onpremise_dc_id != null ? [var.ipg_azure_dc_id, var.ipg_onpremise_dc_id] : [var.ipg_azure_dc_id]
destination_ip_groups = [var.ipg_dnsprivateresolver_id]
destination_ports = ["*"]
protocols = ["Any"]
source_ip_groups = var.ipg_onpremise_dc_id != null ? [var.ipg_azure_dc_id, var.ipg_onpremise_dc_id] : [var.ipg_azure_dc_id]
destination_ip_groups = [var.ipg_dnsprivateresolver_id]
destination_ports = ["53"]

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Probably better to do 53 UDP only

dynamic "rule" {
for_each = var.bastion_config.ipg_rdp_access_ids
content {
name = "allow-bastion-to-${regex(".+\\/(.+)?", rule.value)[0]}-rdp"
Copy link

Choose a reason for hiding this comment

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

What does the regex do?

Copy link
Member

Choose a reason for hiding this comment

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

Get the name of the ip group from its resource ID. Needed to avoid naming collisions.

dynamic "rule" {
for_each = var.bastion_config.ipg_ssh_access_ids
content {
name = "allow-bastion-to-${regex(".+\\/(.+)?", rule.value)[0]}-ssh"
Copy link

Choose a reason for hiding this comment

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

What does the regex do?

Copy link
Member

Choose a reason for hiding this comment

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

Get the name of the ip group from its resource ID. Needed to avoid naming collisions.

application_rule_collection {
name = "rc-application_internet_outbound-${var.stage}"
priority = 130
priority = 150
Copy link

Choose a reason for hiding this comment

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

Change of priority on purpose?
This would make this a breaking change, if someone already uses priority 150

Copy link
Member

Choose a reason for hiding this comment

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

See above. Not a breaking change. No risk of collision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants