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

Pluggable discovery #146

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Pluggable discovery #146

wants to merge 2 commits into from

Conversation

mrbcm
Copy link

@mrbcm mrbcm commented Feb 5, 2021

Fixes issue #144

Currently memcache_rpc.go is tightly coupled with multicast discovery. This commit decouples RPC and discovery and provides a simple pluggable implementation for discovery.

  1. Discovery strategy can be configured at runtime using startup parameter: "discovery-strategy".
  2. Valid values are: vFlowDiscovery, k8sDiscovery.rest
  3. vFlowDiscovery is the default and is used when no config parameter is provided.
  4. Configuration parameters for other discovery mechanisms are provided in discovery.conf file, which should be available in root configuration directory
  5. Sample configuration for k8s is below
  6. In k8s, pods must have permissions to fetch pods in a namespace. Sample RBAC config for such is pasted below

---- discovery.conf ----
k8s-service-name: vflow-dev
k8s-discovery-poll-interval: 10

----- k8s RBAC.yaml ----
kind: ClusterRole
metadata:
name: vflow-cluster-role
rules:

  • apiGroups:
    • ""
      resources:
    • endpoints
    • pods
    • nodes
    • services
      verbs:
    • get
    • list

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: vflow-cluster-role-binding
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: vflow-cluster-role
subjects:

  • kind: ServiceAccount
    name: flowsvcacct-2
    namespace: default

Add a simple plugin based discovery stragegy
Add implmentation for k8s REST API based discovery
@mehrdadrad
Copy link
Collaborator

@mrbcm thanks for your pull request, I'll review it soon!

@mrbcm
Copy link
Author

mrbcm commented Feb 19, 2021

@mehrdadrad Any update on this PR? We have more commits to follow this regarding DNS based discovery in k8s. Wanted to get initial pluggable discovery mechanism in so as not to have a huge PR.

@mehrdadrad
Copy link
Collaborator

Thank you for the PR and sorry for the delay.
My main concern about this PR is instead of calling k8s api directly, it should be through kubernetes/client-go library and informer. Also I think the vFlow image name is a good indicator to find the vFlow pods. In this case we can have two types of discovery (multicast or k8s) and it can be configured in the main vFlow config file. what do you think?

@mrbcm
Copy link
Author

mrbcm commented Mar 1, 2021

Thank you for the review and comments.

I agree with you that client-go client library is probably better and was in fact my first implementation. But I did not want to make a general dependency for a k8s only deployment. So I went with a simpler rest client dependency. This does add additional deps, but a rest client can be used in other use-cases to extend vflow as well. (ex: download whitelist/blacklist maybe?). If you feel that go client is still better, I can update this PR.

As for having a simpler discovery config, I do see additional discovery modes in k8s, 1) using DNS based discovery. This relies on a k8s feature where dns query for a headless service resolves to set of IP addresses of the pods. This mode may be useful in scenarios where required RBAC permissions are not granted. 2) Custom discovery protocols.

Please let me know what you think and thanks.

@mehrdadrad
Copy link
Collaborator

Once we use k8s client-go then the binary size goes up significantly.
Honestly, this feature is not priority and I haven't seen anyone else to request this feature but If you want to rewrite your pr please go w/ k8s client-go. I believe it doesn't need to implement for different discovery modes. image name is the best indicator and it can use the global conf file (discovery: multicast/k8s/disable)

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.

2 participants