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 k8s deployment yaml files for audit purpose only #729

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

benjaminhuo
Copy link
Contributor

@benjaminhuo benjaminhuo commented Jul 18, 2019

Signed-off-by: Benjamin benjamin@yunify.com

What type of PR is this?

/kind cleanup
/kind documentation

Any specific area of the project related to this PR?

/area integrations

What this PR does / why we need it:

DaemonSet is more than enough for just monitoring k8s audit event, a deployment should be enough.

Which issue(s) this PR fixes:

Fixes #725

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
Yes

Deployment YAML files for Kubernetes Audit only purpose.

@benjaminhuo
Copy link
Contributor Author

/assign @mstemm

Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Thanks for sending this PR, really appreciated! 🤗

A few initial observations.

Currently we have integrations/k8s-using-daemonset directory.
The integrations directory probably needs a re-organization.
Just to be in pair with the current structure can we put this integration under integrations/k8s-using-deployment?

Furthermore atm I think that falco-k8s-audit.yaml config file is not something that have to be in the root of the source.

@benjaminhuo benjaminhuo force-pushed the dev branch 2 times, most recently from 2e5b224 to 6a9c8ce Compare July 19, 2019 03:45
@benjaminhuo
Copy link
Contributor Author

benjaminhuo commented Jul 19, 2019

Thanks for sending this PR, really appreciated! 🤗

A few initial observations.

Currently we have integrations/k8s-using-daemonset directory.
The integrations directory probably needs a re-organization.
Just to be in pair with the current structure can we put this integration under integrations/k8s-using-deployment?

Furthermore atm I think that falco-k8s-audit.yaml config file is not something that have to be in the root of the source.

I've made corresponding changes per your suggestions.
One more question, I tried to remove more options from falco.yaml for k8s audit only but I'm not sure which options I should remove and would like to know your suggestions :)

Maybe syscall_event_drops section should be removed and syslog_output could be set to false?

@fntlnz
Copy link
Contributor

fntlnz commented Jul 22, 2019

@mfdii PTAL

@leodido
Copy link
Member

leodido commented Jul 22, 2019

/hold

blocked by #730

@stale
Copy link

stale bot commented Sep 20, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Sep 20, 2019
@fntlnz
Copy link
Contributor

fntlnz commented Sep 20, 2019

I think that this can now be accomplished since #730 had been done (but not released yet).
@benjaminhuo let me know if you need assistance with it.

@stale stale bot removed the wontfix label Sep 20, 2019
@benjaminhuo benjaminhuo changed the title WIP:Add k8s deployment yaml files for audit purpose only Add k8s deployment yaml files for audit purpose only Sep 23, 2019
@benjaminhuo
Copy link
Contributor Author

@fntlnz I've modified my PR to add the new --disable-source flag and removed some extra config and rule files.

Would you help to review this ?

Thanks
Ben

@fntlnz
Copy link
Contributor

fntlnz commented Sep 26, 2019

Looking again! 👀

/assign @fntlnz
/assign @leodido

Copy link
Contributor

@fntlnz fntlnz left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Just a couple of nits then we are good to go!

integrations/k8s-using-deployment/README.md Outdated Show resolved Hide resolved
integrations/k8s-using-deployment/README.md Outdated Show resolved Hide resolved
integrations/k8s-using-deployment/falco.yaml Show resolved Hide resolved
@leodido leodido self-requested a review October 8, 2019 14:53
@fntlnz
Copy link
Contributor

fntlnz commented Oct 8, 2019

ok @benjaminhuo we are just waiting for you to accept the renaming changes then we can merge this!

Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Can you pick the suggestions or give us access to our fork?

In that way we could edit this PR and merge it! :)

@benjaminhuo
Copy link
Contributor Author

benjaminhuo commented Oct 8, 2019

@leodido @fntlnz , Sorry I missed your comments and I've accepted the change now.

Regards,
Ben

Signed-off-by: Benjamin <benjamin@yunify.com>
@benjaminhuo
Copy link
Contributor Author

I've merged your suggestions and force pushed the new changes

Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

🎉

@poiana poiana added the lgtm label Oct 8, 2019
@poiana
Copy link
Contributor

poiana commented Oct 8, 2019

LGTM label has been added.

Git tree hash: 6ab2904d1126e329e76904da8d5d9f49a57821ff

@poiana
Copy link
Contributor

poiana commented Oct 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leodido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana added the approved label Oct 8, 2019
@leodido
Copy link
Member

leodido commented Oct 8, 2019

/hold cancel

@leodido leodido merged commit 4e6d347 into falcosecurity:dev Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use deployment instead of daemonset for k8s audit
5 participants