Ecr fetcher to collect repositories by region by ofiriro3 · Pull Request #293 · elastic/cloudbeat

ofiriro3 · 2022-07-22T06:30:14Z

Fixes: ECR Retrieve AWS resource by region #282

mergify · 2022-07-22T06:30:48Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b ecr-retrieve-image-by-region upstream/ecr-retrieve-image-by-region
git merge upstream/main
git push upstream ecr-retrieve-image-by-region

mergify · 2022-07-22T06:30:49Z

This pull request does not have a backport label. Could you fix it @ofiriro3? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
NOTE: backport-skip has been added to this pull request.

# Conflicts: # resources/fetchers/ecr_fetcher.go # resources/fetchers/ecr_fetcher_test.go

oren-zohar

Nice 🦁

oren-zohar · 2022-07-24T08:18:48Z

+				"^my-account\\.dkr\\.ecr\\.([-\\w]+)\\.amazonaws\\.com\\/([-\\w\\.\\/]+)[:,@]?",
 				"public\\.ecr\\.aws\\/\\w+\\/([-\\w\\.\\/]+)\\:?",


can you please add some documentation about this regex, what are we expecting it to parse, and an example string or something?

oren-zohar · 2022-07-24T08:19:56Z

+	PrivateRepoRegexTemplate = "^%s\\.dkr\\.ecr\\.([-\\w]+)\\.amazonaws\\.com\\/([-\\w\\.\\/]+)[:,@]?"
 	PublicRepoRegex          = "public\\.ecr\\.aws\\/\\w+\\/([-\\w\\.\\/]+)\\:?"


can you please add some documentation about this regex, what are we expecting it to parse, and an example string or something?

oren-zohar · 2022-07-24T08:21:19Z

+		describedRepo, err := describer.Provider.DescribeRepositories(ctx, f.awsConfig, repositories, region)
+		if err != nil {
+			f.log.Errorf("could not retrieve pod's aws repositories for region %s: %w", region, err)
+		}
+		awsRepositories = append(awsRepositories, describedRepo.Repositories...)


if there's an error it will still add the decribedRepo into awsRepositories. Is this by design?

oren-zohar · 2022-07-24T08:32:33Z

+	}

-	f.log.Debugf("sending pods to ecrProviders: %v", repositories)
+	return ""


error instead? we will get no indication if something went wrong here

I looked at other text manipulation examples (for example regex.FindString) and I saw that when there is no match, the method should return an empty string "".

But this case is a bit different, so I am not sure.

Whatever you think - I am ok with both options

Co-authored-by: Oren Zohar <85433724+oren-zohar@users.noreply.github.com>

oren-zohar

the coverage report shows a decrease in coverage for the ecr files, can you give it a look?

oren-zohar · 2022-07-25T07:55:01Z

+	// public.ecr.aws/<aws-alias>/<repository>
 	PublicRepoRegex          = "public\\.ecr\\.aws\\/\\w+\\/([-\\w\\.\\/]+)\\:?"


if this is the expected format, using regex to extract the repo seems like overkill, right?

I am using the regex for two things:

Validating the format of the image so I can be sure it's an ECR image.

Extract the repository name out of the format.

Do you have another suggestion?

ofiriro3 · 2022-07-25T13:00:10Z

the coverage report shows a decrease in coverage for the ecr files, can you give it a look?

I looked at the coverage report, it seems to me that the coverage in the provider was decreased.

The provider code is a wrapper for AWS original provider, and it will be a bit complex (if possible at all) to test it.
Since I added code in providers, the coverage was reduced :/

github-actions · 2022-07-25T16:02:03Z

Cloudbeat CI 🤖

Allure Report: http://csp-allure-reports.s3.amazonaws.com/allure_reports/cloudbeat/prs/293/index.html

adding region to both ecr_provider and ecr_fetcher + tests

40ed8cb

mergify Bot assigned ofiriro3 Jul 22, 2022

mergify Bot added the backport-skip label Jul 22, 2022

ofiriro3 added 2 commits July 22, 2022 09:38

