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

getResourceCondition function is potentially destructive? #85

Open
dee-kryvenko opened this issue Apr 14, 2024 · 7 comments
Open

getResourceCondition function is potentially destructive? #85

dee-kryvenko opened this issue Apr 14, 2024 · 7 comments

Comments

@dee-kryvenko
Copy link

So, trying to define a composition of a multiple resources where one depends on another but does not implement selectors. Probably most typical use case is kind: CompositeConnectionDetails but also things like a IAM policy template that uses ARN of the previous resource.

Since there is a dependency, template would fail at the composition and nothing will happen.

One way to workaround that as far as I understood from the documentation is by using getResourceCondition function. So I ended up with something like this:

  - step: create-iam-policy
    functionRef:
      name: function-go-templating
    input:
      apiVersion: gotemplating.fn.crossplane.io/v1beta1
      kind: GoTemplate
      source: Inline
      inline:
        template: |
          {{- if ne .observed.resources nil }}
          {{- if eq (getResourceCondition "Ready" ( index .observed.resources "kms-key" )).Status "True" }}
          {{- if eq (getResourceCondition "Ready" ( index .observed.resources "s3-bucket" )).Status "True" }}

          apiVersion: iam.aws.upbound.io/v1beta1
          kind: Policy
          metadata:
            annotations:
              gotemplating.fn.crossplane.io/composition-resource-name: "iam-role-policy"
          spec:
            forProvider:
              policy: >-
                {
                  "Version": "2012-10-17",
                  "Statement": [
                    {
                      "Action": [
                        "kms:Encrypt",
                        "kms:Decrypt",
                        "kms:DescribeKey",
                        "kms:GenerateDataKey",
                        "kms:GenerateDataKeyWithoutPlaintext",
                        "kms:GenerateDataKeyPair",
                        "kms:GenerateDataKeyPairWithoutPlaintext",
                        "kms:GenerateMac",
                        "kms:GetPublicKey"
                      ],
                      "Effect": "Allow",
                      "Resource": [
                        "{{ ( index .observed.resources "kms-key" ).resource.status.atProvider.arn }}"
                      ]
                    },
                    {
                      "Action": [
                        "s3:ListBucket",
                        "s3:GetBucketLocation",
                        "s3:ListBucketMultipartUploads"
                      ],
                      "Effect": "Allow",
                      "Resource": [
                        "{{ ( index .observed.resources "s3-bucket" ).resource.status.atProvider.arn }}"
                      ]
                    },
                    {
                      "Action": [
                        "s3:PutObject",
                        "s3:GetObject",
                        "s3:DeleteObject",
                        "s3:ListMultipartUploadParts",
                        "s3:AbortMultipartUpload"
                      ],
                      "Effect": "Allow",
                      "Resource": [
                        "{{ ( index .observed.resources "s3-bucket" ).resource.status.atProvider.arn }}/*"
                      ]
                    }
                  ]
                }
            providerConfigRef:
              name: provider-aws-local-account

          {{- end }}
          {{- end }}
          {{- end }}

  - step: store-status-and-connection-details
    functionRef:
      name: function-go-templating
    input:
      apiVersion: gotemplating.fn.crossplane.io/v1beta1
      kind: GoTemplate
      source: Inline
      inline:
        template: |
          apiVersion: meta.gotemplating.fn.crossplane.io/v1alpha1
          kind: CompositeConnectionDetails
          {{- if eq .observed.resources nil }}
          data: {}
          {{- else }}
          data:

          {{- with ( index .observed.resources "kms-key" ) }}
          {{- if eq (getResourceCondition "Ready" .).Status "True" }}
            AWS_KMS_KEY_ARN: {{ .resource.status.atProvider.arn | b64enc }}
          {{- end }}
          {{- end }}

          {{- with ( index .observed.resources "s3-bucket" ) }}
          {{- if eq (getResourceCondition "Ready" .).Status "True" }}
            AWS_REGION: {{ .resource.status.atProvider.region | b64enc }}
            AWS_BUCKET_NAME: {{ .resource.status.atProvider.id | b64enc }}
            AWS_BUCKET_ARN: {{ .resource.status.atProvider.arn | b64enc }}
          {{- end }}
          {{- end }}

          {{- with ( index .observed.resources "iam-role" ) }}
          {{- if eq (getResourceCondition "Ready" .).Status "True" }}
            AWS_ROLE_ARN: {{ .resource.status.atProvider.arn | b64enc }}
          {{- end }}
          {{- end }}

          {{- end }}

