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

Handle empty converts #181

Merged
merged 4 commits into from
Jan 22, 2024
Merged

Conversation

lsviben
Copy link
Collaborator

@lsviben lsviben commented Jan 21, 2024

Description of your changes

Fix for #179 . Handles the conversion case from v1alpha2 to v1alpha1 when the spec.managementPolicies are unset which was causing the issue.

This was happening when:

  1. A resource existed in v1alpha1 version
  2. The provider was upgraded to v0.11.0 (v1alpha2)
  3. The Composition was updated to use v1alpha2
  4. The resource in the Composition does not have a spec.managementPolices set (relies on defaulting)

So what happened was that the v1alpha2 Object, without the spec.managementPolicies was being converted to v1alpha1 during the Composition reconciliation. This could be because it is updating the existing resource which is still stored in v1alpha1 version, although after the first update, the resource should be stored in v1alpha2 version as its the storage version, based on docs.

As the defaulting happens after conversion, the conversion fails. Now this is handled so that if the resource does not have a spec.managementPolicies set or a CreationTimestamp (this is to differentiate between converting resources paused through management policies and resources from Compositions), the spec.managementPolicy is set to Default.

Additionally, to avoid conversion errors, if the policy we are converting from v1alpha2 to v1alpha1 is unsupported, the spec.managementPolicy field is left empty. I was thinking either to leave it empty, which will default to Default if the resource is created, or set it to Observe to be safe, but decided on Default as it seems like the most fitting based on the other checks. If its not being created, it will be empty and users will need to manual set it. This was needed so that v1alpha2 policies like spec.managementPolicies: ["Observe", "Update"] can be used in the above case as well.

Fixes #179

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Was testing using the reproduction setup for the #179 bug from @haarchri (thanks again for setting it up, really helpful!).

The example from issue, using the setup.sh now passes. The resulting Object has spec.managementPolicies: ["*"]

Also tried with setting an unsupported policy in the Composition like spec.managementPolicies: ["Observe", "Update"] and that works as well. The resulting Object has spec.managementPolicies: ["Observe", "Update"]

@@ -101,7 +101,7 @@ func (src *Object) ConvertTo(dstRaw conversion.Hub) error { // nolint:golint //
case Observe:
dst.Spec.ManagementPolicies = xpv1.ManagementPolicies{xpv1.ManagementActionObserve}
default:
return errors.New("unknown management policy")
return fmt.Errorf("unknown management policy: %v", src.Spec.ManagementPolicy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: could we use errors.Errorf instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure we can!

Signed-off-by: lsviben <[email protected]>
@lsviben lsviben merged commit d851450 into crossplane-contrib:main Jan 22, 2024
8 checks passed
Copy link

Backport failed for release-0.11.0, because it was unable to create/access the git worktree directory.

Please cherry-pick the changes locally.

git fetch origin release-0.11.0
git worktree add -d .worktree/backport-181-to-release-0.11.0 origin/release-0.11.0
cd .worktree/backport-181-to-release-0.11.0
git checkout -b backport-181-to-release-0.11.0
ancref=$(git merge-base cc10a8c08e0e16109fb5d34ef4f4aee84f89d58b b4543db477847d99b1f6da91d18a177bf90894ea)
git cherry-pick -x $ancref..b4543db477847d99b1f6da91d18a177bf90894ea

lsviben added a commit to lsviben/provider-kubernetes that referenced this pull request Jan 22, 2024
lsviben added a commit to lsviben/provider-kubernetes that referenced this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants