-
Notifications
You must be signed in to change notification settings - Fork 20
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
[Integration][AWS] Add new integration #555
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shallow comments
dc1024e
to
da49c5e
Compare
9ceb5c6
to
973f6ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to make the code more simple
integrations/aws/utils.py
Outdated
self.enabled_regions = [] | ||
|
||
async def updateEnabledRegions(self): | ||
session = aioboto3.Session(self.access_key_id, self.secret_access_key, self.session_token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it should know to detect those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session uses by default it's default region, but I would like to scan all of the regions available for each account
integrations/aws/utils.py
Outdated
def isRole(self): | ||
return self.session_token is not None | ||
|
||
async def createSession(self, region: str) -> aioboto3.Session: | ||
if self.isRole(): | ||
return aioboto3.Session(self.access_key_id, self.secret_access_key, self.session_token, region) | ||
else: | ||
return aioboto3.Session(aws_access_key_id=self.access_key_id, aws_secret_access_key=self.secret_access_key, region_name=region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why dont you work only with the role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left it as an option to run the integration outside of AWS since I initially developed it to use a user credentials, it supports both ways though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant you run login with the cli?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could, but running this integration on-prem
might not use a cli
integrations/aws/utils.py
Outdated
else: | ||
return aioboto3.Session(aws_access_key_id=self.access_key_id, aws_secret_access_key=self.secret_access_key, region_name=region) | ||
|
||
async def createSessionForEachRegion(self) -> AsyncIterator[aioboto3.Session]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is that async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aioboto3 Session returns a coroutine
39eded1
to
543b9cf
Compare
integrations/aws/aws_credentials.py
Outdated
async def create_session_for_each_region(self) -> AsyncIterator[aioboto3.Session]: | ||
for region in self.enabled_regions: | ||
yield self.create_session(region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please test a multiple regions account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, tested it, why?
integrations/aws/utils.py
Outdated
def isRole(self): | ||
return self.session_token is not None | ||
|
||
async def createSession(self, region: str) -> aioboto3.Session: | ||
if self.isRole(): | ||
return aioboto3.Session(self.access_key_id, self.secret_access_key, self.session_token, region) | ||
else: | ||
return aioboto3.Session(aws_access_key_id=self.access_key_id, aws_secret_access_key=self.secret_access_key, region_name=region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant you run login with the cli?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more comments
integrations/aws/utils.py
Outdated
def validate_request(request: Request) -> None: | ||
""" | ||
Validates the request by checking for the presence of the API key in the request headers. | ||
""" | ||
api_key = request.headers.get('x-port-aws-ocean-api-key') | ||
if not api_key: | ||
raise ValueError("API key not found in request headers") | ||
if not ocean.integration_config.get("aws_api_key"): | ||
raise ValueError("API key not found in integration config") | ||
if api_key != ocean.integration_config.get("aws_api_key"): | ||
raise ValueError("Invalid API key") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is better way to validate the request
https://docs.aws.amazon.com/IAM/latest/UserGuide/create-signed-request.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using api-destinations
service to send the requests to the ocean integration, So I have no control over the requst, The only auth types avaiable to me were
Oauth, user/password or an api-key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets talk about this
integrations/aws/main.py
Outdated
resource = await describe_single_resource(resource_type, identifier, account_id, body.get("awsRegion")) | ||
for resource_config in matching_resource_configs: | ||
blueprint = resource_config.port.entity.mappings.blueprint.strip('"') | ||
if not resource: # Resource probably deleted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please show me an example for delete event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an event with an AWS S3 Bucket name arrived but the the bucket does not exist on AWS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you show me how it looks like?
integrations/aws/main.py
Outdated
resource_type = body.get("resource_type") | ||
identifier = body.get("identifier") | ||
account_id = body.get("accountId") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason for any of them to not exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in AWS as a user you can create custom events, all of the events of our exporter need to be in the same format in order for our bot to handle them, so if an event body was not formatted correctly we cannot process it.
please take a look at how we currently format the events in our docs (any cloudformation example) https://docs.getport.io/build-your-software-catalog/sync-data-to-catalog/cloud-providers/aws/examples/
integrations/aws/session_manager.py
Outdated
if account['Id'] == self._application_account_id: | ||
# Skip the current account as it is already added | ||
# Replace the Temp account details with the current account details | ||
self._aws_accessible_accounts[0] = account | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we talk about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure let's talk
543b9cf
to
3c52b8d
Compare
14887c8
to
9f428ac
Compare
|
||
@ocean.on_resync() | ||
async def resync_all(kind: str) -> ASYNC_GENERATOR_RESYNC_TYPE: | ||
await update_available_access_credentials() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are u doing it only here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial approach was to periodically refresh the credentials, the resync all method that runs before any other resync method seems like a nice compromise, it seems redundant to me to update on evry type of sync, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now do it in every reync
nothing promise you it will stay that way
you can update the credentials once and then set it to the event parameters that you have already done this to prevent future calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if "CREDENTIALS_CACHE" in event.attributes:
return
fetch
event.attributes["CREDENTIALS_CACHE"] = X
integrations/aws/main.py
Outdated
except Exception as e: | ||
logger.error(f"Failed to process event from aws error: {e}", error=e) | ||
return {"ok": False} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really
think about when we will work over ocean SAAS it can be very helpful to monitor
integrations/aws/main.py
Outdated
blueprint=blueprint, | ||
) | ||
resource.update({KIND_PROPERTY: resource_type, ACCOUNT_ID_PROPERTY: account_id, REGION_PROPERTY: body.get("awsRegion")}) | ||
await ocean.register_raw(resource_config.kind, [_fix_unserializable_date_properties(resource)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very problematic as ocean dont let you pass the resource config itself and therefor you might trigger the same resource parsing multiple times
integrations/aws/main.py
Outdated
resource.update( | ||
{ | ||
KIND_PROPERTY: resource_type, | ||
ACCOUNT_ID_PROPERTY: account_id, | ||
REGION_PROPERTY: region, | ||
IDENTIFIER_PROPERTY: identifier, | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove anything that can already be found in the playload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resources that are not using cloudcontrol
do not always use Identifier
as their identifier key, for example
in EC2 case it is listed as InstanceId
integrations/aws/utils.py
Outdated
response = await getattr(client, describe_method)() | ||
next_token = response.get(marker_param) | ||
for resource in response.get(list_param, []): | ||
resource.update({KIND_PROPERTY: kind, ACCOUNT_ID_PROPERTY: account_id, REGION_PROPERTY: region, IDENTIFIER_PROPERTY: resource.get('Identifier')}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets talk about this i want to see an exmple for the data
integrations/aws/utils.py
Outdated
if kind == ResourceKindsWithSpecialHandling.ACM: | ||
async with session.client('acm') as acm: | ||
response = await acm.describe_certificate(CertificateArn=identifier) | ||
return response.get('Certificate', {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? you are still using .get
420330f
to
334b773
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments
integrations/aws/main.py
Outdated
if response.status_code <= 299: | ||
response.status_code = status.HTTP_500_INTERNAL_SERVER_ERROR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u check if there are status codes that might close the aws pipeline for the webhooks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what did you mean in
close pipeline for webhooks
but according to AWS documentation:
Events associated with error codes 409, 429, and 5xx are retried.
Events associated with error codes 1xx, 2xx, 3xx, and 4xx (excluding 429) aren't retried.
integrations/aws/main.py
Outdated
await ocean.register_raw( | ||
resource_config.kind, [fix_unserializable_date_properties(resource)] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is problematic that you might issue the same kind multiple times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so since they have the same identifier, BTW the Azure
integration does this the same way exactly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added unique to get_matching_kinds_from_config
so that no kinds will be duplicated
aa9bd75
to
77c3d9c
Compare
0872d18
to
db83a0d
Compare
027e5e0
to
f3b1e6f
Compare
Description
Ocean AWS Integration 🥇
basically the same exporter we have today only
What - AWS Integration
Why - Better, faster, more accounts, less installations
How -
Type of change
Please leave one option from the following and delete the rest:
Screenshots
API Documentation
Provide links to the API documentation used for this integration. WIP