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.

Sign up for GitHub

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

Jump to bottom

Convert all singleton lists in the MR APIs to embedded objects #123

Merged
merged 9 commits into from
May 8, 2024
Merged

Convert all singleton lists in the MR APIs to embedded objects #123

merged 9 commits into from
May 8, 2024

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Apr 17, 2024 •
edited
Loading

Description of your changes

Depends on: crossplane/upjet#387

Terraform configuration blocks, even if they have a MaxItems constraint of 1, are (almost) always generated as lists. We
now generate the lists with a MaxItems constraint of 1 as embedded objects in our MR APIs.

This also helps when updating or patching via SSA the (previously list) objects. The merging strategy implemented by SSA requires configuration for associative lists and converting the singleton lists into embedded objects removes the configuration need.

The provider generates the converted embedded objects in the new v1beta2 versions of the CRD APIs and registers upjet's identity converter and the singleton list converters to be invoked, in chain, by the CRD API conversion webhooks. This implies that the v1beta1 versions stay intact, and old clients not aware of the new APIs should continue functioning as before (backwards-compatibility). Any clients willing to use the embedded objects-based APIs should be updated to use the v1beta2 versions of the CRD APIs.

If a resource contains no singleton lists, then the v1beta2 version is not generated for it.

We had to configure field name overrides for HTTP{S}HealthCheck.compute and Version.dialogflowcx so that the correspnding type names for the spec.forProvider, spec.initParameter and state.atProvider fields are not duplicated with the introduction of v1beta2 packages.

We have also removed the SSA merge strategy configurations from the v1beta2 versions of Cluster.container & NodePool.container resources because the related fields are no longer lists and have been converted to embedded objects in their v1beta2 versions.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.
  • New version for the converted CRD APIs
  • Adjust the manually authored example manifests
  • API Conversion functions (invoked by the Conversion webhooks) to convert between the new & old API versions

How has this code been tested

A successful uptest run with the converted APIs is observed here: https://github.com/crossplane-contrib/provider-upjet-azuread/actions/runs/8723779942

Examples from successful uptest runs with conversion Webhooks enabled:

Uptest runs for available uptestable resources:

v1beta1 resources

v1beta2 ressources

@ulucinar ulucinar marked this pull request as draft April 17, 2024 14:30
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/accesspolicy.yaml"

1 similar comment
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

1 similar comment
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar ulucinar force-pushed the embed-singleton branch 2 times, most recently from e53de92 to bf9bf2a Compare April 18, 2024 00:17
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

1 similar comment
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/location.yaml"

@ulucinar
Copy link
Collaborator Author

Hi @sergenyalcin,
Looks like the status subresource we are doing as part of the import tests (after scaling the provider deployment down) is now failing with the conversion webhooks:
https://github.com/crossplane-contrib/provider-upjet-azuread/actions/runs/8749171407/job/24010390302

image

I'll just disable the import tests for v1beta2 (non-storage version) for now. They should be running fine for v1beta1, which does not require a conversion when updated and I think it should be fine to update the status (of v1beta1) when the provider pod is down.

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/location.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/location.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta1/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/location.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

2 similar comments
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar ulucinar marked this pull request as ready for review April 24, 2024 14:52
@turkenf
Copy link
Collaborator

turkenf commented Apr 24, 2024

/test-examples="examples/applications/v1beta1/application.yaml"

@turkenf
Copy link
Collaborator

turkenf commented May 8, 2024

/test-examples="examples/groups/v1beta1/member.yaml"

@turkenf
Copy link
Collaborator

turkenf commented May 8, 2024

/test-examples="examples/serviceprincipaldelegated/v1beta1/permissiongrant.yaml"

@turkenf
Copy link
Collaborator

turkenf commented May 8, 2024

/test-examples="examples/synchronization/v1beta1/job.yaml"

ulucinar added 8 commits May 8, 2024 13:34
@ulucinar
Convert all singleton lists in the MR APIs to embedded objects
442e473 
- Terraform configuration blocks, even if they have a MaxItems
  constraint of 1, are (almost) always generated as lists. We
  now generate the lists with a MaxItems constraint of 1 as
  embedded objects in our MR APIs.
- This also helps when updating or patching via SSA the
  (previously list) objects. The merging strategy implemented
  by SSA requires configuration for associative lists and
  converting the singleton lists into embedded objects removes
  the configuration need.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
Generate all converted singleton list APIs as version v1beta2
5ec9d53 
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
Mark the v1beta1 versions of the generated CRSs as the storage versions
ecb2c55 
to facilitate downgrades

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
Adjust example manifests for converting singleton lists to embedded o…
73aa6fc 
…bjects

- Fix the example manifest examples/directoryroles/v1beta1/customdirectoryrole.yaml

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
CRD API conversion functions to convert between singleton list & embe…
5d78870 
…dded object API versions

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
Add a conversion.singletonConverter CRD API converter between the v1b…
84b0dc5 
…eta1 & v1beta2 versions of

the CRDs that have converted singleton lists in their APIs and enable the conversion webhooks.

