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

Investigate Upstream AppMesh API Changes #7659

Closed
bflad opened this issue Feb 23, 2019 · 18 comments · Fixed by #7858
Closed

Investigate Upstream AppMesh API Changes #7659

bflad opened this issue Feb 23, 2019 · 18 comments · Fixed by #7858
Assignees
Labels
service/appmesh Issues and PRs that pertain to the appmesh service. upstream Addresses functionality related to the cloud provider.
Milestone

Comments

@bflad
Copy link
Contributor

bflad commented Feb 23, 2019

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Potentially Affected Resource(s)

  • aws_appmesh_mesh
  • aws_appmesh_route
  • aws_appmesh_virtual_node
  • aws_appmesh_virtual_router

Description

AWS has publicly announced potentially breaking changes to the AppMesh service API: aws/aws-app-mesh-examples#92

We will need to investigate the proposed upstream changes and determine a path forward for the existing Terraform AWS Provider resources.

@bflad bflad added upstream Addresses functionality related to the cloud provider. service/appmesh Issues and PRs that pertain to the appmesh service. labels Feb 23, 2019
@bcelenza
Copy link
Contributor

I'd be happy to pull request the required changes for AWS App Mesh.

I would also love to understand how this change impacts current users of the Terraform integration with App Mesh, and will keep an eye on this issue during the comment period.

@ewbankkit
Copy link
Contributor

We should probably start with a note in the documentation stating that the aws_appmesh_virtual_node and aws_appmesh_virtual_router resources will change in an upcoming release and link to the AWS GH issue.

@ewbankkit
Copy link
Contributor

@bcelenza I'm totally happy for you to do the PR/collaborate as necessary.

@bcelenza
Copy link
Contributor

Thanks @ewbankkit! Opened a PR for the doc changes.

@ewbankkit
Copy link
Contributor

I added some comments to the announcement.

My thoughts:

  • Add a new aws_appmesh_virtual_service resource (with terraform import functionality as Amazon have already created these)
  • Modify the aws_appmesh_virtual_node and aws_appmesh_virtual_router resources with the relevant changes. State migration should be straightforward

My only concerns are around the short deprecation notice. I don't think we can follow the published best practice of doing a major release just for this and I'm not sure how this fits in with the provider v2.0.0 release timetable, but App Mesh is a preview service so there really should be reduced expectations around API stability.

@bcelenza
Copy link
Contributor

bcelenza commented Feb 25, 2019

Agree with your thoughts. I've read through the best practices, hoping to confirm my assumptions below.

Using VirtualNode as an example of a resource:

  • spec.serviceDiscovery.dns.serviceName to hostname appears to be supported via Renaming a Required Attribute?
  • spec.backends does not have a clear migration path given the structure change. Does Terraform have a mechanism for this sort of migration? It would likely be safe to assume that a string passed would be interpreted as a VirtualService name.

Alternatively, we could just update the resources to reflect the new APIs, and anyone on the current released integration will need to drop and recreate their App Mesh resources with the new structures. This is where my concern for existing adoption comes in to play.

@ewbankkit
Copy link
Contributor

@bcelenza Terraform has robust state migration functionality so we should be able to rewrite state files created with the initial API version into a format that reflects the new API version:

  • service_name -> hostname
  • backends changes from list of virtual_services which are themselves a simple map with a single virtual_service_name string element

We should be able to find an suitable example in the code.

@ewbankkit
Copy link
Contributor

Additional proposed API changes:

@ewbankkit
Copy link
Contributor

@bcelenza Is there a newer API model JSON with the latest proposed changes?

@bcelenza
Copy link
Contributor

bcelenza commented Mar 1, 2019

@ewbankkit Not yet -- we're still waiting for the last pieces of VirtualRouter listeners to reach production before we distribute an updated JSON. ETA Tuesday of next week, if I had to guess.

@bcelenza
Copy link
Contributor

bcelenza commented Mar 7, 2019

Updated Go SDK is now available as v1.17.13.

@ewbankkit
Copy link
Contributor

@bcelenza I am working right now on a draft PR based on the original app-mesh-2019-01-25.trial.json model changes. I'll rebase it on the new AWS SDK.

@ewbankkit
Copy link
Contributor

Will do, just wanted to get my current WIP pushed: #7858.

@ewbankkit
Copy link
Contributor

@bcelenza To clarify dependencies (I saw some error messages during testing):

  • If a virtual service has a virtual node or virtual router provider then that virtual node or virtual router cannot be deleted before virtual service is deleted, correct?
  • The dependency graph is then
    • Mesh => Virtual Node => Virtual Service or
    • Mesh => Virtual Router => Virtual Service

@bcelenza
Copy link
Contributor

bcelenza commented Mar 8, 2019

@bcelenza To clarify dependencies (I saw some error messages during testing):

  • If a virtual service has a virtual node or virtual router provider then that virtual node or virtual router cannot be deleted before virtual service is deleted, correct?

  • The dependency graph is then

    • Mesh => Virtual Node => Virtual Service or
    • Mesh => Virtual Router => Virtual Service

That's correct, the Virtual Service must be deleted before it's provider (VirtualNode or VirtualRouter). The thinking behind this is that the same actor in an organization likely owns both the Virtual Service and its providers, so it is there to protect them from creating an unintended outage of their own service.

@bflad bflad self-assigned this Mar 15, 2019
@bflad bflad added this to the v2.3.0 milestone Mar 15, 2019
@bflad
Copy link
Contributor Author

bflad commented Mar 15, 2019

These changes will go out in version 2.3.0 of the Terraform AWS Provider middle of next week unless we decide to release this as a 3.0.0 due to the breaking changes.

In the future, we may decide to relegate Public Preview and Beta APIs to separate Terraform AWS Provider(s) to prevent this situation in the future.

bflad added a commit that referenced this issue Mar 15, 2019
bflad added a commit that referenced this issue Mar 21, 2019
…here the AWS service is in preview

References: #7659

Backwards compatibility with AWS services in preview is best effort. Here we add a note to affected Terraform resources since they may not follow our normal compatibility promises as outlined on the [HashiCorp Blog](https://www.hashicorp.com/blog/hashicorp-terraform-provider-versioning) and [Extending Terraform documentation](https://www.terraform.io/docs/extend/best-practices/versioning.html). This notice mainly applies to Security Hub at the moment, but could apply to MSK (Kafka) if that is added before GA as well.
@bflad
Copy link
Contributor Author

bflad commented Mar 21, 2019

This has been released in version 2.3.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/appmesh Issues and PRs that pertain to the appmesh service. upstream Addresses functionality related to the cloud provider.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants