SecurityGroup in Composition is created twice

[janwillies]

What happened?

A security group in a crossplane composition is created twice:

CannotCreateExternalResource 24s managed/securitygroup.ec2.aws.crossplane.io failed to create the SecurityGroup resource: InvalidGroup.Duplicate: The security group 'cnp-test-1-7blj8' already exists for VPC 'vpc-abcdef'

Manual creation works fine (outside of a composition).

How can we reproduce it?

composition.yaml:

apiVersion: apiextensions.crossplane.io/v1alpha1
kind: Composition
metadata:
  name: cnp-sg
  labels:
    provider: aws
spec:
  writeConnectionSecretsToNamespace: crossplane-system
  reclaimPolicy: Delete
  compositeTypeRef:
    apiVersion: kubernetes.cnp.example.org/v1alpha1
    kind: CompositeControlPlane    
  resources:
  - base:
      apiVersion: ec2.aws.crossplane.io/v1beta1
      kind: SecurityGroup
      spec:
        forProvider:
          region: eu-central-1
          description: allow access
          ingress:
          - fromPort: 443
            toPort: 443
            ipProtocol: tcp
            ipRanges:
            - cidrIp: 192.168.0.0/24
              description: test
          vpcId: vpc-abcdef
        reclaimPolicy: Delete
        providerConfigRef:
          name: aws-provider        
    patches:
    - fromFieldPath: "metadata.labels"
      toFieldPath: "metadata.labels"
    - fromFieldPath: "metadata.annotations"
      toFieldPath: "metadata.annotations"
    - fromFieldPath: "metadata.name"
      toFieldPath: "spec.forProvider.groupName"

xrd.yaml

apiVersion: apiextensions.crossplane.io/v1alpha1
kind: CompositeResourceDefinition
metadata:
  name: compositecontroleplanes.kubernetes.cnp.example.org
spec:
  defaultCompositionRef:
    name: cnp-sg
  claimNames:
    kind: ControlPlane
    plural: controlplanes
  crdSpecTemplate:
    group: kubernetes.cnp.example.org
    version: v1alpha1
    names:
      kind: CompositeControlPlane
      listKind: CompositeControlPlaneList
      plural: compositecontroleplanes
      singular: compositecontroleplane
    validation:
      openAPIV3Schema:
        type: object
        properties:
          spec: {}

xrc.yaml

apiVersion: kubernetes.cnp.example.org/v1alpha1
kind: ControlPlane
metadata:
  name: cnp-test-1
spec: {}

What environment did it happen in?

Crossplane version: v0.13.0
provider-aws: v0.12.0

[mothershipper]

There may be a second part to this that affects how it works with compositions, but we've noticed that unlike the RDSInstance CRD, the SecurityGroup CRD checks the CRD for an external-name annotation, and decides the security group does not exist if the CRD has yet to be updated. This will cause the controller to try and double create the security group if it's still in progress of bringing one up.

https://github.com/crossplane/provider-aws/blob/master/pkg/controller/ec2/securitygroup/controller.go#L103-L105

[negz]

It's possible this could be due to crossplane/crossplane#1910.

[janwillies]

Not propagating the annotations solves this issue. E.g. removing the following from the composition:

    - fromFieldPath: "metadata.annotations"
      toFieldPath: "metadata.annotations"

[muvaf]

Is there an action item for this? We can't prevent patching the external name annotation since it's valid for other use cases. Also copying the full annotations is kind of fine to some extent if your composition has only one base. So, I'm not sure what we can do here. Feel free to reopen.

[mf-lit]

I've run into this issue but with an IAMPolicy rather than a SecurityGroup #590

@muvaf I'm not clear however why this issue was closed? If we need to explicitly set the name of an an IAMPolicy (or for that matter a SecurityGroup) then we need to pass the crossplane.io/external-name annotation.

[muvaf]

@mf-lit The problem happens only when you do the full copy of the annotations. It'd work if you specify the certain keys you want to patch like the following:

- fromFieldPath: "metadata.annotations[crossplane.io/external-name]"
  toFieldPath: "metadata.annotations[crossplane.io/external-name]"

[mf-lit]

@muvaf Ah ok, good tip, thanks. 👍

I'm still curious though, to me it seems like the inability to pass full annotations would be useful, but since the issue was closed I'm guessing there's a reason not to allow it - what is that?

[muvaf]

It's about the operation itself. When you give the full metadata.annotations it replaces the whole map which includes identifiers provider use for talking with AWS. For example, if external name annotation is empty, it decides to create the resource.