feat: KMP periodic retrieval with k8s requeue #1625

duffney · 2024-07-10T16:18:30Z

Description

Implements a periodic refresh for KMP resources deployed to Kubernetes by using the controller's re-queue functionality.

What this PR does / why we need it:

Adds two new properties to the KMP CRD spec Interval and Refreshable. Interval determines the amount of time in-between refreshes. Refreshable determines if the resources is eligible for refresh.

Logic from the controller's reconcile method has been abstracted into a new refresh interface. TODO: more details

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 #1131

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
This change requires a documentation update

How Has This Been Tested?

Unit Tests:

CRs with nonrefreshable provider
CRs with refreshable provider using default interval (disabled)
CRs with an invalid interval
CRs with user provided interval
CRs with user updated interval

Manual Tests:

Key Management Provider with Azure Key Vault Certificate (Version not specified)

apiVersion: config.ratify.deislabs.io/v1beta1
kind: KeyManagementProvider
metadata:
  name: keymanagementprovider-akv
spec:
  type: azurekeyvault
  interval: 1m
  parameters:
    vaultURI: $AKV_URI
    certificates:
      - name: $certName
    tenantID: $tenantID
    clientID: $clientID

Behavior:

When the certificate is rotated (new version created) the CR errors unable to pull secrets w/ disabled state
- pkg/keymanagementprovider/azurekeyvault/provider.go line:147
After refresh certificate version is updated
artifacts with old cert signatures fail verification (expected)
Verification was successful after artifacts were resigned with new certificate
Unsure if previous versions of the certificate are removed from certs & keys maps

Key Management Provider with Azure Key Vault Certificate (with Specified Version)
- Does verification fail when the cert or key is disabled?

E2E

Validation with refreshed identity permission
Validation with refreshed cert/key
Validation with disabled cert/key

Checklist:

Does the affected code have corresponding tests?
Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
Does this introduce breaking changes that would require an announcement or bumping the major version?
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-07-10T16:23:31Z

Codecov Report

Attention: Patch coverage is 79.38144% with 40 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
pkg/keymanagementprovider/refresh/kubeRefresh.go	83.52%	10 Missing and 4 partials ⚠️
...anagementprovider/refresh/kubeRefreshNamespaced.go	83.52%	10 Missing and 4 partials ⚠️
...lusterresource/keymanagementprovider_controller.go	0.00%	7 Missing ⚠️
...espaceresource/keymanagementprovider_controller.go	0.00%	5 Missing ⚠️

Files	Coverage Δ
...kg/keymanagementprovider/azurekeyvault/provider.go	`69.88% <100.00%> (+0.34%)`	⬆️
pkg/keymanagementprovider/inline/provider.go	`78.37% <100.00%> (+18.37%)`	⬆️
pkg/keymanagementprovider/keymanagementprovider.go	`92.72% <ø> (ø)`
pkg/keymanagementprovider/mocks/client.go	`100.00% <100.00%> (ø)`
pkg/keymanagementprovider/mocks/factory.go	`100.00% <100.00%> (ø)`
pkg/keymanagementprovider/mocks/types.go	`100.00% <100.00%> (+100.00%)`	⬆️
...espaceresource/keymanagementprovider_controller.go	`0.00% <0.00%> (-74.65%)`	⬇️
...lusterresource/keymanagementprovider_controller.go	`0.00% <0.00%> (-74.65%)`	⬇️
pkg/keymanagementprovider/refresh/kubeRefresh.go	`83.52% <83.52%> (ø)`
...anagementprovider/refresh/kubeRefreshNamespaced.go	`83.52% <83.52%> (ø)`

pkg/controllers/clusterresource/keymanagementprovider_controller.go

susanshi · 2024-07-18T23:36:43Z

Would like to understand the scope of this PR. If docs and e2e tests will be included in this PR

susanshi · 2024-07-18T23:39:10Z

Please complete test coverage section once PR is ready for Review. Here are some suggestion, let us know if you have any questions, thanks!

CRs ( various provider) with default provider interval
CRs with user provided interval
CRs with user updated interval
Validation with refreshed identity permission
Validation with refreshed cert/key
Validation with disabled cert/key
backward compat: what's the behavior on existing kmp

duffney · 2024-07-23T14:03:31Z

Would like to understand the scope of this PR. If docs and e2e tests will be included in this PR

