aws-quickstart / quickstart-cisco-duo-network-gateway

AWS Quick Start Team
Apache License 2.0
1 stars 4 forks source link

Fixes for "DNG AWS Quickstart template improvements" issue #23

Closed opavlyshyn closed 11 months ago

opavlyshyn commented 1 year ago

Fixed points described in opened issue - https://github.com/aws-quickstart/quickstart-cisco-duo-network-gateway/issues/21

Points:

Also, fixed linters, as there were errors not related to this changes.

mufaddalq commented 1 year ago

reviewed and approving changes from my end. @sshvans please merge

opavlyshyn commented 1 year ago

reviewed and approving changes from my end. @sshvans please merge

@sshvans could u please take a look at it?

opavlyshyn commented 1 year ago

reviewed and approving changes from my end. @sshvans please merge

@mufaddalq could u please support us to have this merged? as @sshvans is not reachable

opavlyshyn commented 1 year ago

Looks good to me!

EDIT -- it looks like DescribeAddresses would be for Elastic IPs, not Network Interfaces, so the ARN should be different...IF you wanted to specify that.

@BradBloomingdale could u please support us to have this merged? cuz @sshvans is not reachable, we have customer asking for this

BradBloomingdale commented 1 year ago

Looks good to me! EDIT -- it looks like DescribeAddresses would be for Elastic IPs, not Network Interfaces, so the ARN should be different...IF you wanted to specify that.

@BradBloomingdale could u please support us to have this merged? cuz @sshvans is not reachable, we have customer asking for this

I'm actually a customer myself, and don't have code-owner abilities

opavlyshyn commented 1 year ago

Looks good to me! EDIT -- it looks like DescribeAddresses would be for Elastic IPs, not Network Interfaces, so the ARN should be different...IF you wanted to specify that.

@BradBloomingdale could u please support us to have this merged? cuz @sshvans is not reachable, we have customer asking for this

I'm actually a customer myself, and don't have code-owner abilities oh gotcha

BradBloomingdale commented 1 year ago

Looks good to me! EDIT -- it looks like DescribeAddresses would be for Elastic IPs, not Network Interfaces, so the ARN should be different...IF you wanted to specify that.

@BradBloomingdale could u please support us to have this merged? cuz @sshvans is not reachable, we have customer asking for this

I'm actually a customer myself, and don't have code-owner abilities oh gotcha

Something else -- I'm not able to find any examples of ARNs for Elastic IP addresses, either via the CLI or the console. We know the alternative that was in place won't work because it needs to be fully qualified and Cloudformation will throw an error.

And it appears that Cloudformation doesn't provide an ARN for them either:

Return values
Ref
When you pass the logical ID of this resource to the intrinsic Reffunction, Refreturns the Elastic IP address.

For more information about using the Reffunction, see [Ref](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-ref.html).

Fn::GetAtt
The Fn::GetAttintrinsic function returns a value for a specified attribute of this type. The following are the available attributes and sample return values.

For more information about using the Fn::GetAttintrinsic function, see [Fn::GetAtt](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-getatt.html).

AllocationId
The ID that AWS assigns to represent the allocation of the address for use with Amazon VPC. This is returned only for VPC elastic IP addresses. For example, eipalloc-5723d13e.

PublicIp
The Elastic IP address.

From: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-eip.html

opavlyshyn commented 1 year ago

Looks good to me! EDIT -- it looks like DescribeAddresses would be for Elastic IPs, not Network Interfaces, so the ARN should be different...IF you wanted to specify that.

@BradBloomingdale could u please support us to have this merged? cuz @sshvans is not reachable, we have customer asking for this

I'm actually a customer myself, and don't have code-owner abilities oh gotcha

Something else -- I'm not able to find any examples of ARNs for Elastic IP addresses, either via the CLI or the console.

And it appears that Cloudformation doesn't provide an ARN for them either:

Return values
Ref
When you pass the logical ID of this resource to the intrinsic Reffunction, Refreturns the Elastic IP address.

For more information about using the Reffunction, see [Ref](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-ref.html).

Fn::GetAtt
The Fn::GetAttintrinsic function returns a value for a specified attribute of this type. The following are the available attributes and sample return values.

For more information about using the Fn::GetAttintrinsic function, see [Fn::GetAtt](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-getatt.html).

AllocationId
The ID that AWS assigns to represent the allocation of the address for use with Amazon VPC. This is returned only for VPC elastic IP addresses. For example, eipalloc-5723d13e.

PublicIp
The Elastic IP address.

From: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-eip.html

yep, that's right..I`d rather leave it as it was for now, then will spend some time investigating it. as customer would like to know when it could be merged and ready.

opavlyshyn commented 11 months ago

@mufaddalq could u please advise what granular permission we can grant for the AdminServerInstancePolicy instead of * in both place for Resource: '*'?

image
opavlyshyn commented 11 months ago

Hey @BradBloomingdale! Could u please deploy and check the latest changes again?

BradBloomingdale commented 11 months ago

Hey @BradBloomingdale! Could u please deploy and check the latest changes again?

I'm assuming that the EC2 instance ARN could work if the API calls are targeting addresses, but I am unable to test because we only have this solution running in production.

mufaddalq commented 11 months ago

@sshvans : Reviewed and approved from my side. Can we please merge this PR?

mufaddalq commented 11 months ago

@sshvans : Reviewed and approved from my side. Can we please merge this PR?

@sshvans : Gentle reminder for this. Please merge.

sshvans commented 11 months ago

/do-e2e-tests

aws-ia-ci[bot] commented 11 months ago

While attempting to create a backend CI pipeline for this project, I encountered an error loading the .metadata file from the main branch and this PR. Here is a valid example for your reference.