-
Notifications
You must be signed in to change notification settings - Fork 147
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
Add option for forced tunneling through TRE's Firewall #4238
Draft
yuvalyaron
wants to merge
5
commits into
microsoft:main
Choose a base branch
from
yuvalyaron:4237-support-firewall-force-tunnel
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+61
−3
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
30c3c2f
Add option for forced tunneling through TRE's Firewall
yuvalyaron 65fcaff
fix linting issues
yuvalyaron 20c4349
refine doc
yuvalyaron 60f3106
rename force tunnel route
yuvalyaron 5da1a4d
Merge branch 'main' of https://github.com/microsoft/AzureTRE into 423…
yuvalyaron File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# Forced Tunneling to External Firewall in TRE | ||
|
||
Forced tunneling ensures that all traffic from TRE is routed through a specific external firewall. This guarantees that all data passes through the firewall for inspection, control, or further processing before reaching its destination. | ||
|
||
To setup forced tunneling to an external firewall, follow these steps: | ||
|
||
## 1. Set the rp_bundle_values Parameter in the config.yaml file | ||
Provide the external firewall's IP address: | ||
|
||
```json | ||
rp_bundle_values: '{"firewall_force_tunnel_ip":"10.0.0.4"}' | ||
``` | ||
This automatically creates a route table to direct TRE’s traffic to the specified IP. | ||
|
||
## 2. Manually Connect TRE to Your Firewall | ||
Configure connectivity between TRE’s VNet and your external firewall using one of the following methods: | ||
|
||
1. **VNet Peering**: Peer the TRE VNet with your firewall’s VNet. | ||
1. **ExpressRoute**: Use a private connection for firewalls located on-premises. | ||
1. **Site-to-Site VPN**: Establish a VPN connection as an alternative. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we are using
rp_bundle_values
rather than exposing it as a parameter and giving it its own variable in a similar way tofirewal_sku
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make that change, but from what I understand,
rp_bundle_values
are intended for passing parameters to the resource processor, which are used only by the bundles and not the processor itself. Should I change?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If configured as a parameter for the bundle, the IP can be passed as part of the deployment message.
Hmm, I can see
firewall_sku
was set on the RP as an env var. @tamirkamara think you worked on that, why was it done this way, rather than passed as a property inmake firewall-install
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that was me :-) It may not have been the best way to pass it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, my view is that we should be passing both firewall sku and the tunnel IP as props, as part of
firewall-install
here:AzureTRE/Makefile
Lines 304 to 313 in c79d9e1
Not sure how easy that is to do... The
rp_bundle_values
will work, but believe it was intended for setting something that should not be passed via the API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you test it? I can't see them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that if you set these values and run
make tre-deploy
from your local machine the RP will get the values and will be able to use them (same for the CI).As for debug I don't think we tried. Are you sure you followed all steps to make these available after updating your config file? At any case, it works the 2 ways it counts :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They aren't output to
private.env
so it's not going to work unless that's updated somehow. I should have been searching foros.environ["RP_BUNDLE_custom_key_1"]
, but it's still not there. Saying that, it's the same for Firewall SKU.IMHO they should be properties on the bundle so they can be configured via the TRE API/UI, are visible via the UI, without destroying and recreating the resource processor.
If you want to get it in as is without the changes suggested, then do it and I can amend it next week with a further PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But they are params in the bundle so maybe we can add them to the template schema to allow setting via the API. But TBH, passing it through the scripts isn't great and I don't think this is the right time to get into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are the one who added the code to allow the passing props into the shared services script ;-), all I showed above is where they need to be specified.
Lets start by adding them to the template schema.