My thinking was this PR would include the changes to the KMP resource that enabled the refresh possible for clustered and namedspaced resources of the KMP. I was thinking the e2e tests would be a separate PR but, I don't have a strong opinion. So, if it's preferred to have them in this PR, that's fine with me also.

susanshi · 2024-07-24T05:33:06Z

I think e2e can be a separate PR if this PR is covered by manual testing. @duffney , do you think this PR will be ready for review this week?

duffney · 2024-07-24T14:17:16Z

I think e2e can be a separate PR if this PR is covered by manual testing. @duffney , do you think this PR will be ready for review this week?

@susanshi the PR is ready for another around of review. :) I just pushed changes that add the refresh interface to namespaced resources. I'll work on manually testing these changes and documenting the steps. That'll give me a better idea of how to build the e2e test for the next PR.

susanshi · 2024-07-30T00:42:06Z

discussed in PR review , for 1.3 default to empty string ( disable refresh) , in 1.4, once we support N version of cert , default to 1min. (Note the breaking scenario of disabled key/cert)

yizha1 · 2024-07-30T03:51:52Z

@susanshi is the default to 1 min too short? The key/cert is not likely to be rotated in such a short period normally.

duffney · 2024-07-30T19:31:32Z

CRs with user updated interval

@susanshi, can you elaborate on what you're meaning by this test? Does this mean that the user defined and interval and then replaced it with a different value later on?

binbin-li · 2024-07-31T14:50:20Z

pkg/controllers/clusterresource/keymanagementprovider_controller.go

