feat: KMP periodic retrieval with k8s requeue

[duffney]

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)

apiVersion: config.ratify.deislabs.io/v1beta1
kind: KeyManagementProvider
metadata:
  name: kmpprovider-akv-versioned
spec:
  type: azurekeyvault
  refreshinterval: 1m
  parameters:
    vaultURI: $AKV
    certificates:
      - name: $certName 
        version: $version
    tenantID: $tenantID
    clientID: $clientID

Behavior:

• When the KMP is configured with a versioned certificate refreshing the resource does not update the version of the certificate stored in the resource. For example. if a new latest version of the certificate the resource will not update to use the latest version.

• Does verification fail when the cert or key is disabled?
  • No, when a the KMP is created with an enabled cert that later becomes disabled it remains in the cert/key maps and allows for verification to succeed. See [Bug] Disabled certificates pass verification  #1699 for details.
  • Yes, when a KMP is created with a disabled certificate the provider returns an error before the cert/key maps are populated.

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]

Codecov Report

Attention: Patch coverage is 83.52941% with 42 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
pkg/keymanagementprovider/refresh/kubeRefresh.go	81.81%	13 Missing and 5 partials ⚠️
...anagementprovider/refresh/kubeRefreshNamespaced.go	81.81%	13 Missing and 5 partials ⚠️
...lusterresource/keymanagementprovider_controller.go	85.71%	1 Missing and 1 partial ⚠️
...espaceresource/keymanagementprovider_controller.go	85.71%	1 Missing and 1 partial ⚠️
pkg/keymanagementprovider/refresh/factory.go	88.23%	1 Missing and 1 partial ⚠️

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%)	⬆️
...lusterresource/keymanagementprovider_controller.go	68.18% <85.71%> (-6.47%)	⬇️
...espaceresource/keymanagementprovider_controller.go	59.09% <85.71%> (-15.56%)	⬇️
pkg/keymanagementprovider/refresh/factory.go	88.23% <88.23%> (ø)
pkg/keymanagementprovider/refresh/kubeRefresh.go	81.81% <81.81%> (ø)
... and 1 more

[susanshi]

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

[susanshi]

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]

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]

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]

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]

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]

@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]

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?

[susanshi]

@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]

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]

@duffney thanks for PR. Overall looks good. Do you think we need to add a new interval value in the helm chart for AKV?

[duffney]

@duffney thanks for PR. Overall looks good. Do you think we need to add a new interval value in the helm chart for AKV?

You're most welcome! And yes, adding a refreshinterval to the helm chart makes a lot of sense to me. Having it there prevents the user from having to redeploy the CRD configuration to enable refreshing. Would that involve adding a new value to charts/ratify/values.yml and adding an if to the kmp templates?

[akashsinghal]

@duffney thanks for PR. Overall looks good. Do you think we need to add a new interval value in the helm chart for AKV?

You're most welcome! And yes, adding a refreshinterval to the helm chart makes a lot of sense to me. Having it there prevents the user from having to redeploy the CRD configuration to enable refreshing. Would that involve adding a new value to charts/ratify/values.yml and adding an if to the kmp templates?

@duffney yes that's correct. Maybe you can add the refresh interval for AKV KMP here:

ratify/charts/ratify/values.yaml

Line 70 in 56ffab4

azurekeyvault:

. You would also update the default AKV KMP template and add the new value to the README of the charts folder

[susanshi]

please review factory at , 85d7006

[akashsinghal]

LGTM. Thanks for addressing my comments!

[binbin-li]

lgtm, thanks for adding the feature and unit tests!