vmware-archive / kafka-trigger

Kubernetes CRD controller for Kafka topic as event source for Kubeless functions
Apache License 2.0
28 stars 34 forks source link

Default k8s services domains are hardcoded in trigger, can`t specify it in config #22

Open mgolovatiy-atconsulting-ru opened 4 years ago

mgolovatiy-atconsulting-ru commented 4 years ago

Is this a BUG REPORT or FEATURE REQUEST?: bug

What happened: After kafka message put into queue, kafka-trigger-controller pod shows error in logs

time="2020-02-20T16:23:07Z" level=error msg="Failed to send message to function: Post http://nodejs-kafka-consumer-sample.<test_namespace_here>.svc.cluster.local:8080: dial tcp: lookup nodejs-kafka-consumer-sample.<test_namespace_here>.svc.cluster.local on 10.233.0.3:53: no such host"

What you expected to happen: Trigger send message to function pod nodejs-kafka-consumer-sample, can see logged event in its logs.

May be this domain should be got from configMap (kubeless-config?).

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?: kubelet run with --cluster-domain=k8s.test option Sources have hardcoded literal with domain: event_sender.go: 57

 fmt.Sprintf("http://%s.%s.svc.cluster.local:%s", funcName, namespace, funcPort)

Seems to be bug.

Environment:

andresmgot commented 4 years ago

Hi @mgolovatiy-atconsulting-ru,

Thanks for reporting the issue. You mention that the cluster domain has been changed in your case. What would be the correct URL for your function? (Rather than http://%s.%s.svc.cluster.local:%s)

mgolovatiy-atconsulting-ru commented 4 years ago

svc.cluster.local -> k8s.test that specified as cluster-domain param in kube. So correct solution should be fmt.Sprintf("http://%s.%s.%s:%s", funcName, namespace, kubeDomain, funcPort) where kubeDomain should be configured in configMap or another place.

Or, may be, should query domain via k8s api, don`t know if it is possible. If i know go, i could make PR as soon as noticed this.

andresmgot commented 4 years ago

In theory the .svc is not part of the domain so http://%s.%s.svc.%s:%s", funcName, namespace, kubeDomain, funcPort) should work for you as well.

I see that parameter available in the kubelet configmap present in the kube-system namespace but it seems a bit difficult to retrieve. Probably it's better to configure it just as a flag for the kafka-trigger-controller deployment.

Marking this up for grabs since I don't have the bandwidth to work on this right now. Happy to help to anyone to want to give this a try.

mgolovatiy-atconsulting-ru commented 4 years ago

@andresmgot I suggest a solution without introducing an enviroment variable:

  1. kubeless-> add getClusterDomain function (in utils.k8sutil) - shall it be usefull?

  2. kafka-trigger -> add the same function in utils.k8sutil (is it another package?)

    func getClusterDomain() string {
    apiSvc := "kubernetes.default.svc"
    
    clusterDomain := "cluster.local"
    
    cname, err := net.LookupCNAME(apiSvc)
    if err != nil {
        return clusterDomain
    }
    
    clusterDomain = strings.TrimPrefix(cname, apiSvc)
    clusterDomain = strings.TrimSuffix(clusterDomain, ".")
    
    return clusterDomain
    }

    Seems to be pretty simple.

  3. kafka-trigger -> fix event_sender.GetHTTPReq :

    req, err := http.NewRequest(method, fmt.Sprintf("http://%s.%s.svc.%s:%s", funcName, namespace, getClusterDomain(), funcPort), strings.NewReader(body))

Don't have unix, so cant test well, even can't run makefile.

Need CR due to my zero go-expirence

andresmgot commented 4 years ago

Comments in the PR. I think it's easier to leave this code in kafka trigger for the moment (no need to move it kubeless core).

mgolovatiy-atconsulting-ru commented 4 years ago

Couldn`t find in which repo new image was put, wish to use it with my test enviroment. Can i use some snapshot or only must wait for stable version?

andresmgot commented 4 years ago

I have a working environment so I have built the image for you, you can use it as andresmgot/kafka-trigger-controller:mgolovatiy-atconsulting-ru and test it

mgolovatiy-atconsulting-ru commented 4 years ago

Thank u

mgolovatiy-atconsulting-ru commented 4 years ago

My solution doesn't work, have the same error. Don't understand why net.LookupCNAME("kubernetes.default.svc") returns wrong domain.

This question seems to be raised long before: https://github.com/kubeless/kubeless/issues/832 I think it is a good idea to specify domain on creating function or trigger. May be it should be reopened?

andresmgot commented 4 years ago

Sorry to hear that.

I think it is a good idea to specify domain on creating function or trigger. May be it should be reopened?

Well, it's not related to a specific function or trigger but to a cluster. I think this should be a flag for the trigger controller just as it was started in https://github.com/kubeless/kubeless/pull/857.

I think you are close with your implementation, you just need to use a command flag instead of trying to auto-resolve the domain.

andresmgot commented 4 years ago

closed by mistake

mgolovatiy-atconsulting-ru commented 4 years ago

May be PR https://github.com/kubeless/kubeless/pull/857 is good enough? I only don't like empty param check (if clusterDomain == ""), should not it be checked to null too?

mgolovatiy-atconsulting-ru commented 4 years ago

I wish to test kubeless image with PR #857 changes in my enviroment

andresmgot commented 4 years ago

857 is not syntactically correct (it doesn't compile) so it's not usable as it is