And then it got me thinking... what if for whatever reason one of these resources become not ready? Composition will void dependent resources from the output and consequentially Crossplane will try to delete them. Am I missing something or doing something wrong? Any better ways to handle these dependencies? Am I supposed to just make sure that if dependencies are not ready, at least some incomplete version of the resource is still being produced? That doesn't solve the connection details though, as keys may become temporarily corrupted...

@Menni27
Copy link

Menni27 commented Apr 17, 2024

+1

@girdharshubham
Copy link
Contributor

girdharshubham commented May 10, 2024

Yes.

The resource in your conditional block would be recreated. I just tried it out with gcp with GlobalAddress and Connection

...
            {{ if and
                ( $.observed.resources )
                (eq (getResourceCondition "Ready" (index $.observed.resources "servicenetworking-address")).Status "True")
            }}
...

Upon deleting the global address the connection resource got scheduled for deletion and then a new one took its place.

kubectl delete globaladdress shbhm-servicenetworking-address
globaladdress.compute.gcp.upbound.io "shbhm-servicenetworking-address" deleted


kubectl describe connection shbhm-servicenetworking-connection
  Normal  DeletedExternalResource  12s (x5 over 27s)  managed/servicenetworking.gcp.upbound.io/v1beta1, kind=connection  Successfully requested deletion of external resource

kubectl describe connection shbhm-servicenetworking-connection
  Normal  CreatedExternalResource  5s    managed/servicenetworking.gcp.upbound.io/v1beta1, kind=connection  Successfully requested creation of external resource

@dee-kryvenko
Copy link
Author

I might be missing something, but how are we supposed to use this function then? Until it can pass composition i.e. render successfully, it will not be creating root level resources, and until it creates root level resources - it cannot render rest of the composition. The most important reason I am using this function and not patch and transform function is that the latter do not seem to have any access to the context published by another function, so I am really in the catch 22 here. I'm sure I'm simply missing something since everyone else seem to be using it just fine?

@jan-di
Copy link
Contributor

jan-di commented Sep 2, 2024

Hi, I've contributed the function to this repo. Our use case is to have dependencies in our compositions, to ensure that certain resources are only spawned, when their dependencies are already there - as there would be many reconcilement issues if they were created at the same time.

We also noticed the problem you mentioned, and handle it like this:

  1. For all resources in our compositions, we check two states:
{{ $exists := not (empty (index (.observed.resources | default (dict)) $resourceName)) }}
{{ $ready := eq ((index (.observed.resources | default (dict)) $resourceName) | getResourceCondition "Ready").Status "True" }}
  1. When checking if a resource should be created, we have two conditions
  • All dependencies are up and ready (relevant for initial creation)
  • The mentioned resource exists (for after the initial creation)
{{ if or $subnet.exists ($network.ready) }}
# yaml of subnet resource
{{ end }}

This ensures, that a resource is never destructed due to a dependency becoming unready.

@dee-kryvenko
Copy link
Author

Correct me if I'm wrong, but that's not always possible to do. For example in my use case, the dependencies are referenced, specifically - their status field:

  - step: store-status-and-connection-details
    functionRef:
      name: function-go-templating
    input:
      apiVersion: gotemplating.fn.crossplane.io/v1beta1
      kind: GoTemplate
      source: Inline
      inline:
        template: |
          apiVersion: meta.gotemplating.fn.crossplane.io/v1alpha1
          kind: CompositeConnectionDetails
          {{- if eq .observed.resources nil }}
          data: {}
          {{- else }}
          data:

          {{- with ( index .observed.resources "kms-key" ) }}
          {{- if eq (getResourceCondition "Ready" .).Status "True" }}
            AWS_KMS_KEY_ARN: {{ .resource.status.atProvider.arn | b64enc }}
          {{- end }}
          {{- end }}

          {{- with ( index .observed.resources "s3-bucket" ) }}
          {{- if eq (getResourceCondition "Ready" .).Status "True" }}
            AWS_REGION: {{ .resource.status.atProvider.region | b64enc }}
            AWS_BUCKET_NAME: {{ .resource.status.atProvider.id | b64enc }}
            AWS_BUCKET_ARN: {{ .resource.status.atProvider.arn | b64enc }}
          {{- end }}
          {{- end }}

          {{- with ( index .observed.resources "iam-role" ) }}
          {{- if eq (getResourceCondition "Ready" .).Status "True" }}
            AWS_ROLE_ARN: {{ .resource.status.atProvider.arn | b64enc }}
          {{- end }}
          {{- end }}

          {{- end }}

