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

Use configmap to configure the data source of filter framework #749

Merged

Conversation

yingjianjian
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

Use configmap to configure the data source of filter framework

Which issue(s) this PR fixes:

Fixes #699

@openyurt-bot openyurt-bot added the size/L size/L: 100-499 label Feb 26, 2022
@yingjianjian yingjianjian force-pushed the OptimizeFilterFramework branch 15 times, most recently from da14338 to c3878c9 Compare February 26, 2022 08:26
@openyurt-bot openyurt-bot added size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Feb 26, 2022
@yingjianjian yingjianjian force-pushed the OptimizeFilterFramework branch 4 times, most recently from d3f9934 to 228494c Compare February 26, 2022 15:39
@rambohe-ch
Copy link
Member

@yingjianjian Very appreciate for making pull request of #699 , at first please merge two commits into one commit.

@yingjianjian yingjianjian force-pushed the OptimizeFilterFramework branch from 228494c to 8ab3960 Compare February 27, 2022 03:53
@yingjianjian
Copy link
Member Author

Merged

@rambohe-ch
Copy link
Member

/assign @zzguang

@yingjianjian yingjianjian force-pushed the OptimizeFilterFramework branch from ab9f871 to e61b1e8 Compare March 21, 2022 08:34
@yingjianjian
Copy link
Member Author

fixed

@yingjianjian yingjianjian force-pushed the OptimizeFilterFramework branch 2 times, most recently from 57d3377 to 95b8d65 Compare March 21, 2022 09:38
@rambohe-ch
Copy link
Member

@zzguang PTAL

@yingjianjian yingjianjian force-pushed the OptimizeFilterFramework branch from 95b8d65 to bb5e972 Compare March 21, 2022 10:02
@openyurt-bot openyurt-bot added the approved approved label Mar 21, 2022
@zzguang
Copy link
Member

zzguang commented Mar 21, 2022

Generally, it looks good to me, but I have a suggestion I am not sure whether it makes sense:
defaultFilterCfg = map[string]string{
MasterServiceFilterName: "kubelet/services#list;watch",
EndpointsFilterName: "nginx-ingress-controller/endpoints#list;watch",
DiscardCloudServiceFilterName: "kube-proxy/services#list;watch",
ServiceTopologyFilterName: "kube-proxy/endpointslices#list;watch",
}
For the current FilterNames, MasterServiceFilterName and DiscardCloudServiceFilterName are both services filtering for different components, how about combining them together into a single "ServicesFilterName", then in the filter manager, it checks all the components that registers the service resource to decide whether to do the services filtering operation. This aims to de-couple the resources and components completely, so that if a new component wants to add filtering for a current resource in future, it won't need to add new filter name any more.
If the suggestion is reasonable, the filter names will be named clearly by the resources names, and the current filter names can be refined like below:
ServicesFilterName: "kubelet/services#list;watch",
ServicesFilterName: "kube-proxy/services#list;watch",
EndpointsFilterName: "nginx-ingress-controller/endpoints#list;watch",
EndpointslicesFilterName: "kube-proxy/endpointslices#list;watch",

This suggestion is just a reference for you, just ignore it if I am wrong, thanks!

@rambohe-ch
Copy link
Member

rambohe-ch commented Mar 22, 2022

Generally, it looks good to me, but I have a suggestion I am not sure whether it makes sense: defaultFilterCfg = map[string]string{ MasterServiceFilterName: "kubelet/services#list;watch", EndpointsFilterName: "nginx-ingress-controller/endpoints#list;watch", DiscardCloudServiceFilterName: "kube-proxy/services#list;watch", ServiceTopologyFilterName: "kube-proxy/endpointslices#list;watch", } For the current FilterNames, MasterServiceFilterName and DiscardCloudServiceFilterName are both services filtering for different components, how about combining them together into a single "ServicesFilterName", then in the filter manager, it checks all the components that registers the service resource to decide whether to do the services filtering operation. This aims to de-couple the resources and components completely, so that if a new component wants to add filtering for a current resource in future, it won't need to add new filter name any more. If the suggestion is reasonable, the filter names will be named clearly by the resources names, and the current filter names can be refined like below: ServicesFilterName: "kubelet/services#list;watch", ServicesFilterName: "kube-proxy/services#list;watch", EndpointsFilterName: "nginx-ingress-controller/endpoints#list;watch", EndpointslicesFilterName: "kube-proxy/endpointslices#list;watch",

This suggestion is just a reference for you, just ignore it if I am wrong, thanks!

@zzguang I think we need to name filter based on the function of filter, not the resource of filter. so it's a good idea to keep MasterServiceFilterName and DiscardCloudServiceFilterName unchanged, because only from the name meaning of filters end users can decide to use this filter or not. if we combine these two meaningful filters into service filter, i think that end user will feel confused.

btw: based on the function of filters, i think we can merge endpoints filter for ingress controller into serviceTopology filter for traffic control. and we can optimize this merge in another pull request.

@zzguang
Copy link
Member

zzguang commented Mar 22, 2022

Okay, I just feel a little confusing about the filter naming rule, if we name it by the feature name, then the "EndpointsFilterName" doesn't follow this rule, and if another feature/component wants to add endpoints filtering in future, it may lead to some un-consistent concepts.
As you say, I think we can merge it at current stage, and optimize it in another PR in future.

@rambohe-ch
Copy link
Member

/lgtm
/approve

@openyurt-bot openyurt-bot added the lgtm lgtm label Mar 22, 2022
@openyurt-bot openyurt-bot merged commit 6390e6b into openyurtio:master Mar 22, 2022
@openyurt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rambohe-ch, yingjianjian

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rambohe-ch
Copy link
Member

Fixes #773

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved approved lgtm lgtm size/XL size/XL: 500-999
Projects
None yet
4 participants