fix: missing status update in KMP controller #1761

duffney · 2024-08-29T16:21:09Z

Description

What this PR does / why we need it:

In this PR the writeKMProviderStatus and helper functions were moved from the refresher back to the controller to ensure status messages get written to the KMP resource.

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

Fixes #

#1733

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Test A
Test B

Checklist:

Does the affected code have corresponding tests?
Do all new files have appropriate license header?

Post Merge Requirements

MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

codecov · 2024-08-29T18:35:47Z

Codecov Report

Attention: Patch coverage is 95.50000% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/keymanagementprovider/mocks/client.go	55.55%	7 Missing and 1 partial ⚠️
pkg/keymanagementprovider/mocks/types.go	85.71%	1 Missing ⚠️

Files with missing lines	Coverage Δ
...lusterresource/keymanagementprovider_controller.go	`94.11% <100.00%> (+25.93%)`	⬆️
...espaceresource/keymanagementprovider_controller.go	`94.04% <100.00%> (+34.95%)`	⬆️
pkg/keymanagementprovider/mocks/factory.go	`100.00% <100.00%> (ø)`
pkg/keymanagementprovider/refresh/factory.go	`100.00% <100.00%> (+11.76%)`	⬆️
pkg/keymanagementprovider/refresh/kubeRefresh.go	`100.00% <100.00%> (+18.18%)`	⬆️
pkg/keymanagementprovider/mocks/types.go	`91.66% <85.71%> (-8.34%)`	⬇️
pkg/keymanagementprovider/mocks/client.go	`55.55% <55.55%> (-44.45%)`	⬇️

... and 20 files with indirect coverage changes

binbin-li · 2024-08-30T02:48:23Z

pkg/controllers/namespaceresource/keymanagementprovider_controller.go

+	isFetchSuccessful = true
+	emptyErrorString := ""
+
+	writeKMProviderStatus(ctx, r, &keyManagementProvider, logger, isFetchSuccessful, emptyErrorString, lastFetchedTime, status)


nit: we can remove line 122 and pass true directly, same to emptyErrorString

binbin-li · 2024-08-30T02:48:40Z

pkg/controllers/namespaceresource/keymanagementprovider_controller.go