Merge branch 'main' into ecr-retrieve-image-by-region

14a14ca

# Conflicts: # resources/fetchers/ecr_fetcher.go # resources/fetchers/ecr_fetcher_test.go

Fixing test

3f2bc71

ofiriro3 changed the title ~~adding region to both ecr_provider and ecr_fetcher + tests~~ Ecr fetcher to collect repositories by region Jul 22, 2022

ofiriro3 added 2 commits July 22, 2022 10:01

Self review

dca0d80

Add license to ecr_repository_describer_mock.go

b502d78

ofiriro3 marked this pull request as ready for review July 22, 2022 07:46

ofiriro3 requested a review from a team as a code owner July 22, 2022 07:46

ofiriro3 requested a review from oren-zohar July 22, 2022 07:46

oren-zohar requested changes Jul 24, 2022

View reviewed changes

ofiriro3 and others added 4 commits July 24, 2022 19:12

Adding bundle.tag.gz to gitignore

5237e89

Update resources/fetchers/ecr_fetcher_test.go

92c287d

Co-authored-by: Oren Zohar <85433724+oren-zohar@users.noreply.github.com>

Code review changes

5d2341c

Adding comments to tests

455e6fb

ofiriro3 requested a review from oren-zohar July 25, 2022 07:18

oren-zohar requested changes Jul 25, 2022

View reviewed changes

Stabilizing the flaky test

a7cc6ee

ofiriro3 requested a review from oren-zohar July 25, 2022 13:00

adding error to Extract region interface

388d462

oren-zohar approved these changes Jul 25, 2022

View reviewed changes

Merge branch 'main' into ecr-retrieve-image-by-region

e28fd18

ofiriro3 enabled auto-merge (squash) July 25, 2022 16:52

ofiriro3 merged commit 298e0d2 into elastic:main Jul 25, 2022

orouz added a commit to orouz/cloudbeat that referenced this pull request Sep 16, 2023

Tidy up test data generators (elastic#293)

ca84a28

orestisfl pushed a commit to orestisfl/cloudbeat that referenced this pull request Oct 11, 2023

[Security Policies] Tidy up test data generators (elastic#293)

5c175ec

		"^my-account\\.dkr\\.ecr\\.([-\\w]+)\\.amazonaws\\.com\\/([-\\w\\.\\/]+)[:,@]?",
		"public\\.ecr\\.aws\\/\\w+\\/([-\\w\\.\\/]+)\\:?",

		PrivateRepoRegexTemplate = "^%s\\.dkr\\.ecr\\.([-\\w]+)\\.amazonaws\\.com\\/([-\\w\\.\\/]+)[:,@]?"
		PublicRepoRegex = "public\\.ecr\\.aws\\/\\w+\\/([-\\w\\.\\/]+)\\:?"

		// public.ecr.aws/<aws-alias>/<repository>
		PublicRepoRegex = "public\\.ecr\\.aws\\/\\w+\\/([-\\w\\.\\/]+)\\:?"

Uh oh!

Conversation

ofiriro3 commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify Bot commented Jul 22, 2022

Uh oh!

mergify Bot commented Jul 22, 2022

Uh oh!

oren-zohar left a comment

Choose a reason for hiding this comment

Uh oh!

oren-zohar Jul 24, 2022

Choose a reason for hiding this comment

Uh oh!

oren-zohar Jul 24, 2022

Choose a reason for hiding this comment

Uh oh!

oren-zohar Jul 24, 2022

Choose a reason for hiding this comment

Uh oh!

oren-zohar Jul 24, 2022

Choose a reason for hiding this comment

Uh oh!

ofiriro3 Jul 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oren-zohar left a comment

Choose a reason for hiding this comment

Uh oh!

oren-zohar Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

ofiriro3 Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

ofiriro3 commented Jul 25, 2022

Uh oh!

github-actions Bot commented Jul 25, 2022

Cloudbeat CI 🤖

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ofiriro3 commented Jul 22, 2022 •

edited

Loading