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
Remove global variables, pass context across all components and improve error handling #603
Conversation
pkg/controller/controller.go
Outdated
"k8s.io/apimachinery/pkg/api/meta" | ||
"k8s.io/client-go/dynamic" | ||
"k8s.io/client-go/dynamic/dynamicinformer" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put 3rd party and local packages into separate groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like if imports are sorted. Currently, imports are messed up in almost all files. Unfortunately, the goimport
doesn't handle well the situation where the groups are created “manually” (separated by new line).
It will be good to find and integrate some tool which does it. I found only one that does it in the correct way: https://github.com/incu6us/goimports-reviser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the following command it went through all files, but didn't change anything:
for file in $(find . -name '*.go'); do
goimports-reviser -file-path $file -rm-unused -set-alias -format -local github.com/infracloudio/botkube -project-name github.com/infracloudio/botkube -list-diff
done
I didn't do any newlines in imports manually. I disabled "Optimize imports on the fly" in IntellIJ, maybe that'll help.
I will fix the imports manually for now.
EDIT: Fixed
Fixes: #568 |
@pkosiec is it possible to break the PR into multiple? Its becomes very difficult to review all at once :) |
Hi @PrasadG193,
|
Okay.
My bad. I didn't check the description |
Update: I noticed a crash when a custom Kubeconfig wasn't specified (last-minute changes, which I tested only locally). Now it is fixed on the latest commit 0e2e762. Sorry! Use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only checked the code. When changes will be applied, I will test that e2e 👍
pkg/controller/controller.go
Outdated
"k8s.io/apimachinery/pkg/api/meta" | ||
"k8s.io/client-go/dynamic" | ||
"k8s.io/client-go/dynamic/dynamicinformer" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like if imports are sorted. Currently, imports are messed up in almost all files. Unfortunately, the goimport
doesn't handle well the situation where the groups are created “manually” (separated by new line).
It will be good to find and integrate some tool which does it. I found only one that does it in the correct way: https://github.com/incu6us/goimports-reviser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested PR changes with:
- Slack
- Mattermost
- Discord
- MS Teams - will check on Fix MS Teams integration #607
Testing scenario:
- install with
kubectl
enabled, play with kubectl commands - create Pod with latest tag
kubectl run nginx --image=nginx
to check recommendations - play with k8s events create/scale/delete deploy
- disable/enable notifications
However, there are two other integration:
- ElasticSearch
- Webhook
are we going to test them too?
Thanks @mszostok. I tested both with the following scenarios: ElasticsearchInstall Elasticsearch: helm repo add elastic https://helm.elastic.co
helm install elasticsearch elastic/elasticsearch --set replicas=1 --set resources.requests.cpu="100m" --set resources.requests.memory="512M" --wait Create cat > /tmp/values.yaml << ENDOFFILE
communications:
elasticsearch:
enabled: true
server: http://elasticsearch-master.default:9200
username: elastic
appPassword: changeme
config:
settings:
clustername: sample
kubectl:
enabled: true
enabled: true
image:
registry: docker.io
repository: pkosiec/botkube
tag: remove-global-vars-v3
ENDOFFILE Install BotKube: helm install botkube --namespace botkube ./helm/botkube -f /tmp/values.yaml --wait Do port forward: kubectl port-forward svc/elasticsearch-master 9200 Imitate pod failure: kubectl delete po -n kube-system metrics-server-<press TAB> See Elasticsearch indices: curl http://localhost:9200/_cat/indices Copy the index name with See Elasticsearch index details: INDEX_NAME="botkube-2022-06-06"
curl http://localhost:9200/$INDEX_NAME/_search\?pretty See logged events. Click to expand{
"took" : 9,
"timed_out" : false,
"_shards" : {
"total" : 1,
"successful" : 1,
"skipped" : 0,
"failed" : 0
},
"hits" : {
"total" : {
"value" : 12,
"relation" : "eq"
},
"max_score" : 1.0,
"hits" : [
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "RNifOIEBzVn4jNmi6l97",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "v1/pods error",
"Kind" : "Pod",
"Name" : "elasticsearch-master-2",
"Namespace" : "default",
"Messages" : [
"0/1 nodes are available: 1 node(s) didn't match pod anti-affinity rules."
],
"Type" : "error",
"Reason" : "FailedScheduling",
"Error" : "",
"Level" : "error",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "0001-01-01T00:00:00Z",
"Count" : 0,
"Action" : "Scheduling",
"Resource" : "v1/pods",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "Q9ifOIEBzVn4jNmi6V9D",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "v1/pods error",
"Kind" : "Pod",
"Name" : "elasticsearch-master-1",
"Namespace" : "default",
"Messages" : [
"0/1 nodes are available: 1 node(s) didn't match pod anti-affinity rules."
],
"Type" : "error",
"Reason" : "FailedScheduling",
"Error" : "",
"Level" : "error",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "0001-01-01T00:00:00Z",
"Count" : 0,
"Action" : "Scheduling",
"Resource" : "v1/pods",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "RdifOIEBzVn4jNmi8F-z",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "v1/pods error",
"Kind" : "Pod",
"Name" : "elasticsearch-master-2",
"Namespace" : "default",
"Messages" : [
"0/1 nodes are available: 1 node(s) didn't match pod anti-affinity rules."
],
"Type" : "error",
"Reason" : "FailedScheduling",
"Error" : "",
"Level" : "error",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "0001-01-01T00:00:00Z",
"Count" : 0,
"Action" : "Scheduling",
"Resource" : "v1/pods",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "RtifOIEBzVn4jNmi8l9B",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "v1/pods error",
"Kind" : "Pod",
"Name" : "elasticsearch-master-1",
"Namespace" : "default",
"Messages" : [
"0/1 nodes are available: 1 node(s) didn't match pod anti-affinity rules."
],
"Type" : "error",
"Reason" : "FailedScheduling",
"Error" : "",
"Level" : "error",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "0001-01-01T00:00:00Z",
"Count" : 0,
"Action" : "Scheduling",
"Resource" : "v1/pods",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "R9ifOIEBzVn4jNmi_V_3",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "v1/pods error",
"Kind" : "Pod",
"Name" : "elasticsearch-master-2",
"Namespace" : "default",
"Messages" : [
"skip schedule deleting pod: default/elasticsearch-master-2"
],
"Type" : "error",
"Reason" : "FailedScheduling",
"Error" : "",
"Level" : "error",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "0001-01-01T00:00:00Z",
"Count" : 0,
"Action" : "Scheduling",
"Resource" : "v1/pods",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "SNifOIEBzVn4jNmi_1-I",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "v1/pods error",
"Kind" : "Pod",
"Name" : "elasticsearch-master-1",
"Namespace" : "default",
"Messages" : [
"skip schedule deleting pod: default/elasticsearch-master-1"
],
"Type" : "error",
"Reason" : "FailedScheduling",
"Error" : "",
"Level" : "error",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "0001-01-01T00:00:00Z",
"Count" : 0,
"Action" : "Scheduling",
"Resource" : "v1/pods",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "SdigOIEBzVn4jNmiol9o",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "v1/pods created",
"Kind" : "Pod",
"Name" : "metrics-server-ff9dbcb6c-wj2mv",
"Namespace" : "kube-system",
"Messages" : null,
"Type" : "create",
"Reason" : "",
"Error" : "",
"Level" : "info",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "2022-06-06T10:47:40Z",
"Count" : 0,
"Action" : "",
"Resource" : "v1/pods",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "StigOIEBzVn4jNmiol_a",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "apps/v1/deployments updated",
"Kind" : "Deployment",
"Name" : "metrics-server",
"Namespace" : "kube-system",
"Messages" : null,
"Type" : "update",
"Reason" : "",
"Error" : "",
"Level" : "warn",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "2022-06-06T10:47:40.475935655Z",
"Count" : 0,
"Action" : "",
"Resource" : "apps/v1/deployments",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "S9igOIEBzVn4jNmio18O",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "apps/v1/deployments updated",
"Kind" : "Deployment",
"Name" : "metrics-server",
"Namespace" : "kube-system",
"Messages" : null,
"Type" : "update",
"Reason" : "",
"Error" : "",
"Level" : "warn",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "2022-06-06T10:47:40.5422313Z",
"Count" : 0,
"Action" : "",
"Resource" : "apps/v1/deployments",
"Recommendations" : null,
"Warnings" : null
}
},
{
"_index" : "botkube-2022-06-06",
"_type" : "botkube-event",
"_id" : "TNigOIEBzVn4jNmiqF9x",
"_score" : 1.0,
"_source" : {
"Code" : "",
"Title" : "v1/pods error",
"Kind" : "Pod",
"Name" : "metrics-server-ff9dbcb6c-wj2mv",
"Namespace" : "kube-system",
"Messages" : [
"Readiness probe failed: HTTP probe failed with statuscode: 500"
],
"Type" : "error",
"Reason" : "Unhealthy",
"Error" : "",
"Level" : "error",
"Cluster" : "sample",
"Channel" : "",
"TimeStamp" : "2022-06-06T10:47:41Z",
"Count" : 1,
"Action" : "",
"Resource" : "v1/pods",
"Recommendations" : null,
"Warnings" : null
}
}
]
}
} WebhookInstall echo Webhook server: helm repo add ealenn https://ealenn.github.io/charts
helm install echo-server ealenn/echo-server --set application.logs.ignore.ping=true --set application.enable.environment=false --wait Create cat > /tmp/values.yaml << ENDOFFILE
communications:
webhook:
enabled: true
url: http://echo-server.default
config:
settings:
clustername: sample
kubectl:
enabled: true
enabled: true
image:
registry: docker.io
repository: pkosiec/botkube
tag: remove-global-vars-v3
ENDOFFILE Install BotKube: helm install botkube --namespace botkube ./helm/botkube -f /tmp/values.yaml --wait Watch the logs:
Imitate pod failure: kubectl delete po -n kube-system metrics-server-<press TAB> See the logs from echo-server. Result: Click to expand{"name":"echo-server","hostname":"echo-server-8696cffbd9-tdwhk","pid":1,"level":30,"host":{"hostname":"echo-server.default","ip":"::ffff:10.42.0.125","ips":[]},"http":{"method":"POST","baseUrl":"","originalUrl":"/","protocol":"http"},"request":{"params":{},"query":{},"cookies":{},"body":{"meta":{"kind":"Pod","name":"metrics-server-ff9dbcb6c-5qj8v","namespace":"kube-system","cluster":"sample"},"status":{"type":"error","level":"error","reason":"Unhealthy","messages":["Readiness probe failed: Get \"https://10.42.0.128:4443/readyz\": dial tcp 10.42.0.128:4443: connect: connection refused"]},"summary":"Error Occurred in Pod: *kube-system/metrics-server-ff9dbcb6c-5qj8v* in *sample* cluster\n```\nReadiness probe failed: Get \"https://10.42.0.128:4443/readyz\": dial tcp 10.42.0.128:4443: connect: connection refused\n```","timestamp":"2022-06-06T11:16:20Z"},"headers":{"host":"echo-server.default","user-agent":"Go-http-client/1.1","content-length":"572","content-type":"application/json","accept-encoding":"gzip"}},"msg":"Mon, 06 Jun 2022 11:16:20 GMT | [POST] - http://echo-server.default/","time":"2022-06-06T11:16:20.456Z","v":0}
{"name":"echo-server","hostname":"echo-server-8696cffbd9-tdwhk","pid":1,"level":30,"host":{"hostname":"echo-server.default","ip":"::ffff:10.42.0.125","ips":[]},"http":{"method":"POST","baseUrl":"","originalUrl":"/","protocol":"http"},"request":{"params":{},"query":{},"cookies":{},"body":{"meta":{"kind":"Pod","name":"metrics-server-ff9dbcb6c-csvw4","namespace":"kube-system","cluster":"sample"},"status":{"type":"delete","level":"critical"},"summary":"Pod *kube-system/metrics-server-ff9dbcb6c-csvw4* has been deleted in *sample* cluster\n","timestamp":"2022-06-06T11:16:19Z"},"headers":{"host":"echo-server.default","user-agent":"Go-http-client/1.1","content-length":"289","content-type":"application/json","accept-encoding":"gzip"}},"msg":"Mon, 06 Jun 2022 11:16:21 GMT | [POST] - http://echo-server.default/","time":"2022-06-06T11:16:21.450Z","v":0}
{"name":"echo-server","hostname":"echo-server-8696cffbd9-tdwhk","pid":1,"level":30,"host":{"hostname":"echo-server.default","ip":"::ffff:10.42.0.125","ips":[]},"http":{"method":"POST","baseUrl":"","originalUrl":"/","protocol":"http"},"request":{"params":{},"query":{},"cookies":{},"body":{"meta":{"kind":"Pod","name":"metrics-server-ff9dbcb6c-5qj8v","namespace":"kube-system","cluster":"sample"},"status":{"type":"error","level":"error","reason":"Unhealthy","messages":["Readiness probe failed: HTTP probe failed with statuscode: 500"]},"summary":"Error Occurred in Pod: *kube-system/metrics-server-ff9dbcb6c-5qj8v* in *sample* cluster\n```\nReadiness probe failed: HTTP probe failed with statuscode: 500\n```","timestamp":"2022-06-06T11:16:21Z"},"headers":{"host":"echo-server.default","user-agent":"Go-http-client/1.1","content-length":"458","content-type":"application/json","accept-encoding":"gzip"}},"msg":"Mon, 06 Jun 2022 11:16:21 GMT | [POST] - http://echo-server.default/","time":"2022-06-06T11:16:21.526Z","v":0}} |
…ve error handling
… details from logs #610 ##### ISSUE TYPE - Feature Pull Request - Bug fix Pull Request - Docs Pull Request ##### SUMMARY ~⚠️ **NOTE:** For convienence, this PR depends on #603 and needs to be rebased when #603 is merged (it should happen very soon). Actual changes: a6d91f21f25f00d679572e62b45cd2396d571075~ As the upstream fix has been merged (slack-go/slack#1067), this PR updates `slack-go/slack` dependency and removes no longer needed workaround. See the original issue: #579 See the PR that introduced the workaround: #583 ##### TESTING > **NOTE:** You can also use `pkosiec/botkube:rm-slack-workaround` Docker image for testing. The image was repushed **after** rebase. 1. Checkout this PR 2. Edit `comm_config.yaml` and enable Slack 3. Export needed envs: ```bash export KUBECONFIG=/Users/$USER/.kube/config export LOG_LEVEL=info ``` 4. Run BotKube locally: ```bash go run ./cmd/botkube/main.go ``` 5. Start Slack huddle and say something, then leave 6. Observe logs still without event details
##### ISSUE TYPE - Bug fix Pull Request ##### SUMMARY ~⚠️ **NOTE:** This PR depends on #603 and needs to be rebased when #603 is merged. Actual changes: da0a7befa39b9e3b68f81734e428d584b6ec9b8f~ This PR fixes Teams and possibly also Lark integrations. Since v0.12.4 MS Teams HTTP server didn't handle requests properly because of 404 errors. It was caused by using default HTTP handlers configuration, which was changed in #603. However, #603 still didn't solve the issue fully, as the servers still didn't handle proper prefixes. This PR introduces https://github.com/gorilla/mux which can handle paths by prefix. Please see the details: #606 (comment) Fixes #606 ##### TESTING Checkout this PR and install BotKube: ```bash cat > /tmp/values.yaml << ENDOFFILE communications: teams: enabled: true appID: 'dbf68a92-26a0-4305-8e9b-8616500abf2f' appPassword: 'abc' config: settings: clustername: sample kubectl: enabled: true image: registry: docker.io repository: pkosiec/botkube tag: fix-bot-srvs # this image was repushed after rebase ENDOFFILE helm install botkube --namespace botkube ./helm/botkube -f /tmp/values.yaml --wait ``` Forward port: ```bash kubectl port-forward -n botkube svc/botkube 3978 ``` And test these calls: ```bash curl http://localhost:3978/metrics # not anymore, metrics are now accessible only under 2112 port as they should curl http://localhost:3978/bots/teams # it works curl http://localhost:3978/bots/teams/v1/messages # it works ``` You can also edit service to check the metrics server (you can also do it from Helm chart configuration, but the ServiceMonitor CRD needs to be available in the cluster): ```bash kubectl edit svc -n botkube botkube ``` ```yaml spec: # (...) ports: # (...) - name: metrics port: 2112 targetPort: 2112 ``` Forward port: ```bash kubectl port-forward -n botkube svc/botkube 2112 ``` And test these calls: ```bash curl http://localhost:2112/metrics # not anymore, metrics are now accessible only under 2112 port as they should curl http://localhost:2112/bots/teams # doesn't work anymore ``` Unfortunately, I couldn't test Lark integration as there's no instruction how to set it up, and I couldn't start the server with invalid/empty credentials.
ISSUE TYPE
SUMMARY
This is just a start, as later (during new features development) we should refactor each component/package in isolation. But after this huge PR it should be possible to do it.
Fixes #220
TODO
After first review of this PR if the new filter engine approach is accepted:
TESTING
Tested manually with Mattermost, Discord and Slack.
You can use the following image:
pkosiec/botkube:remove-global-vars-v2