Skip to content

Fix ecs.Service resource's external name configuration and update loop issue - #1950

Merged
sergenyalcin merged 1 commit into
crossplane-contrib:mainfrom
sergenyalcin:fix-ecs-service
Jan 20, 2026
Merged

Fix ecs.Service resource's external name configuration and update loop issue#1950
sergenyalcin merged 1 commit into
crossplane-contrib:mainfrom
sergenyalcin:fix-ecs-service

Conversation

@sergenyalcin

@sergenyalcin sergenyalcin commented Jan 15, 2026

Copy link
Copy Markdown
Collaborator

Description of your changes

This PR fixes the ecs.Service resource's external name configuration and resolves an update loop issue caused by incorrect task definition reference handling.

Problem

The ECS Service resource had two critical issues:

  1. Incorrect external name ID construction: The external name configuration in config.go used an old implementation that concatenated parameters.cluster + parameters.name for ID construction. This didn't match the actual AWS resource ID format (the full ARN).
  2. Task definition update loop: When task definitions were referenced and their revision changed (e.g., from family:1 to family:2), the controller would incorrectly detect a diff and trigger unnecessary updates because it was comparing the full ARN against just the family name.

Changes

External Name Configuration

  • The external name configuration for this resource was previously done in the config.go file using custom GetExternalNameFn and GetIDFn functions
  • This was an old implementation created before TemplatedStringAsIdentifier existed, essentially mimicking what that function does
  • Replaced the old implementation with TemplatedStringAsIdentifier using the proper ARN template: arn:aws:ecs:<region>:<account_id>:service/{{ .parameters.cluster }}/{{ .external_name }}
  • Everything remains the same except the ID calculation function (GetIDFn):
    • Before: Concatenation of parameters.cluster + parameters.name fields
    • Now: Correct ID format (full ARN of the resource)

Task Definition Reference

  • Changed task definition reference extractor from reference.ExternalName() to common.ARNExtractor() in both ForProvider and InitProvider for v1beta1, v1beta2, and namespaced APIs
  • Added custom diff handler to ignore task definition changes that only affect revision numbers (e.g., family:35 vs arn:aws:ecs:region:account:task-definition/family:35)

I have:

  • Read and followed Crossplane's contribution process.
  • Run make generate and committed the results (ideally in a separate commit).
  • Not made any manual changes to generated files, and verified this with make check-diff.

How has this code been tested

Tested locally via uptest.
https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/21032450859

…p issue

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

Copy link
Copy Markdown
Collaborator Author

/test-examples="examples/ecs/cluster/v1beta1/service.yaml"

@sergenyalcin
sergenyalcin marked this pull request as ready for review January 15, 2026 14:02
@sergenyalcin
sergenyalcin merged commit 9c2757b into crossplane-contrib:main Jan 20, 2026
9 checks passed
@sergenyalcin
sergenyalcin deleted the fix-ecs-service branch January 20, 2026 13:54
pyrex41 pushed a commit to pyrex41/provider-upjet-aws that referenced this pull request Feb 14, 2026
… a plain family name

The TerraformCustomDiff handler for aws_ecs_service assumes
taskDefinition is always an ARN containing "/". When set to a plain
family name (e.g. "my-service"), strings.Split returns a single-element
slice and tdParts[1] panics with index out of range.

This panic is caught by controller-runtime's global recover, which
silently prevents ALL drift detection on the resource while leaving the
Synced condition at its previous value (True). No error is reported,
no conditions are updated, and no reconciliation occurs — the resource
appears healthy but is completely unmanaged.

Add a bounds check on tdParts before accessing index 1. When
taskDefinition is a plain family name (no "/"), the diff is passed
through unchanged, letting Terraform handle the comparison normally.

Fixes the panic introduced in crossplane-contrib#1950.
erhancagirici pushed a commit to pyrex41/provider-upjet-aws that referenced this pull request Mar 10, 2026
… a plain family name

