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: Replications and WAF alignment for App Store - avm/res/app-configuration/configuration-store #1696

Merged
merged 41 commits into from
May 1, 2024
Merged

feat: Replications and WAF alignment for App Store - avm/res/app-configuration/configuration-store #1696

merged 41 commits into from
May 1, 2024

Conversation

JFolberth
Copy link
Contributor

@JFolberth JFolberth commented Apr 18, 2024

Description

  • Added replicas
  • Update settings per pester results
  • ReadMes Update
  • Configured replicas for WAF alignment
  • Removed orphan Markdown

Fixes #1502
Closes #1502
-->

Pipeline Reference

Pipeline
avm.res.app-configuration.configuration-store

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

@JFolberth JFolberth requested review from a team as code owners April 18, 2024 02:54
@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 Apr 18, 2024
@JFolberth
Copy link
Contributor Author

@matebarabas @eriqua

Looking for feedback on this on two items.

I was not sure on replicas what to do with locations...I found avm\res\aad\domain-service\tests\e2e\waf-aligned\main.test.bicep where locations are hard coded and figured maybe this is how it should be handled?

I also per https://azure.github.io/PSRule.Rules.Azure/en/rules/Azure.AppConfig.AuditLogs/ Audit logs are required and I wasn't sure what to put for diagnostic log locations for the default testing. I left them all (Event Hub, Storage Account, Workspace). I felt this was heavy and not sure on guidance.

@JFolberth JFolberth changed the title WAF Alignment Feat: Replications Apr 18, 2024
@JFolberth JFolberth changed the title Feat: Replications Feat: Replications for App Store Apr 18, 2024
@eriqua eriqua changed the title Feat: Replications for App Store Feat: Replications and WAF alignment for App Store Apr 18, 2024
@eriqua eriqua added Class: Resource Module 📦 This is a resource module and removed Needs: Triage 🔍 Maintainers need to triage still labels Apr 18, 2024
@eriqua
Copy link
Contributor

eriqua commented Apr 18, 2024

@matebarabas @eriqua

Looking for feedback on this on two items.

I was not sure on replicas what to do with locations...I found avm\res\aad\domain-service\tests\e2e\waf-aligned\main.test.bicep where locations are hard coded and figured maybe this is how it should be handled?

I also per https://azure.github.io/PSRule.Rules.Azure/en/rules/Azure.AppConfig.AuditLogs/ Audit logs are required and I wasn't sure what to put for diagnostic log locations for the default testing. I left them all (Event Hub, Storage Account, Workspace). I felt this was heavy and not sure on guidance.

For the last point, those rules are currently not blocking as they are not part of the WAF reliability pillar. I'm not saying we shouldn't care about them, just that are not a priority 1 and can be taken for example in a next PR.

That said, as a rule of thumb, defaults tests should only cover required parameters, so audit logs rules can be exempted for defaults tests only. The exemption file specific for defaults tests is https://github.com/Azure/bicep-registry-modules/blob/main/avm/utilities/pipelines/staticValidation/psrule/.ps-rule/min-suppress.Rule.yaml

@JFolberth
Copy link
Contributor Author

@matebarabas @eriqua
Looking for feedback on this on two items.
I was not sure on replicas what to do with locations...I found avm\res\aad\domain-service\tests\e2e\waf-aligned\main.test.bicep where locations are hard coded and figured maybe this is how it should be handled?
I also per https://azure.github.io/PSRule.Rules.Azure/en/rules/Azure.AppConfig.AuditLogs/ Audit logs are required and I wasn't sure what to put for diagnostic log locations for the default testing. I left them all (Event Hub, Storage Account, Workspace). I felt this was heavy and not sure on guidance.

For the last point, those rules are currently not blocking as they are not part of the WAF reliability pillar. I'm not saying we shouldn't care about them, just that are not a priority 1 and can be taken for example in a next PR.
That said, as a rule of thumb, defaults tests should only cover required parameters, so audit logs rules can be exempted for defaults tests only. The exemption file specific for defaults tests is https://github.com/Azure/bicep-registry-modules/blob/main/avm/utilities/pipelines/staticValidation/psrule/.ps-rule/min-suppress.Rule.yaml

@JFolberth now finally replying on the first point :)
The CI uses a region rotation for the deployment. So every test deployment may target a different location randomly picked up from this list: eastus, uksouth, northeurope and eastasia. That is unless a location is hardcoded in the test file, like the example you mentioned for AADDS. In this case, I guess we can rely on the default logic for the primary resource, while we should hardcode the location for the replica in the test, as you did already.
I'm not an expert of App Store, but I'd expect it is not allowed to use the same location for the primary resource and the replica. So if, for example, the rotation logic picks 'eastus' for the primary resource, using 'eastus' for the replica would probably fail the deployment. Is that correct? If so, for the hardcoded replica location we should pick a set not including the 4 locations we are rotating from.

I am not overly familiar with app stores as well. I have updated. My thought was with two to 1.) ensure we hit a region that the resource is not deployed to and 2.) ensure it can handle multiple locations. I have just updated it not to use westus and centralus so this should be accomplished.

Sounds great. Just added a suggestion for the other test file to do the same. Overall it looks good. As a final step, could you please

  • Rerun the set-AVMModule utility to update the Readmes
  • Retrigger the pipeline manually to verify if the run works after enabling purge protection by default. Based on the result I may need to look into the CI deployment removal logic.

Thanks!

All updated!

@AlexanderSehr AlexanderSehr requested a review from eriqua April 29, 2024 20:06
Copy link
Contributor

@eriqua eriqua left a comment

Choose a reason for hiding this comment

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

🆗

@eriqua eriqua merged commit 817153b into Azure:main May 1, 2024
5 checks passed
@JFolberth JFolberth deleted the fix/configuration-store/orphan branch May 1, 2024 12:50
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
Development

Successfully merging this pull request may close these issues.

[WAF - Reliability Review] - avm/res/app-configuration/configuration-store
2 participants