-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: introduce CRD KongPluginInstallation #400
Conversation
47374d0
to
d42c158
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought on the conditions for this type:
- make
Ready
a primary type for this CRD - it can have its reason set to
Fetched
when status isTrue
- it can have its reason set to
Failed
when status isFalse
and there are problems fetching the image - it can have its reason set to
Unknown
initially
I'm interested to hear your thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some brevity copyedits and two major bits:
- Retrieval and installation states are independent and should have separate condition types.
- It's unclear whether we'll allow reuse of the same KongPluginInstallation across mulitple DataPlanes. If so, that has major implications for status, since there are now multiple simultaneous installation states on the same CR.
For the second, only allowing one is simpler and maybe good practice (there's no way to accidentally update a bunch of other Deployments by modifying the shared resource). The effort to duplicate them is low and the impact probably is also--even if we don't cache image downloads for the same URL, plugins aren't huge files and re-downloading them doesn't waste much bandwidth.
6286ec0
to
a69a3a2
Compare
For #400 (review)
|
From sync discussion: We want to limit a given KPI to a single DataPlane or Gateway, so we can use the simpler condition-based status. We have to use a pull model (push from the KPI to its installation target would create all sorts of fun security problems), so we'll presumably need an additional "you tried to install this on multiple gateways, you can't" condition. We should persist the original installation in such a case, but won't be able to tell which request was first from existing conditions (they indicate if the plugin is installed, but not where). We should add a status field or some other mechanism to indicate what gateway resource "owns" a given KPI so that bad configuration doesn't disrupt a working environment. AFAIK OCI URLs indicate image version, so we shouldn't need to poll the URL for updated code--we will know an update occurred because the URL actually changed. This and other operator features can trigger a Deployment update and new Pod rollout, which may be undesirable. We may want to consider some indicator that allows DataPlane/Gateway resource owners to indicate "hey, even if there's a change to other operator resources that require an update and rollout, wait until I give the okay". The specifics of this would need their own design doc though, so for now it's just something to keep in mind and track any related user requests. Re:
We can know for certain that we've also successfully added the associated Volume(Mount) and envvars to the Deployment, so we should report as much. As long as we've added that configuration to the Deployment, we can reasonably expect Kubernetes to realize it, or report if it can't through normal channels (Deployment status will indicate if the expected Pods for the latest revision are happy or not). |
Actually: what stands in our way to allow multi use of KPI? I'd imagine that
Am I missing something? |
a69a3a2
to
a07b2de
Compare
This is all set--retrieval side seems fine.
Depends on where the status info ultimately lives. If it's on the KPI CR and there are multiple clients, we need a status spec that can present that info. If it ends up as part of the client (DataPlane/GatewayConfiguration) status, then those client resource statuses can reflect their local state. |
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
a07b2de
to
eb24bd6
Compare
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
I've updated the PR description to include that the scope for status in |
9ccc852
to
cddbd33
Compare
What this PR does / why we need it:
This PR introduces a new CRD for dealing with Custom Kong Plugins. The name of that CRD is
KongPluginInstallation
. It's a namespace CRD that may be extended in the future. It allows referencing K8s secrets from arbitrary chosen namespaces. Here the minimal set of fields is presented. For status, it follows an approach of GWAPI objects.This PR contains only
KongPluginInstallation
, which describes whether an image with a plugin has been successfully fetched and unpacked. The user will learn about the actual status of installation in the status field of CRDs, which will reference the instance ofKongPluginInstallation
.Which issue this PR fixes
Closes #378
Special notes for your reviewer:
When K8s deals with fetching images it allows to specify
imagePullPolicy
toIfNotPresent
,Always
,Never
. The default behavior is tricky. Thus I decided to not add this field and to fetch images always.Also, machinery for generating boilerplate was adjusted as part of this PR to use mise installed kustomize.
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect significant changes