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

Pass communication settings as a k8s secret #226

Merged
merged 10 commits into from Dec 18, 2019

Conversation

slalwani97
Copy link
Contributor

@slalwani97 slalwani97 commented Dec 2, 2019

Fixes #211

Signed-off-by: Sumit Lalwani sumit.lalwani97@gmail.com

slalwani97 added 2 commits Dec 2, 2019
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
@slalwani97 slalwani97 changed the title Add secret Pass communication settings as a k8s secret Dec 2, 2019
@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Dec 3, 2019

Thanks for the PR @slalwani97!
Could you please also update helm chart (https://github.com/infracloudio/botkube/tree/develop/helm/botkube) and all-in-one-tls.yaml (https://github.com/infracloudio/botkube/blob/develop/deploy-all-in-one-tls.yaml)?

You also need split config file we use for e2e test (https://github.com/infracloudio/botkube/tree/develop/test) to make CI build happy

slalwani97 added 2 commits Dec 3, 2019
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
@slalwani97
Copy link
Contributor Author

@slalwani97 slalwani97 commented Dec 3, 2019

Sure, I have updated the PR. Please have a look. @PrasadG193

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Dec 9, 2019

LGTM

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Dec 9, 2019

Just a minor suggestion @slalwani97 ,
Can we split config.yaml into - resource_config.yaml and comm_config.yaml
What do you think?

@slalwani97
Copy link
Contributor Author

@slalwani97 slalwani97 commented Dec 9, 2019

Do you mean to change the name of the yaml files, right?
i.e config.yaml -> resource-config.yaml and communication.yaml -> comm_config.yaml

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Dec 9, 2019

@slalwani97 Yes

slalwani97 added 3 commits Dec 9, 2019
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
@slalwani97
Copy link
Contributor Author

@slalwani97 slalwani97 commented Dec 9, 2019

Sure @PrasadG193 , I have updated the names with some variable names as well. I have a few doubts:

  1. configWatcher func in the controller.go - uses config file, also adds the config file (now resource_config) in the watcher. So is comm_config file should also be added in watcher?
  2. showControllerConfig func in the executor.go - uses config file too, and reads it. Should the comm_config file be read here as well?
    Probably these lines will break I think:
    // hide sensitive info
    c.Communications.Slack.Token = ""
    c.Communications.ElasticSearch.Password = ""
    

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Dec 11, 2019

Thanks @slalwani97. Could you please post the helm install command? Just wanted to check if there is any change in that

@PrasadG193 PrasadG193 requested a review from sanketsudake Dec 11, 2019
@slalwani97
Copy link
Contributor Author

@slalwani97 slalwani97 commented Dec 12, 2019

@PrasadG193

helm install --version v0.9.1 --name botkube --namespace botkube \
--set communicationConfig.communications.slack.enabled=true \
--set communicationConfig.communications.slack.channel=<SLACK_CHANNEL_NAME> \
--set communicationConfig.communications.slack.token=<SLACK_API_TOKEN_FOR_THE_BOT> \
--set resourceConfig.settings.clustername=<CLUSTER_NAME> \
--set resourceConfig.settings.allowkubectl=<ALLOW_KUBECTL> \
--set image.repository=infracloudio/botkube \
--set image.tag=v0.9.1 \
infracloudio/botkube

@PrasadG193 PrasadG193 requested a review from bhavin192 Dec 14, 2019
upgradeNotifier: true

communicationConfig:
# Communication settings
communications:
Copy link
Member

@PrasadG193 PrasadG193 Dec 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

communicationConfig.communications seems to be redundant. We can remove communicationConfig and move communications at root level

Copy link
Contributor Author

@slalwani97 slalwani97 Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PrasadG193 I have updated the PR. PTAL, I am not sure if this is the correct approach for this.

# Settings for Webhook
webhook:
enabled: true
url: 'WEBHOOK_URL' # e.g https://example.com:80
Copy link
Member

@PrasadG193 PrasadG193 Dec 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add newline.

slalwani97 added 2 commits Dec 16, 2019
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
@slalwani97
Copy link
Contributor Author

@slalwani97 slalwani97 commented Dec 17, 2019

helm install --version v0.9.1 --name botkube --namespace botkube \
--set communications.slack.enabled=true \
--set communications.slack.channel=<SLACK_CHANNEL_NAME> \
--set communications.slack.token=<SLACK_API_TOKEN_FOR_THE_BOT> \
--set config.settings.clustername=<CLUSTER_NAME> \
--set config.settings.allowkubectl=<ALLOW_KUBECTL> \
--set image.repository=infracloudio/botkube \
--set image.tag=v0.9.1 \
infracloudio/botkube

Copy link
Contributor

@bhavin192 bhavin192 left a comment

Checked the Helm chart related changes, LGTM!

@bhavin192
Copy link
Contributor

@bhavin192 bhavin192 commented Dec 17, 2019

Signed-off-by: Sumit Lalwani <sumit.lalwani97@gmail.com>
@PrasadG193 PrasadG193 merged commit 8b2e0f9 into infracloudio:develop Dec 18, 2019
1 check passed
@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Dec 18, 2019

Merged! Thanks for the PR @slalwani97

communications:
{{- with .Values.communications }}
{{- toYaml . | nindent 4 }}
{{- end }}
Copy link
Member

@PrasadG193 PrasadG193 Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slalwani97 Unfortunately we had to revert the commit. The problem is with the indentation here. The secret gets created with following format. That's why it couldn't get unmarshalled into struct

# Communication settings
communications:
elasticsearch:
  enabled: false
  index:
    name: botkube
    replicas: 0
    shards: 1
    type: botkube-event
  password: ELASTICSEARCH_PASSWORD
  server: ELASTICSEARCH_ADDRESS
  username: ELASTICSEARCH_USERNAME
mattermost:
  channel: dev-alerts
  enabled: false
  notiftype: short
  team: MATTERMOST_TEAM
  token: xxxx
  url: xxxx
slack:
  channel: general
  enabled: true
  notiftype: short
  token: xxxxxxxx
webhook:
  enabled: false
  url: WEBHOOK_URL

Copy link
Contributor Author

@slalwani97 slalwani97 Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably changing nindent to 6 will solve the problem.

Copy link
Contributor Author

@slalwani97 slalwani97 Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/config $ cat  comm_config.yaml 
# Communication settings
communications:
  elasticsearch:
    enabled: false
    index:
      name: botkube
      replicas: 0
      shards: 1
      type: botkube-event
    password: ELASTICSEARCH_PASSWORD
    server: ELASTICSEARCH_ADDRESS
    username: ELASTICSEARCH_USERNAME
  mattermost:
    channel: MATTERMOST_CHANNEL
    enabled: false
    notiftype: short
    team: MATTERMOST_TEAM
    token: MATTERMOST_TOKEN
    url: MATTERMOST_SERVER_URL
  slack:
    channel: SLACK_CHANNEL
    enabled: false
    notiftype: short
    token: SLACK_API_TOKEN
  webhook:
    enabled: false
    url: WEBHOOK_URL

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Dec 19, 2019

Could you please address this and raise another PR?

@slalwani97
Copy link
Contributor Author

@slalwani97 slalwani97 commented Dec 19, 2019

Addressed in PR #233

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Dec 19, 2019

Thanks @slalwani97

mergify bot pushed a commit that referenced this issue May 25, 2021
)

##### ISSUE TYPE
 - Feature Pull Request

##### SUMMARY
I have implemented in the helm chart the possibility to pass the communication config as a k8s secret and configure in the value files via an existingSecret. It can be configured in the values file via these:

```yaml
communications:
    existingSecret: false
    existingSecretName: ""
```

Fixes #319 #211 #226 #233
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.

3 participants