search

vmware-archive / http-trigger

Kubernetes CRD controller for http invocation of Kubeless functions
Apache License 2.0
9 stars 27 forks source link

works #14

Open hjanuschka opened 3 years ago

hjanuschka commented 3 years ago

fixes: https://github.com/kubeless/kubeless/issues/1130

tested it against 1.19.2 tbh, it is my first try at golang, and k8s ecosystem, it works if i build it - and modify the image of the trigger-controller.

i think the main dep. that fails is: k8s.io/client-go v7.0.0+incompatible

let me know how to proceed

edit: also not sure if it fixes 1.19 and broke all others :/, but i can now use serverless framework with http triggers in prod 🍺

hjanuschka commented 3 years ago

thx, i ll give the mentioned commit a look.

hjanuschka commented 3 years ago

python change pushed - hope that it fixes the tests.

Unrelated to your PR, we should probably update Go version for not having to include vendor changes, but that's probably for other PR.

not sure how to do,is there a similar repo, i can take a look and give it a try?

hjanuschka commented 3 years ago

can you merge this ?

andresmgot commented 3 years ago

There is still this change pending: https://github.com/kubeless/http-trigger/pull/14/files/df349b0c79fd06a9a6864cbc9167515c5fa37e69#r499471515 (we need to use utils.GetInClusterConfig to avoid regressions).

Also @sepetrov is working in a similar PR here: https://github.com/kubeless/http-trigger/pull/15/files The difference is that in that PR, we are updating the Go version to 1.15, allowing to remove all the changes in vendor/. Maybe we can just merge that one once it's ready.

sepetrov commented 3 years ago

I still need to debug some issues in https://github.com/kubeless/http-trigger/pull/15 - minikube removed the default http backend in v1.12.0. I suspect this might be causing issues with the ingresses created by the HTTP trigger controller. Will need to look into this later when I have some free time.

andresmgot commented 3 years ago

minikube removed the default http backend in v1.12.0.

It may be easier to use the nginx ingress chart.

sepetrov commented 3 years ago

minikube removed the default http backend in v1.12.0.

It may be easier to use the nginx ingress chart.

@andresmgot, if I understand correctly, you are suggesting using https://github.com/kubernetes/ingress-nginx/blob/controller-v0.40.2/deploy/static/provider/cloud/deploy.yaml#L340. Unfortunately the flag --default-backend-service and the default backend service were removed.

andresmgot commented 3 years ago

well, I was talking about the nginx chart: https://hub.kubeapps.com/charts/nginx/nginx-ingress but I forgot that we don't have helm in the CI system so it may be more complicated to install it.

faxioman commented 3 years ago

@andresmgot what about generating the yaml manifests using the the helm chart end put these manifests in /tests? Could be an acceptable way?

andresmgot commented 3 years ago

well, I think it'd easier and more readable to just download the helm binary and install the chart from the URL (e.g. helm install mynginx https://example.com/charts/nginx-1.2.3.tgz).

faxioman commented 3 years ago

@andresmgot @sepetrov the problem is related to the different output format for:

kubectl get ingress

A new column "ingress class" is added when using k8s 1.18. Changing script/libtest.bash from:

kubectl get ingress | grep $func | grep "$domain" | awk '{print $3}' | grep "$ip"

to

kubectl get ingress | grep $func | grep "$domain" | awk '{print $4}' | grep "$ip"

solves the problem ... but breaks 1.17 compatibility. I'm changing this script formatting the output using kubectl instead of grepping/awkapping.

So we can continue to rely on minikube ingress addon.

faxioman commented 3 years ago

I've opened a pull request to @sepetrov fork with normalized colums for command "kubectl get ingress". I was unable to get rid of awk/grep ... but, anyway, it works :)