WIP CNTRLPLANE-2711: introduce configuration api for vault kms plugin#1954
WIP CNTRLPLANE-2711: introduce configuration api for vault kms plugin#1954flavianmissi wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@flavianmissi: This pull request references CNTRLPLANE-2711 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api
branch
6 times, most recently
from
0bfa6fd to
5b652a8
Compare
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
This is WIP but just wanted to drop my some initial comments
|
|
||
| #### Feature Gate | ||
|
|
||
| A new feature gate `KMSv2` will be introduced and enabled in |
There was a problem hiding this comment.
It is better to mention about the requirement of new EncryptionType In order to keep TPv1 running, we need to have separate EncryptionType https://github.com/openshift/api/blob/b3c6104577797f804d921ac05ad91aa4186818f2/config/v1/types_apiserver.go#L234 which is bound to this feature gate.
| mustRegister() | ||
| ``` | ||
|
|
||
| Additionally, the existing `KMSEncryptionProvider` feature gate is extended to |
There was a problem hiding this comment.
That would mean that we will have to ask enabling 2 feature gates for that purpose. Do we really need this?. Why can't we tombstone KMSEncryptionProvider and all its types and invent something new (just belongs to KMSv2)?
| provider type in the `KMSProviderType` enum. | ||
|
|
||
| ```go | ||
| FeatureGateKMSv2 = newFeatureGate("KMSv2"). |
There was a problem hiding this comment.
nitpick:
| FeatureGateKMSv2 = newFeatureGate("KMSv2"). | |
| FeatureGateKMSv2 = newFeatureGate("KMSEncryptionv2"). |
to emphasize that this is v2 of which feature gate
|
|
||
| #### Vault KMSConfig | ||
|
|
||
| The following config is extracted from [Vault KMS Plugin documentation](https://github.com/hashicorp/web-unified-docs/blob/ab6191e4856b52a59a87fe0f17703671a7317ec6/content/vault/v1.21.x/content/docs/deploy/kubernetes/kms/configuration.mdx). |
There was a problem hiding this comment.
Private document, maybe we should remove
| The `KMSConfig` type is extended to include the Vault provider: | ||
|
|
||
| ```diff | ||
| type KMSConfig struct { |
There was a problem hiding this comment.
We definitely do not need to carry the burden of previous design.
| kind: Secret | ||
| metadata: | ||
| name: vault-approle | ||
| namespace: openshift-config |
There was a problem hiding this comment.
ah, I see. This is a different namespace than openshift-config-managed.
| name: cluster | ||
| spec: | ||
| encryption: | ||
| type: KMS |
There was a problem hiding this comment.
This should be different. Otherwise, controllers will use v1.
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api
branch
from
5b652a8 to
9f56ba5
Compare
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api
branch
from
9f56ba5 to
3dbfdfe
Compare
|
@flavianmissi: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
No description provided.