Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CRD KongVault to manage custom vaults in Kong gateway #5332

Closed
1 of 2 tasks
randmonkey opened this issue Dec 14, 2023 · 5 comments · Fixed by #5354
Closed
1 of 2 tasks

Add CRD KongVault to manage custom vaults in Kong gateway #5332

randmonkey opened this issue Dec 14, 2023 · 5 comments · Fixed by #5354
Assignees
Labels
area/CRD Changes in existing CRDs or introduction of new ones area/feature New feature or request
Milestone

Comments

@randmonkey
Copy link
Contributor

randmonkey commented Dec 14, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Problem Statement

Split from #4559.

Kong gateway supports to create custom vaults (https://docs.konghq.com/gateway/3.5.x/kong-enterprise/secrets-management/) to store sensitive data and reference them by vault URI in plugin configurations. So we need to define a KongVault CRD to represent a custom Kong vault. This issue tracks the implementation of KongVault CRD.

Proposed Solution

Add a KongVault CRD in configuration.konghq.com/v1alpha1 group/version to represent a custom Kong vault.

Additional information

No response

Acceptance Criteria

  • Has the definition and basical CEL validation rules of KongVault CRD.
@randmonkey randmonkey added area/feature New feature or request area/CRD Changes in existing CRDs or introduction of new ones labels Dec 14, 2023
@randmonkey randmonkey added this to the KIC v3.1.x milestone Dec 14, 2023
@randmonkey randmonkey self-assigned this Dec 14, 2023
@mheap
Copy link
Member

mheap commented Dec 18, 2023

@randmonkey A couple of questions:

  1. Why v1alpha1? This feels like a low-risk change to me with a well defined schema.
  2. Why CEL? We need to delegate validation to Gateway's validation endpoint anyway. The two validations could get out of sync

@pmalek
Copy link
Member

pmalek commented Dec 18, 2023

Why v1alpha1? This feels like a low-risk change to me with a well defined schema.

Am I reading this correctly that we'd like to push it up to let's say v1beta1 group to give users a hint that it's a bit more stable/mature API than alpha?

I'd argue that we err on the safe side and release alpha first and after a reasonable soak time push it up to beta. WDYT?

@mheap
Copy link
Member

mheap commented Dec 18, 2023

Yep, I'd have defaulted to v1beta1 for this resource. There are maintenance costs for promoting (in code + docs), and I don't see much value here

@randmonkey
Copy link
Contributor Author

@randmonkey A couple of questions:

  1. Why v1alpha1? This feels like a low-risk change to me with a well defined schema.
  2. Why CEL? We need to delegate validation to Gateway's validation endpoint anyway. The two validations could get out of sync
  1. Why v1alpha1: I do not have the confidence that there will be no major changes on this API so I first add it in v1alpha1. If we have the common sense that there will be no major changes (say remove or change the semantic of existing fields) on the API, we may move it to v1beta1.
  2. I think the major validation should happen on gateway side, but the CELs are for the most basic validations, like backend cannot be empty, prefix should be unique (This seems not to have a supporting CEL expression?)

@mheap
Copy link
Member

mheap commented Dec 18, 2023

👍 on CEL

As soon as this API is shipped, customers will adopt it. I'd prefer to spend extra cycles before shipping and provide something that we're confident with in v1beta1 than ship an alpha and potentially change it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CRD Changes in existing CRDs or introduction of new ones area/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants