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

Added Private Endpoint deployment option #609

Open
wants to merge 15 commits into
base: features/private
Choose a base branch
from

Conversation

patkje75
Copy link

@patkje75 patkje75 commented Feb 19, 2024

  • Added code for Private Endpoints and updated the .\finops-hub\README.md
  • Added parameters to storage.bicep

πŸ› οΈ Description

Possibility to do deploy FinOps hub with private endpoints.

πŸ”¬ How did you test this change?

  • 🀏 Lint tests
  • 🀞 PS -WhatIf / az validate
  • πŸ‘ Manually deployed + verified
  • πŸ’ͺ Unit tests
  • πŸ™Œ Integration tests

πŸ™‹β€β™€οΈ Do any of the following that apply?

  • 🚨 This is a breaking change.
  • 🀏 The change is less than 20 lines of code.

πŸ“‘ Did you update docs/changelog.md?

  • βœ… Updated changelog (required for dev PRs)
  • ➑️ Will add log in a future PR (feature branch PRs only)
  • ❎ Log not needed (small/internal change)

πŸ“– Did you update documentation?

  • βœ… Public docs in docs (required for dev)
  • βœ… Internal dev docs in src (required for dev)
  • ➑️ Will add docs in a future PR (feature branch PRs only)
  • ❎ Docs not needed (small/internal change)

@patkje75
Copy link
Author

@patkje75 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term β€œYou” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

Copy link
Collaborator

@flanakin flanakin left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Too much going on πŸ˜“

src/templates/finops-hub/README.md Outdated Show resolved Hide resolved
src/templates/finops-hub/README.md Outdated Show resolved Hide resolved
src/templates/finops-hub/README.md Outdated Show resolved Hide resolved
src/templates/finops-hub/README.md Show resolved Hide resolved
src/templates/finops-hub/README.md Outdated Show resolved Hide resolved
src/templates/finops-hub/modules/keyVault.bicep Outdated Show resolved Hide resolved
src/templates/finops-hub/modules/keyVault.bicep Outdated Show resolved Hide resolved
src/templates/finops-hub/modules/storage.bicep Outdated Show resolved Hide resolved
src/templates/finops-hub/modules/storage.bicep Outdated Show resolved Hide resolved
src/templates/finops-hub/modules/hub.bicep Outdated Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Attention πŸ‘‹ Issue or PR needs to be reviewed by the author or it will be closed due to no activity label Mar 17, 2024
@flanakin flanakin removed their assignment Mar 17, 2024
Copy link
Contributor

@sebassem sebassem left a comment

Choose a reason for hiding this comment

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

LGTM, however I have some comments in addition to Michael's

  • Use only the subnetResourceId to indicate a private deployment and drop the publicNetworkAccess parameter
  • If subnetResourceId is specified, lets assess if we can have the deployment scripts also deployed in private, see this and this . Update the readme accordingly if applicable. We will probably need to have a dedicated storage account for scripts since this scenario only supports service endpoints
  • Move the private endpoint deployment for keyVault into the keyVault module for better code organization

@flanakin flanakin added this to the 0.3 milestone Mar 26, 2024
@bradlovas
Copy link

Hi team, any updates on this. Lots of customers extremely interested in deploying this however not being able to deploy with private endpoints is a blocker for them as it's against policy. Thanks!

@patkje75
Copy link
Author

Sorry, I had to down prioritize this due to a hectic schedule, I hope I can look into this and update the requested changes within the coming weeks.

@patkje75
Copy link
Author

patkje75 commented May 8, 2024

Sorry for the delay I have had a very busy time and I will soon be out for a while. I hope I have adressed all you comments.

@sebassem
Copy link
Contributor

Sorry for the delay I have had a very busy time and I will soon be out for a while. I hope I have adressed all you comments.

No worries, I just submitted a PR to your fork with some changes. Please review and test if possible so we can finalize and merge :)

CC: @flanakin @arthurclares

@patkje75
Copy link
Author

@sebassem deploying the resources work, however the deployment script fails

image

It tries to access the FinOps storage account . I haven't had the time to look into all changes you have made, I only removed a Private Endpoint for blob storage because one already exsists.
I am sorry but I wont't have the time to dig into this, I am leaving for holidays in two weeks and I have to finish up two large projects, sorry.

@sebassem
Copy link
Contributor

@sebassem deploying the resources work, however the deployment script fails

image