If resource is not ready but exists, this composition will fail for the same reason I need to use these if statements to begin with - if composition fails, dependent resources will not be created in the first place. On the other hand, if resource becomes not ready - conditioned portion will disappear from my composition. Am I getting it right or did you mean something else @jan-di?

@jan-di
Copy link
Contributor

jan-di commented Sep 7, 2024

I was talking mainly about the creation of composed resources. I think, the XR Status and the connection details are somewhat of a special case. We actually dont check for the Ready state of composed resources when using their status fields in the XR Status/Connection secret, instead we just use a with clause

          data:
          {{- with ( index .observed.resources "kms-key" ).resource.status.atProvider.arn  }}
            AWS_ROLE_ARN: {{ . | b64enc }}
          {{- end }}

So the respective status fields are always there, when the subresources exists independent of their status. In the end, its up to consument of the XR to wait until the XR is ready itself until its guranteed that everything output in the status is ready.

@dee-kryvenko
Copy link
Author

Yeah, that's just one example - it's not necessarily limited to it... here's another example, composing KMS encrypted S3 with IAM role for it:

          apiVersion: iam.aws.upbound.io/v1beta1
          kind: Policy
          metadata:
            annotations:
              gotemplating.fn.crossplane.io/composition-resource-name: "iam-role-policy"
          spec:
            forProvider:
              # The none:NoSuchActionExists is there to make it reconcile without an error while kms and s3 resources are not yet ready
              # Otherwise AWS will not allow empty IAM policy
              policy: >-
                {
                  "Version": "2012-10-17",
                  "Statement": [
                    {
                        "Effect": "Allow",
                        "Action": "none:NoSuchActionExists",
                        "Resource": "*"
                    }
                    {{- if ne .observed.resources nil }}
                    {{- if eq (getResourceCondition "Ready" ( index .observed.resources "kms-key" )).Status "True" }}
                    ,
                    {
                      "Action": [
                        "kms:Encrypt",
                        "kms:Decrypt",
                        "kms:DescribeKey",
                        "kms:GenerateDataKey",
                        "kms:GenerateDataKeyWithoutPlaintext",
                        "kms:GenerateDataKeyPair",
                        "kms:GenerateDataKeyPairWithoutPlaintext",
                        "kms:GenerateMac",
                        "kms:GetPublicKey"
                      ],
                      "Effect": "Allow",
                      "Resource": [
                        "{{ ( index .observed.resources "kms-key" ).resource.status.atProvider.arn }}"
                      ]
                    }
                    {{- end }}
                    {{- if eq (getResourceCondition "Ready" ( index .observed.resources "s3-bucket" )).Status "True" }}
                    ,
                    {
                      "Action": [
                        "s3:ListBucket",
                        "s3:GetBucketLocation",
                        "s3:ListBucketMultipartUploads"
                      ],
                      "Effect": "Allow",
                      "Resource": [
                        "{{ ( index .observed.resources "s3-bucket" ).resource.status.atProvider.arn }}"
                      ]
                    }
                    ,
                    {
                      "Action": [
                        "s3:PutObject",
                        "s3:GetObject",
                        "s3:DeleteObject",
                        "s3:ListMultipartUploadParts",
                        "s3:AbortMultipartUpload"
                      ],
                      "Effect": "Allow",
                      "Resource": [
                        "{{ ( index .observed.resources "s3-bucket" ).resource.status.atProvider.arn }}/*"
                      ]
                    }
                    {{- end }}
                    {{- end }}
                  ]
                }

In a case crossplane "thinks" there is something wrong with either kms/s3, it will stip out the role of the relevant policies, potentially causing production outage. There are lots more examples, not everything fits into this model of "selectors", some resources are just missing them, some is not a simple "reference" like in the policy above.

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

No branches or pull requests

4 participants