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

[Bug]: Unexpected Behavior with IAM Role Name Collision Handling #1131

Closed
1 task done
JanillLema opened this issue Feb 6, 2024 · 3 comments
Closed
1 task done

[Bug]: Unexpected Behavior with IAM Role Name Collision Handling #1131

JanillLema opened this issue Feb 6, 2024 · 3 comments
Labels
bug Something isn't working needs:triage stale

Comments

@JanillLema
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Affected Resource(s)

"iam.aws.upbound.io/v1beta1 - Role"

Resource MRs required to reproduce the bug

composition snippet:

  mode: Pipeline
  pipeline:
  - step: render-templates
    functionRef:
      name: function-go-templating
    input:
      apiVersion: gotemplating.fn.crossplane.io/v1beta1
      kind: GoTemplate
      source: Inline
      inline:
        template: |
          {{ $claim  	:= .observed.composite.resource }}
          {{ $parameters := .observed.composite.resource.spec }}

          {{ $serviceAccountName := default (printf "s--%s" $parameters.appName) $parameters.serviceAccount.name }}

          {{ $iamRoleManagedResourceName := lower ((printf "%s-%s" $serviceAccountName $parameters.clusterName) | replace "_" "-") }}
          `}}

          {{ $clusterOidcProviderUrl := .Values.clusterOidcProviderIssuer | replace "https://" "" }}

          ---
          # Create IAM role 
          apiVersion: iam.aws.upbound.io/v1beta1
          kind: Role
          metadata:
            name: {{`{{ $iamRoleManagedResourceName  }}`}}
            annotations:
              gotemplating.fn.crossplane.io/composition-resource-name: role
          spec:
            forProvider:
              assumeRolePolicy: | 
                {
                  "Version": "2012-10-17",
                  "Statement": [
                    {
                      "Effect": "Allow",
                      "Principal": {
                        "Federated": "arn:aws:iam::{{`{{ $parameters.awsAccountID }}`}}:oidc-provider/{{ $clusterOidcProviderUrl }}"
                      },
                      "Action": "sts:AssumeRoleWithWebIdentity",
                      "Condition": {
                        "StringEquals": {
                          "{{ $clusterOidcProviderUrl }}:aud": "sts.amazonaws.com",
                          "{{ $clusterOidcProviderUrl }}:sub": "system:serviceaccount:{{`{{ $claim.spec.claimRef.namespace }}`}}:{{`{{ $serviceAccountName }}`}}"
                        }
                      }
                    }
                  ]
                }
              tags: {{`{{ $parameters.tags | toJson }}`}}
            providerConfigRef:
              name: {{`{{ $claim.spec.claimRef.namespace }}`}}

Steps to Reproduce

  1. identify an existing iam role on aws
  2. set the name of a role managed resource to the name of the existing role

What happened?

I am using crossplane to create an IRSA composite resource. In my design, a tenant is able set the name of the role through the roleName parameter on their claim. This works as expected. However, I am testing two edge cases:

  1. The tenant sets the name to that of an existing role managed by a different tenant in the same AWS account
  2. The tenant accidentally creates multiple claims with the same role name

In scenario 1, the claim imports the existing role and updates the role to match the configurations set on the claim (policies, tags, permission boundaries). I assumed imports where only achieved by setting the externalname annotation of the managed resource. I would have expected an error stating that a role with the same name already exists. Similar to how this would work in terraform.

In scenario 2, the first claim that is applied creates the role. The second claim does not create an additional role or return an error that the role managed resource already exists. I expected that the second claim would be in an unready state since a managed resource of the same kind and name already exists and is managed by the first claim.

Any guidelines on how to enable tenants to set the names of resources with proper name collision handling would be appreciated. I see that these scenarios are true for policies as well. Not sure if this is the expected behavior of crossplane and instead I should allow crossplane to determine the name of the role.

Relevant Error Output Snippet

No response

Crossplane Version

1.14.4

Provider Version

v0.47.1

Kubernetes Version

1.26

Kubernetes Distribution

EKS

Additional Info

related slack thread: https://crossplane.slack.com/archives/C05E0UE46S2/p1706300164414329

@mbbush
Copy link
Collaborator

mbbush commented Jun 12, 2024

Sorry this took so long for anyone to answer you. I stumbled upon this today while looking for something else and thought I could at least give it an answer for other users with similar questions, even if you've likely moved on by this point.

I assumed imports where only achieved by setting the externalname annotation of the managed resource. I would have expected an error stating that a role with the same name already exists. Similar to how this would work in terraform.

Your assumption is incorrect, and is one of the ways in which the crossplane resource model (XRM) differs from terraform.

The existence of a managed resource in the cluster is an expression of the operator's intent that the corresponding external resource (in this case, an IAM role) should exist, with configuration matching what's in the managed resource's spec. The crossplane provider is then responsible for making that happen.

There are some resources that do not work this way. An aws secretsmanager Secret is one example. But the reason for this behavior difference is a technical limitation: secretsmanager secrets use a generated suffix in their identifier in AWS that the provider can't determine ahead of time.

Copy link

This provider repo does not have enough maintainers to address every issue. Since there has been no activity in the last 90 days it is now marked as stale. 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.

@github-actions github-actions bot added the stale label Sep 11, 2024
Copy link

This issue is being closed since there has been no activity for 14 days since marking it as stale. If you still need help, feel free to comment or reopen the issue!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs:triage stale
Projects
None yet
Development

No branches or pull requests

4 participants