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

Add flag to disable events collection in helm #205

Merged
merged 11 commits into from Sep 24, 2019
Merged

Conversation

maimaisie
Copy link
Contributor

Description

Add eventCollectionEnabled default to true.

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

Copy link
Contributor

@frankreno frankreno left a comment

Choose a reason for hiding this comment

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

LGTM

@samjsong
Copy link
Contributor

samjsong commented Sep 23, 2019

Found two concerns while playing around with this:

  1. When running the setup script outside of helm, when deploy is set to true, we apply fluentd-sumologic.yaml, but this yaml file deploys the events resources. This is only a problem for non-helm users.
    I'm not sure how to get around this without either having to do some sort of hacky thing in the setup script itself to remove those things from the file (really don't think this is the way to go), or maintain a second fluentd-sumologic.yaml file that doesn't deploy events collection (it would also autogenerated, but still not ideal). Any ideas?

  2. I noticed for this COLLECT_EVENTS -e flag, we have a default false when the -e flag is specified, so when I tried

deploy/docker/setup/setup.sh -c ssong-k8s-7 -k ssong-k8s-7 -e false <ENDPOINT> <ID> <KEY>

it didn't work; you actually have to do

deploy/docker/setup/setup.sh -c COLLECTOR -k CLUSTER -e <ENDPOINT> <ID> <KEY>

which is better, but inconsistent with our other flags -d and -y which currently require users to pass in a value. We could change the way -d and -y work, but that'd be backwards incompatible for non-helm users. What do we think?

@rvmiller89
Copy link
Contributor

Could we leave the setup script alone (always create events endpoint, even if nothing will be sent to it)? Only Helm users should expect changing a value in values.yaml should have an effect -- non-Helm users won't be using values.yaml at all.

@samjsong
Copy link
Contributor

The setup script creates the sources on Sumo side though, and we use the setup script in the helm deploy - so if we don't touch the setup script, then helm users won't get any events resources deployed on their clusters but still have the events HTTP source created in Sumo

@rvmiller89
Copy link
Contributor

Right -- is that so horrible? It's just a dormant source they would only see in the Manage Collection page.

@samjsong
Copy link
Contributor

I see your point - I guess this also allows users to seamlessly reenable events collection if they so desire in the future, without recreating the sumologic secret. +1

@samjsong samjsong merged commit 921d81b into master Sep 24, 2019
@samjsong samjsong deleted the maisie-toggle-events branch September 24, 2019 00:25
psaia pushed a commit to psaia/sumologic-kubernetes-collection that referenced this pull request May 25, 2021
Co-authored-by: BrianWanner <BrianWanner@users.noreply.github.com>
andrzej-stencel added a commit that referenced this pull request Jul 15, 2021
Changes since v1.12.2-sumo-0:
- feat: Build images for ARM #187
- chore(deps): bump fluent/fluentd from v1.12.2-debian-1.0 to v1.12.2-debian-1.1 #195
- chore(deps): Upgrade `rexml` gem to 3.2.5 fix CVE-2021-28965 #205 #206 #209
- chore(deps): Upgrade `rdoc` gem to 6.3.1 to fix CVE-2021-31799 #227
- chore(deps): Upgrade `addressable` gem to 2.8.0 to fix a vulnerability reported by Sysdig #292 #293 #294 #296
- chore(deps): Upgrade `ruby` in builder image from 2.6.6 to 2.6.8 #296
- chore(deps): Upgrade all system packages in Docker image #296
andrzej-stencel added a commit that referenced this pull request Jul 16, 2021
Changes since v1.12.2-sumo-0:
- feat: Build images for ARM #187
- chore(deps): bump fluent/fluentd from v1.12.2-debian-1.0 to v1.12.2-debian-1.1 #195
- chore(deps): Upgrade `rexml` gem to 3.2.5 fix CVE-2021-28965 #205 #206 #209
- chore(deps): Upgrade `rdoc` gem to 6.3.1 to fix CVE-2021-31799 #227
- chore(deps): Upgrade `addressable` gem to 2.8.0 to fix a vulnerability reported by Sysdig #292 #293 #294 #296
- chore(deps): Upgrade `ruby` in builder image from 2.6.6 to 2.6.8 #296
- chore(deps): Upgrade all system packages in Docker image #296
sumo-backporter bot pushed a commit that referenced this pull request Jul 16, 2021
Changes since v1.12.2-sumo-0:
- feat: Build images for ARM #187
- chore(deps): bump fluent/fluentd from v1.12.2-debian-1.0 to v1.12.2-debian-1.1 #195
- chore(deps): Upgrade `rexml` gem to 3.2.5 fix CVE-2021-28965 #205 #206 #209
- chore(deps): Upgrade `rdoc` gem to 6.3.1 to fix CVE-2021-31799 #227
- chore(deps): Upgrade `addressable` gem to 2.8.0 to fix a vulnerability reported by Sysdig #292 #293 #294 #296
- chore(deps): Upgrade `ruby` in builder image from 2.6.6 to 2.6.8 #296
- chore(deps): Upgrade all system packages in Docker image #296
andrzej-stencel added a commit that referenced this pull request Jul 16, 2021
Changes since v1.12.2-sumo-0:
- feat: Build images for ARM #187
- chore(deps): bump fluent/fluentd from v1.12.2-debian-1.0 to v1.12.2-debian-1.1 #195
- chore(deps): Upgrade `rexml` gem to 3.2.5 fix CVE-2021-28965 #205 #206 #209
- chore(deps): Upgrade `rdoc` gem to 6.3.1 to fix CVE-2021-31799 #227
- chore(deps): Upgrade `addressable` gem to 2.8.0 to fix a vulnerability reported by Sysdig #292 #293 #294 #296
- chore(deps): Upgrade `ruby` in builder image from 2.6.6 to 2.6.8 #296
- chore(deps): Upgrade all system packages in Docker image #296
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.

None yet

4 participants