-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: Amazon Elastic Container Registry plugin #304
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Blake R <[email protected]>
Thanks @blakeromano its awesome you can contribute this! With RIV it will take me a little while to spin it up locally to properly check it out. Initially I see some changes we'll want to make for consistency with the other plugins:
|
Signed-off-by: Blake R <[email protected]>
@niallthomson Thanks for the initial feedback! I updated it so it is now using the catalog entity and does the lookup that way now RBAC should be respected. I also updated to use an ARN rather than a name. From my understanding ECR API essentially is based off the registryId (which is the accountID), the region and then the repository name so I'll look more into the resource location pattern from the common library but at least it is using the ARN for now so at least there is alignment on using ARN versus having a one off on the name usage for now. |
ECR_ARN_ANNOTATION, | ||
]); | ||
|
||
const images = await this.ecrClient.send( |
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 could end up not returning all the images. Far fetched, but not impossible.
Three things I would suggest:
1 - Make maxResults
configurable and make it the maximum number of images that will be fetched
2 - Use a maxBatchSize
for each request - possibly configurable? -
3- Iterate getting maxBatchSize
entries until you get no nextToken
or you reach maxResults
What do you think?
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 I agree with you my main thoughts are:
- How easy do you make it to blow up the UI or by the backend by reading a ton of data into either the server memory?
- It kind of feels like pagination in the frontend may make the most sense but I think would require a decent amount of rethinking of frontend logic.
I think I'll implement 1 for now as I think to your point that makes the most sense to me and is the easiest to implement.
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.
In the CodePipeline plugin we just hard-coded the limit to 100 executions for now.
I'm happy with this also being hard-coded to the same number and we open a different issue to figure out a consistent way to handle configuring these.
router.use((req, res, next) => { | ||
const cacheKey = req.originalUrl; | ||
|
||
console.log(`Cache key ${cacheKey}`); |
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.
Use the logger
or remove - I guess it's a leftover from debugging?
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 also is https://github.com/awslabs/backstage-plugins-for-aws/blob/main/plugins/cost-insights/backend/src/service/router.ts#L58 but I do agree I moved it to a debug log with logger
It looks good overall, @blakeromano let me know what you think of the comments. |
Signed-off-by: Blake R <[email protected]>
Signed-off-by: Blake R <[email protected]>
@fabbazon Thanks for comments I think I addressed everything you mentioned. |
annotations: | ||
aws.amazon.com/aws-codepipeline-tags: component=example-website | ||
aws.amazon.com/aws-codebuild-project-tags: component=example-website | ||
aws.amazon.com/amazon-ecs-service-tags: component=example-website | ||
aws.amazon.com/cost-insights-tags: component=example-website | ||
aws.amazon.com/aws-ecr-repository-arn: 1234567890.dkr.ecr.us-east-1.amazonaws.com/example-website |
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.
Hmmmm, I want this to work with this repo and the example CDK without any modification as all the other examples do.
Think we have two options:
- Add back in an annotation
aws-ecr-repository-name
, which doesn't have an equivalent in the other plugins - Support tags. The backend code for this is relatively simple because we've abstracted the hard work. The frontend gets annoying because you have to be able to handle potentially getting multiple repositories back.
Issue # (if applicable)
Closes #1
Reason for this change
We want to visualize data surrounding ECR images including specifically Image Scan Results
Description of changes
This adds a new ECR plugin for the frontend and backend which results in a new tab in the entity page showing up (if configured with the correct annotation).
Description of how you validated changes
I have run this locally with an AWS account with and without AWS Inspector enabled which both seem to render as expected.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license