It tries to access the FinOps storage account . I haven't had the time to look into all changes you have made, I only removed a Private Endpoint for blob storage because one already exsists. I am sorry but I wont't have the time to dig into this, I am leaving for holidays in two weeks and I have to finish up two large projects, sorry.

I assume you don't have a private dns zone for blob storage ? Without it the deployment script won't be able to access it

@patkje75
Copy link
Author

You were right, it was the DNS, we have policies managing registration of private endpoints to the private DNS zones and it hadn't applied yet. I was trying to do to many things at once and didn't check that, sorry.

It seem to work now, I noticed the the name of the export directory had changed from msexports to only exports.
Probably thats why it didn't work the first time I did deploy, since that time th DNS registration worked and everything seemed OK but the pipeline wasn't triggered, now I know why.

I have updated the README.md as well with the new expected name of the export folder.

@patkje75
Copy link
Author

Too fast, I saw the pipeline kick off and left, today I see that i failed.
image
Operation on target Convert CSV failed: ErrorCode=MappingColumnNameNotFoundInSourceFile,'Type=Microsoft.DataTransfer.Common.Shared.HybridDeliveryException,Message=Column 'AvailabilityZone' specified in column mapping cannot be found in 'part_0_0001.csv' source file.,Source=Microsoft.DataTransfer.ClientLibrary,'

Seems like the column AvailabilityZone is missing in the CSV-file. I am using the Amortized cost metric in the Export as written in the README.md.

@sebassem
Copy link
Contributor

sebassem commented May 23, 2024

You were right, it was the DNS, we have policies managing registration of private endpoints to the private DNS zones and it hadn't applied yet. I was trying to do to many things at once and didn't check that, sorry.

It seem to work now, I noticed the the name of the export directory had changed from msexports to only exports. Probably thats why it didn't work the first time I did deploy, since that time th DNS registration worked and everything seemed OK but the pipeline wasn't triggered, now I know why.

I have updated the README.md as well with the new expected name of the export folder.

No worries, thanks for testing this. If the name change affects other parts of the code we can set it back to msexports, maybe I renamed it by mistake. Please let me know when you are happy with your testing so we can merge :)

@sebassem
Copy link
Contributor

Too fast, I saw the pipeline kick off and left, today I see that i failed. image Operation on target Convert CSV failed: ErrorCode=MappingColumnNameNotFoundInSourceFile,'Type=Microsoft.DataTransfer.Common.Shared.HybridDeliveryException,Message=Column 'AvailabilityZone' specified in column mapping cannot be found in 'part_0_0001.csv' source file.,Source=Microsoft.DataTransfer.ClientLibrary,'

Seems like the column AvailabilityZone is missing in the CSV-file. I am using the Amortized cost metric in the Export as written in the README.md.

@flanakin @arthurclares

#### Deployment

1. Login
> az login
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this to Powershell so it aligns with the module we're shipping.

2. Set target subscription
> az account set -s `<SubscriptionId>`
3. Execute deployment
> az deployment group create --resource-group `<RG Name>` --location `<location>` --template-file .\main.bicep --parameters hubName='`<hubName>`' subnetResourceId='`<subnetResourceId>`' scriptsSubnetResourceId='`<scriptsSubnetResourceId>`'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to Powershell

Comment on lines +95 to +99
- The private deployment assumes that the target environment already has the virtual network where the private endpoints will be deployed.
- The virtual network needs to have two subnets; one for the private endpoints and one for the deployment scripts needed for the FinOps Hub.
- The virtual network for the deployment scripts needs to have the [needed configuration](https://learn.microsoft.com/azure/azure-resource-manager/bicep/deployment-script-vnet).
- A service endpoint configured for `Microsoft.Storage`
- A subnet delegation for `Microsoft.ContainerInstance/containerGroups`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could provide this as part of the deployment so customers can retro-fit to their environment rather than leaving it undefined.

Copy link
Contributor

@sebassem sebassem Jun 6, 2024

Choose a reason for hiding this comment

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

@MSBrett are you suggesting deploying the needed infra (vnet, dns zones ,....etc) ? And should it be deployed by default or with a flag ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to just ship it at a vnet which can be peered to a hub as required. What's the minimum address space we need for this?

Copy link
Author

Choose a reason for hiding this comment

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

I would like to have the option to use an existing VNet as well, because deploying a new VNet for a single purpose won't fit all environments, however, this will require some more work, but will give an option for all.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention πŸ‘‹ Issue or PR needs to be reviewed by the author or it will be closed due to no activity Tool: FinOps hubs Data pipeline solution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants