fix: parseImageRef tag parsing (#2411) #2412

danchenko-dmitry · 2025-01-31T12:42:20Z

Description

I've changed behaviour of parseImageRef func.
before:
In case when imageRef -> "my-private-repo.company.com/my-app:some-tag@sha256:1420cefd4b20014b3361951c22593de6e9a2476bbbadd1759464eab5bfc0d34f"
It returns empty tag in Artifact.Tag;

after:
In new version this behaviour fixed and it returns tag instead empty string.
you can see it it "15. private registry image ref tag & with digest" test.

Related issues

Close Empty tag in all reports when deployed with digester #2411

Checklist

I've read the guidelines for contributing to this repository.
I've added tests that prove my fix is effective or that my feature works.
[ Not need] I've updated the documentation with the relevant information (if needed).
[Not need] I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

CLAassistant · 2025-01-31T12:42:26Z

All committers have signed the CLA.

afdesk · 2025-02-03T06:33:58Z

pkg/plugins/trivy/plugin.go

+		hasTag = true
+	}
+
+	ref, err := containerimage.ParseReference(namePart, containerimage.WeakValidation)


this function calls twice.

afdesk · 2025-02-03T06:34:11Z

pkg/plugins/trivy/plugin.go

+	}
+
+	if hasDigest {
+		digestRef, err := containerimage.ParseReference(imageRef, containerimage.WeakValidation)


the second call.

afdesk · 2025-02-03T06:35:05Z

pkg/plugins/trivy/plugin.go

+	var hasDigest bool
+	var hasTag bool


do we really need these variables?
artifact.Digest == "" means hasDigest is false, right?

afdesk · 2025-02-03T06:36:33Z

pkg/plugins/trivy/plugin.go

+	if len(parts) > 1 {
+		hasDigest = true
+	}
+	if len(strings.Split(namePart,":"))>1{ 
+		hasTag = true
+	}


i think we should improve these if-statements

afdesk · 2025-02-03T06:39:06Z

pkg/plugins/trivy/plugin.go

+	case ok && hasTag:
+		artifact.Tag = tagged.TagStr()
+	case ok && !hasTag && !hasDigest:
+		artifact.Tag = tagged.TagStr()
+	case ok && hasTag && hasDigest:
+		artifact.Tag = tagged.TagStr()


ok should be true for all cases, right?
and artifact.Tag = tagged.TagStr() calls for all cases.
honestly it doesn't look like a good way to check tags/digest.

afdesk · 2025-02-03T06:41:05Z

@danchenko-dmitry thanks for your contribution and your time.

i left some comments. and think we should refactor this code.
and please, fix the linter suggestions.

afdesk · 2025-02-03T06:56:25Z

@danchenko-dmitry please correct me.
it seems google/go-containerregistry should parse such cases correctly: google/go-containerregistry#391 (google/go-containerregistry#351)

afdesk · 2025-02-03T07:14:06Z

also in any case, you can take a look at NewDigest function to re-use the code, it looks like a good sample

danchenko-dmitry · 2025-02-03T10:09:37Z

google/go-containerregistry#39

I've prepared demo repo to demonstrate how it works:
https://github.com/danchenko-dmitry/tag-sandbox
This fix doesn't work in our case.

afdesk · 2025-02-03T11:04:32Z

@danchenko-dmitry thanks!
it seems i understood a reason - containerimage.ParseReference returns iegher tag or digest, right?
can we do something simplier to get a tag and a digest? for ex:

update:

artifactTag = strings.TrimPrefix(strings.TrimSuffix(strings.TrimPrefix(imageRef, ref.Context().Name()), "@"+artifactDigest), ":")

https://go.dev/play/p/d3BZ2biZf_p

I'm not sure about this solution. Just the first idea.
maybe there is something more gracefully

wdyt?

danchenko-dmitry · 2025-02-03T15:41:35Z

wdyt?

Well, thank you for your support.
I've tried to refactor this code and make it more readable and more clear.
I've tried to consider all your comments and here is all I've got.
I hope you will be satisfied .. :-)

simar7 · 2025-02-03T23:39:18Z

@danchenko-dmitry thanks for your PR! It looks like the tests and the linter are failing. Could you look into fixing those?

danchenko-dmitry · 2025-02-04T09:44:18Z

@danchenko-dmitry thanks for your PR! It looks like the tests and the linter are failing. Could you look into fixing those?

Done. Thanks!

afdesk · 2025-02-04T09:57:34Z

pkg/plugins/trivy/plugin.go

@@ -155,7 +155,7 @@ func (p *plugin) ParseReportData(ctx trivyoperator.PluginContext, imageRef strin

 	imageDigest := p.getImageDigest(reports)

-	registry, artifact, err := p.parseImageRef(imageRef, imageDigest)
+	registry, artifact, err := ParseImageRef(imageRef, imageDigest)


why do we need this change?

My point is:

We don't need this function as a method. It doesn't use private or public variables.

Making it a standalone function simplifies testing (we don't need to initialize the object plugin ...)

afdesk · 2025-02-04T10:02:51Z

pkg/plugins/trivy/plugin_test.go

+			name:     "7. short repo with private repo with tag",
+			imageRef: "my-private-repo.company.com/my-app:1.2.3",
+			imageID:  "sha256:2bc57c6bcb194869d18676e003dfed47b87d257fce49667557fb8eb1f324d5d6",
+			registry: v1alpha1.Registry{
+				Server: "my-private-repo.company.com",
+			},
+			artifact: v1alpha1.Artifact{
+				Repository: "my-app",
+				Digest:     "sha256:2bc57c6bcb194869d18676e003dfed47b87d257fce49667557fb8eb1f324d5d6",
+				Tag:        "1.2.3",
+			},
+			err: nil,
+		},


what are the diifs between "2. with tag"?
is there a specific case for private repositories in ParseImageRef?

Agree. I thought that more samples to test in different situations make it more covered by tests.
But your point also has meaning.
Less code - less support. If some case will be needed in the future it is easy to add it.
I will remove duplicates.

afdesk · 2025-02-04T10:03:31Z

pkg/plugins/trivy/plugin.go

+	server := ""
+	repository := ""
+	digest := ""
+	tag := ""


do we really need these variables?

Well, these variables "collect" values from two sources: containerimage.NewTag and containerimage.NewDigest.

In cases "server" and "repository" we don't care what source will used when both are available. (because they are equal)

"digest" and "tag" fills if it available.

The whole structure of code was made to simplify flow and remove sophisticated conditions.

I think using these variables (as one place collects data) will be simpler than other any cases.

afdesk · 2025-02-04T10:06:33Z

pkg/plugins/trivy/plugin.go

+	_tag, _err := containerimage.NewTag(refParts[0], containerimage.WeakValidation)
+	if _err == nil {
+		pTag = &_tag
 	}
-	artifact := v1alpha1.Artifact{
-		Repository: ref.Context().RepositoryStr(),
+
+	_digest, _err := containerimage.NewDigest(imageRef, containerimage.WeakValidation)
+	if _err == nil {
+		pDigest = &_digest


it looks like rewriting containerimage.ParseReference(imageRef)

Yes, thats right!
It was an aim ... Using ParseReference will cause second call with other arguments.
I've decided to rewrite ParseReference and "customize" it logic & lay code on library then lay on own custom code.

artifact.Tag = strings.TrimPrefix(strings.TrimSuffix(strings.TrimPrefix(imageRef, ref.Context().Name()), "@"+artifactDigest), ":")

doesn't this code work correctly?

I mean if you keep your test and add this one-line code for digest, will the tests pass?

trivy-operator/pkg/plugins/trivy/plugin.go

Lines 230 to 231 in 164ac24

case containerimage.Digest:

artifact.Digest = t.DigestStr()

artifact.Tag = strings.TrimPrefix(strings.TrimSuffix(strings.TrimPrefix(imageRef, ref.Context().Name()), "@"+artifactDigest), ":")

doesn't this code work correctly?

This code works correctly. I've checked it on another PR #2418
So we have two opportunities.

afdesk · 2025-02-04T10:07:41Z

@danchenko-dmitry thanks for your work!

I left some comments.
the main opinions are next:

can we simplify changes into smaller, more manageable updates?
Some tests appear to be redundant. Could we review and remove any that don't add significant value?

Let me know what you think!

danchenko-dmitry · 2025-02-04T11:21:18Z

I left some comments. the main opinions are next:

can we simplify changes into smaller, more manageable updates?

Totally agree. The previous version of the function wasn't simple and clear. Rewriting was simpler than smooth evolution ...
I know - it is not the best practice for collaboration... But in these particular case, I've decided to perform it quicker.

Some tests appear to be redundant. Could we review and remove any that don't add significant value?

Yes, I will remove redundant tests. You are right that less code is easiest to maintain.

Let me know what you think!

Thanks for your support! I motivated to do code better.

afdesk · 2025-02-05T06:52:02Z

in favor #2418

github-actions bot added the bug label Jan 31, 2025

danchenko-dmitry mentioned this pull request Jan 31, 2025

Empty tag in all reports when deployed with digester #2411

Closed

fix: parseImageRef tag parsing (aquasecurity#2411)

b6be3e5

danchenko-dmitry force-pushed the logging-ver-0.23.0 branch from cc7eacc to b6be3e5 Compare January 31, 2025 14:31

afdesk requested changes Feb 3, 2025

View reviewed changes

Refactor

cf71a12

linter fixes

3492ab8

danchenko-dmitry requested a review from afdesk February 4, 2025 09:45

afdesk requested changes Feb 4, 2025

View reviewed changes

afdesk requested a review from simar7 February 4, 2025 10:09

danchenko-dmitry added 9 commits February 4, 2025 11:37

remove 5 testcase

09c84b9

sort test cases 1

6f94493

sort test cases 2

3cd5967

sort test cases 3

55f70a8

add test case docker registry image ref with tag

01df993

sort test cases 4

6b81f21

sort test cases 5

9a71c4d

remove redundand test cases

ece71e7

sort test cases 6

7eb27c4

danchenko-dmitry added 2 commits February 4, 2025 12:54

renumerate test cases

a0bf6d2

improve test cases

ae65b3a

danchenko-dmitry mentioned this pull request Feb 4, 2025

fix: parseImageRef tag absence issue when digest in imageRef #2418

Merged

3 tasks

danchenko-dmitry requested a review from afdesk February 4, 2025 15:54

afdesk closed this Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: parseImageRef tag parsing (#2411) #2412

fix: parseImageRef tag parsing (#2411) #2412

danchenko-dmitry commented Jan 31, 2025 •

edited

Loading

CLAassistant commented Jan 31, 2025 •

edited

Loading

afdesk Feb 3, 2025

afdesk Feb 3, 2025

afdesk Feb 3, 2025

afdesk Feb 3, 2025

afdesk Feb 3, 2025

afdesk commented Feb 3, 2025

afdesk commented Feb 3, 2025 •

edited

Loading

afdesk commented Feb 3, 2025

danchenko-dmitry commented Feb 3, 2025

afdesk commented Feb 3, 2025 •

edited

Loading

danchenko-dmitry commented Feb 3, 2025

simar7 commented Feb 3, 2025

danchenko-dmitry commented Feb 4, 2025

afdesk Feb 4, 2025

danchenko-dmitry Feb 4, 2025

afdesk Feb 4, 2025

danchenko-dmitry Feb 4, 2025

afdesk Feb 4, 2025

danchenko-dmitry Feb 4, 2025

afdesk Feb 4, 2025

danchenko-dmitry Feb 4, 2025

afdesk Feb 4, 2025 •

edited

Loading

afdesk Feb 4, 2025

danchenko-dmitry Feb 4, 2025

afdesk commented Feb 4, 2025 •

edited

Loading

danchenko-dmitry commented Feb 4, 2025 •

edited

Loading

afdesk commented Feb 5, 2025

fix: parseImageRef tag parsing (#2411) #2412

fix: parseImageRef tag parsing (#2411) #2412

Conversation

danchenko-dmitry commented Jan 31, 2025 • edited Loading

Description

Related issues

Checklist

CLAassistant commented Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afdesk commented Feb 3, 2025

afdesk commented Feb 3, 2025 • edited Loading

afdesk commented Feb 3, 2025

danchenko-dmitry commented Feb 3, 2025

afdesk commented Feb 3, 2025 • edited Loading

danchenko-dmitry commented Feb 3, 2025

simar7 commented Feb 3, 2025

danchenko-dmitry commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afdesk Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afdesk commented Feb 4, 2025 • edited Loading

danchenko-dmitry commented Feb 4, 2025 • edited Loading

afdesk commented Feb 5, 2025

danchenko-dmitry commented Jan 31, 2025 •

edited

Loading

CLAassistant commented Jan 31, 2025 •

edited

Loading

afdesk commented Feb 3, 2025 •

edited

Loading

afdesk commented Feb 3, 2025 •

edited

Loading

afdesk Feb 4, 2025 •

edited

Loading

afdesk commented Feb 4, 2025 •

edited

Loading

danchenko-dmitry commented Feb 4, 2025 •

edited

Loading