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.

Sign up for GitHub

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

feat(SIDM-3410-ips-preview): remove filter pattern in preview #272

Merged
merged 1 commit into from
Nov 14, 2019
Merged

feat(SIDM-3410-ips-preview): remove filter pattern in preview #272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions infrastructure/idam-preview.tfvars
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,5 @@ asp_name_override = "idam-idam-preview"
asp_rg_override = "idam-idam-preview"

capacity = 1

strategic_policies_privateIpsFilterPattern = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we experiment with setting this to

10.97\\.\\d+\\.\\d+

What I'm thinking is:

  • this will filter out proxy IPs when accessing the app through the application gateway, hence allowing us to test manually without hitting the ForgeRock issue
  • it will not filter out the app service IP, hence allowing the functional tests to pass

This theory might prove wrong, but could we try it out please? :)

2 changes: 2 additions & 0 deletions infrastructure/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,7 @@ module "idam-web-public" {
STRATEGIC_SERVICE_URL = "${local.idam_api_url}"

GA_TRACKING_ID = "${var.ga_tracking_id}"

STRATEGIC_POLICIES_PRIVATEIPSFILTERPATTERN = "${var.strategic_policies_privateIpsFilterPattern}"
}
}
5 changes: 5 additions & 0 deletions infrastructure/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,4 +77,9 @@ variable asp_rg_override {
variable vault_name_override {
description = "Vault Name"
default = ""
}

variable "strategic_policies_privateIpsFilterPattern" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The TF variable name doesn't need to match the application property name. Maybe we can simplify to:

vnet_private_ip_pattern

Copy link
Contributor

Choose a reason for hiding this comment

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

Conversely, I think retaining the same name make it much easier to identify mistakes and issues in the marshalling of variables through the provisioning tasks.

description = "Private IPs Filter Pattern for Policies Evaluation"
default = "10\\.\\d+\\.\\d+\\.\\d+"
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the default was \Q10\E

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah you don't actually need to quote 10 in \Q \E
I've added in the first commit because I'm used to quoting stuff in regex but then Krem raised a point and it's actually not needed

}