Add full configuration file for Event Handler helm chart#62049
Add full configuration file for Event Handler helm chart#62049
Conversation
kshi36
changed the title
|
|
||
| // Timeout is the time poller will wait before the new request if there are no events in the queue | ||
| Timeout time.Duration `help:"Polling timeout" default:"5s" env:"FDFWD_TIMEOUT"` | ||
| Timeout time.Duration `help:"Polling timeout" default:"10s" env:"FDFWD_TIMEOUT"` |
There was a problem hiding this comment.
All default values for helm chart values.yaml were added based on internal event handler CLI default values. However, I set CLI's timeout to "10s" which matches existing reference examples as well as current helm chart default value.
| FluentdCert string `help:"fluentd TLS certificate file" type:"existingfile" env:"FDWRD_FLUENTD_CERT"` | ||
| FluentdCert string `help:"fluentd TLS certificate file" type:"existingfile" env:"FDFWD_FLUENTD_CERT,FDWRD_FLUENTD_CERT"` |
There was a problem hiding this comment.
Unless I am missing something, FDWRD_ prefix is inconsistent/typo, since FDFWD_ is adopted most everywhere else. I included the fixed env var as well as retained old env var, for backwards compatibility. README also reflects new preferred env var.
Alternatively can include both FDWRD_ and FDFWD_ prefixes everywhere for maximum consistency...
kshi36
force-pushed
the
kevin/event-handler-chart-options
branch
from
d93eee4 to
75823ca
Compare
github-actions
bot
requested review from
hugoShaka,
marcoandredinis and
tigrato
There was a problem hiding this comment.
Thank you for converting the chart to the default values format :)
Once this PR is merged, can you open a followup PR to change examples/chart/Makefile, add render-chart-ref-event-handler and check-chart-ref-event-handle so we use the comments from values.yaml to generate the reference?
| {{- if .Values.eventHandler.lock }} | ||
| {{- if .Values.eventHandler.lock.enabled }} | ||
| lock-enabled = {{ .Values.eventHandler.lock.enabled }} | ||
| {{- end }} | ||
| {{- if .Values.eventHandler.lock.failedAttemptsCount }} | ||
| lock-failed-attempts-count = {{ .Values.eventHandler.lock.failedAttemptsCount }} | ||
| {{- end }} | ||
| {{- if .Values.eventHandler.lock.period }} | ||
| lock-period = {{ .Values.eventHandler.lock.period | quote }} | ||
| {{- end }} | ||
| {{- if .Values.eventHandler.lock.for }} | ||
| lock-for = {{ .Values.eventHandler.lock.for | quote }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Thanks to this PR I learned that the event handler is able to lock users out of Teleport. Now I want to unlearn this 🫠
This is not a great design because we stream event with potentially quite some delay (on athena we can be up to 5 minute late).
There was a problem hiding this comment.
Ok down the rabbit hole, we use some external rate-limiter that is mentioned in go's hall of shame https://go.dev/src/runtime/timestub.go?s=610:641
The fact it uses the current time make the logic completely broken because:
- we might be backfilling events, so we should use the event time instead of the current time
- on some backends we are consuming event potentially by chunks, so we only get event batches and they essentially all appear at the same time
I filed: #62252 , no need to do it now but I'll add this to the backlog.
public-teleport-github-review-bot
bot
removed request for
marcoandredinis and
tigrato
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* Added full configuration file for Event Handler helm chart * Fix snapshot * Fix test * Remove exitOnLastEvent, refresh options; refactoring * Refactor, fix test
3 participants
Closes #53356
Part of #60411
This PR updates the Event Handler helm chart to set any arbitrary configuration option. In addition, the Event Handler README was updated with all existing options, and the CLI extended with more environment variables and more dump statements.
A follow-up docs PR will be made to auto-generate the helm chart reference and replace the existing static version (via
render-helm-ref).Manual Tests
tomlfile (setting all added config options) is correctly parsed by teleport-event-handler plugin when callingFDFWD_prefix) and old environment variables (FDWRD_prefix) both correctly set CLI options' values.changelog: Added full configuration file for
teleport-plugin-event-handlerhelm chartchangelog: Added full environment variable configuration for event handler CLI