Claim with out of sync EC2 instance incorrectly shows SYNCED: True

[spkane]

What happened?

I created my first configuration package to do some exploration of Crossplane and its functionality. This configuration package creates a very simple EC2 instance and an IAM user.

I installed the configuration package and then applied a claim, which worked as expected.

The claim allows the user to set the AWS Instance Type. In the initial claim, I set it to t2.medium.

I then edited the claim.yaml and changed the instanceType parameter to t2.small and re-applied it. I expected nothing to change, since this would require Crossplane to destroy the instance, however, I did expect the managed resource to go out of sync, which did not happen.

$ kubectl get instance.ec2.aws.crossplane.io/my-userserver-m5ccd-g2b82
NAME                        READY   SYNCED   ID                    STATE     AGE
my-userserver-m5ccd-g2b82   True    True     i-05d49e573792960c7   running   22m

$ kubectl describe instance.ec2.aws.crossplane.io/my-userserver-m5ccd-g2b82 | grep "Instance Type"
    Instance Type:                         t2.small
    Instance Type:      t2.medium

How can we reproduce it?

Configuration Package

• definition.yaml

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: xuserservers.example.org
spec:
  group: example.org
  names:
    kind: XUserServer
    plural: xuserservers
  claimNames:
    kind: UserServer
    plural: userservers
  connectionSecretKeys: []
  defaultCompositionRef:
    name: example
  versions:
  - name: v1alpha1
    served: true
    referenceable: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            type: object
            properties:
              parameters:
                type: object
                properties:
                  instanceType:
                    type: string
                  iamPath:
                     type: string
                required:
                - instanceType
            required:
            - parameters
          status:
            type: object
            properties:
              address:
                description: IP address of this cloud instance.
                type: string

• composition.yaml

apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: userserver.example.org
  labels:
    crossplane.io/xrd: xuserservers.example.org
    provider: aws
spec:
  compositeTypeRef:
    apiVersion: example.org/v1alpha1
    kind: XUserServer
  writeConnectionSecretsToNamespace: crossplane-system
  resources:
  - name: cloudinstance
    base:
      apiVersion: ec2.aws.crossplane.io/v1alpha1
      kind: Instance
      spec:
        forProvider:
          imageId: "ami-06f29effee622eb00"
          region: "us-west-2"
          instanceType: "t2.micro"
    patches:
    - type: FromCompositeFieldPath
      fromFieldPath: spec.parameters.instanceType
      toFieldPath: spec.forProvider.instanceType
      policy:
        fromFieldPath: Required
    readinessChecks:
    - type: None
  - name: clouduser
    base:
      apiVersion: iam.aws.crossplane.io/v1beta1
      kind: User
      spec:
        forProvider:
          path: "/"
    patches:
    - type: FromCompositeFieldPath
      fromFieldPath: spec.parameters.iamPath
      toFieldPath: spec.forProvider.path
      policy:
        fromFieldPath: Required
    readinessChecks:
    - type: None

• cat crossplane.yaml

apiVersion: meta.pkg.crossplane.io/v1alpha1
kind: Configuration
metadata:
  name: aws-iam-user-and-ec2-instance
  annotations:
    guide: custom
    provider: aws
spec:
  crossplane:
    version: ">=v1.9.0-0"
  dependsOn:
    - provider: crossplane/provider-aws
      version: ">=v0.30.1"

Claims

• claim.yaml

apiVersion: example.org/v1alpha1
kind: UserServer
metadata:
  namespace: default
  name: my-userserver
  annotations:
    crossplane.io/external-name: superhero
spec:
  compositionSelector:
    matchLabels:
      crossplane.io/xrd: xuserservers.example.org
      provider: aws
  writeConnectionSecretToRef:
    name: my-user-instance-details
  parameters:
    instanceType: t2.medium
    iamPath: /

• claim-changed.yaml

