Skip to content

Commit

Permalink
Apply suggestion after review
Browse files Browse the repository at this point in the history
  • Loading branch information
mszostok committed Jun 30, 2022
1 parent b2820d9 commit 79b872e
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

Created on 2022-06-14 by Mateusz Szostok ([@mszostok](https://github.com/mszostok))

This is the alternative solutions to address both the configuration design issues and multichannel support. It was suppressed by the [Bindings](https://github.com/infracloudio/botkube/pull/626) proposal.
This is the alternative solutions to address both the configuration design issues and multichannel support. It was superseded by the [Bindings](https://github.com/infracloudio/botkube/pull/626) proposal.

## Proposal
## Syntax

The `policy` name can be also change to `profile`, or `presets` or others. This also enables an option to create catalog of predefined policies that can be consumed by end-users.
This section describes changes in the configuration syntax. As a result, we enable routing notifications to individual channels and limiting channel access to a specific Namespace. **Syntax is not backward compatible.**

1. New configuration syntax.
```yaml
Expand Down Expand Up @@ -64,6 +64,11 @@ The `policy` name can be also change to `profile`, or `presets` or others. This
events:
- error
```
The `policy` name can be also changed to `profile`, or `preset` or others. Later, we can create catalog of predefined policies that can be consumed by end-users.

## Use cases

This section describes example configurations that enable the requested use-cases.

### Route BotKube notifications to individual channels

Expand Down Expand Up @@ -291,7 +296,7 @@ policies:
### Others

If you use the `botkube.io/channel: <channel_name>` annotation, notifications are sent to a given channel even if not authorized. IMO, it's a bug.
We can now check if there is a matching communications.{name}.policyBinding.{channel} and if policy allows to send such events based on `policies.[.name == foo].notifications`.
We can now check if there is a matching `communications.{name}.policyBinding.{channel}` and if policy allows to send such events based on `policies.[.name == foo].notifications`.

## Alternatives

Expand Down Expand Up @@ -375,5 +380,5 @@ policies:

## Resources

- [PR that adds profiles to BotKube configuration](https://github.com/infracloudio/botkube/pull/291)
- [First implementation, which was based on profiles](https://github.com/infracloudio/botkube/pull/291). Unfortunately, this pull request is too outdated, and the work would need to be started from the ground. Additionally, it doesn't address the syntax issues.
- [Root feature Epic](https://github.com/infracloudio/botkube/issues/596)
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Configuration API syntax issues

Created on 2022-06-15 by Mateusz Szostok ([@mszostok](https://github.com/mszostok))

This document describes found issue with the current syntax for BotKube configuration.

## Communications
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

Created on 2022-06-20 by Mateusz Szostok ([@mszostok](https://github.com/mszostok))

| Status |
|-------------|
| `POSTPONED` |
> **Note**
>
> It was superseded by the [Bindings](https://github.com/infracloudio/botkube/pull/626) proposal. See the [**Summary**](#summary) section to learn more.
<!-- toc -->

Expand All @@ -20,6 +20,7 @@ Created on 2022-06-20 by Mateusz Szostok ([@mszostok](https://github.com/mszosto
* [Communicator](#communicator)
+ [Template](#template-2)
+ [Instance](#instance-2)
* [Mutators](#mutators)
- [Consequences](#consequences)
* [Minimum changes](#minimum-changes)
* [Follow-up changes](#follow-up-changes)
Expand Down Expand Up @@ -85,17 +86,6 @@ Domains:
1. (Cluster)MutatorTemplate
2. (Cluster)Mutator
Currently, I don't see any candidate for this.
| Filter Name | Description | Note |
|-------------------------|-----------------------------------------------------------------------------------|-----------------------------------------|
| ImageTagChecker | Checks and adds recommendation if 'latest' image tag is used for container image. | Move as recommendation notificator. |
| IngressValidator | Checks if services and tls secrets used in ingress specs are available. | Move as recommendation notificator. |
| ObjectAnnotationChecker | Checks if annotations botkube.io/* present in object specs and filters them. | Remove it. |
| PodLabelChecker | Checks and adds recommendations if labels are missing in the pod specs. | Move as recommendation notificator. |
| NamespaceChecker | Checks if event belongs to blocklisted namespaces and filter them. | Remove it. It will be per resource now. |
| NodeEventsChecker | Sends notifications on node level critical events. | Move as K8s events notificator. |
Initially, all executors and notifiers can be marked as built-in. The `spec.plugin.built-in: {name}` marks that a given functionality is built-in. We can later extract it into separate plugin (probably Docker image).

### Executors
Expand Down Expand Up @@ -362,9 +352,22 @@ status:
#lastTransitionTime:
```

### Mutators

Currently, I don't see any sensible candidates for this kind. The **ObjectAnnotationChecker** and **NamespaceChecker** are redundant if new syntax will be implemented. In my opinion, it's better to remove them to simplify the configuration settings. However, to do it right, first we should add a new metric to check how many users are using it.

| Filter Name | Description | Note |
|-------------------------|-----------------------------------------------------------------------------------|-----------------------------------------|
| ImageTagChecker | Checks and adds recommendation if 'latest' image tag is used for container image. | Move as recommendation notificator. |
| IngressValidator | Checks if services and tls secrets used in ingress specs are available. | Move as recommendation notificator. |
| ObjectAnnotationChecker | Checks if annotations botkube.io/* present in object specs and filters them. | Remove it. |
| PodLabelChecker | Checks and adds recommendations if labels are missing in the pod specs. | Move as recommendation notificator. |
| NamespaceChecker | Checks if event belongs to blocklisted namespaces and filter them. | Remove it. It will be per resource now. |
| NodeEventsChecker | Sends notifications on node level critical events. | Move as K8s events notificator. |

## Consequences

This section described necessary changes if proposal will be accepted.
This section described necessary changes if the implementation idea will be accepted and planned in the roadmap.

### Minimum changes

Expand Down

0 comments on commit 79b872e

Please sign in to comment.