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

Cannot change issuer field in FederatedIdentityCredential #622

Closed
gravufo opened this issue Jan 10, 2024 · 5 comments
Closed

Cannot change issuer field in FederatedIdentityCredential #622

gravufo opened this issue Jan 10, 2024 · 5 comments
Labels
bug Something isn't working community

Comments

@gravufo
Copy link
Contributor

gravufo commented Jan 10, 2024

What happened?

I created a simple FederatedIdentityCredential using the spec here: https://marketplace.upbound.io/providers/upbound/provider-azure-managedidentity/v0.41.0/resources/managedidentity.azure.upbound.io/FederatedIdentityCredential/v1beta1

It creates successfully. Then, I edit the issuer field to set a different value and I get the following:

- lastTransitionTime: "2024-01-10T22:29:44Z"
  message: async update failed: refuse to update the external resource: cannot change the value of the argument "issuer" from "old" to "new"
  reason: AsyncUpdateFailure
  status: "False"
  type: LastAsyncOperation 

How can we reproduce it?

  1. Create a valid FederatedIdentityCredential
  2. Wait until it is synced and ready
  3. Edit the resource's spec.forProvider.issuer field
  4. Describe the resource to see the status.condition field containing the error

Note: Even though the update fails, it still shows the resource as Synced + Ready = True. Is that also a bug?

What environment did it happen in?

  • Crossplane Version: v1.14.5
  • Provider Version: v0.41.0
  • Kubernetes Version: v1.28.3
  • Kubernetes Distribution: AKS
@gravufo gravufo added bug Something isn't working needs:triage labels Jan 10, 2024
@gravufo
Copy link
Contributor Author

gravufo commented Jan 10, 2024

Note that the azurerm provider does not mention any field being immutable: https://registry.terraform.io/providers/hashicorp/azuread/latest/docs/resources/application_federated_identity_credential
The Azure Portal also seems to allow editing the field.

Also, it may affect more fields than just issuer, so that could also be checked/validated if it uses the same code path.

@turkenf
Copy link
Collaborator

turkenf commented Jan 22, 2024

Hi @gravufo,

Thanks for bringing up this. Updating the issuer field requires deleting and recreating the existing resource due to the behavior of the underlying provider. This is a situation that we do not allow according to the Crossplane Resource Model.

@gravufo
Copy link
Contributor Author

gravufo commented Jan 22, 2024

@turkenf Thank you for your response!
You are indeed right. I am used to the azurerm documentation mentioning when fields force a recreate and hadn't thought checking the actual provider code.
I understand why this behaviour is replicated here.

My follow up question would be: is it possible to simply refuse the resource apply rather than accept the apply and then shoot an error that says update failed? I feel like for this type of error, it's best to fail as early as possible.

Thanks!

@turkenf
Copy link
Collaborator

turkenf commented Jan 23, 2024

My follow up question would be: is it possible to simply refuse the resource apply rather than accept the apply and then shoot an error that says update failed? I feel like for this type of error, it's best to fail as early as possible.

Yes, possible, the related issue: crossplane/upjet#78

@gravufo
Copy link
Contributor Author

gravufo commented Jan 23, 2024

Awesome! Will close this issue then!
Thank you for the help :)

@gravufo gravufo closed this as completed Jan 23, 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 community
Projects
None yet
Development

No branches or pull requests

2 participants