Add full support for SSO resources #928
Conversation
mbbush
requested review from
ulucinar,
sergenyalcin and
turkenf
as code owners
| p.AddResourceConfigurator("aws_identitystore_group", func(r *config.Resource) { | ||
| // Display name is required by terraform, and while it's not part of the external name or terraform id, it is | ||
| // how the group is displayed, and it's immutable. | ||
| r.ExternalName.IdentifierFields = append(r.ExternalName.IdentifierFields, "display_name") |
There was a problem hiding this comment.
Is there an easy, idiomatic golang way to append this only if it's not already present? In e.g. python I'd probably do it by converting to a set and then back to a list. Or maybe it's not worth worrying about since this happens at generation time.
Or maybe it's better to leave this off? I added it in an effort to get the field marked as required in the generated schema, but subsequently I realized that many fields which are actually required by terraform are not marked as required in the schema, but are instead validated at the kubernetes level using slightly more sophisticated logic that takes into account initProvider and managementPolicies.
Conceptually it makes sense that the display name would be an identifier field. But it's not necessary to mark it as such in order for the managed resource to work properly.
|
@turkenf This is ready for another look. |
|
@turkenf Ready for your review again. Is there anything else this needs before merging? I'm hoping to get this in the October release so I can start using these resources next week at work. |
|
I am closing and reopening PR to run license/cla again. |
| @@ -95,6 +95,7 @@ spec: | |||
| description: Key-value map of resource tags. | |||
| type: object | |||
| required: | |||
| - instanceArn | |||
There was a problem hiding this comment.
This is a breaking change. It's a consequence of normalizing the external name for this resource.
From a practical point of view, this will only affect Observe only resources, because this parameter was always required by terraform, and therefore included in the kubernetes validation. If I make it no longer required, if this is missing, and the external name annotation is set to the permission set id (the normalized external name I chose), crossplane will be unable to construct the terraform id, which contains the instance arn, so it won't be able to import the existing resource.
I could remove it from the IdentifierFields in the external name configuration, which would avoid the breaking schema change, but it feels like it really should be required, especially since omitting it will cause imports to fail.
Another option would be to change the external name to include the instance arn (perhaps by changing it from the short id of the permission set to the full ARN of the permission set, which embeds the id of the instance, from which the instance arn can be derived). But that seems like sacrificing the long-term user experience in order to avoid a minor breaking api schema change.
There was a problem hiding this comment.
With this removed from IdentifierFields in the external name config, the following examples still work:
- A resource that specifies all the fields in
spec.forProviderwith default management policies - A resource that doesn't include this field in
spec.forProvider, but does include it inspec.initProvider, with default management policies (this field is immutable, so there's really no reason someone would want to put it inspec.initProvider) - A resource with only the
Observemanagement policy, that specifies the external name annotation and populates this field to the correct value inspec.forProvider
This doesn't work:
- A resource with only the
Observemanagement policy, that specifies the external name annotation, but doesn't populate this field. This passes the kubernetes validator, but fails at runtime because it can't construct the terraform id.
There was a problem hiding this comment.
I can think of a handful of ways forward here:
- Normalize the external name to just the provider-generated part of the ARN, and leave
instanceArnas an identifier field. This is the approach the existing PR takes, and if this was a new resource, as opposed to an update to an existing one, this seems like it would be the clear winner. - Normalize the external name, but don't specify
instanceArnas an identifier field. This will cause an error for Observe resources as mentioned above. - Use the permission set ARN as the external name (as opposed to the current external name which matches the terraform id:
${permission_set_arn},${instance_arn}). This could be done without specifyinginstanceArnas an identifier field because the instance arn can be derived from the permission set arn. - Leave the current external name which matches the terraform id. This would certainly be the smallest change, but it's also a fairly cumbersome external name to have to use.
One factor that seems relevant to deciding is what happens to existing managed resources during a provider upgrade, when the external name config changes. I'm hoping that as long as the resources were ready, crossplane would simply reconstruct the external name from the terraform id, using the new logic, which would provide the correct result, but I'm not sure quite how best to test this. Would it be sufficient to take a MR that has become ready and simply edit its external-name annotation to the old format? Would there be other edits I should do at the same time? Or is it necessary to do a "full" test, where I actually do a provider upgrade on my test cluster?
| // SSO Managed Policy Attachments can be imported using the name, path, permission_set_arn, and instance_arn separated by a comma (,) | ||
| // Example: TestPolicy,/,arn:aws:sso:::permissionSet/ssoins-2938j0x8920sbj72/ps-80383020jr9302rk,arn:aws:sso:::instance/ssoins-2938j0x8920sbj72 | ||
| // This can't really be normalized. | ||
| "aws_ssoadmin_customer_managed_policy_attachment": config.TemplatedStringAsIdentifier("", "{{ (index .parameters.customer_managed_policy_reference 0).name }},{{ (index .parameters.customer_managed_policy_reference 0).path }},{{ .parameters.permission_set_arn }},{{ .parameters.instance_arn }}"), |
There was a problem hiding this comment.
This results in the crossplane external name matching the terraform id, which is quite long, but it occurs to me that perhaps it doesn't need to?
This is quite possibly a bad idea, but it's not immediately obvious to me why we couldn't just have the external-name match metadata.name, and construct the terraform id by ignoring the external name and applying the template to the parameters.
|
@turkenf Anything else I can do to get this merged? |
|
@mbbush, there is nothing you can do right now, it is waiting for the team to review, it will be reviewed as soon as possible, thank you. |
|
@mbbush Thank you for this contribution. I reviewed the PR. Let me talk about two parts of this. The first part is adding/configuring new resources. I think this part is ready for merge. The second part is changing the external name configurations for two resources. I am concerned about the breaking API changes. We do not want to introduce any breaking API changes in minor versions. It seems that for the In the mid/long-term, we want to switch to XValidation-Rules where possible so that fields behave as required. So, ideally, we want to leave no required fields in the schemas and ensure that the relevant fields act as mandatory with XValidation-Rules. For the In short, if we address these normalizations without breaking changes (API or external name syntax), I think the normalization part will be okay for the minor version. However, if this is not the case and if we need some breaking changes for normalization, I will suggest separating the PR into two parts. And merging the adding new resources part here (for six resources). Then, we may consider handling the remaining external name normalizations in another PR for the major version releases. |
|
Thanks for the review @sergenyalcin! I've rebased on the latest main, removed the normalization of The external name for PermissionSetInlinePolicy is changing, from The only thing I can think of possibly breaking as a result would be a composition or gitops manifest that was constructing the external name for an If you still think it's important to not change the external name for I'll rerun the tests tomorrow and edit the top comment with results. |
Yes, I was trying to say that the external names are also part of the public API. The API normalization is important in the context of the provider quality. Before, we observed some issues with the breaking external name changes that could not be handled and caused a few problems. So, external names are part of the public API. If it is also okay for you, I think we may consider removing the Thank you for bearing with me throughout this PR. |
|
@sergenyalcin this is rebased, tested, and ready for review with the changes you requested. |
|
Great, thanks. I don't have the write access to merge it myself, so someone else will have to click the button. |
Description of your changes
mainThis incorporates the changes in #920 and has been tested, so I'll close #920.
Fixes #536 (mostly)
Fixes #888
Closes #920
I have:
make check-diff testto validate the unit tests and generated code. Running the linter causes out-of-memory errors (it was using more than 50GB).How has this code been tested
I removed the manual-intervention annotations, substituted in my account-specific values, and ran
make uptestwith all the examples in this PR. All the tests passed (create, update, import, delete).I did discover one breaking change from the switch to the no-fork provider. Details at #1013
Here's a snapshot of
while true; do date; kubectl get managed; donefrom the time the first uptest step completed, showing everything synced and ready.How can someone else test this code?
AWS SSO can only be enabled in the "management account" of an organization. The easiest way to do this is in a standalone personal account. You can go to SSO identity center in the aws console to enable this, which will allow you to make your account into an organization. This will allow you to generate both an "identity store id" and an "sso instance arn" which are the two parameters you need to manually supply to the identitystore and ssoadmin example resources, respectively.
The example resources must be created in the same region as your SSO instance. I made my SSO instance in us-east-1, so that's the region I committed to all the example manifests.
AWS does not charge for any of these SSO resources, so there were no financial considerations to testing with a personal account.