-
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
base: main
Are you sure you want to change the base?
Add option for forced tunneling through TRE's Firewall #4238
Conversation
Unit Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 60f3106. ♻️ This comment has been updated with latest results. |
Provide the external firewall's IP address: | ||
|
||
```json | ||
rp_bundle_values: '{"firewall_force_tunnel_ip":"10.0.0.4"}' |
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 to firewal_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 in make 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:
Lines 304 to 313 in c79d9e1
deploy-shared-service: | |
$(call target_title, "Deploying ${DIR} shared service") \ | |
&& . ${MAKEFILE_DIR}/devops/scripts/check_dependencies.sh porter,env \ | |
&& ${MAKEFILE_DIR}/devops/scripts/ensure_cli_signed_in.sh $${TRE_URL} \ | |
&& cd ${DIR} \ | |
&& ${MAKEFILE_DIR}/devops/scripts/deploy_shared_service.sh $${PROPS} | |
firewall-install: | |
$(MAKE) bundle-build bundle-publish bundle-register deploy-shared-service \ | |
DIR=${MAKEFILE_DIR}/templates/shared_services/firewall/ BUNDLE_TYPE=shared_service |
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.
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 for os.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.
Resolves #4237
What is being addressed
Added the option for force tunnel TRE's Firewall to an external firewall
How is this addressed
firewall_force_tunnel_ip
parameter torp_bundle_values
, when set, the following are created:After that, users have to manually connect TRE's VNet to the external firewall (e.g. through VNet Peering).