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

AppMesh VirtualNode creation fails due to broken servicediscovery.Service.from_service_attributes call #4286

Closed
tsykora-verimatrix opened this issue Sep 29, 2019 · 4 comments · Fixed by #4313
Assignees
Labels
@aws-cdk/aws-appmesh Related to AWS App Mesh bug This issue is a bug. language/python Related to Python bindings

Comments

@tsykora-verimatrix
Copy link

Tldr version: It is not possible to create AppMesh VirtualNode in Fargate environment because CloudMap service is already created by ecs.FargateService and servicediscovery.Service.from_service_attributes call to look up that CloudMap service is broken

Detailed description: When trying to create AppMesh VirtualNode using servicediscovery.Service.from_service_attributes as cloud_map_service parameter for appmesh.VirtualNode call we get the following error:

jsii.errors.JSIIError: Cannot read property 'namespaceName' of undefined

Using method servicediscovery.Service.from_service_attributes to import existing CloudMap service rather than letting AppMesh to create a new service is common when working with Fargate. The reason is that the ecs.FargateService call is using parameter cloud_map_options to create a new CloudMap ServiceDiscovery Service. This is mandatory in order to register the FargateTasks with CloudMap. Since the CloudMap Service is created during creation of FargateService, the only option is to import existing CloudMap service when creating AppMesh VirtualNode. It is not possible to let AppMesh VirtualNode to create that CloudMap service as it has been already created by ecs.FargateService. Moreover the object type in servicediscovery.Service (required by appmesh.VirtualNode) is IService which is different to ecs.FargateService that is using CloudMapOptions type of object. So althought we need to reference the same CloudMap service, it has to be looked up instead of just referenced from previous step, however the lookup method is broken.

By checking source code in aws_cdk/aws_appmesh/init.py we can see that namespaceName is not included in class VirtualNode nor class CfnVirtualNode as parameter. Instead it is included in AwsCloudMapServiceDiscoveryProperty.
In the TypeScript version of the documentation the same call to create a new VirtualNode namespace_name is listed as required parameter:
https://docs.aws.amazon.com/cdk/api/latest/docs/aws-appmesh-readme.html

const node = mesh.addVirtualNode('virtual-node', {
  hostname: 'node-a',
  **namespace,**
  listeners: {
    portMappings: [
      {
        port: 8081,
        protocol: appmesh.Protocol.HTTP,
      },
    ],
    healthChecks: [
      {
        healthyThreshold: 3,
        intervalMillis: 5000, // minimum
        path: `/health-check-path`,
        port: 8080,
        protocol: appmesh.Protocol.HTTP,
        timeoutMillis: 2000, // minimum
        unhealthyThreshold: 2,
      },
    ],
  },
});

Reproduction Steps

Step1: create Fargate service first along with CloudMap service (see cloud_map_options)

fargate_service = ecs.FargateService(self,
            <your service name>,
            service_name=<your service name>,
            task_definition=task_def,
            assign_public_ip=False,
            cloud_map_options=ecs.CloudMapOptions(
                 dns_record_type=servicediscovery.DnsRecordType.A,
                 dns_ttl=core.Duration.seconds(10),
                 name=short_service_name
            ),
            platform_version=ecs.FargatePlatformVersion.LATEST,
            security_group=<your sg>,
            cluster=tenant_cluster,
            desired_count=0,
            enable_ecs_managed_tags=True,
        )

Step 2: try to create AppMesh virtual node (behaviour is the same for both methods to create virtual node - that means by using add virtual node method or creating separated virtual node and referencing the appmesh)

        virtual_node = appmesh.VirtualNode(self,
            virtual_node_name,
            mesh=<your app mesh>,
            cloud_map_service=servicediscovery.Service.from_service_attributes(self,
                 <your service name>,
                 dns_record_type=servicediscovery.DnsRecordType.A,
                 routing_policy=servicediscovery.RoutingPolicy.MULTIVALUE,
                 service_arn=<name space arn>,
                 service_id=<name space id>,
                 service_name=<your service name>,
            ),
            listener=appmesh.VirtualNodeListener(
                health_check=appmesh.HealthCheck(
                    protocol=appmesh.Protocol.TCP,
                ),
                port_mapping=appmesh.PortMapping(
                    port=<your application ports>,
                    protocol=appmesh.Protocol.TCP
                ),
            ),
            virtual_node_name=virtual_node_name
        )

Error Log