- Use kustomize to inject the spec.conversion stanzas to the generated CRDs
- Dependency version Bumps: kind -> v0.21.0, up -> v0.28.0, uptest -> v0.11.1
- Pin uxp  version to 1.14.6-up.1

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
Enable conversion Webhooks and add the "--certs-dir" command-line opt…
3922403 
…ion to set

the webhook TLS certificates directory.

- Duplicate GVKs in zz_register.go are removed.
- Hubs and spokes are now generated from scratch at each "make generate".

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
Add CROSSPLANE_NAMESPACE to the env. of uptest
c88df8f 
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar ulucinar force-pushed the embed-singleton branch 2 times, most recently from 97f194c to 803e88e Compare May 8, 2024 10:50
@ulucinar
Copy link
Collaborator Author

ulucinar commented May 8, 2024

/test-examples="examples/groups/v1beta1/member.yaml"

@ulucinar
Copy link
Collaborator Author

ulucinar commented May 8, 2024

/test-examples="examples/serviceprincipaldelegated/v1beta1/permissiongrant.yaml"

@ulucinar
Copy link
Collaborator Author

ulucinar commented May 8, 2024

/test-examples="examples/synchronization/v1beta1/job.yaml"

@ulucinar ulucinar force-pushed the embed-singleton branch from 803e88e to 9be46b2 Compare May 8, 2024 14:04
@ulucinar
Copy link
Collaborator Author

ulucinar commented May 8, 2024

/test-examples="examples/groups/v1beta1/member.yaml"

@ulucinar
Copy link
Collaborator Author

ulucinar commented May 8, 2024

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

sergenyalcin
sergenyalcin approved these changes May 8, 2024
View reviewed changes
Copy link
Collaborator

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar LGTM!

@ulucinar
Register the API types with the API resolver's runtime scheme
990d474 
- Bump upjet to commit 03a207b6416a
- Bump uptest to version v0.12.0-9.gac371c9

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar ulucinar force-pushed the embed-singleton branch from 9be46b2 to 990d474 Compare May 8, 2024 14:19
@ulucinar
Copy link
Collaborator Author

ulucinar commented May 8, 2024

/test-examples="examples/conditionalaccess/v1beta2/accesspolicy.yaml"

@ulucinar
Copy link
Collaborator Author

ulucinar commented May 8, 2024

/test-examples="examples/groups/v1beta1/member.yaml"

@ulucinar ulucinar merged commit c33d7a8 into crossplane-contrib:main May 8, 2024
11 checks passed
@ulucinar ulucinar deleted the embed-singleton branch May 8, 2024 14:35
@turkenf
Copy link
Collaborator

turkenf commented May 16, 2024

Upgrade Test

    conditions:
    - applications:
      - includedApplications:
        - All
  • Edited the package with package: xpkg.upbound.io/upbound/provider-azuread:v1.2.0-rc.0.16.g14395d1
NAME               INSTALLED   HEALTHY   PACKAGE                                                            AGE
provider-azuread   True        True      xpkg.upbound.io/upbound/provider-azuread:v1.2.0-rc.0.16.g14395d1   11m
  • and we can see the following outputs from get -o yaml
    • apiVersion:
    apiVersion: conditionalaccess.azuread.upbound.io/v1beta2
    • embedded objects:
       conditions:
     applications:
       includedApplications:
       - All
    • conditions:
     - lastTransitionTime: "2024-05-16T15:54:56Z"
   reason: Available
   status: "True"
   type: Ready
 - lastTransitionTime: "2024-05-16T15:54:49Z"
   reason: ReconcileSuccess
   status: "True"
   type: Synced
 - lastTransitionTime: "2024-05-16T15:54:53Z"
   reason: Success
   status: "True"
   type: LastAsyncOperation
 - lastTransitionTime: "2024-05-16T16:03:17Z"
   reason: UpToDate
   status: "True"
   type: Test

Upgrade test successful

Downgrade Test

    conditions:
      applications:
        includedApplications:
        - All
  • Edited the package with package: xpkg.upbound.io/upbound/provider-azuread:v1.1.0
NAME               INSTALLED   HEALTHY   PACKAGE                                           AGE
provider-azuread   True        True      xpkg.upbound.io/upbound/provider-azuread:v1.1.0   8m32s
  • and we can see the following outputs from get -o yaml
    • apiVersion:
    apiVersion: conditionalaccess.azuread.upbound.io/v1beta1
    • singleton lists:
     conditions:
 - applications:
   - includedApplications:
     - All
    • conditions:
     - lastTransitionTime: "2024-05-16T16:18:52Z"
   reason: Available
   status: "True"
   type: Ready
 - lastTransitionTime: "2024-05-16T16:18:46Z"
   reason: ReconcileSuccess
   status: "True"
   type: Synced
 - lastTransitionTime: "2024-05-16T16:18:48Z"
   reason: Success
   status: "True"
   type: LastAsyncOperation
 - lastTransitionTime: "2024-05-16T16:24:42Z"
   reason: UpToDate
   status: "True"
   type: Test

> kubectl delete managed --all
accesspolicy.conditionalaccess.azuread.upbound.io "example" deleted

Downgrade test successful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergenyalcin sergenyalcin sergenyalcin approved these changes

@turkenf turkenf Awaiting requested review from turkenf turkenf is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ulucinar @turkenf @sergenyalcin