+		return ctrl.Result{}, err
+	}
+
+	config := map[string]interface{}{


we can define a struct instead of creating a map.

That would make things a bit easier. Great suggestion, I'll start working on converting the map to a struct.

@binbin-li I've got some questions on how we could use a struct instead of a map and support other refresher types without pulling in refresher specific values into the controller logic. Or even if that's a necessary concern. I'll bring it up at the next community meeting. :)

discussed in community meeting, see example at

ratify/httpserver/types.go

Line 36 in 7519519

type VerificationResponse struct {

binbin-li · 2024-08-30T02:52:39Z

pkg/controllers/namespaceresource/keymanagementprovider_controller.go

+	}
+
+	config := map[string]interface{}{
+		"type":                    refresherType,


nit: I would rename it to refresherType since we have another providerType

binbin-li · 2024-08-30T03:25:03Z

pkg/keymanagementprovider/refresh/kubeRefresh.go

 // Create creates a new KubeRefresher instance
 func (kr *KubeRefresher) Create(config map[string]interface{}) (Refresher, error) {
-	client, ok := config["client"].(client.Client)
+	provider, ok := config["provider"].(kmp.KeyManagementProvider)


Usually we need to check exist first and then do type assertion to avoid panic. But if we make config a struct instead of map, we could fetch it via config.Provider.

junczhu · 2024-09-02T08:01:09Z

pkg/controllers/clusterresource/keymanagementprovider_controller.go

+func (r *KeyManagementProviderReconciler) ReconcileWithType(ctx context.Context, req ctrl.Request, refresherType string) (ctrl.Result, error) {
+	logger := logrus.WithContext(ctx)
+
+	var resource = req.Name


QQ: Shall we use resource := req.Name here

Excellent catch, you have a keen eye @junczhu! Yes := should be used here.

binbin-li · 2024-09-06T14:21:06Z

pkg/controllers/clusterresource/keymanagementprovider_controller.go

+// updateKMProviderErrorStatus updates the key management provider status with error, brief error and last fetched time
+func updateKMProviderErrorStatus(keyManagementProvider *configv1beta1.KeyManagementProvider, errorString string, operationTime *metav1.Time) {
+	// truncate brief error string to maxBriefErrLength
+	briefErr := errorString


actually we made some updates to the briefErr construction recently, could you help update it following the new pattern: https://github.com/ratify-project/ratify/blob/dev/pkg/controllers/clusterresource/policy_controller.go#L121

thanks!

Done. :) I think I implemented it correctly.

wonder if it's the latest commit, we can set briefErr by:
keyManagementProvider.Status.BriefError = err.GetConciseError(constants.MaxBriefErrLength)

binbin-li · 2024-09-09T03:38:25Z

pkg/keymanagementprovider/refresh/refresh.go

+	GetStatus() interface{}
+}
+
+type RefresherConfig struct {


nit: could add some comments to Exported struct and fields

binbin-li · 2024-09-09T04:48:00Z

pkg/controllers/clusterresource/keymanagementprovider_controller.go

+// updateKMProviderErrorStatus updates the key management provider status with error, brief error and last fetched time
+func updateKMProviderErrorStatus(keyManagementProvider *configv1beta1.KeyManagementProvider, errorString string, operationTime *metav1.Time) {
+	// truncate brief error string to maxBriefErrLength
+	briefErr := errorString


wonder if it's the latest commit, we can set briefErr by:
keyManagementProvider.Status.BriefError = err.GetConciseError(constants.MaxBriefErrLength)

binbin-li · 2024-09-09T05:00:51Z

pkg/controllers/clusterresource/keymanagementprovider_controller.go

 	if err != nil {
+		writeKMProviderStatus(ctx, r, &keyManagementProvider, logger, false, err.Error(), lastFetchedTime, nil)


probably we also need to set KMP errors since keys/certs are not fetched yet.

kmp.SetCertificateError(resource, kmpErr) kmp.SetKeyError(resource, kmpErr)

binbin-li · 2024-09-12T05:31:12Z

@duffney PR lgtm, could you update commits to be verified so that we can merge it?

Signed-off-by: akashsinghal <[email protected]>

duffney requested review from akashsinghal, binbin-li, jimmyraywv, luisdlp, susanshi and toddysm as code owners August 29, 2024 16:21

duffney force-pushed the issue-1733 branch from b69f88e to deef25b Compare August 29, 2024 18:31

duffney force-pushed the issue-1733 branch from deef25b to 925eafb Compare August 29, 2024 19:31

binbin-li reviewed Aug 30, 2024

View reviewed changes

junczhu reviewed Sep 2, 2024

View reviewed changes

duffney force-pushed the issue-1733 branch from 53fe1ec to bf50d5c Compare September 4, 2024 17:47

binbin-li reviewed Sep 6, 2024

View reviewed changes

duffney force-pushed the issue-1733 branch from da11a4a to d20b8e1 Compare September 6, 2024 15:39

binbin-li reviewed Sep 9, 2024

View reviewed changes

binbin-li previously approved these changes Sep 11, 2024

View reviewed changes

binbin-li enabled auto-merge (squash) September 12, 2024 02:39

auto-merge was automatically disabled September 12, 2024 13:00
Head branch was pushed to by a user without write access

duffney force-pushed the issue-1733 branch from 7a45f33 to bb46081 Compare September 12, 2024 13:00

refactor: move writeKMProviderStatus to controller

7123a16

duffney dismissed binbin-li’s stale review via 7123a16 September 12, 2024 13:05

duffney force-pushed the issue-1733 branch from bb46081 to 7123a16 Compare September 12, 2024 13:05

binbin-li approved these changes Sep 12, 2024

View reviewed changes

binbin-li merged commit b32db85 into ratify-project:dev Sep 12, 2024
19 checks passed

akashsinghal pushed a commit to akashsinghal/ratify that referenced this pull request Sep 13, 2024

fix: missing status update in KMP controller (ratify-project#1761)

377e98d

Signed-off-by: akashsinghal <[email protected]>

binbin-li pushed a commit to binbin-li/ratify that referenced this pull request Sep 14, 2024

fix: missing status update in KMP controller (ratify-project#1761)

8574733

duffney deleted the issue-1733 branch September 25, 2024 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: missing status update in KMP controller #1761

fix: missing status update in KMP controller #1761

duffney commented Aug 29, 2024

codecov bot commented Aug 29, 2024 •

edited

Loading

binbin-li Aug 30, 2024

binbin-li Aug 30, 2024

duffney Sep 3, 2024

duffney Sep 4, 2024

susanshi Sep 5, 2024

binbin-li Aug 30, 2024

binbin-li Aug 30, 2024

junczhu Sep 2, 2024

duffney Sep 3, 2024

binbin-li Sep 6, 2024

duffney Sep 6, 2024

binbin-li Sep 9, 2024

binbin-li Sep 9, 2024

binbin-li Sep 9, 2024

binbin-li Sep 9, 2024

binbin-li commented Sep 12, 2024

		if err != nil {
		writeKMProviderStatus(ctx, r, &keyManagementProvider, logger, false, err.Error(), lastFetchedTime, nil)

fix: missing status update in KMP controller #1761

fix: missing status update in KMP controller #1761

Conversation

duffney commented Aug 29, 2024

Description

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Type of change

How Has This Been Tested?

Checklist:

Post Merge Requirements

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

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

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

binbin-li commented Sep 12, 2024

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

codecov bot commented Aug 29, 2024 •

edited

Loading