Enable ASO #3723
Enable ASO #3723
Conversation
k8s-ci-robot
added
release-note
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3723 +/- ##
==========================================
- Coverage 55.57% 55.53% -0.05%
==========================================
Files 190 190
Lines 19523 19547 +24
==========================================
+ Hits 10850 10855 +5
- Misses 8064 8080 +16
- Partials 609 612 +3
☔ View full report in Codecov by Sentry. |
k8s-ci-robot
added
size/L
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/test ? |
|
@nojnhuh: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test pull-cluster-api-provider-azure-e2e /test pull-cluster-api-provider-azure-apiversion-upgrade |
|
/test pull-cluster-api-provider-azure-apiversion-upgrade /test pull-cluster-api-provider-azure-e2e /test pull-cluster-api-provider-azure-capi-e2e |
|
@nojnhuh Is this ready for review or would you like some more time to hack away at this? |
Feel free to leave any feedback. I'm not expecting to make any significant changes to this, but there's some orthogonal work that will land in other PRs that this PR depends on. So I'll continue to rebase/force-push this branch until I remove the [WIP]. |
k8s-ci-robot
added
size/XXL
|
Pardon me, just flake hunting 🔍 /test pull-cluster-api-provider-azure-apiversion-upgrade |
|
Workload Identity test should be passing now? |
k8s-ci-robot
removed
the
needs-rebase
|
Just pushed a couple of commits to bring back the old /test pull-cluster-api-provider-azure-e2e-optional |
k8s-ci-robot
added
size/XL
| @@ -59,7 +59,7 @@ func (s *Service) Name() string { | |||
|
|
|||
| // Reconcile idempotently creates or updates a resource group. | |||
| func (s *Service) Reconcile(ctx context.Context) error { | |||
| ctx, _, done := tele.StartSpanWithLogger(ctx, "groups.Service.Reconcile") | |||
| ctx, _, done := tele.StartSpanWithLogger(ctx, "asogroups.Service.Reconcile") | |||
There was a problem hiding this comment.
this won't be possible yet since we're having to keep both packages side by side for now for UserAssigned identity but thoughts on removing the "aso" prefix everywhere in naming and going back to just "groups" as service name once we remove legacy groups?
There was a problem hiding this comment.
Yes, that's the plan.
| AZURE_CLIENT_ID: ${AZURE_CLIENT_ID_B64:=""} | ||
| AZURE_CLIENT_SECRET: ${AZURE_CLIENT_SECRET_B64:=""} | ||
| # Per-resource Secrets will be created based on a Cluster's AzureClusterIdentity. | ||
| AZURE_SUBSCRIPTION_ID: "" |
There was a problem hiding this comment.
uh is that behavior we expect? if not is there an issue tracking this in ASO to make them optional?
| @@ -0,0 +1,55 @@ | |||
| # Azure Service Operator | |||
There was a problem hiding this comment.
thanks for adding docs! 📖
|
@CecileRobertMichon: GitHub didn't allow me to request PR reviews from the following users: matthchr. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-rebase
|
/test pull-cluster-api-provider-azure-e2e-optional |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 7674a93b9bb98dcef7b90350d647a9c6b5d2e6b1
|
|
|
||
| ### Installation | ||
|
|
||
| Beginning with CAPZ v1.11.0, ASO's control plane will be installed automatically by `clusterctl` in the |
There was a problem hiding this comment.
Is this a permanent behavior or at some future date BYO ASO might be supported?
There was a problem hiding this comment.
We haven't yet discussed any plans to support BYO ASO. I think the main roadblock to that is that clusterctl expects certain labels applied to the CRDs for things like clusterctl move to work. In theory, if users add those themselves to the ASO CRDs, then things should work, but that would require a fair bit of testing that we haven't done yet.
And I think we would need clusterctl init to support some conditional logic that could enable/disable the ASO install alongside CAPZ to support existing users relying on ASO being installed that way.
There was a problem hiding this comment.
Do we have an issue to discuss BYO ASO?
There was a problem hiding this comment.
I'm not necessarily saying you need one until such time as a user asks about it - just something worth thinking about ahead of time as it is possible it will come up.
There was a problem hiding this comment.
Opened #3900 to track
There was a problem hiding this comment.
Looks great to me @nojnhuh! Just a few minor questions from my end.
| { | ||
| Scope: azure.ManagedClusterID(s.SubscriptionID(), s.ResourceGroup(), s.ManagedClusterSpec().ResourceName()), | ||
| Tags: s.AdditionalTags(), | ||
| Annotation: azure.ManagedClusterTagsLastAppliedAnnotation, | ||
| }, | ||
| } | ||
| if s.UseLegacyGroups { | ||
| specs = append(specs, azure.TagsSpec{ |
There was a problem hiding this comment.
If using legacy groups, would we need both Tags Specs? I may be misunderstanding here but it looks like we will have two copies of the additional tags here.
There was a problem hiding this comment.
One tags spec is for the resource group and one is for the managed cluster. With ASO, tags for a resource are reconciled alongside the rest of the resource instead of with the separate tags service which these serve as input to. So for an ASO resource, we don't need to define a tags spec.
| Namespace: namespace, | ||
| DeletionTimestamp: &metav1.Time{Time: time.Now()}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This may be out of scope for this PR but it could be worth adding some test cases for UseLegacyGroups: true to cover all cases.
There was a problem hiding this comment.
I did try adding some but it turned out to be super messy and I couldn't justify fighting through that since ideally we'll drop that flag entirely in the next week or so. I can come back to these if we think it will stick around much longer than that.
|
/lgtm |
|
I squashed most of this but left two commits so we can revert using ASO for resource groups independently from installing ASO altogether if need be. /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Quota issue should be fixed. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR enables all of the currently-merged ASO functionality which was previously inaccessible to users. Specifically, this change will install ASO alongside CAPZ and use it to manage resource groups.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #3527, fixes #3523, fixes #3532
Special notes for your reviewer:
This PR is a work-in-progress opened early to start getting test signal and accumulate changes from other contributors as necessary. Reviews are welcome but I plan to rebase/force-push this branch frequently before removing the [WIP] status.
TODOs:
Release note: