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

Migrate to XP ManagementPolicies #163

Merged
merged 26 commits into from
Jan 11, 2024
Merged

Conversation

lsviben
Copy link
Collaborator

@lsviben lsviben commented Nov 28, 2023

Description of your changes

Provider-kubernetes started using its own version of ManagementPoliciey before the core Crossplane. In the meantime, Crossplane developed its own native ManagementPolicies. This PR tries to smoothly migrate to the Crossplane ones.

⚠️ Bumps Object version from v1alpha1 to v1alpha2. There should not be any manual steps needed, as we are using conversion webhook to convert from v1alpha1 to v1alpha2.

⚠️ This will not work with Crossplane versions v1.14.0 and v1.14.1 because the provider service was assigned a wrong name, fixed in 1.14.2.

Fixes #162

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

make e2e

make e2e

local on kind with examples

  1. local deploy based on the Readme
  2. Go through examples/object/policy and test each yaml.
  3. Test with examples/in-composition

and

there is a difference between

kubectl delete/get -f examples/object/object.yaml
and
kubectl delete/get -f objects.kubernetes.crossplane.io sample-namespace

the first one will convert the result back to v1alpha1 if the object.yaml is in that version, the second one will just use v1beta1 without conversion (stored version).

This is important for DELETE for example because if the conversion FROM v1beta1 to v1alpha1 is not implemented correctly, the DELETED object will be saved as the "translated" object v1alpha2 -> v1alpha1 -> v1alpha2. If the translation back to v1alpha1 is not made correctly it could lose some important fields.(learned that the hard way :) )

Tested with a configuration.

@lsviben
Copy link
Collaborator Author

lsviben commented Nov 28, 2023

For now the combo that translates from ObserveDelete - > ["Observe", "Delete"] is not supported by default by controller-runtime. Ill add a PR which adds it to the list

@lsviben
Copy link
Collaborator Author

lsviben commented Nov 28, 2023

Still needs something to keep the objject webhook config in the yaml. WIP

@lsviben
Copy link
Collaborator Author

lsviben commented Nov 29, 2023

For now the combo that translates from ObserveDelete - > ["Observe", "Delete"] is not supported by default by controller-runtime. Ill add a PR which adds it to the list

added a PR for above

@lsviben
Copy link
Collaborator Author

lsviben commented Dec 1, 2023

The codecov/project fails but not sure where to add more tests. I assume this is because v1beta1 Object was added? Most code is added in conversion and that has tests

Copy link
Collaborator

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Looking pretty good, great work @lsviben 🙌

I like the way how you solved injecting conversion config to CRDs, even though it introduced another layer, I agree that there is no better way.

Two concerns that I have in general :

  1. This would require a Crossplane version with the recent fix, so:
  • We should test what happens if someone with an old XP version, bumps provider-kubernetes to latest with this change.
  • Make it explicit as much as we can that this requires a specific XP version.
  1. Could you also test how this behaves when Objects are part of compositions and may be compositions being part of Configurations. So, basically, what would be the migration path we suggest? Nothing to do by users? can we validate that?

Makefile Show resolved Hide resolved
cmd/provider/main.go Show resolved Hide resolved
examples/object/migration/default.yaml Outdated Show resolved Hide resolved
examples/object/policy/observe.yaml Show resolved Hide resolved
internal/controller/object/object.go Outdated Show resolved Hide resolved
@lsviben
Copy link
Collaborator Author

lsviben commented Dec 15, 2023

During my testing I noticed that in some cases the provider-kubernetes logs:

http: TLS handshake error from 192.168.183.0:54694: EOF

Not sure yet why. Other then this in the logs, the conversion works fine, and nothing else looks amiss. But I would like to check more before merging this. cc: @turkenh

@turkenh
Copy link
Collaborator

turkenh commented Dec 18, 2023

For those who want to test this PR with the latest state (i.e. dea49e1), I just built/pushed an image which could be installed/upgraded to:

index.docker.io/turkenh/provider-kubernetes:v0.10.0-rc.0.39.gdea49e1

turkenh and others added 17 commits January 9, 2024 10:07
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Looks like we need to have at least v1beta1 for conversion,
otherwise getting:

invalid: spec.conversion.conversionReviewVersions: Invalid value: []string{"v1alpha2"}: must include at least one of v1, v1beta1

Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
@lsviben
Copy link
Collaborator Author

lsviben commented Jan 10, 2024

I noticed TLS errors in the logs:

2024/01/10 13:27:39 http: TLS handshake error from x.x.x.0:33524: EOF
2024/01/10 13:27:39 http: TLS handshake error from x.x.x.0:56824: EOF
2024/01/10 13:27:39 http: TLS handshake error from x.x.x.0:48834: EOF

Everything seems to be working fine with the conversion, and nothing came out of my investigation. Found that it seems like a known issue from Go which appears on Kubernetes .

So dropping further investigation of this for now, the PR should be good to go

Copy link
Collaborator

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Thank you @lsviben 🙌

I've successfully validated that conversion works well and observed no functional issues. Regarding the TLS error logs, I would suggest not blocking this PR and create a separate ticket to track it further.

@turkenh turkenh merged commit cc10a8c into crossplane-contrib:main Jan 11, 2024
7 of 8 checks passed
@lsviben lsviben mentioned this pull request Jan 12, 2024
@@ -184,7 +184,7 @@ spec:
string:
fmt: "%s-argocd"
- base:
apiVersion: kubernetes.crossplane.io/v1alpha1
apiVersion: kubernetes.crossplane.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

think this needs to be fixed in master branch

kubernetes.crossplane.io/v1alpha2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is actually the ProviderConfig, we didnt touch versioning of that. But the below one for the Object is wrong, ill fix it. Thanks for noticing!

@@ -206,7 +206,7 @@ spec:
readinessChecks:
- type: None
- base:
apiVersion: kubernetes.crossplane.io/v1alpha1
apiVersion: kubernetes.crossplane.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

think this needs to be fixed in master branch

kubernetes.crossplane.io/v1alpha2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from custom ManagementPolicy to Crossplane native ManagementPolicies
3 participants