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

CAMEL-12647 : Problem in setting region for camel AWS-SQS endpoint #2424

Closed
wants to merge 1 commit into from
Closed

CAMEL-12647 : Problem in setting region for camel AWS-SQS endpoint #2424

wants to merge 1 commit into from

Conversation

saravanakumar1987
Copy link
Contributor

PR created for Bug : https://issues.apache.org/jira/browse/CAMEL-12647

Thanks,
Saravanakumar


assertEquals("region", endpoint.getConfiguration().getRegion());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not this assert be null before this change?

Copy link
Contributor Author

@saravanakumar1987 saravanakumar1987 Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Region value was taken from ARN which is wrong. Take a look at the source file changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? sqs://arn:aws:sqs:US_EAST_2:account:MyQueue?amazonSQSClient=#amazonSQSClient&accessKey=xxx&secretKey=yyy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARN is actually created like “arn:aws:sqs:us-east-2:account:MyQueue”. You can try creating a SQS Queue in AWS and check the syntax of ARN. It’s not the right place to get Region. Later the value is directly used in com.amazonaws API and gives problem in enum. Check the error message I posted in CAMEL-12647. Also aws-sns does not have this problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, region in ARN on sqs component possibly becomes irrelevant. Doesn't it? Maybe number of parts in ARN would require extre length checks and conditions control by this line. What do you think @oscerd ?

Copy link
Contributor Author

@saravanakumar1987 saravanakumar1987 Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. camel aws-sqs component uses com.amazonaws.AmazonWebServiceClient and it does not require ARN to get instance of it. The required parameters are parsed by camel from the given ARN.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example of arn for an sqs endpoint is arn:aws:sqs:us-east-1:123456789012:MyQueue. The problem is that we are using the enum value of the Regions class, so probably we may need to double check if the region is specified as arn, then we need to manipulate it to use it and make it compliant to be used for the enum. In my opinion the PR must be reviewed in this way.


assertEquals("region", endpoint.getConfiguration().getRegion());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example of arn for an sqs endpoint is arn:aws:sqs:us-east-1:123456789012:MyQueue. The problem is that we are using the enum value of the Regions class, so probably we may need to double check if the region is specified as arn, then we need to manipulate it to use it and make it compliant to be used for the enum. In my opinion the PR must be reviewed in this way.

@saravanakumar1987
Copy link
Contributor Author

@oscerd Do you say the region should be picked from ARN and deprecate the URI option "region" for the endpoint ? It can not be used for creating client com.amazonaws.AmazonWebServiceClient though. Because the enum expects the region to be in correct format.

@oscerd
Copy link
Contributor

oscerd commented Jul 13, 2018

No. Arn is a thing, while endpoint region option is different stuff. If you use an arn, you don't need to specify a region as endpoint option, because the configuration will be populated starting from the arn. In case the region is obtained from the arn we need to make his value good for the enum. We don't have to deprecate anything.

@saravanakumar1987
Copy link
Contributor Author

saravanakumar1987 commented Jul 13, 2018

Hi @oscerd ,

In that case, it can not be fixed in our camel code. Because the Regions is the enum from AWS SDK : https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/regions/Regions.html

I have actually made changes in this commit similar to how AWS-SNS component works.

Could you clarify what code changes can be performed here in this PR?

Thanks,
Saravanakumar

@oscerd
Copy link
Contributor

oscerd commented Jul 13, 2018

Suppose you have the parts[3] of the arn, that is "us-east-1"

On line 63 of the SQSComponent class you need to do:

configuration.setRegion(Regions.fromName(parts[3]).toString())

@oscerd
Copy link
Contributor

oscerd commented Jul 13, 2018

In this way you'll be able to use the region in the following code

@saravanakumar1987
Copy link
Contributor Author

saravanakumar1987 commented Jul 13, 2018

@oscerd @onderson I updated the PR with new changes. Please review.

@oscerd
Copy link
Contributor

oscerd commented Jul 13, 2018

Thanks. Merged on master, 2.22.x and 2.21.x

@oscerd oscerd closed this Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants