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

KEP for decoupling KIC components #2252

Closed
wants to merge 0 commits into from
Closed

Conversation

shaneutt
Copy link
Contributor

@shaneutt shaneutt commented Feb 7, 2022

What this PR does / why we need it:

Previously we had considered KEP 1 declined due to other higher precedent priorities, but now as there are multiple related and synergistic features in flight such as the imminent preview release of Gateway support and the Kong Kubernetes Team considering a plan for what a v1 Operator release might look like and we're also getting recent updates on old (but still valid) issues such as #702 and #986 now seems like a good time to revamp and reconsider how we resolve some of the traditionally tightly coupled components in KIC.

@shaneutt shaneutt added priority/medium area/kep Enhancment Proposals labels Feb 7, 2022
@shaneutt shaneutt self-assigned this Feb 7, 2022
@shaneutt shaneutt temporarily deployed to Configure ci February 7, 2022 18:04 Inactive
@shaneutt shaneutt force-pushed the shaneutt/update-kep-0001 branch from bccdf2d to 536ae18 Compare February 7, 2022 18:07
@shaneutt shaneutt temporarily deployed to Configure ci February 7, 2022 18:07 Inactive
@shaneutt shaneutt temporarily deployed to Configure ci February 7, 2022 18:29 Inactive
@shaneutt shaneutt marked this pull request as ready for review February 7, 2022 19:09
@shaneutt shaneutt requested a review from a team as a code owner February 7, 2022 19:09
@rainest
Copy link
Contributor

rainest commented Feb 7, 2022

Per #986 (comment) I'm skeptical of the benefit of separating the webhook. You'll still need to update it, and those updates still have the same outage characteristics, so we're effectively just moving the problem elsewhere. IMO a default PDB makes more sense, but I'm not sure if a MinAvailable of 1 would play nicely with our default single-replica Deployments. Brief research suggests the answer is "it doesn't", so we'd probably need to make that chart-only with a conditional MinAvailable if the replica count >= 2. This would probably be a breaking change due to our existing PDB configuration.

One->many DB-less instance management needs to address the security concerns with exposing the admin API in the simplest implementation, as there's no standard way to secure a DB-less admin API listen. I think we'd probably need to create a Golang implementation of the hybrid mode protocol and make the controller an imitation control plane node so we can benefit from its built-in mTLS requirement.

@seh
Copy link
Contributor

seh commented Feb 8, 2022

there's no standard way to secure a DB-less admin API listen

Are you ruling out NetworkPolicy to handle that? You could reject ingress to the Admin port from pods other than the ingress controllers.

@rainest
Copy link
Contributor

rainest commented Feb 8, 2022

@seh
Copy link
Contributor

seh commented Feb 8, 2022

it looks like it's off by default in the major providers

Yes, that's sad. We use it to good effect with Calico in our AWS-hosted clusters. In GKE, well, we have plans. We're not using the "V2 dataplane" yet.

@shaneutt shaneutt closed this Feb 10, 2022
@shaneutt shaneutt force-pushed the shaneutt/update-kep-0001 branch from 536ae18 to 8be5889 Compare February 10, 2022 14:37
@shaneutt shaneutt deleted the shaneutt/update-kep-0001 branch February 10, 2022 14:37
@shaneutt
Copy link
Contributor Author

shaneutt commented Feb 10, 2022

Given the contention about the webhook I'm going to drop this change in favor of keeping that as a separate concern so it doesn't overwhelm the conversation.

#2256 is the replacement, and the webhook stuff will be considered separately.

@shaneutt shaneutt temporarily deployed to gcloud February 10, 2022 15:03 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants