Added log message for API call failures #7792

prady0t · 2023-07-09T20:17:50Z

Signed-off-by: Pradyot Ranjan rickprimeranjan@gmail.com

Explanation

There were no log messages for API call failures. dl.logger.Error(err, "Failed to load data: ") now prints the error along with log message.

Related issue

Closes #7734
Closes #4774

Milestone of this PR

/milestone 1.10.2

What type of PR is this

Proposed Changes

Proof Manifests

Checklist

I have read the contributing guidelines.
I have read the PR documentation guide and followed the process including adding proof manifests to this PR.
This is a bug fix and I have added unit tests that prove my fix is effective.
This is a feature and I have added CLI tests that are applicable.
My PR needs to be cherry picked to a specific release branch which is .
My PR contains new or altered behavior to Kyverno and
- CLI support should be added and my PR doesn't contain that functionality.
- I have added or changed the documentation myself in an existing PR and the link is:
- I have raised an issue in kyverno/website to track the documentation update and the link is:

Further Comments

welcome · 2023-07-09T20:17:52Z

Thanks for opening your first Pull Request here! Please check out our Contributing guidelines and confirm that you Signed off.

chipzoller · 2023-07-10T02:12:07Z

@prady0t, please show in the Proof section what this log message looks like when generated and how we can test your code to see it for ourselves. Also, please use correct closing keywords; "solves" will not automatically link to an issue. Please follow and complete the PR template as requested.

prady0t · 2023-07-11T06:53:40Z

@chipzoller Sorry for the incomplete PR. I'll make required changes. I'm having some query regarding cli output which I asked earlier on slack. Can you please help me with that?
Discussion link -> https://kubernetes.slack.com/archives/C032MM2CH7X/p1689022298176299?thread_ts=1688386337.541699&cid=C032MM2CH7X

realshuting · 2023-07-11T07:53:39Z

@prady0t - the changes break the builds, can you fix it?

prady0t · 2023-07-11T08:05:24Z

@realshuting It fails because of this: not enough arguments in call to enginecontext.NewDeferredLoader Let me pass
logr.Logger{} as the argument to enginecontext.NewDeferredLoader in that way built will not fail.

codecov · 2023-07-11T14:06:35Z

Codecov Report

Merging #7792 (c7f05c5) into main (5b9c13a) will decrease coverage by 0.11%.
The diff coverage is 16.01%.

❗ Current head c7f05c5 differs from pull request most recent head 6fd7739. Consider uploading reports for the commit 6fd7739 to get more accurate results

@@            Coverage Diff             @@
##             main    #7792      +/-   ##
==========================================
- Coverage   32.97%   32.87%   -0.11%     
==========================================
  Files         234      239       +5     
  Lines       22426    22515      +89     
==========================================
+ Hits         7396     7402       +6     
- Misses      14272    14354      +82     
- Partials      758      759       +1

Impacted Files	Coverage Δ
api/kyverno/v1/clusterpolicy_types.go	`15.90% <0.00%> (-0.76%)`	⬇️
api/kyverno/v1/policy_types.go	`15.90% <0.00%> (-0.76%)`	⬇️
api/kyverno/v1/spec_types.go	`17.03% <0.00%> (-0.66%)`	⬇️
api/kyverno/v1/zz_generated.defaults.go	`0.00% <0.00%> (ø)`
api/kyverno/v2alpha1/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
api/kyverno/v2alpha1/zz_generated.defaults.go	`0.00% <0.00%> (ø)`
api/kyverno/v2beta1/clusterpolicy_types.go	`18.42% <0.00%> (-1.03%)`	⬇️
api/kyverno/v2beta1/policy_types.go	`18.42% <0.00%> (-1.03%)`	⬇️
api/kyverno/v2beta1/spec_types.go	`18.34% <0.00%> (-0.89%)`	⬇️
api/kyverno/v2beta1/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
... and 46 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

chipzoller · 2023-07-11T14:29:06Z

Suspected to close #4774

realshuting

Please fix the linter issue.

pkg/engine/context/deferred.go

realshuting · 2023-07-12T08:49:59Z

pkg/engine/factories/contextloaderfactory.go

@@ -85,30 +85,30 @@ func (l *contextLoader) newLoader(
 	if entry.ConfigMap != nil {
 		if l.cmResolver != nil {
 			l := loaders.NewConfigMapLoader(ctx, l.logger, entry, l.cmResolver, jsonContext)
-			return enginecontext.NewDeferredLoader(entry.Name, l)
+			return enginecontext.NewDeferredLoader(entry.Name, l, logr.Logger{})


There's a logger built in l, you should re-use it.

To use l.logger which is builtin, I would have to rename l here:

kyverno/pkg/engine/factories/contextloaderfactory.go

Line 87 in 5577eb8

l := loaders.NewConfigMapLoader(ctx, l.logger, entry, l.cmResolver, jsonContext)

Otherwise it creates conflicts here while giving l.logger as a parameter:

kyverno/pkg/engine/factories/contextloaderfactory.go

Line 88 in 5577eb8

return enginecontext.NewDeferredLoader(entry.Name, l)

Should I rename it to something like lm everywhere?

prady0t · 2023-07-13T03:25:52Z

Please fix the linter issue.

I'm sorry but I can't see the linter issue anymore. Can you please approve linter workflow so that I can see why it's failing? Or maybe you can suggest how to fix.

JimBugwadia

Looks good! A few minor comments.

JimBugwadia · 2023-07-13T08:28:16Z

pkg/engine/context/deferred.go

@@ -34,7 +37,11 @@ func (dl *deferredLoader) HasLoaded() bool {
 }

 func (dl *deferredLoader) LoadData() error {
-	return dl.loader.LoadData()
+	if err := dl.loader.LoadData(); err != nil {
+		dl.logger.Error(err, "Failed to load data: ")


Please start log messages with a lower case. It would also be helpful to add the name.

dl.logger.Errorf(err, "failed to load data for %s", dl.name)

JimBugwadia · 2023-07-13T08:29:51Z

pkg/engine/factories/contextloaderfactory.go

@@ -85,30 +85,30 @@ func (l *contextLoader) newLoader(
 	if entry.ConfigMap != nil {
 		if l.cmResolver != nil {
 			l := loaders.NewConfigMapLoader(ctx, l.logger, entry, l.cmResolver, jsonContext)
-			return enginecontext.NewDeferredLoader(entry.Name, l)
+			return enginecontext.NewDeferredLoader(entry.Name, l, logr.Logger{})
 		} else {
 			l.logger.Info("disabled loading of ConfigMap context entry %s", entry.Name)
 			return nil, nil
 		}
 	} else if entry.APICall != nil {
 		if client != nil {
 			l := loaders.NewAPILoader(ctx, l.logger, entry, jsonContext, jp, client)


You can rename the l in l := loaders.NewAPILoader(... to something like ldr.

prady0t · 2023-07-13T09:21:53Z

Sorry for similar commits. DCO test keeps on failing even with the signed-off message

MarcelMue · 2023-07-13T10:21:35Z

Sorry for similar commits. DCO test keeps on failing even with the signed-off message

All commits need to be signed - so if you only sign the latest one, it won't work. I would recommend that you squash the commits in this PR and sign the single commit then.

prady0t · 2023-07-14T08:55:04Z

Sorry for similar commits. DCO test keeps on failing even with the signed-off message

All commits need to be signed - so if you only sign the latest one, it won't work. I would recommend that you squash the commits in this PR and sign the single commit then.

@MarcelMue I think i messed up somewhere as I'm unable to do that (maybe because I keep merging remote-tracking branch into main). Should I close this PR and make a new one?

MarcelMue · 2023-07-14T09:31:23Z

Sorry for similar commits. DCO test keeps on failing even with the signed-off message

All commits need to be signed - so if you only sign the latest one, it won't work. I would recommend that you squash the commits in this PR and sign the single commit then.

@MarcelMue I think i messed up somewhere as I'm unable to do that (maybe because I keep merging remote-tracking branch into main). Should I close this PR and make a new one?

What have you tried? I feel like solution like this should "just work":
https://stackoverflow.com/a/50880042

Just make sure you sign the commit in step two described on stackoverflow.

prady0t · 2023-07-14T11:13:44Z

Sorry for similar commits. DCO test keeps on failing even with the signed-off message

All commits need to be signed - so if you only sign the latest one, it won't work. I would recommend that you squash the commits in this PR and sign the single commit then.

@MarcelMue I think i messed up somewhere as I'm unable to do that (maybe because I keep merging remote-tracking branch into main). Should I close this PR and make a new one?

What have you tried? I feel like solution like this should "just work": https://stackoverflow.com/a/50880042

Just make sure you sign the commit in step two described on stackoverflow.

Ah! this is what I was looking for. Thanks for the help

prady0t · 2023-07-14T14:13:56Z

@MarcelMue The above method was not working so I read DCO docs and did this instead : git rebase HEAD~17 --signoff and then git push --force-with-lease origin main which produced these results🥲. What should I do?

prady0t · 2023-07-15T08:44:27Z

I've made another PR with same changes. Please review. #7834

prady0t requested review from MarcelMue, realshuting and MariamFahmy98 as code owners July 9, 2023 20:17

prady0t force-pushed the main branch 2 times, most recently from a6bc2e5 to 1ed9fa9 Compare July 9, 2023 20:43

realshuting requested changes Jul 12, 2023

View reviewed changes

realshuting added milestone 1.10.2 cherry-pick-required labels Jul 12, 2023

JimBugwadia requested changes Jul 13, 2023

View reviewed changes

prady0t force-pushed the main branch from 375e797 to 6fd7739 Compare July 14, 2023 13:59

prady0t requested review from vishal-chdhry, chipzoller, eddycharly and treydock as code owners July 14, 2023 13:59

prady0t closed this Jul 15, 2023

prady0t force-pushed the main branch from 6fd7739 to 2803f4f Compare July 15, 2023 06:13

prady0t mentioned this pull request Jul 15, 2023

Added log message for API call failures #7834

Merged

9 tasks

eddycharly removed cherry-pick-required milestone 1.10.2 labels Sep 27, 2023

Added log message for API call failures #7792

Added log message for API call failures #7792

Uh oh!

Conversation

prady0t commented Jul 9, 2023 • edited by chipzoller Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Related issue

Milestone of this PR

What type of PR is this

Proposed Changes

Proof Manifests

Checklist

Further Comments

Uh oh!

welcome bot commented Jul 9, 2023

Uh oh!

chipzoller commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prady0t commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realshuting commented Jul 11, 2023

Uh oh!

prady0t commented Jul 11, 2023

Uh oh!

codecov bot commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chipzoller commented Jul 11, 2023

Uh oh!

realshuting left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

realshuting Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

prady0t Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

prady0t commented Jul 13, 2023

Uh oh!

JimBugwadia left a comment

Choose a reason for hiding this comment

Uh oh!

JimBugwadia Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

JimBugwadia Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

prady0t commented Jul 13, 2023

Uh oh!

MarcelMue commented Jul 13, 2023

Uh oh!

prady0t commented Jul 14, 2023

Uh oh!

MarcelMue commented Jul 14, 2023

Uh oh!

prady0t commented Jul 14, 2023

Uh oh!

prady0t commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prady0t commented Jul 15, 2023

Uh oh!

Uh oh!

prady0t commented Jul 9, 2023 •

edited by chipzoller

Loading

chipzoller commented Jul 10, 2023 •

edited

Loading

prady0t commented Jul 11, 2023 •

edited

Loading

codecov bot commented Jul 11, 2023 •

edited

Loading

prady0t commented Jul 14, 2023 •

edited

Loading