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

Switch to the new API for marking as required the fields for some resources #490

Merged

Conversation

sergenyalcin
Copy link
Collaborator

@sergenyalcin sergenyalcin commented Mar 19, 2024

Description of your changes

This PR switches to the new API for marking as required the fields. The new API will mark as required the fields during the generation without any native resource schema change. For mor details of this new API please see this PR. The base strategies we decided in this PR:

  • Upjet's config.MarkAsRequired still modifies the native schema (no change in its implementation) but it's now deprecated in favor of config.Resource.MarkAsRequired.
  • Other clients of the config.MarkAsRequired are not updated to use the new config.Resource.MarkAsRequired in this PR because the config.MarkAsRequired modifies Terraform's runtime behavior (potentially also outside the context of the defaulting CustomizeDiff functions) by manipulating the native schema, and so we would like to test those resources which are updated to use the new config.Resource.MarkAsRequired. So it's a larger effort to transition all the clients of the config.MarkAsRequired and we limit the scope of this PR by only updating the configurations of the following resources.

Fixes #472

The root cause of #472 is native schema change of the zone and region field. The new CustomizeDiff functions of the underlying provider, DefaultProviderRegion and DefaultProviderZone, checks the Computed fields of the resource schemas and gives some decisions according to the values. At this point, because of manipulating the resource schema (what we must not do), we observe this issue. By this change, we will revert the schema changes for the affected resources. The affected resources were determined according to the two following rules:

  • The resource configuration should contain schema manipulation via MarkAsRequired for zone or region fields.
  • The native schema of resource should contain at least on of the mentioned CustomizeDiff functions.

Switched to the new API for the following resources:

  1. Environment.composer
  2. Job.cloudscheduler
  3. Autoscaler.compute
  4. Instance.compute
  5. InstanceGroup.compute
  6. InstanceGroupManager.compute
  7. NetworkEndpointGroup.compute
  8. ResourcePolicy.compute
  9. TargetPool.compute
  10. RegionInstanceGroupManager.compute
  11. RegionAutoScaler.compute
  12. ServiceAttachment.compute
  13. EntryGroup.datacatalog

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.

How has this code been tested

  1. Instance.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8343310426
  2. Job.cloudscheduler: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8343708323
  3. InstanceGroup.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8343704491
  4. ResourcePolicy.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8343851010
  5. EntryGroup.datacatalog: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8343873861
  6. RegionAutoScaler.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8344124389
  7. NetworkEndpointGroup.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8344103249
  8. RegionInstanceGroupManager.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8344548614
  9. ServiceAttachment.compute: Manually (because of manual intervention need)
  10. TargetPool.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8344649274
  11. Autoscaler.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8345034404
  12. InstanceGroupManager.compute: https://github.com/crossplane-contrib/provider-upjet-gcp/actions/runs/8345034404
  13. Environment.composer: Manually (because of service account permission issues)

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/instance.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/instancegroup.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/cloudscheduler/job.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/composer/environment.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/autoscaler.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/resourcepolicy.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/datacatalog/entrygroup.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/networkendpointgroup.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/regionautoscaler.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/regioninstancegroupmanager.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/targetpool.yaml"

…ix the example manifest of Environment.composer

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/composer/environment.yaml"

@sergenyalcin
Copy link
Collaborator Author

/test-examples="examples/compute/autoscaler.yaml"

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@@ -399,10 +399,6 @@ func Configure(p *config.Provider) { //nolint: gocyclo
config.MarkAsRequired(r.TerraformResource, "region")
})

p.AddResourceConfigurator("google_compute_region_per_instance_config", func(r *config.Resource) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this resource no longer exist? Is this unnecessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I deleted this statement because this configuration does not have an effect. The region field is generated as a Reference field, and we mark the reference fields as Optional regardless of their status in the scheme. Also, as can be seen, even though the configuration was deleted, no diff occurred after generation. This shows that this configuration block has no function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the explanation.

@@ -13,9 +13,3 @@ metadata:
spec:
forProvider:
region: us-east1
config:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we have to remove this configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked the registry documentation, and this usage is no longer valid. Also, there was a basic usage in the example of registry. I changed this example to a basic one to avoid leading the user to the view that the config block is required. In this context, we can consider providing different example manifests for different purposes and use cases, which we discussed before.

When the relevant configuration is used in this form, error messages related to the cloud provider's own version change are seen. This points to a behavioral change in the cloud provider for the relevant resource. The relevant behavior change is not mentioned in the underlying provider documentation. In this context, we cannot rely on the relevant documentation to detect such behavioral changes in future underlying provider version updates. However, it may be possible to try to detect changes in the underlying provider's dependency on cloud providers.

@ulucinar
Copy link
Collaborator

Hi @sergenyalcin,
Let's also discuss in the PR description the strategy we decided on in crossplane/upjet#381:
a. Upjet's config.MarkAsRequired still modifies the native schema (no change in its implementation) but it's now deprecated in favor of config.Resource.MarkAsRequired.
b. This PR only updates & tests those resources whose configuration uses config.MarkAsRequired and their corresponding Terraform resources employ the new project and zone defaulting CustomizeDiffs -> This is already mentioned in the PR description.
c. Other clients of the config.MarkAsRequired are not updated to use the new config.Resource.MarkAsRequired in this PR because the config.MarkAsRequired modifies Terraform's runtime behavior (potentially also outside the context of the defaulting CustomizeDiff functions) by manipulating the native schema, and so we would like to test those resources which are updated to use the new config.Resource.MarkAsRequired. So it's a larger effort to transition all the clients of the config.MarkAsRequired and we limit the scope of this PR by only updating the configurations of the resources in [b].

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @sergenyalcin, lgtm.

@sergenyalcin sergenyalcin merged commit dec6492 into crossplane-contrib:main Mar 20, 2024
9 checks passed
Copy link

Backport failed for release-1.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-1.0
git worktree add -d .worktree/backport-490-to-release-1.0 origin/release-1.0
cd .worktree/backport-490-to-release-1.0
git checkout -b backport-490-to-release-1.0
ancref=$(git merge-base be3db998f3d0479f02d24e2b10e3a6ef1ea45394 b9c3ab8a6b920162b31f7b039a734fc58e35dae2)
git cherry-pick -x $ancref..b9c3ab8a6b920162b31f7b039a734fc58e35dae2

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

Successfully merging this pull request may close these issues.

[Bug]: Creation of compute Instances (and others) fails with SetNew only operates on computed keys
2 participants