From 9c244cdd108667f8e1dd1c5e143c125c1fcb2458 Mon Sep 17 00:00:00 2001 From: Cem Mergenci Date: Thu, 16 Jan 2025 22:11:32 +0300 Subject: [PATCH 1/4] Remove diff calculation in observe-only reconciliation. Signed-off-by: Cem Mergenci --- pkg/controller/external_tfpluginsdk.go | 59 +++++++++++++++----------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/pkg/controller/external_tfpluginsdk.go b/pkg/controller/external_tfpluginsdk.go index cb408d3d..9ce3cdb5 100644 --- a/pkg/controller/external_tfpluginsdk.go +++ b/pkg/controller/external_tfpluginsdk.go @@ -111,15 +111,16 @@ type Resource interface { } type terraformPluginSDKExternal struct { - ts terraform.Setup - resourceSchema Resource - config *config.Resource - instanceDiff *tf.InstanceDiff - params map[string]any - rawConfig cty.Value - logger logging.Logger - metricRecorder *metrics.MetricRecorder - opTracker *AsyncTracker + ts terraform.Setup + resourceSchema Resource + config *config.Resource + instanceDiff *tf.InstanceDiff + params map[string]any + rawConfig cty.Value + logger logging.Logger + metricRecorder *metrics.MetricRecorder + opTracker *AsyncTracker + isManagementPoliciesEnabled bool } func getExtendedParameters(ctx context.Context, tr resource.Terraformed, externalName string, cfg *config.Resource, ts terraform.Setup, initParamsMerged bool, kube client.Client) (map[string]any, error) { @@ -294,14 +295,15 @@ func (c *TerraformPluginSDKConnector) Connect(ctx context.Context, mg xpresource } return &terraformPluginSDKExternal{ - ts: ts, - resourceSchema: c.config.TerraformResource, - config: c.config, - params: params, - rawConfig: rawConfig, - logger: logger, - metricRecorder: c.metricRecorder, - opTracker: opTracker, + ts: ts, + resourceSchema: c.config.TerraformResource, + config: c.config, + params: params, + rawConfig: rawConfig, + logger: logger, + metricRecorder: c.metricRecorder, + opTracker: opTracker, + isManagementPoliciesEnabled: c.isManagementPoliciesEnabled, }, nil } @@ -460,6 +462,7 @@ func (n *terraformPluginSDKExternal) getResourceDataDiff(tr resource.Terraformed } func (n *terraformPluginSDKExternal) Observe(ctx context.Context, mg xpresource.Managed) (managed.ExternalObservation, error) { //nolint:gocyclo + var err error n.logger.Debug("Observing the external resource") if meta.WasDeleted(mg) && n.opTracker.IsDeleted() { @@ -492,15 +495,22 @@ func (n *terraformPluginSDKExternal) Observe(ctx context.Context, mg xpresource. diffState.Attributes = nil diffState.ID = "" } - instanceDiff, err := n.getResourceDataDiff(mg.(resource.Terraformed), ctx, diffState, resourceExists) - if err != nil { - return managed.ExternalObservation{}, errors.Wrap(err, "cannot compute the instance diff") + + n.instanceDiff = nil + policySet := sets.New(mg.(resource.Terraformed).GetManagementPolicies()...) + observeOnlyPolicy := sets.New(xpv1.ManagementActionObserve) + isObserveOnlyPolicy := policySet.Equal(observeOnlyPolicy) + if !isObserveOnlyPolicy || !n.isManagementPoliciesEnabled { + n.instanceDiff, err = n.getResourceDataDiff(mg.(resource.Terraformed), ctx, diffState, resourceExists) + if err != nil { + return managed.ExternalObservation{}, errors.Wrap(err, "cannot compute the instance diff") + } } - if instanceDiff == nil { - instanceDiff = tf.NewInstanceDiff() + if n.instanceDiff == nil { + n.instanceDiff = tf.NewInstanceDiff() } - n.instanceDiff = instanceDiff - noDiff := instanceDiff.Empty() + + noDiff := n.instanceDiff.Empty() if !resourceExists && mg.GetDeletionTimestamp() != nil { gvk := mg.GetObjectKind().GroupVersionKind() @@ -533,7 +543,6 @@ func (n *terraformPluginSDKExternal) Observe(ctx context.Context, mg xpresource. return managed.ExternalObservation{}, errors.Wrap(err, "cannot marshal the attributes of the new state for late-initialization") } - policySet := sets.New[xpv1.ManagementAction](mg.(resource.Terraformed).GetManagementPolicies()...) policyHasLateInit := policySet.HasAny(xpv1.ManagementActionLateInitialize, xpv1.ManagementActionAll) if policyHasLateInit { specUpdateRequired, err = mg.(resource.Terraformed).LateInitialize(buff) From 2f967cf07c8985d8997f8c72a881f05da347a392 Mon Sep 17 00:00:00 2001 From: Cem Mergenci Date: Thu, 16 Jan 2025 23:29:01 +0300 Subject: [PATCH 2/4] Rename `noDiff` to `hasDiff`. Signed-off-by: Cem Mergenci --- pkg/controller/external_tfpluginsdk.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/external_tfpluginsdk.go b/pkg/controller/external_tfpluginsdk.go index 9ce3cdb5..d911de14 100644 --- a/pkg/controller/external_tfpluginsdk.go +++ b/pkg/controller/external_tfpluginsdk.go @@ -510,7 +510,7 @@ func (n *terraformPluginSDKExternal) Observe(ctx context.Context, mg xpresource. n.instanceDiff = tf.NewInstanceDiff() } - noDiff := n.instanceDiff.Empty() + hasDiff := !n.instanceDiff.Empty() if !resourceExists && mg.GetDeletionTimestamp() != nil { gvk := mg.GetObjectKind().GroupVersionKind() @@ -556,11 +556,11 @@ func (n *terraformPluginSDKExternal) Observe(ctx context.Context, mg xpresource. return managed.ExternalObservation{}, errors.Errorf("could not set observation: %v", err) } - if noDiff { + if !hasDiff { n.metricRecorder.SetReconcileTime(mg.GetName()) } if !specUpdateRequired { - resource.SetUpToDateCondition(mg, noDiff) + resource.SetUpToDateCondition(mg, !hasDiff) } // check for an external-name change if nameChanged, err := n.setExternalName(mg, stateValueMap); err != nil { @@ -572,7 +572,7 @@ func (n *terraformPluginSDKExternal) Observe(ctx context.Context, mg xpresource. return managed.ExternalObservation{ ResourceExists: resourceExists, - ResourceUpToDate: noDiff, + ResourceUpToDate: !hasDiff, ConnectionDetails: connDetails, ResourceLateInitialized: specUpdateRequired, }, nil From 525abba0fa2f7915c00dd6a96bfa317429d210ec Mon Sep 17 00:00:00 2001 From: Cem Mergenci Date: Thu, 30 Jan 2025 02:15:27 +0300 Subject: [PATCH 3/4] Add TODOs for other external clients. Signed-off-by: Cem Mergenci --- pkg/controller/external.go | 4 ++++ pkg/controller/external_tfpluginfw.go | 3 +++ 2 files changed, 7 insertions(+) diff --git a/pkg/controller/external.go b/pkg/controller/external.go index cdfbed60..4a30379e 100644 --- a/pkg/controller/external.go +++ b/pkg/controller/external.go @@ -330,6 +330,10 @@ func (e *external) Observe(ctx context.Context, mg xpresource.Managed) (managed. if e.eventHandler != nil { e.eventHandler.Forget(rateLimiterStatus, mg.GetName()) } + + // TODO(cem): Consider skipping diff calculation (terraform plan) to + // avoid potential config validation errors in the import path. See + // https://github.com/crossplane/upjet/pull/461 plan, err := e.workspace.Plan(ctx) if err != nil { return managed.ExternalObservation{}, errors.Wrap(err, errPlan) diff --git a/pkg/controller/external_tfpluginfw.go b/pkg/controller/external_tfpluginfw.go index 61297228..b6f6f89f 100644 --- a/pkg/controller/external_tfpluginfw.go +++ b/pkg/controller/external_tfpluginfw.go @@ -330,6 +330,9 @@ func (n *terraformPluginFrameworkExternalClient) Observe(ctx context.Context, mg } } + // TODO(cem): Consider skipping diff calculation to avoid potential config + // validation errors in the import path. See + // https://github.com/crossplane/upjet/pull/461 planResponse, hasDiff, err := n.getDiffPlanResponse(ctx, tfStateValue) if err != nil { return managed.ExternalObservation{}, errors.Wrap(err, "cannot calculate diff") From 43311a8459699f486f0c57a15d102dd873687663 Mon Sep 17 00:00:00 2001 From: Cem Mergenci Date: Thu, 30 Jan 2025 16:33:18 +0300 Subject: [PATCH 4/4] Add generics expression for compatibility with local environments. We discovered that GoLand failed to build without the generics expression, whereas VS Code warned that it was unnecessary. Signed-off-by: Cem Mergenci --- pkg/controller/external_tfpluginsdk.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/external_tfpluginsdk.go b/pkg/controller/external_tfpluginsdk.go index d911de14..6e59cdd4 100644 --- a/pkg/controller/external_tfpluginsdk.go +++ b/pkg/controller/external_tfpluginsdk.go @@ -497,7 +497,7 @@ func (n *terraformPluginSDKExternal) Observe(ctx context.Context, mg xpresource. } n.instanceDiff = nil - policySet := sets.New(mg.(resource.Terraformed).GetManagementPolicies()...) + policySet := sets.New[xpv1.ManagementAction](mg.(resource.Terraformed).GetManagementPolicies()...) observeOnlyPolicy := sets.New(xpv1.ManagementActionObserve) isObserveOnlyPolicy := policySet.Equal(observeOnlyPolicy) if !isObserveOnlyPolicy || !n.isManagementPoliciesEnabled {