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

Add NoFork External Connectors & Clients #294

Merged
merged 60 commits into from
Nov 16, 2023
Merged

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Nov 1, 2023

Description of your changes

This PR adds two new managed.Connecters and two new managed.ExternalClients for reconciling Terraformed resources without forking any Terraform CLI or Terraform provider processes. The new external client implementation consumes the Terraform provider's Go provider schema and invokes the CRUD functions registered in that schema via the terraform-plugin-sdk utility functions.

The two new external clients are the controller.noForkExternal and the controller.noForkAsyncExternal and are implemented in pkg/controller/external_nofork.go and pkg/controller/external_async_nofork.go, respectively. The async client is composed using the sync client and all the interaction with the underlying Terraform provider via the Terraform plugin SDK is implemented in pkg/controller/external_nofork.go.

Both clients employ an in-memory Terraform state cache as these clients no longer construct Terraform workspaces while reconciling their resources. We have an initial evaluation of the memory cost of this cache with 10k MRs and it's in the order of a few hundred megabytes (~200 MB), much smaller than the footprint of the process fork-based clients.

I have:

  • Read and followed Upjet'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.

How has this code been tested

Tested in crossplane-contrib/provider-upjet-aws#938 & crossplane-contrib/provider-upjet-aws#910.

ulucinar and others added 29 commits November 1, 2023 19:23
- Switch to Resource.RefreshWithoutUpgrade & Resource.Apply

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Cem Mergenci <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Cem Mergenci <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
…ource schema elements

- Add support for explicitly configuring fields to be added to the Observation types.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
… Terraform configuration parameters

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
…ization option by default

- May result in unnecessary drift if some fields are not late-initialized.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
…ion back

- Late-initialization of nil values from corresponding zero-values
  results in a late-initialization loop and prevents the resource
  from becoming ready.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
- Terraformed resources which are reconciled using the synchronous
  no-fork external client also use the in-memory Terraform state
  cache. This was needed because there are resources who put
  certain attributes into the TF state only during the Create call.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar ulucinar force-pushed the no-fork branch 2 times, most recently from e241dfc to d4fd8af Compare November 2, 2023 15:57
… schema and config.Resource.OperationTimeouts

- The timeouts specified in upjet resource configuration (config.Resource.OperationTimeouts)
  prevails any defaults configured in the Terraform schema.

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
mergenci and others added 7 commits November 3, 2023 16:26
* Remove all length keys, which are suffixed with % or #, from
initProvider-exclusive diffs. Map and set diffs are successfully
applied without these length keys. List diffs require length keys. To
be addressed later.

Signed-off-by: Cem Mergenci <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Sergen Yalçın <[email protected]>
erhancagirici and others added 5 commits November 13, 2023 18:44
Signed-off-by: Sergen Yalçın <[email protected]>
Signed-off-by: Sergen Yalçın <[email protected]>
Signed-off-by: Sergen Yalçın <[email protected]>
- Add unit test for InitProvider

Signed-off-by: Sergen Yalçın <[email protected]>
Copy link
Collaborator

@erhancagirici erhancagirici left a comment

Choose a reason for hiding this comment

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

Thanks for the great work, LGTM!

Copy link
Member

@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.

Thank you! LGTM!

@ulucinar ulucinar merged commit da16191 into crossplane:main Nov 16, 2023
6 checks passed
@ulucinar ulucinar deleted the no-fork branch November 16, 2023 12:19
@cnuss
Copy link

cnuss commented Nov 16, 2023

Question 🙏

We've up-jetted some providers internally and would like to adopt these wonderful performace improvements. How do we migrate to this? We used generating-a-provider.md to make them a few months back.

If there's a doc or quip somewhere and I missed it feel free to link it, thank you!

@jeanduplessis
Copy link
Collaborator

@cnuss we're going to fast-follow the 1.0.0 release with some documentation updates that will address this.
Here's the PR that implemented the new Upjet version in provider-aws as an example if you want to have a look: https://github.com/upbound/provider-aws/pull/938/files

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.

6 participants