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

ASOAPI: add resource mutator framework #4793

Merged
merged 3 commits into from
May 1, 2024

Conversation

nojnhuh
Copy link
Contributor

@nojnhuh nojnhuh commented Apr 30, 2024

What type of PR is this?
/kind bug

What this PR does / why we need it:

The main gap in the ASOAPI implementations at the moment is that certain parts of the CAPI API that map to Azure resource parameters, like MachinePool spec.replicas -> ManagedClustersAgentPool spec.count, are not ultimately propagated to ASO. This PR adds a framework for making those kinds of transformations and a few examples.

The way this works is roughly:

  • Before the spec.resources are reconciled, some functions can be run to inject/modify values to the ASO resources (mutators.ApplyMutators()).
    • These dynamic values are *not* persisted back to the CAPZ resource because the ClusterClass controller will start to fight with CAPZ, to prevent duplicate data from appearing in CAPZ specs (e.g. AzureASOManagedControlPlane's spec.version and an embeddedManagedCluster's spec.kubernetesVersion), and to maintain the philosophy that spec is what's defined by the user.
  • For each individual mutated field, CAPZ will return an error if the user defines a value that's different from what CAPZ needs to set. The error instructs the user to either match CAPZ or remove that field from their ASO resource embedded in the CAPZ spec.
  • When optional CAPI fields, like MachinePool's spec.template.spec.version, are nil, CAPZ will allow users to define the equivalent field on the ASO resource (ManagedClustersAgentPool's spec.orchestratorVersion) to any value allowed by Azure. In this context of a node pool's Kubernetes version, this allows users to leverage the AKS API's flexibility to define only MAJOR.MINOR versions which aren't allowed in the MachinePool API (Allow omitting the k8s patch version in azuremanagedcontrolplanes #4111).
  • Mutations are performed on unstructured representations of resources to reduce our dependence on knowledge of individual AKS API versions.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #4713

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 30, 2024
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 85.83333% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 62.60%. Comparing base (d817ab3) to head (dc69aa6).

Files Patch % Lines
exp/mutators/azureasomanagedcontrolplane.go 85.88% 6 Missing and 6 partials ⚠️
exp/mutators/mutator.go 80.00% 3 Missing and 1 partial ⚠️
...trollers/azureasomanagedcontrolplane_controller.go 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4793      +/-   ##
==========================================
+ Coverage   62.48%   62.60%   +0.12%     
==========================================
  Files         196      198       +2     
  Lines       16188    16285      +97     
==========================================
+ Hits        10115    10196      +81     
- Misses       5333     5343      +10     
- Partials      740      746       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

Looks good overall on first pass! Just some questions on my end and I'll go through this again to understand everything better.

exp/mutators/azureasomanagedcontrolplane.go Outdated Show resolved Hide resolved
exp/mutators/azureasomanagedcontrolplane.go Show resolved Hide resolved
exp/mutators/mutator_test.go Show resolved Hide resolved
@nojnhuh
Copy link
Contributor Author

nojnhuh commented Apr 30, 2024

/hold to squash fixups

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2024
Copy link
Contributor

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

My only thoughts for the future would be to make it a bit more explicit in the libraries (rename exported methods?) that what we're doing here assumes RAW JSON, or to generalize and implement for RAW JSON, while allowing pluggable data structures as well in the future.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: fd7280a0e3d5ddeb182446bdf887bc2033c2bbda

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2024
@nojnhuh
Copy link
Contributor Author

nojnhuh commented May 1, 2024

squashed!
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2024
@willie-yao
Copy link
Contributor

/lgtm

@nojnhuh Just checking: Did you mean to squash to 3 commits?

@nojnhuh
Copy link
Contributor Author

nojnhuh commented May 1, 2024

/lgtm

@nojnhuh Just checking: Did you mean to squash to 3 commits?

Yes, I think each one is distinct enough to warrant staying separate.

@k8s-ci-robot k8s-ci-robot merged commit abba169 into kubernetes-sigs:main May 1, 2024
19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 1, 2024
@nojnhuh nojnhuh deleted the v2-mutators branch May 1, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants