-
Notifications
You must be signed in to change notification settings - Fork 1
V2 Improvements #2
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
Conversation
50b1f9a to
5c93cdd
Compare
WIP: Improved version of CNOE AWS Reference Implementation - New directory structure - Leverages ArgoCD Application Sets - Deploy the IDP to a remote EKS cluster from a kind cluster (ie. idpbuilder) Fixes: #49 --------- Signed-off-by: Pankaj Walke <punkwalker@gmail.com> Signed-off-by: Carlos Santana <carrlos@amazon.com> Co-authored-by: Carlos Santana <carrlos@amazon.com> Signed-off-by: Bowen Sun <bowensun@gmail.com>
…refactor components for azure + remove aws specific configs Signed-off-by: Bowen Sun <bowensun@gmail.com>
Signed-off-by: Bowen Sun <bowensun@gmail.com>
Signed-off-by: Bowen Sun <bowensun@gmail.com>
Signed-off-by: Bowen Sun <bowensun@gmail.com>
Signed-off-by: Bowen Sun <bowensun@gmail.com>
Signed-off-by: Bowen Sun <bowensun@gmail.com>
csantanapr
left a comment
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.
Thank you @nusnewob and team for this great V2 implementation, I captured some items that I think we should go back to the aws implementation and do the same way you did here
Everything looks good the only thing would be good to update the is the appset-chart to match aws to have consistency make it more top level folder
🔍 Azure Reference Implementation PR Review
✅ Approved Aspects
- Correct ArgoCD ApplicationSet Pattern: Properly implements GitOps Bridge with ApplicationSets ✓
- Modern Build System - Taskfile: Superior to shell scripts for cross-platform automation ✓
- Modern Deployment Tool - Helmfile: Declarative Helm chart management vs manual helm commands ✓
- Azure-Native Authentication: Proper Azure Workload Identity implementation ✓
- External Secrets Integration: Correctly uses Azure Key Vault ✓
- Better Metadata Usage: Uses
.metadata.annotationsappropriately ✓
⚠️ Required Changes Before Merge (3% Misalignment)
Critical (Must Fix)
-
Directory Structure Alignment:
Current: packages/charts/appset/ Required: packages/appset-chart/ -
Bootstrap Path Fix in
packages/bootstrap.yaml:# Change from: path: charts/appset # To: path: appset-chart
☁️ Azure-Specific Differences (Correctly Implemented)
These differences are appropriate and expected for Azure:
-
External DNS Configuration:
- Uses Azure DNS provider vs AWS Route53
- Requires Azure config file mount vs AWS IAM roles
-
Crossplane Providers:
- Uses
provider-family-azurevsprovider-aws - Includes Azure-specific providers (authorization, managedidentity)
- Uses
-
Authentication Method:
- Uses Azure Workload Identity vs AWS Pod Identity
- Different annotation patterns (
azure.workload.identity/*)
-
Secret Management:
- Integrates with Azure Key Vault vs AWS Secrets Manager
- Different external-secrets configuration
🚀 Future Azure Enhancement Recommendations
- Add Azure Compositions: Create equivalent to AWS
crossplane-compositionspackage for Azure-specific resource compositions
📊 Overall Assessment
- Alignment Score: 97% with AWS reference
- Recommendation: Approve with required changes
- Key Strengths: Superior build system (Taskfile + Helmfile) + proper metadata usage + better crossplane structure + appropriate Azure adaptations
Status: Ready to merge after addressing the 2 structural alignment issues above.
🔄 Recommendations for AWS Reference Implementation
The Azure implementation introduces superior practices that should be backported to AWS:
-
Adopt Taskfile: Replace shell scripts with Taskfile (reference: [
Taskfile.yml]https://github.com/livewyer-ops/reference-implementation-azure/blob/v2/Taskfile.yml)) for better maintainability and cross-platform support -
Adopt Helmfile for Bootstrap: Replace manual helm commands with Helmfile (reference:
helmfile.yaml) for declarative ArgoCD and ApplicationSet deployment -
Use
.metadata.annotations: Update AWS implementation to use annotations instead of labels inpackages/addons/values.yamlfor:.metadata.annotations.pathRouting(instead of.metadata.labels.path_routing).metadata.annotations.domain(instead of.metadata.labels.domain).metadata.annotations.letsencryptEnv(instead of.metadata.labels.letsencrypt_env)
-
Simplify Crossplane Provider Structure: Remove separate
crossplane-aws-upboundpackage and integrate providers directly into main crossplane configuration like Azure does
Signed-off-by: Bowen Sun <bowensun@gmail.com>
csantanapr
left a comment
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.
/lgtm
Description
Improve deployment process:
Taskfile.ymlto replace shell scriptshelmfile.yamlto manage initial helm chart installpackages/to work on Azure AKS, and remove EKS specific configsAdditional Information
Type of Change