apiVersion: example.org/v1alpha1
kind: UserServer
metadata:
  namespace: default
  name: my-userserver
  annotations:
    crossplane.io/external-name: superhero
spec:
  compositionSelector:
    matchLabels:
      crossplane.io/xrd: xuserservers.example.org
      provider: aws
  writeConnectionSecretToRef:
    name: my-user-instance-details
  parameters:
    instanceType: t2.small
    iamPath: /test/

What environment did it happen in?

• Crossplane version: crossplane/crossplane:v1.9.0
• Crossplane CLI version: v1.9.0
• Cloud: AWS
• K8s Client Version: v1.24.4
• Kustomize Version: v4.5.4
• K8s Server Version: v1.23.6+k3s1
• Kubernetes distribution: k3s in a colima VM
• Desktop OS: macOS 12.5.1
• VM OS: Alpine Linux v3.14
• Kernel: Linux colima 5.10.109-0-virt #1-Alpine SMP Mon, 28 Mar 2022 11:20:52 +0000 x86_64 Linux

[spkane]

@bobh66 mentioned in Slack that the related code is here:

provider-aws/pkg/controller/ec2/instance/controller.go

Line 195 in 4040d5c

if !cmp.Equal(current, &cr.Spec.ForProvider) {

[spkane]

	fmt.Printf("\n\ncurrent : %#v\n", current.InstanceType)
	fmt.Printf("\n\ncr.spec : %#v\n", cr.Spec.ForProvider.InstanceType)
	fmt.Printf("\n\nobserved: %#v\n", observed.InstanceType)
	if !cmp.Equal(current, &cr.Spec.ForProvider) {
		if err := e.kube.Update(ctx, cr); err != nil {
			return managed.ExternalObservation{}, errors.Wrap(err, errKubeUpdateFailed)
		}
	}

Adding these debug lines with this use-case results in:

current : "t2.small"

cr.spec : "t2.small"

observed: "t2.medium"

So, the comparison at line +195+ is indeed equal, but the observed value is what should be resulting in things being out of sync.

I am not familiar with this code at all, but I am wondering, if the issue here is that since InstanceType can not be updated after the fact, maybe it is ignored when considering whether the object is in sync?

[spkane]

This is the code that determined if an instance it up-to-date:

provider-aws/pkg/clients/ec2/instance.go

Line 61 in 4040d5c

func IsInstanceUpToDate(spec manualv1alpha1.InstanceParameters, instance types.Instance, attributes ec2.DescribeInstanceAttributeOutput) bool {

[spkane]

@bobh66 Do you know if the sync status only takes into consideration fields that can be updated after object creation?

That would make some sense, but it seems like, in that case, there should be a 3rd value for sync (or an additional field), so that SYCNED could be set to something like:

• true (clean): completely in sync (and has no field divergence)
• dirty: not in sync (and has un-sync-able field divergence)
• false: not in sync (and has sync-able field divergence)

[bobh66]

I don't know this code all that well but as a user I would expect the SYNCED field to represent the comparison of what exists in AWS with what was requested in the Managed Resource. So if the user changes the instanceType in the claim/MR, and it no longer matches what is built in AWS, then SYNCED should be False, regardless of whether the provider is actually capable of re-syncing those changes or not. If the instanceType field cannot be updated without a delete/rebuild then it should probably be immutable, but that's a whole separate can of worms. If the provider cannot update the instanceType then there should be an Event attached to the managed resource that indicates that the user's requested update cannot be achieved and that the MR should be (manually) deleted and allowed to re-create.

For now this all may be wishful thinking, but it's clear that it's probably not quite right as it is.

[negz]

I believe what's happening here is that the underlying external resource field is immutable so we have no logic to try to update it, but also no way to mark the corresponding Kubernetes field as immutable. This results in the API server letting you change a field that it shouldn't, resulting in us appearing to silently accept the change and then do nothing about it. crossplane/crossplane-tools#40 tracks addressing this.

[github-actions]

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.