ENG-9397: Repository Binding Resource Changes #329
Conversation
hbaackmann
requested review from
wcmjunior,
gengdahlCyral,
Yowgf and
VictorGFM
There was a problem hiding this comment.
Hi @hbaackmann, I've added some comments for your consideration. The other parts of the code look good!
| DeleteContext: resourceRepositoryBindingDelete, | ||
|
|
||
| SchemaVersion: 1, | ||
| StateUpgraders: []schema.StateUpgrader{ |
There was a problem hiding this comment.
If we remove the StateUpgraders, old state versions of the cyral_repository_binding will probably cause an error when trying to apply the resource (See - this is for the Framework but I believe it also applies to SDKv2). How are we planning to do the state migration in this case? Why are we not using the StateUpgraders to do the state migration by keeping the StateUpgrader for v0 and adding another one for v1?
There was a problem hiding this comment.
Good point, I wasn't sure how to handle that. I can re-add the old state upgraders-- but I figured there is no point, since the migration script will need to be run anyway (in order to upgrade to 4.0, all bindings will be migrated in the CP and then imported into terraform, like the previous migration). I investigated using the state upgraders for migration, however I decided against it because I was concerned with the fact that if the StateUpgrade fails for some reason, it would be hard to catch and address it (since state upgrade happens automatically when terraform apply is run). Additionally, since bindings are migrated in the CP using various validations, I figured it was safer to just import those back into terraform to eliminate potential inconsistencies. Also, since listeners need to be created based on bindings, there needs to be a script anyway.
There was a problem hiding this comment.
I see. In this case makes sense to remove the StateUpgraders - since we need to have a migration script anyway.
| // SidecarAsIdPAccessGateway is set to false to stop | ||
| // using the bound sidecar as the Access Gateway for Identity | ||
| // Provider users. This is needed so that the binding can | ||
| // be deleted, otherwise it will throw a validation error. | ||
| resourceData := getRepoBindingDataFromResource(d) | ||
| resourceData.SidecarAsIdPAccessGateway = false | ||
| if err := updateRepositoryBinding(c, resourceData); err != nil { | ||
| return createError("Unable to delete repository binding", | ||
| fmt.Sprintf("%v", err)) | ||
| } |
There was a problem hiding this comment.
As I understand, we removed the access gateway flag from the binding resource, and according to this discussion we will have a separate API to manage the access gateway. If that's the case, this is great, especially from a Terraform perspective, the way we designed this access gateway flag in the past caused a lot of unwanted behavior when managing the bindings (The code above is one of the examples, as it was a workaround to avoid errors caused by this gateway flag when trying to delete the binding).
There was a problem hiding this comment.
That is correct! There will be an entirely new resource for access gateways.
| # MongoDB replica set. Cyral will automatically/dynamically identify | ||
| # the remaining nodes of the replication cluster. | ||
| host = "mycluster-shard-00-01.example.mongodb.net" | ||
| # MongoDB replica set. You can explictly specify the host and port of |
There was a problem hiding this comment.
@gengdahlCyral Can you review the comments on this guide? I need some guidance here to make sure that everything is explained properly :)
| Type: repositoryBindingResourceSchemaV0(). | ||
| CoreConfigSchema().ImpliedType(), | ||
| Upgrade: upgradeRepositoryBindingV0, | ||
| ListenerBindingKey: { |
There was a problem hiding this comment.
@gengdahlCyral Can you also make sure all of the descriptions seem correct to you?
There was a problem hiding this comment.
LGTM @hbaackmann! Very good to know that we simplified a lot how to calculate the total number of bound ports with this new binding/listener design. The docs also look good to me, although would be better if we could have someone else reviewing them, just to make sure the guides cover all the examples we want to provide to users with this new design.
* bindings * bound ports data source and test * fix query params for bound ports * update all documentation
* bindings * bound ports data source and test * fix query params for bound ports * update all documentation
Description of the change
This PR contains changes to update the cyral_repository_bound_ports resource for Port Multiplexing.
The bound ports data source has been disabled, and will be updated in a separate PR.
Note: Once the changes to the repository resource were merged in, I replaced my original PR with this PR, since I ended up switching branches.
Type of change
Checklists
Development
Code review
Testing