The TerraformCustomDiff handler for aws_ecs_service assumes
taskDefinition is always an ARN containing "/". When set to a plain
family name (e.g. "my-service"), strings.Split returns a single-element
slice and tdParts[1] panics with index out of range.

This panic is caught by controller-runtime's global recover, which
silently prevents ALL drift detection on the resource while leaving the
Synced condition at its previous value (True). No error is reported,
no conditions are updated, and no reconciliation occurs — the resource
appears healthy but is completely unmanaged.

Add a bounds check on tdParts before accessing index 1. When
taskDefinition is a plain family name (no "/"), the diff is passed
through unchanged, letting Terraform handle the comparison normally.

Fixes the panic introduced in crossplane-contrib#1950.
pyrex41 pushed a commit to pyrex41/provider-upjet-aws that referenced this pull request Mar 10, 2026
… a plain family name

The TerraformCustomDiff handler for aws_ecs_service assumes
taskDefinition is always an ARN containing "/". When set to a plain
family name (e.g. "my-service"), strings.Split returns a single-element
slice and tdParts[1] panics with index out of range.

This panic is caught by controller-runtime's global recover, which
silently prevents ALL drift detection on the resource while leaving the
Synced condition at its previous value (True). No error is reported,
no conditions are updated, and no reconciliation occurs — the resource
appears healthy but is completely unmanaged.

Add a bounds check on tdParts before accessing index 1 in both
config/cluster/ecs/config.go and config/namespaced/ecs/config.go.
When taskDefinition is a plain family name (no "/"), the diff is passed
through unchanged, letting Terraform handle the comparison normally.

Fixes the panic introduced in crossplane-contrib#1950.

Signed-off-by: Reuben Brooks <reuben.brooks@facilitygrid.com>
erhancagirici pushed a commit to pyrex41/provider-upjet-aws that referenced this pull request Mar 10, 2026
… a plain family name

The TerraformCustomDiff handler for aws_ecs_service assumes
taskDefinition is always an ARN containing "/". When set to a plain
family name (e.g. "my-service"), strings.Split returns a single-element
slice and tdParts[1] panics with index out of range.

This panic is caught by controller-runtime's global recover, which
silently prevents ALL drift detection on the resource while leaving the
Synced condition at its previous value (True). No error is reported,
no conditions are updated, and no reconciliation occurs — the resource
appears healthy but is completely unmanaged.

Add a bounds check on tdParts before accessing index 1 in both
config/cluster/ecs/config.go and config/namespaced/ecs/config.go.
When taskDefinition is a plain family name (no "/"), the diff is passed
through unchanged, letting Terraform handle the comparison normally.

Fixes the panic introduced in crossplane-contrib#1950.

Signed-off-by: Reuben Brooks <reuben.brooks@facilitygrid.com>
erikmiller-gusto pushed a commit to erikmiller-gusto/provider-upjet-aws that referenced this pull request Mar 25, 2026
… a plain family name

The TerraformCustomDiff handler for aws_ecs_service assumes
taskDefinition is always an ARN containing "/". When set to a plain
family name (e.g. "my-service"), strings.Split returns a single-element
slice and tdParts[1] panics with index out of range.

This panic is caught by controller-runtime's global recover, which
silently prevents ALL drift detection on the resource while leaving the
Synced condition at its previous value (True). No error is reported,
no conditions are updated, and no reconciliation occurs — the resource
appears healthy but is completely unmanaged.

Add a bounds check on tdParts before accessing index 1 in both
config/cluster/ecs/config.go and config/namespaced/ecs/config.go.
When taskDefinition is a plain family name (no "/"), the diff is passed
through unchanged, letting Terraform handle the comparison normally.

Fixes the panic introduced in crossplane-contrib#1950.

Signed-off-by: Reuben Brooks <reuben.brooks@facilitygrid.com>
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.

2 participants