File "/usr/local/lib/python3.7/site-packages/aws_cdk/aws_appmesh/__init__.py", line 3131, in add_virtual_node return jsii.invoke(self, "addVirtualNode", [id, props]) File "/usr/local/lib/python3.7/site-packages/jsii/_kernel/__init__.py", line 104, in wrapped return _recursize_dereference(kernel, fn(kernel, *args, **kwargs)) File "/usr/local/lib/python3.7/site-packages/jsii/_kernel/__init__.py", line 267, in invoke args=_make_reference_for_native(self, args), File "/usr/local/lib/python3.7/site-packages/jsii/_kernel/providers/process.py", line 346, in invoke return self._process.send(request, InvokeResponse) File "/usr/local/lib/python3.7/site-packages/jsii/_kernel/providers/process.py", line 316, in send raise JSIIError(resp.error) from JavaScriptError(resp.stack) jsii.errors.JSIIError: Cannot read property 'namespaceName' of undefined

Environment

  • **CLI Version : 1.9.0 **
  • Framework Version:
  • OS : Mac OS
  • Language : Python

Other


This is 🐛 Bug Report

@tsykora-verimatrix tsykora-verimatrix added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 29, 2019
@SomayaB SomayaB added the @aws-cdk/aws-appmesh Related to AWS App Mesh label Sep 30, 2019
@SomayaB SomayaB added the language/python Related to Python bindings label Sep 30, 2019
@SomayaB SomayaB assigned RomainMuller and unassigned rix0rrr Sep 30, 2019
@dastbe
Copy link
Contributor

dastbe commented Sep 30, 2019

I'm much less experienced w/ CDK, but looking at the input

cloud_map_service=servicediscovery.Service.from_service_attributes(self,
                 <your service name>,
                 dns_record_type=servicediscovery.DnsRecordType.A,
                 routing_policy=servicediscovery.RoutingPolicy.MULTIVALUE,
                 service_arn=<name space arn>,
                 service_id=<name space id>,
                 service_name=<your service name>,
            ),

EDIT: also, the values that you would have for service_arn/service_id don't sound correct. I would expect service_arn/id to be the ones for the service, not the namespace.

vs. the implementation of from_service_attributes

  public static fromServiceAttributes(scope: Construct, id: string, attrs: ServiceAttributes): IService {
    class Import extends ServiceBase {
      public namespace: INamespace;
      public serviceId = attrs.serviceId;
      public serviceArn = attrs.serviceArn;
      public dnsRecordType = attrs.dnsRecordType;
      public routingPolicy = attrs.routingPolicy;
      public serviceName = attrs.serviceName;
    }

it looks like the namespace isn't actually getting retrieved? This appears to be a bug in servicediscovery and not the appmesh package.

@tsykora-verimatrix
Copy link
Author

@dastbe I too thought the service_arn and service_id are related to service, but documentation says otherwise: https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-servicediscovery.Service.html (see Properties)

Regarding the namespace that isn't retrieved - you might be right, it could be also service discovery problem, not only AppMesh. However, there is also a discrepancy in documentation and implementation of AppMesh. In the example docs that I posted originally you can see that namespace name is provided as a parameter to Appmesh.addVirtualNode, but it disappeared from the implementation. Maybe it was there originally but was removed later on for some reason?

@dastbe
Copy link
Contributor

dastbe commented Sep 30, 2019

@tsykora-verimatrix looking at the docs you referenced for addVirtualNode, what you replaced with**namespace** was originally cloudMapService: service,. I think what's causing confusion is the CloudMap docs, as there's confusion between what is the service and what is the namespace.

It's always been the case that the cloudMapService should be a cloudmap service, and that object should encapsulate information about both the service and the namespace. The from_service_attributes call isn't resolving the namespace information, which is required.

rix0rrr added a commit that referenced this issue Oct 1, 2019
- Fix the missing initialization of `namespace` on CloudMap's
  `Service.fromServiceAttributes()`.
- Expose the created CloudMap Service on ECS services so that
  `fromServiceAttributes()` doesn't need to be called at all.
- Fix a number of property initialization errors across the library.

Fixes #4286.

BREAKING CHANGE: `cloudmap.Service.fromServiceAttributes` takes a newly
required argument `namespace`.
@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 1, 2019

Thank you for reporting the issue with fromServiceAttributes. Ideally, you wouldn't have to use that, but unfortunately it seems there is no good way to get to the CloudMap Service from an ECS object after it's been created.

I have rectified this in the linked PR.

@rix0rrr rix0rrr removed the needs-triage This issue or PR still needs to be triaged. label Oct 1, 2019
rix0rrr added a commit that referenced this issue Oct 4, 2019
…ce (#4313)

- Fix the missing initialization of `namespace` on CloudMap's
  `Service.fromServiceAttributes()`.
- Expose the created CloudMap Service on ECS services so that
  `fromServiceAttributes()` doesn't need to be called at all.
- Fix a number of property initialization errors across the library.

Fixes #4286.

BREAKING CHANGE: `cloudmap.Service.fromServiceAttributes` takes a newly
required argument `namespace`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-appmesh Related to AWS App Mesh bug This issue is a bug. language/python Related to Python bindings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants