finops-toolkit icon indicating copy to clipboard operation
finops-toolkit copied to clipboard

Added Private Endpoint deployment option

Open patkje75 opened this issue 1 year ago • 11 comments

  • 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
  • [X] 🤞 PS -WhatIf / az validate
  • [X] 👍 Manually deployed + verified
  • [ ] 💪 Unit tests
  • [X] 🙌 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)
  • [x] ✅ 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 avatar Feb 19 '24 12:02 patkje75

@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

patkje75 avatar Feb 19 '24 14:02 patkje75

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!

bradlovas avatar Apr 24 '24 14:04 bradlovas

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 avatar Apr 25 '24 21:04 patkje75

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.

patkje75 avatar May 08 '24 15:05 patkje75

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

sebassem avatar May 15 '24 16:05 sebassem

@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.

patkje75 avatar May 22 '24 15:05 patkje75

@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

sebassem avatar May 22 '24 16:05 sebassem

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 avatar May 22 '24 20:05 patkje75

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.

patkje75 avatar May 23 '24 05:05 patkje75

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 avatar May 23 '24 06:05 sebassem

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

sebassem avatar May 23 '24 06:05 sebassem

@allcontributors Please add patkje75 for code

flanakin avatar Nov 17 '24 06:11 flanakin

@flanakin

I've put up a pull request to add @patkje75! :tada:

allcontributors[bot] avatar Nov 17 '24 06:11 allcontributors[bot]

We ended up tweaking the design a bit and pulling this into a new pull request. Thanks for the contribution!

flanakin avatar Nov 17 '24 06:11 flanakin