-		// Note: for backwards compatibility in upgrade scenarios, Ratify will only log a warning statement.
-		logger.Warn("Certificate Store already exists. Key management provider and certificate store should not be configured together. Please migrate to key management provider and delete certificate store.")
+	// check if kr.client is nil
+	if kr.Client == nil {


curious if we can check r.Client == nil before line 44?

binbin-li · 2024-07-31T15:03:01Z

api/v1beta1/keymanagementproviders_types.go

@@ -31,6 +31,9 @@ type KeyManagementProviderSpec struct {
 	// Name of the key management provider
 	Type string `json:"type,omitempty"`

+	// +kubebuilder:default=""
+	Interval string `json:"interval,omitempty"`


wonder if we should call it interval or refreshInterval?

binbin-li · 2024-07-31T15:06:16Z

pkg/keymanagementprovider/refresh/refresh.go

+	"context"
+)
+
+type Refresher interface {


nit: could you add comments on exported interface and API functions?

binbin-li · 2024-07-31T15:18:00Z

pkg/controllers/clusterresource/keymanagementprovider_controller.go

-		}
-
-		return ctrl.Result{}, client.IgnoreNotFound(err)
+	kr := refresh.KubeRefresher{


Since we already defined Refresher interface, I feel we can have a factory for it and create an Refresher object instead of a concrete implementation struct.

susanshi · 2024-07-31T20:21:20Z

@susanshi is the default to 1 min too short? The key/cert is not likely to be rotated in such a short period normally.

Hi Yi, the scenario in mind how long does customer have to wait for the refresh. In scenarios like changes in permission role assignment, customer might want to wait short amount of time to validate that the cert retrieval succeeded. KV providers have thousands of transaction limit per 10 sec, there is no concern from the request throttling side.

susanshi · 2024-07-31T20:34:24Z

config/crd/bases/config.ratify.deislabs.io_keymanagementproviders.yaml

@@ -50,6 +50,9 @@ spec:
          spec:
            description: KeyManagementProviderSpec defines the desired state of KeyManagementProvider
            properties:
+              interval:


Suggestion to add some samples of refresh interval configured AKV in the samples dir thanks! , https://github.com/ratify-project/ratify/tree/dev/config/samples

susanshi · 2024-07-31T20:41:14Z

charts/ratify/crds/keymanagementprovider-customresourcedefinition.yaml

-    subresources:
-      status: {}
+                interval:
+                  default: ""


please add description for the interval property. Suggestion to also mention this is only application to "refreshable" providers

examples of the allowed values would also be helpful .thanks!

susanshi · 2024-08-02T01:27:07Z

CRs with user updated interval

@susanshi, can you elaborate on what you're meaning by this test? Does this mean that the user defined and interval and then replaced it with a different value later on?

thanks for confirming @joshuaphelpsms. Yes that is correct, exactly , "the user defined and interval and then replaced it with a different value later on"

akashsinghal changed the title ~~KMP periodic retrieval with Kubernetes requeue~~ feat: KMP periodic retrieval with k8s requeue Jul 10, 2024

akashsinghal reviewed Jul 15, 2024

View reviewed changes

pkg/controllers/clusterresource/keymanagementprovider_controller.go Outdated Show resolved Hide resolved

duffney force-pushed the issue-1131/refresher branch from dae0f30 to 234e835 Compare July 17, 2024 14:10

duffney force-pushed the issue-1131/refresher branch from ca14514 to 7f195ad Compare July 24, 2024 14:11

duffney force-pushed the issue-1131/refresher branch 3 times, most recently from a237ec3 to ef0ec99 Compare July 30, 2024 00:01

duffney added 15 commits July 30, 2024 14:03

Mod: Add interval and refreshable to spec

846b93e

feat: refresher interface

7fe72dc

fix: formatting and license

d20b22d

test: kuberefresher

3910f60

test: kuberefresher helpers

473525e

refactor: IsRefreshable KMP interface method

89bbd75

fix: add IsRefreshable method

32b544b

refactor: kmp spec interval as string

9285afb

mod: rm todo

f07528f

mod: rm comment

b49c76e

style: use embedded type

e6442ac

feat: namespaced refresher interface

fb3698c

test: mock factory & test refreshable logic

a8feccc

refactor: use kubeRefreshNamespaced in controller

c97844d

mod: add license blocks

03dc90b

refactor: kmp.spec.interval default to empty string disabling refresh

20f09ea

duffney force-pushed the issue-1131/refresher branch from ef0ec99 to 20f09ea Compare July 30, 2024 19:04

binbin-li reviewed Jul 31, 2024

View reviewed changes

susanshi reviewed Jul 31, 2024

View reviewed changes

tests: increase refresher coverage

a0540d2

susanshi reviewed Jul 31, 2024

View reviewed changes

susanshi marked this pull request as ready for review August 1, 2024 01:40

susanshi requested review from jimmyraywv, luisdlp and toddysm as code owners August 1, 2024 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: KMP periodic retrieval with k8s requeue #1625

feat: KMP periodic retrieval with k8s requeue #1625

duffney commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

susanshi commented Jul 18, 2024

susanshi commented Jul 18, 2024 •

edited

Loading

duffney commented Jul 23, 2024

susanshi commented Jul 24, 2024

duffney commented Jul 24, 2024

susanshi commented Jul 30, 2024 •

edited

Loading

yizha1 commented Jul 30, 2024

duffney commented Jul 30, 2024

binbin-li Jul 31, 2024

binbin-li Jul 31, 2024

binbin-li Jul 31, 2024

binbin-li Jul 31, 2024

susanshi commented Jul 31, 2024

susanshi Jul 31, 2024

susanshi Jul 31, 2024

susanshi Jul 31, 2024

susanshi commented Aug 2, 2024

feat: KMP periodic retrieval with k8s requeue #1625

Are you sure you want to change the base?

feat: KMP periodic retrieval with k8s requeue #1625

Conversation

duffney commented Jul 10, 2024 • edited Loading

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 Jul 10, 2024 • edited Loading

Codecov Report

susanshi commented Jul 18, 2024

susanshi commented Jul 18, 2024 • edited Loading

duffney commented Jul 23, 2024

susanshi commented Jul 24, 2024

duffney commented Jul 24, 2024

susanshi commented Jul 30, 2024 • edited Loading

yizha1 commented Jul 30, 2024

duffney commented Jul 30, 2024

binbin-li Jul 31, 2024

Choose a reason for hiding this comment

binbin-li Jul 31, 2024

Choose a reason for hiding this comment

binbin-li Jul 31, 2024

Choose a reason for hiding this comment

binbin-li Jul 31, 2024

Choose a reason for hiding this comment

susanshi commented Jul 31, 2024

susanshi Jul 31, 2024

Choose a reason for hiding this comment

susanshi Jul 31, 2024

Choose a reason for hiding this comment

susanshi Jul 31, 2024

Choose a reason for hiding this comment

susanshi commented Aug 2, 2024

duffney commented Jul 10, 2024 •

edited

Loading

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 Jul 10, 2024 •

edited

Loading

susanshi commented Jul 18, 2024 •

edited

Loading

susanshi commented Jul 30, 2024 •

edited

Loading