fix(vulnerability): returning remediated vulnerabilities by kanstantsinbuklis-sap · Pull Request #1180 · cloudoperators/heureka

kanstantsinbuklis-sap · 2026-04-21T12:23:15Z

Description

In this PR I've returned cache to the remediation handler and added logic to invalidate cached entries during remediation creation. Also I've added context propagation from the request to database layer

What type of PR is this? (check all applicable)

Related Tickets & Documents

Related Issue fix(vulnerability): returning remediated vulnerabilities #1121

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help
Separate ticket for tests # (issue/pr)

Added to documentation?

📜 README.md
🤝 Documentation pages updated
🙅 no documentation needed
(if applicable) generated OpenAPI docs for CRD changes

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 107 out of 107 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-22T08:55:10Z

+			decodedKey, err := cache.DecodeKey(key, cache.DefaultKeyHash)
+			if err != nil {
+				wrappedErr := appErrors.InternalError(string(op), "Remediation", "", err)
+				applog.LogError(rh.logger, wrappedErr, logrus.Fields{
+					"remediation": remediation,


Cache invalidation decodes keys using cache.DefaultKeyHash, but the cache’s configured KeyHash may differ (SHA256/SHA512/etc.), in which case decoding will fail and invalidation won’t run. Consider exposing the active key-hash (or a decode helper) on the Cache implementation, or avoid decoding entirely by storing non-hashed/tagged keys for invalidation.

Can you check this comment @kanstantsinbuklis-sap

Added method to get key hash depending on config

Copilot · 2026-04-22T08:55:11Z

+	stmt, filterParameters, err := s.buildServiceStatement(context.Background(), baseQuery, filter, false, order, l)
 	if err != nil {
 		l.Error("Error preparing statement: ", err)
 		return nil, err


getServiceAttr accepts a ctx, but it currently uses context.Background() when preparing the statement. This breaks request context propagation (timeouts/cancellation) and defeats the purpose of adding ctx. Use the passed ctx here.

Copilot · 2026-04-22T08:55:11Z

 	// Execute the query
-	rows, err := stmt.Queryx(filterParameters...)
+	rows, err := stmt.QueryxContext(context.Background(), filterParameters...)
 	if err != nil {
 		l.Error("Error executing query: ", err)
 		return nil, err


getServiceAttr uses stmt.QueryxContext(context.Background(), ...) instead of the provided ctx, so query cancellation/timeouts won’t propagate. Use ctx for the query call as well.

Copilot · 2026-04-22T08:55:11Z

 	rows, err := performListScan(
+		context.Background(),
 		stmt,
 		filterParameters,
 		l,


GetAllIssueCursors builds the statement with the passed ctx, but then calls performListScan(context.Background(), ...). This drops request context propagation for the query (timeouts/cancellation). Pass ctx through to performListScan.

Copilot · 2026-04-22T08:55:11Z

+			if strings.Contains(decodedKey, fmt.Sprintf("\"issue_id\":[%d]\"", newRemediation.IssueId)) &&
+				(strings.Contains(decodedKey, "GetIssuesWithAggregations") || strings.Contains(decodedKey, "GetIssues") ||
+					strings.Contains(decodedKey, "GetAllIssueCursors")) {


The invalidation match uses "issue_id" in the decoded cache key, but entity.IssueFilter serializes the issue id field as "id" (json tag id). This condition will never match, so the stale Issue cache entries won’t be invalidated after remediation creation. Update the match to use the correct serialized field name (and consider matching the function name + structured JSON instead of brittle substrings).

Can you check this comment @kanstantsinbuklis-sap

Added additional check

Copilot · 2026-04-22T08:55:12Z

+	if err := rows.Err(); err != nil {
+		msg := "Error while iterating over result rows"
+		l.WithFields(
+			logrus.Fields{
+				"error":      err.Error(),
+				"parameters": filterParameters,
+			}).Error(msg)
+
+		return nil, fmt.Errorf("%s", msg)
+	}


When rows.Err() is non-nil, performListScan logs the underlying error but returns fmt.Errorf("%s", msg), losing the original error and making troubleshooting harder. Return an error that wraps the underlying cause (e.g., include %w).

MR2011 · 2026-04-22T13:56:49Z

 	})

-	issues, err := app.ListIssues(f, opt)
+	// TODO: remove it


Deleted redundant comment

MR2011 · 2026-04-22T13:57:10Z

-RUN mockery
-# generate graphql code
-RUN cd internal/api/graphql && go run github.com/99designs/gqlgen generate
+# # generate mock code files


Uncomment again please

I'm not sure we need to generate mocks and GraphQL in the final image, It has to be earlier
BWT I've mentioned that we avoid some dependencies caching, ldflags and dockerignore that also can reduce the final image size

FROM --platform=${BUILDPLATFORM:-linux/amd64} golang:1.26 AS builder WORKDIR /app COPY go.mod go.sum ./ RUN go mod download COPY . . RUN CGO_ENABLED=0 go build \ -trimpath \ -ldflags="-s -w" \ -o /bin/heureka \ cmd/heureka/main.go FROM --platform=${TARGETPLATFORM:-linux/amd64} gcr.io/distroless/static-debian12:nonroot LABEL source_repository="https://github.com/cloudoperators/heureka" USER nonroot:nonroot COPY --from=builder /bin/heureka /heureka ENTRYPOINT ["/heureka"]

Maybe this Dockerfile will be more applicable?

The mocks are only required for tests, but the GraphQL generation is required for the app, so it needs to be generated before the build.

michalkrzyz · 2026-04-22T14:48:26Z

+	key, err := c.CacheKey(fnname, fn, filterArgs(args...)...)
 	if err != nil {
-		return zero, fmt.Errorf("cache (key): Could not create cache key")
+		return zero, fmt.Errorf("cache (key): could not create cache key")


I have to say I am not convinced about this particular solution. It checks for args to find context in there to delete it. In the other function (getCallParameters) it checks for context type I am not sure why it is necessary...that is solution with too many conditions and a bit complex logic behind it and I am not sure we want it here.
Consider two other options:

Define new template function: CallCachedWithContext, where context will be passed explicitly in named argument, this way we will be able to skip context in key easily, and add it to the call if necessary. The only requirement is that context will be first argument of called function, but this is what we have in all functions and as far as I know most (if not all) library functions using context have it in first argument.
If we do not use cache other way (without context) we can even remove callcached (but I would not do that).

Second option is to use lambda function to bind call with context and omit it in parameters
I think you can easily create generic wrapper to remove context parameter and inject it in original call
so in CallCached you have param:
....
LocalContext(ctx, ir.database.GetIssueRepositories)
....
which will remove context from the function and use local context if necessary.
This way we will avoid changes in current cache code.
and this option seems to be very good usage of first class citizen function in golang

@MR2011

michalkrzyz

Is context change in cache related to the fix of the problem with returning remediated vulnerabilities? If not maybe we should split it, because both changes seems that they need to be worked out.

michalkrzyz · 2026-04-28T06:50:49Z

 		return nil, wrappedErr
 	}

+	if rh.cache != nil {


Is this cache handling function implemented in handler? This is not acceptable, we cannot mix those layers.
If we really want such handler we need to think about the proper pattern to handle that. We can't just mix logical layers this way.

I think that proper solution in here is to change GetAll interface function which is exposing too much and is not acceptable solution, to interface function named Invalidate(.*) which will take some regexp/wildcard/list of strings/wildcard/regex or whatever to filter through keys (decoded inside cache layer)

BTW GetAll is not a good name
'Get All Cache' - what do you expect to be done under such statement?
I would prefer to use GetAllKeys or even GetAllValidKeys

michalkrzyz · 2026-04-28T06:54:17Z

 	return getReturnValues[T](out)
 }

+func callDisabledWithContext[T any](ctx context.Context, fn any, args ...any) (T, error) {


this is redundant function, please remove it and use common one

michalkrzyz · 2026-04-28T06:55:54Z

+		if err == nil {
+			c.IncHit()
+			c.LaunchRefresh(func() {
+				_, _ = callFn[T](c, ttl, key, v, in)


If you are not using any special handling in refresh procedure, why don't you just use bind-like lambda to hide the context?

michalkrzyz · 2026-04-28T06:57:02Z

+func callEnabledWithContext[T any](ctx context.Context, c Cache, ttl time.Duration, fnname string, fn any, args ...any) (T, error) {
+	var zero T
+
+	v, in, err := getCallParametersWithContext(fn, ctx, args...)


this is something I wanted to avoid, why don't you use fn, args... with the old function?

michalkrzyz · 2026-04-28T06:57:25Z

 	return v, in, nil
 }

+func getCallParametersWithContext(fn any, ctx context.Context, args ...any) (reflect.Value, []reflect.Value, error) {


This is redundant function

michalkrzyz · 2026-04-28T07:11:43Z

 	return valStr, true, nil
 }

+func (imc *InMemoryCache) GetAll() ([]string, error) {


Is this getting all keys? I don't think we want invalid (ttl elapsed) keys to be considered here

When getting keys from valkey and redis, you won't get invalid keys. It makes sense to rename the function to GetAllKeys, but GetAllValidKeys doesn't make sense

…emediated-vulnerabilities

michalkrzyz · 2026-04-30T14:15:13Z

 	})

+	// TODO: add context
+	ctx := context.TODO()


I think we can use Background in here, because that is what we want.

michalkrzyz · 2026-04-30T14:18:02Z


 	// Trigger autopatch whenever a scanner run has completed successfully
-	if _, err := srh.database.Autopatch(); err != nil {
+	if _, err := srh.database.Autopatch(context.TODO()); err != nil {


context.Background() instead of TODO

michalkrzyz · 2026-04-30T14:18:55Z


 		// Fetch IssueRepositories
-		issueRepositories, err := db.GetIssueRepositories(&entity.IssueRepositoryFilter{
+		issueRepositories, err := db.GetIssueRepositories(context.TODO(), &entity.IssueRepositoryFilter{


context.Background() - because we will never have foreground context here anyway

michalkrzyz · 2026-04-30T14:19:37Z


 	// Get Issue Variants
 	issueVariants, err := db.GetServiceIssueVariants(
+		context.TODO(),


context.TODO() -> context.Background()

michalkrzyz · 2026-04-30T14:19:52Z

 		Debug("Building map of IssueVariants for issues related to assigned Component Version")

-	issueVariantMap, err := shared.BuildIssueVariantMap(db, &entity.ServiceIssueVariantFilter{
+	issueVariantMap, err := shared.BuildIssueVariantMap(context.TODO(), db, &entity.ServiceIssueVariantFilter{


context.TODO() -> context.Background()

michalkrzyz · 2026-04-30T14:20:01Z

 			Debug("Fetching issue matches related to assigned Component Instance")

-		issue_matches, err := db.GetIssueMatches(&entity.IssueMatchFilter{
+		issue_matches, err := db.GetIssueMatches(context.TODO(), &entity.IssueMatchFilter{


context.TODO() -> context.Background()

michalkrzyz · 2026-04-30T14:20:19Z


 		// Fetch services
-		services, err := db.GetServices(&entity.ServiceFilter{}, []entity.Order{})
+		services, err := db.GetServices(context.TODO(), &entity.ServiceFilter{}, []entity.Order{})


context.TODO() -> context.Background()

michalkrzyz · 2026-04-30T14:30:14Z

 	return vc.client.Do(vc.ctx, vc.client.B().Del().Key(key).Build()).Error()
 }

+func (vc *ValkeyCache) InvalidateByMatch(keys []string, keyMatcher func(string) bool) error {


I think this is some unused code and should be removed. Either we will extend interface or we define this function in cache.go both options are ok but we have to decide to use one.

Anyway why don't we implement it as interface function instead of GetAllKeys?

Moved this as the method for valkey and in memory caches

michalkrzyz

Generally very good work, but there are still minor change needed regarding the fix (cache interface extension)

…emediated-vulnerabilities

michalkrzyz

It is a very good job! Thank you. @kanstantsinbuklis-sap

kanstantsinbuklis-sap force-pushed the kanstantsinbuklis-sap/issue-1121/returning-remediated-vulnerabilities branch 4 times, most recently from 817ec99 to e62b880 Compare April 21, 2026 14:39

kanstantsinbuklis-sap marked this pull request as ready for review April 21, 2026 14:52

kanstantsinbuklis-sap requested a review from drochow as a code owner April 21, 2026 14:52

Copilot AI review requested due to automatic review settings April 21, 2026 14:52

kanstantsinbuklis-sap requested review from MR2011, copyonwrite, dorneanu and michalkrzyz as code owners April 21, 2026 14:52

Copilot started reviewing on behalf of kanstantsinbuklis-sap April 21, 2026 14:53 View session

kanstantsinbuklis-sap linked an issue Apr 21, 2026 that may be closed by this pull request

fix(vulnerability): returning remediated vulnerabilities #1121

Closed

2 tasks

Copilot AI reviewed Apr 21, 2026

View reviewed changes

MR2011 requested a review from Copilot April 22, 2026 08:48

Copilot started reviewing on behalf of MR2011 April 22, 2026 08:48 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

kanstantsinbuklis-sap force-pushed the kanstantsinbuklis-sap/issue-1121/returning-remediated-vulnerabilities branch 2 times, most recently from 637fec5 to 445b392 Compare April 22, 2026 12:30

MR2011 reviewed Apr 22, 2026

View reviewed changes

michalkrzyz requested changes Apr 22, 2026

View reviewed changes

fix(vulnerability): returning remediated vulnerabilities

60cb523

kanstantsinbuklis-sap force-pushed the kanstantsinbuklis-sap/issue-1121/returning-remediated-vulnerabilities branch from 445b392 to 60cb523 Compare April 23, 2026 10:30

trigger actions

3063681

kanstantsinbuklis-sap force-pushed the kanstantsinbuklis-sap/issue-1121/returning-remediated-vulnerabilities branch 2 times, most recently from 54241d1 to 1cb9f6f Compare April 27, 2026 13:28

kanstantsinbuklis-sap requested review from MR2011 and michalkrzyz April 27, 2026 13:37

michalkrzyz requested changes Apr 28, 2026

View reviewed changes

Merge branch 'main' into kanstantsinbuklis-sap/issue-1121/returning-r…

be04783

…emediated-vulnerabilities

kanstantsinbuklis-sap force-pushed the kanstantsinbuklis-sap/issue-1121/returning-remediated-vulnerabilities branch from 1cb9f6f to be04783 Compare April 30, 2026 12:26

Automatic application of license header

edcf446

kanstantsinbuklis-sap requested a review from michalkrzyz April 30, 2026 12:36

michalkrzyz reviewed Apr 30, 2026

View reviewed changes

michalkrzyz requested changes Apr 30, 2026

View reviewed changes

Merge branch 'main' into kanstantsinbuklis-sap/issue-1121/returning-r…

6847be4

…emediated-vulnerabilities

kanstantsinbuklis-sap force-pushed the kanstantsinbuklis-sap/issue-1121/returning-remediated-vulnerabilities branch from 0985985 to 6847be4 Compare May 4, 2026 07:58

kanstantsinbuklis-sap requested a review from michalkrzyz May 4, 2026 08:00

michalkrzyz approved these changes May 4, 2026

View reviewed changes

MR2011 approved these changes May 4, 2026

View reviewed changes

MR2011 merged commit c8709b4 into main May 4, 2026
10 of 11 checks passed

MR2011 deleted the kanstantsinbuklis-sap/issue-1121/returning-remediated-vulnerabilities branch May 4, 2026 11:08

Conversation

kanstantsinbuklis-sap commented Apr 21, 2026

Description

What type of PR is this? (check all applicable)

Related Tickets & Documents

Added tests?

Added to documentation?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalkrzyz Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalkrzyz left a comment

Choose a reason for hiding this comment

Uh oh!

michalkrzyz Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

michalkrzyz Apr 22, 2026 •

edited

Loading

michalkrzyz Apr 28, 2026 •

edited

Loading

michalkrzyz left a comment •

edited

Loading