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

chore: Improve KubernetesRouter selection based on apiGroup #1750

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kahirokunn
Copy link
Contributor

Allow .spec.targetRef to specify custom resources, which typically use mesh routers. Prevent unnecessary resources by strictly handling Deployment and DaemonSet based on apiGroup derived from apiVersion.

Allow `.spec.targetRef` to specify custom resources, which typically use mesh routers.
Prevent unnecessary resources by strictly handling Deployment and DaemonSet based on apiGroup derived from apiVersion.

Signed-off-by: kahirokunn <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 11.11111% with 8 lines in your changes missing coverage. Please review.

Project coverage is 39.30%. Comparing base (febc327) to head (ae879cd).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
pkg/router/factory.go 0.00% 7 Missing ⚠️
pkg/controller/finalizer.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1750      +/-   ##
==========================================
+ Coverage   39.27%   39.30%   +0.02%     
==========================================
  Files         284      284              
  Lines       22379    22393      +14     
==========================================
+ Hits         8789     8801      +12     
- Misses      12643    12645       +2     
  Partials      947      947              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aryan9600
Copy link
Member

aryan9600 commented Jan 6, 2025

could you share an example here? have you tested this change with a custom resource?

@kahirokunn
Copy link
Contributor Author

@aryan9600 Thank you for your response and for taking the time to review the proposal.

Based on my review of Flagger’s codebase, it appears that there are currently no custom resources that manage workloads in Flagger. However, with the pull request at fluxcd/flagger#1682 (introducing Knative support), we see the first concrete example of such a resource.

In the case of Knative, the existing KubernetesDefaultRouter becomes unnecessary because Knative already handles certain routing aspects on its own. I believe Knative is just the first step, and Flagger has enormous potential to integrate with other projects as well—such as combining Flagger with KEDA (for HTTP autoscaling) or other CNCF serverless initiatives. In drafting these features, I discovered that the Service created by KubernetesDefaultRouter can collide with the Services generated by a custom resource. That risk leads me to believe that expanding Flagger’s ability to handle custom resources in a more granular, configurable manner is important for unlocking its full potential.

Regarding testing, this feature has not yet been fully validated with a custom resource, simply because Flagger does not include any custom resources by default. Both I and the author of the Knative support PR would like to see Flagger gain the ability to handle additional custom resources in the future. For that reason, allowing .spec.targetRef to point to custom resources—and ensuring we avoid unnecessary conflicts with existing Flagger routers—seems like a worthwhile enhancement.

I appreciate your feedback and look forward to working together to refine this approach. If you have further suggestions or would like to discuss testing strategies in more detail, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants