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

update(falcosidekick): Upgrade to v2.28 #512

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

Lowaiz
Copy link
Contributor

@Lowaiz Lowaiz commented Jul 7, 2023

What type of PR is this?

/kind feature

If this PR will release a new chart version please make sure to also uncomment the following line:

/kind chart-release

Any specific area of the project related to this PR?

/area falcosidekick-chart

What this PR does / why we need it:

Update values, env and secrets to fit new features of the next falcosidekick update

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Ping to @Issif, with the large milestone that v2.28 is, I think that might help a little bit.

Checklist

  • Chart Version bumped
  • Variables are documented in the README.md
  • CHANGELOG.md updated

@poiana poiana requested review from alacuku and bencer July 7, 2023 13:07
@poiana poiana added the size/XL label Jul 7, 2023
@Issif Issif changed the title wip: update(falco-sidekick): Upgrade to v2.28 wip: update(falcosidekick): Upgrade to v2.28 Jul 24, 2023
@Issif
Copy link
Member

Issif commented Jul 24, 2023

Can you update this PR with this last feature, I'll review then falcosecurity/falcosidekick#565

Thanks

@Lowaiz Lowaiz force-pushed the update/falcosidekick-2.28 branch from b37d993 to 039e4c6 Compare July 26, 2023 09:22
@Lowaiz
Copy link
Contributor Author

Lowaiz commented Jul 26, 2023

Updated to the last merged PR.
Ping @Issif

@Lowaiz Lowaiz force-pushed the update/falcosidekick-2.28 branch from 7e8813d to 443ab5d Compare July 27, 2023 17:06
@Lowaiz Lowaiz force-pushed the update/falcosidekick-2.28 branch from 443ab5d to b5041fa Compare July 27, 2023 17:06
@Lowaiz Lowaiz changed the title wip: update(falcosidekick): Upgrade to v2.28 update(falcosidekick): Upgrade to v2.28 Jul 27, 2023
falcosidekick/CHANGELOG.md Outdated Show resolved Hide resolved
falcosidekick/CHANGELOG.md Outdated Show resolved Hide resolved
falcosidekick/CHANGELOG.md Outdated Show resolved Hide resolved
falcosidekick/CHANGELOG.md Outdated Show resolved Hide resolved
falcosidekick/CHANGELOG.md Show resolved Hide resolved
Comment on lines 89 to +104
- name: MUTUALTLSFILESPATH
value: {{ .Values.config.mutualtlsfilespath | quote }}
- name: MUTUALTLSCLIENT_CERTFILE
value: {{ .Values.config.mutualtlsclient.certfile | quote }}
- name: MUTUALTLSCLIENT_KEYFILE
value: {{ .Values.config.mutualtlsclient.keyfile | quote }}
- name: MUTUALTLSCLIENT_CACERTFILE
value: {{ .Values.config.mutualtlsclient.cacertfile | quote }}
- name: TLSSERVER_DEPLOY
value: {{ .Values.config.tlsserver.deploy | quote }}
- name: TLSSERVER_CERTFILE
value: {{ .Values.config.tlsserver.certfile | quote }}
- name: TLSSERVER_KEYFILE
value: {{ .Values.config.tlsserver.keyfile | quote }}
- name: TLSSERVER_CACERTFILE
value: {{ .Values.config.tlsserver.cacertfile | quote }}
Copy link
Member

Choose a reason for hiding this comment

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

We need to go further, as nothing is already there to add the cert and key files, these settings are useless. We need to mount secrets as volumes.

Copy link
Contributor Author

@Lowaiz Lowaiz Jul 28, 2023

Choose a reason for hiding this comment

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

Does including TLS and mTLS server configs is a repetition of the ingress job ?

With the way of doing thing inside Kubernetes, do we need to let the possibility to activate this kind of feature inside the pod ?

Copy link
Contributor Author

@Lowaiz Lowaiz Jul 28, 2023

Choose a reason for hiding this comment

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

I think the best method to handle the TLS and mTLS use cases, for the chart, is to use the Ingress options and not let these 2 happen in the pod.

Copy link
Member

Choose a reason for hiding this comment

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

Some users expect the pods to communicate with TLS, but OK. Let's keep as is for now.

# -- CA certification file for client certification if mutualtls is true
cacertfile: "/etc/certs/server/ca.crt"
# -- port to serve http server serving selected endpoints
notlsport: 2810
Copy link
Member

Choose a reason for hiding this comment

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

we need to open this port in the service and ingress with the excluded paths from TLS

@Lowaiz Lowaiz force-pushed the update/falcosidekick-2.28 branch from e41e56c to 7a69b1e Compare July 28, 2023 13:06
@poiana poiana added the lgtm label Jul 28, 2023
@poiana
Copy link
Contributor

poiana commented Jul 28, 2023

LGTM label has been added.

Git tree hash: 630f9b68a0a4eaf0b55f8f74e8d61413d6494752

@poiana
Copy link
Contributor

poiana commented Jul 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Issif, Lowaiz

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

@Issif
Copy link
Member

Issif commented Jul 28, 2023

@Lowaiz can you open the PR for the falco chart now please?

@poiana poiana merged commit 4053932 into falcosecurity:master Jul 28, 2023
5 checks passed
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.

None yet

3 participants