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

feat: Create\Update User Defined Types with sensitive values to include @secure() decorator. - avm/res/container-instance/container-group #1919

Merged
merged 10 commits into from
May 14, 2024

Conversation

rodney-almeida
Copy link
Contributor

Description

Update containerType (user defined type) to set environmentVariables.secureValue to be of type secureString.
Followed the same logic and created the user defined type for imageRegistryCredentials (imageRegistryCredentialType) as it also has a password attribute.

Fixes #1868
Closes #1868
-->

Pipeline Reference

Pipeline
avm.res.container-instance.container-group

Type of Change

  • Update to CI Environment or utlities (Non-module effecting changes)
  • Azure Verified Module updates:
    • Bugfix containing backwards compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
      • Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
      • The bug was found by the module author, and no one has opened an issue to report it yet.
    • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
    • Breaking changes and I have bumped the MAJOR version in version.json.
    • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • I have run Set-AVMModule locally to generate the supporting module files.
  • My corresponding pipelines / checks run clean and green without any errors or warnings

RodneyAlmeidaTEKenable and others added 9 commits April 26, 2024 15:09
The container group main.json file was updated to use a newer version of the bicep generator (0.27.1.19265) and a different template hash. Additionally, the livenessProbe property was added to the container group definition.

In the test deployment file, the commented out environment variables were removed and the CLIENT_ID and CLIENT_SECRET environment variables were added with test values.
@rodney-almeida rodney-almeida requested review from a team as code owners May 13, 2024 11:15
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels May 13, 2024
@rodney-almeida rodney-almeida changed the title Rodney almeida 1868 feat: Create\Update User Defined Types with sensitive values to include @secure() decorator. May 13, 2024
@AlexanderSehr
Copy link
Contributor

Looks great @rodney-almeida,
thank you for your contribution, pipeline-badge and all. @JPEasier please take a look when you have a moment :)

@matebarabas matebarabas changed the title feat: Create\Update User Defined Types with sensitive values to include @secure() decorator. feat: Create\Update User Defined Types with sensitive values to include @secure() decorator. - avm/res/container-instance/container-group May 13, 2024
@matebarabas matebarabas added Class: Resource Module 📦 This is a resource module and removed Needs: Triage 🔍 Maintainers need to triage still labels May 13, 2024
@JPEasier
Copy link
Contributor

Thank You @rodney-almeida for your contribution, looks greate! 🚀

@JPEasier JPEasier merged commit b2790c0 into Azure:main May 14, 2024
10 checks passed
@rodney-almeida rodney-almeida deleted the rodney-almeida-1868 branch May 14, 2024 08:07
@rodney-almeida
Copy link
Contributor Author

@JPEasier, looks like the pipeline failed to delete some resources when running the tests after the merge. As a result the new version hasn't been published. You able to kick off the pipeline again?

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented May 15, 2024

@JPEasier, looks like the pipeline failed to delete some resources when running the tests after the merge. As a result the new version hasn't been published. You able to kick off the pipeline again?

Hey @rodney-almeida & @JPEasier, just fyi, I saw this and already tried troubleshooting. The workflow is facing the same issue I was in the Portal. The service is borderline refusing to be removed. This happend with 2 or 3 other services in the past were we ended up needing support questions to get them removed. I hope this won't be needed here, and I'll keep on trying. Once the service is removed, we can re-run only the 'failed jobs' of the failed pipeline.
Fyi @JPEasier, we should not start a new workflow run for main as this would not tigger the publishing (as there would be no diff).

Note: Ok I give up, opening a support ticket

@rodney-almeida
Copy link
Contributor Author

@AlexanderSehr, any luck with the support ticket?
Keen to stop using my local bandaid version and get back to using the AVM module :)
Thanks

@AlexanderSehr
Copy link
Contributor

@AlexanderSehr, any luck with the support ticket? Keen to stop using my local bandaid version and get back to using the AVM module :) Thanks

Hey @rodney-almeida, the ticket was acknoledged but is yet to be resolved. I'll keep you posted 💪

@rodney-almeida
Copy link
Contributor Author

@AlexanderSehr, any luck with the support ticket? Keen to stop using my local bandaid version and get back to using the AVM module :) Thanks

Hey @rodney-almeida, the ticket was acknoledged but is yet to be resolved. I'll keep you posted 💪

Any news?
Thanks

@AlexanderSehr
Copy link
Contributor

@AlexanderSehr, any luck with the support ticket? Keen to stop using my local bandaid version and get back to using the AVM module :) Thanks

Hey @rodney-almeida, the ticket was acknoledged but is yet to be resolved. I'll keep you posted 💪

Any news? Thanks

Not yet, no. Just yesterday I was asked by the ticket owner to try and run the removal via PowerShell and the Azure CLI, which, as expected, did not help.

@rodney-almeida
Copy link
Contributor Author

@AlexanderSehr any luck yet?

@AlexanderSehr
Copy link
Contributor

@AlexanderSehr any luck yet?

Yes and no. I did get some feedback that the issue is connected to the deployed MSI (and the order of removal) which is why I opened the PR #2180.
I have to port the PR over to my fork though to test there, as the AVM test subscription still has the faulty container-group deployed, and I cannot deploy over it. Support is still working on that part.

Will let you know once I was able to validate that the suggested update to the removal logic helped.

hundredacres pushed a commit to hundredacres/bicep-registry-modules that referenced this pull request Jun 19, 2024
@secure() decorator. - `avm/res/container-instance/container-group` (Azure#1919)

## Description
Update containerType (user defined type) to set
environmentVariables.secureValue to be of type secureString.
Followed the same logic and created the user defined type for
imageRegistryCredentials (imageRegistryCredentialType) as it also has a
password attribute.

Fixes Azure#1868
Closes Azure#1868
-->

## Pipeline Reference

| Pipeline |
| -------- |

[![avm.res.container-instance.container-group](https://github.com/rodney-almeida/bicep-registry-modules/actions/workflows/avm.res.container-instance.container-group.yml/badge.svg?branch=rodney-almeida-1868)](https://github.com/rodney-almeida/bicep-registry-modules/actions/workflows/avm.res.container-instance.container-group.yml)

## Type of Change

<!-- Use the check-boxes [x] on the options that are relevant. -->

- [ ] Update to CI Environment or utlities (Non-module effecting
changes)
- [ ] Azure Verified Module updates:
- [X] Bugfix containing backwards compatible bug fixes, and I have NOT
bumped the MAJOR or MINOR version in `version.json`:
- [X] Someone has opened a bug report issue, and I have included "Closes
#{bug_report_issue_number}" in the PR description.
- [ ] The bug was found by the module author, and no one has opened an
issue to report it yet.
- [ ] Feature update backwards compatible feature updates, and I have
bumped the MINOR version in `version.json`.
- [ ] Breaking changes and I have bumped the MAJOR version in
`version.json`.
  - [ ] Update to documentation

## Checklist

- [X] I'm sure there are no other open Pull Requests for the same
update/change
- [X] I have run `Set-AVMModule` locally to generate the supporting
module files.
- [X] My corresponding pipelines / checks run clean and green without
any errors or warnings

<!-- Please keep up to day with the contribution guide at
https://aka.ms/avm/contribute/bicep -->

---------

Co-authored-by: Rodney Almeida <[email protected]>
@rodney-almeida
Copy link
Contributor Author

@AlexanderSehr any luck yet?

Yes and no. I did get some feedback that the issue is connected to the deployed MSI (and the order of removal) which is why I opened the PR #2180. I have to port the PR over to my fork though to test there, as the AVM test subscription still has the faulty container-group deployed, and I cannot deploy over it. Support is still working on that part.

Will let you know once I was able to validate that the suggested update to the removal logic helped.

Sorry to bug @AlexanderSehr but have we had any progress on this?

@AlexanderSehr
Copy link
Contributor

@AlexanderSehr any luck yet?

Yes and no. I did get some feedback that the issue is connected to the deployed MSI (and the order of removal) which is why I opened the PR #2180. I have to port the PR over to my fork though to test there, as the AVM test subscription still has the faulty container-group deployed, and I cannot deploy over it. Support is still working on that part.
Will let you know once I was able to validate that the suggested update to the removal logic helped.

Sorry to bug @AlexanderSehr but have we had any progress on this?

Hey @rodney-almeida,
nothing to be satsified with, no.

The PR with the presumed change got merged some time ago: #2202

But the last response I got from support (from the 9th) was that they're waiting for the escelation in the back to proceed. So we're waiting, the middle-man is waiting, and we can just hope that to gears are turning somewhere. I'll ping the engineer again regardless.

@RodneyAlmeidaTEKenable
Copy link
Contributor

@AlexanderSehr, I know this is an old request but have we been able to get anywhere with this? I came across this today because I needed to use this module again and still have to revert to using my local copy as the version from this PR hasn't been published yet.

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented Oct 2, 2024

@AlexanderSehr, I know this is an old request but have we been able to get anywhere with this? I came across this today because I needed to use this module again and still have to revert to using my local copy as the version from this PR hasn't been published yet.

oh wow. Thanks for calling this out @RodneyAlmeidaTEKenable. The pipeline started working again quite a while ago but it seems as no change that influences the module's functionality was merged since, no new version was published. Let me give it a shot with a new PR.

Edit: Once this is merged, a new module version should be available: #3392

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Class: Resource Module 📦 This is a resource module Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
5 participants