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

Can't get a CfnInstance with instance.getInstance() #4080

Closed
johnjelinek opened this issue Sep 15, 2019 · 21 comments
Closed

Can't get a CfnInstance with instance.getInstance() #4080

johnjelinek opened this issue Sep 15, 2019 · 21 comments
Assignees
Labels
bug This issue is a bug. language/java Related to Java bindings p1

Comments

@johnjelinek
Copy link

johnjelinek commented Sep 15, 2019

🐛 Bug Report

What is the problem?

Can't get a CfnInstance with instance.getInstance(). I was hoping to get a CfnInstance to see if I can pass it in to Targets of CfnMaintenanceWindowTargetProps.

Reproduction Steps

(let [app (new App)
      stack (new Stack app "stack" stack-props)
      vpc (new Vpc stack "vpc")
      instance (new Instance stack "server" instance-props)
      window (new CfnMaintenanceWindow stack "window" maintenance-window-props)
      target (new CfnMaintenanceWindowTarget stack "target" maintenance-window-target-props)]
  (.getInstance instance)
  (.synth app))

Verbose Log

Execution error (IllegalArgumentException) at com.fasterxml.jackson.databind.introspect.POJOPropertyBuilder/getSetter (POJOPropertyBuilder.java:497).
Conflicting setter definitions for property "networkInterfaces": software.amazon.awscdk.services.ec2.CfnInstance#setNetworkInterfaces(1 params) vs software.amazon.awscdk.services.ec2.CfnInstance#setNetworkInterfaces(1 params)
class software.amazon.jsii.JsiiException

or

Execution error (IllegalArgumentException) at com.fasterxml.jackson.databind.introspect.POJOPropertyBuilder/getSetter (POJOPropertyBuilder.java:497).
Conflicting setter definitions for property "blockDeviceMappings": software.amazon.awscdk.services.ec2.CfnInstance#setBlockDeviceMappings(1 params) vs software.amazon.awscdk.services.ec2.CfnInstance#setBlockDeviceMappings(1 params)

Environment

  • CDK CLI Version: 1.8.0 (build 5244f97)
  • Module Version: 1.8.0.DEVPREVIEW
  • OS: MacOS Mojave
  • Language: Java

Other information

JsiiObjectMapper.java: 57 software.amazon.jsii.JsiiObjectMapper/treeToValue
JsiiObject.java: 111 software.amazon.jsii.JsiiObject/jsiiGet
Instance.java: 66 software.amazon.awscdk.services.ec2.Instance/getInstance
RestFn.java: 1523 clojure.lang.RestFn/invoke
AFn.java: 22 clojure.lang.AFn/run
AFn.java: 22 clojure.lang.AFn/run
Thread.java: 748 java.lang.Thread/run
@johnjelinek johnjelinek added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 15, 2019
@johnjelinek
Copy link
Author

johnjelinek commented Sep 15, 2019

If I pass the instance in a list directly to .targets, I get this stack trace:

Resolution error: Resolution error: Trying to resolve() a Construct at /Resources/${Token[maintenance-window-target.LogicalID.82]}/Properties/targets/0/node.

@johnjelinek
Copy link
Author

hmm ... I'm starting to think maybe Instance isn't all that baked and that I need to be building a CfnInstance at this point.

@jeshan
Copy link

jeshan commented Sep 15, 2019

you need to show us what your instance-props, maintenance-window-props and maintenance-window-target-props look like

@johnjelinek
Copy link
Author

johnjelinek commented Sep 15, 2019

@jeshan: here's the whole kit n' caboodle:

(ns cdk.app
  (:import (software.amazon.awscdk.core App
                                        CfnOutput
                                        CfnOutputProps
                                        Environment
                                        Stack
                                        StackProps)
           (software.amazon.awscdk.services.ec2 GenericWindowsImage
                                                Instance
                                                InstanceClass
                                                InstanceProps
                                                InstanceSize
                                                InstanceType
                                                Port
                                                SubnetConfiguration
                                                SubnetSelection
                                                SubnetType
                                                Vpc
                                                VpcProps
                                                WindowsImage
                                                WindowsVersion)
           (software.amazon.awscdk.services.iam ManagedPolicy
                                                ServicePrincipal
                                                Role
                                                RoleProps)
           (software.amazon.awscdk.services.ssm CfnMaintenanceWindow
                                                CfnMaintenanceWindowProps
                                                CfnMaintenanceWindowTarget
                                                CfnMaintenanceWindowTargetProps)))

(def keypair-name "my-keypair")
(def default-cidr "172.16.0.0/16")
(def windows-instance-type
  (InstanceType/of InstanceClass/BURSTABLE3 InstanceSize/LARGE))
(def windows-rdp-port (Port/tcp 3389))

(def latest-windows-ami
  (new WindowsImage WindowsVersion/WINDOWS_SERVER_2019_ENGLISH_FULL_BASE))

(def pinned-windows-ami
  (new GenericWindowsImage {"us-east-2" "ami-017894519635aafd2"}))

(def public-subnet-selection
  (-> (SubnetSelection/builder)
      (.subnetType SubnetType/PUBLIC)
      .build))

(defn instance-props [vpc role]
  (-> (InstanceProps/builder)
      (.instanceType windows-instance-type)
      (.keyName keypair-name)
      (.machineImage pinned-windows-ami)
      (.role role)
      (.vpc vpc)
      (.vpcSubnets public-subnet-selection)
      .build))

(def instance-role-props
  (-> (RoleProps/builder)
      (.assumedBy (new ServicePrincipal "ec2.amazonaws.com"))
      (.managedPolicies [(ManagedPolicy/fromAwsManagedPolicyName
                          "service-role/AmazonEC2RoleforSSM")
                         (ManagedPolicy/fromAwsManagedPolicyName
                          "AmazonSSMFullAccess")])
      .build))

(def stack-props
  (-> (StackProps/builder)
      (.env (-> (Environment/builder)
                (.region "us-east-2")
                .build))
      .build))

(def subnet-configuration
  (-> (SubnetConfiguration/builder)
      (.name "Public")
      (.subnetType SubnetType/PUBLIC)
      .build))

(def vpc-props
  (-> (VpcProps/builder)
      (.cidr default-cidr)
      (.subnetConfiguration [subnet-configuration])
      .build))

(def maintenance-window-props
  (-> (CfnMaintenanceWindowProps/builder)
      (.allowUnassociatedTargets false)
      (.cutoff 2)
      (.schedule "cron(* 17 * * ? *)")
      (.description
       "Maintenance window to allow for patching windows instances")
      (.duration 6)
      (.name "example-windows-maintenance-window")
      .build))

(defn maintenance-window-target-props [instance maintenance-window]
  (-> (CfnMaintenanceWindowTargetProps/builder)
      (.ownerInformation "Example")
      (.description "Maintenance Window for Example instance")
      (.resourceType "INSTANCE")
      (.targets [{"key" "InstanceIds" "values" []}])
      (.name "example-patch-targets")
      (.windowId (.getRef maintenance-window))
      .build))

;; entry-point

(defn -main []
  (let [app (new App)
        stack (new Stack app "example" stack-props)
        vpc (new Vpc stack "example-vpc" vpc-props)
        instance-patching-role (new Role stack "patching-role"
                                    instance-role-props)
        maintenance-window (new CfnMaintenanceWindow stack "maintenance-window"
                                maintenance-window-props)
        instance (new Instance stack "server" (instance-props
                                               vpc
                                               instance-patching-role))
        maintenance-window-target (new CfnMaintenanceWindowTarget maintenance-window "maintenance-window-target"
                                       (maintenance-window-target-props instance maintenance-window))]
    (doto (.getConnections instance)
      (.allowFromAnyIpv4 windows-rdp-port "RDP"))
    (.synth app)))

@johnjelinek
Copy link
Author

note: I was trying to shove instance in another way with (.targets [{"key" "InstanceIds" "values" [instance]}]) -- but I think that would also mean I need to (.getInstance instance), which results in the error referenced above

@johnjelinek
Copy link
Author

it looks like I am able to finagle my way to success with this:

(.targets [{"key" "tag:Name" "values" [(.getPath (.getNode instance))]}])

@johnjelinek
Copy link
Author

ideally this would just work: (.targets [instance])

@SomayaB SomayaB added language/java Related to Java bindings @aws-cdk/aws-cloudformation Related to AWS CloudFormation labels Sep 16, 2019
@SomayaB SomayaB added the needs-reproduction This issue needs reproduction. label Sep 16, 2019
@johnjelinek
Copy link
Author

This also resolves: (.targets [{"key" "InstanceIds" "values" [(.getInstanceId instance)]}])

@eladb eladb removed language/java Related to Java bindings @aws-cdk/aws-cloudformation Related to AWS CloudFormation labels Sep 23, 2019
@eladb eladb assigned rix0rrr and unassigned RomainMuller and eladb Sep 23, 2019
@rix0rrr rix0rrr added the language/java Related to Java bindings label Sep 25, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 25, 2019

One note about .targets: anything class that starts with Cfn is at the L1 level, and those classes know nothing about interacting with "objects", they only deal with primitive types like strings (ARNs, typically), numbers etc.

So it is expected that you have to pass ARN into .targets for the object you're pointing to, instead of the object itself. Once we eventually get L2 support for the construct you're using there, you will be able to pass instance objects directly.

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 25, 2019

As for the other error, looks like JSII problem. Here is the repro in Java:

package com.myorg;

import software.amazon.awscdk.core.Construct;
import software.amazon.awscdk.core.Stack;
import software.amazon.awscdk.core.StackProps;
import software.amazon.awscdk.services.ec2.AmazonLinuxImage;
import software.amazon.awscdk.services.ec2.CfnInstance;
import software.amazon.awscdk.services.ec2.Instance;
import software.amazon.awscdk.services.ec2.InstanceProps;
import software.amazon.awscdk.services.ec2.InstanceType;
import software.amazon.awscdk.services.ec2.Vpc;

public class HelloStack extends Stack {
    public HelloStack(final Construct parent, final String id) {
        this(parent, id, null);
    }

    public HelloStack(final Construct parent, final String id, final StackProps props) {
        super(parent, id, props);

        Vpc vpc = new Vpc(this, "VPC");

        Instance inst = new Instance(this, "Inst", InstanceProps.builder()
            .instanceType(new InstanceType("t2.micro"))
            .machineImage(new AmazonLinuxImage())
            .vpc(vpc)
            .build());

        CfnInstance inner = inst.getInstance();
        System.out.println(inner);
    }
}
  testStack(com.myorg.HelloStackTest): com.fasterxml.jackson.databind.JsonMappingException: Conflicting setter definitions for property "blockDeviceMappings": software.amazon.awscdk.services.ec2.CfnInstance#setBlockDeviceMappings(1 params) vs software.amazon.awscdk.services.ec2.CfnInstance#setBlockDeviceMappings(1 params)(..)

@rix0rrr rix0rrr removed needs-triage This issue or PR still needs to be triaged. needs-reproduction This issue needs reproduction. labels Sep 25, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 25, 2019

The reason probably being that the Java code contains an overloaded setter that Jackson can't pick between:

    /**
     * `AWS::EC2::Instance.BlockDeviceMappings`.
     * 
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-instance.html#cfn-ec2-instance-blockdevicemappings
     */
    @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.External)
    public void setBlockDeviceMappings(final software.amazon.awscdk.core.IResolvable value) {
        this.jsiiSet("blockDeviceMappings", value);
    }

    /**
     * `AWS::EC2::Instance.BlockDeviceMappings`.
     * 
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-instance.html#cfn-ec2-instance-blockdevicemappings
     */
    @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.External)
    public void setBlockDeviceMappings(final java.util.List<java.lang.Object> value) {
        this.jsiiSet("blockDeviceMappings", value);
    }

@RomainMuller
Copy link
Contributor

Yeah @rix0rrr is probably right... Somehow the overloads make Jackson angry... It's odd however how this hasn't "always" been an issue, since those aren't entirely new... And the current Java code shouldn't even attempt to use Jackson on those (although I have to be validating this assumption).

@magJ
Copy link

magJ commented Oct 2, 2019

Hacky workaround:

JsiiObjectMapper.INSTANCE.setAnnotationIntrospector(object : AnnotationIntrospector() {
    override fun version(): Version {
        return Version.unknownVersion()
    }

    override fun resolveSetterConflict(config: MapperConfig<*>?, setter1: AnnotatedMethod, setter2: AnnotatedMethod): AnnotatedMethod? {
        return setter1
    }
})

@rix0rrr rix0rrr added the p1 label Oct 24, 2019
@johnjelinek
Copy link
Author

johnjelinek commented Nov 12, 2019

Any updates on this? I'd like to get the CfnInstance from an Instance and make the EBS volume encrypted. (no change as of 1.16.1)

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 13, 2019

Are you still seeing the Jackson errors?

@johnjelinek
Copy link
Author

Yes, no change as of 1.16.1

@RomainMuller
Copy link
Contributor

This might require a code generation update. If I don't get any other major issues (🤞🏻) I'll spend some time on this one specifically tomorrow to get to the bottom of it.

@johnjelinek
Copy link
Author

Here's the latest error from 1.16.3:

Error:
Execution error (IllegalArgumentException) at com.fasterxml.jackson.databind.introspect.POJOPropertyBuilder/getSetter (POJOPropertyBuilder.java:497).
Conflicting setter definitions for property "sourceDestCheck": software.amazon.awscdk.services.ec2.CfnInstance#setSourceDestCheck(1 params) vs software.amazon.awscdk.services.ec2.CfnInstance#setSourceDestCheck(1 params)

RomainMuller added a commit to aws/jsii that referenced this issue Nov 14, 2019
Upon deserializing (`ObjectMapper#treeToValue`), Jackson will first
validate the `valueType` for whether ti can be deserialized with the
"standard" bean deserializer. This will bail out if there are multiple
ambiguous setters for a given property (they are ambiguous if they have
parameters of non-trivial types, such as `List`, some class, ...).

In order to remove the problem, this changes the deserialization path so
that instead of asking for deserialization into the needed instance type
directly, `JsiiObject` will be requested instead when the declared type
is a sub-class of that. Since such types are passed by-reference, the
custom de-serializer modifier will correctly determine the "right" class
to use (the declared type, or a subtype of it), and return this... But
Jackson will not get the chance to be confused or unhappy about that
type's structure.

This was the root cause of aws/aws-cdk#4080
mergify bot pushed a commit to aws/jsii that referenced this issue Nov 14, 2019
* fix(java): remove Jackson confusion with certain patterns

Upon deserializing (`ObjectMapper#treeToValue`), Jackson will first
validate the `valueType` for whether ti can be deserialized with the
"standard" bean deserializer. This will bail out if there are multiple
ambiguous setters for a given property (they are ambiguous if they have
parameters of non-trivial types, such as `List`, some class, ...).

In order to remove the problem, this changes the deserialization path so
that instead of asking for deserialization into the needed instance type
directly, `JsiiObject` will be requested instead when the declared type
is a sub-class of that. Since such types are passed by-reference, the
custom de-serializer modifier will correctly determine the "right" class
to use (the declared type, or a subtype of it), and return this... But
Jackson will not get the chance to be confused or unhappy about that
type's structure.

This was the root cause of aws/aws-cdk#4080

* add test specifically on structs
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 25, 2019

I believe this has been fixed in the latest release.

@rix0rrr rix0rrr closed this as completed Nov 25, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 25, 2019

Oh no it has been reverted.

@rix0rrr rix0rrr reopened this Nov 25, 2019
rix0rrr pushed a commit to aws/jsii that referenced this issue Nov 26, 2019
(Reintroduce #987).

Upon deserializing (`ObjectMapper#treeToValue`), Jackson will first
validate the `valueType` for whether ti can be deserialized with the
"standard" bean deserializer. This will bail out if there are multiple
ambiguous setters for a given property (they are ambiguous if they have
parameters of non-trivial types, such as `List`, some class, ...).

In order to remove the problem, this changes the deserialization path so
that instead of asking for deserialization into the needed instance type
directly, `JsiiObject` will be requested instead when the declared type
is a sub-class of that. Since such types are passed by-reference, the
custom de-serializer modifier will correctly determine the "right" class
to use (the declared type, or a subtype of it), and return this... But
Jackson will not get the chance to be confused or unhappy about that
type's structure.

This was the root cause of aws/aws-cdk#4080
mergify bot pushed a commit to aws/jsii that referenced this issue Nov 26, 2019
(Reintroduce #987).

Upon deserializing (`ObjectMapper#treeToValue`), Jackson will first
validate the `valueType` for whether ti can be deserialized with the
"standard" bean deserializer. This will bail out if there are multiple
ambiguous setters for a given property (they are ambiguous if they have
parameters of non-trivial types, such as `List`, some class, ...).

In order to remove the problem, this changes the deserialization path so
that instead of asking for deserialization into the needed instance type
directly, `JsiiObject` will be requested instead when the declared type
is a sub-class of that. Since such types are passed by-reference, the
custom de-serializer modifier will correctly determine the "right" class
to use (the declared type, or a subtype of it), and return this... But
Jackson will not get the chance to be confused or unhappy about that
type's structure.

This was the root cause of aws/aws-cdk#4080
@RomainMuller
Copy link
Contributor

The fix for this will roll out in the next CDK release. I'll resolve this issue for now, please re-open if you still encounter the problem once the new release (probably 1.19.0) is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. language/java Related to Java bindings p1
Projects
None yet
Development

No branches or pull requests

7 participants