Skip to content

helm: Add configuration option for endpoint source IP verification #34056

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 25, 2024
Merged

helm: Add configuration option for endpoint source IP verification #34056

merged 1 commit into from
Oct 25, 2024

Conversation

CiraciNicolo
Copy link
Contributor

@CiraciNicolo CiraciNicolo commented Jul 29, 2024 •
edited
Loading

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

This PR implements a way to manage SourceIPVerification flag from Cilium's config file.

Fixes: #33889

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 29, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jul 29, 2024
@CiraciNicolo
Copy link
Contributor Author

I opened this PR as draft, so we can discuss if the solution is accepted. Therefore we can also implements other config flags that are now hardcoded.

@CiraciNicolo
Copy link
Contributor Author

@squeed any feedback?

@github-actions GitHub Actions
Copy link

This pull request 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.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 30, 2024
@CiraciNicolo
Copy link
Contributor Author

@squeed any update? 😊

@squeed
Copy link
Contributor

squeed commented Sep 2, 2024

@CiraciNicolo apologies, didn't realize you were waiting on me. This seems like the right approach.

(Do note that we make changes to values.yaml.tmpl then run make -C install to update.

@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 3, 2024
@SkalaNetworks SkalaNetworks mentioned this pull request Sep 23, 2024
Closed
@SkalaNetworks
Copy link
Contributor

@CiraciNicolo wanna switch this out of draft?

@CiraciNicolo
Copy link
Contributor Author

@SkalaNetworks working on this right now, will implement other parameters and ask for review

@CiraciNicolo CiraciNicolo reopened this Oct 2, 2024
@CiraciNicolo CiraciNicolo marked this pull request as ready for review October 2, 2024 10:54
@CiraciNicolo CiraciNicolo requested review from a team as code owners October 2, 2024 10:54
@ldelossa ldelossa removed their request for review October 2, 2024 18:03
@joestringer
Copy link
Member

joestringer commented Oct 4, 2024 •
edited
Loading

I'd suggest to drop the conntrack local case from this PR. This was deprecated: #34358 .

Allowing configuration of source IP verification makes sense though, but we should probably add the flag to the global flags in daemon/cmd/daemon_main.go. It would also help to rename the PR to something like "Add configuration flag for Source IP Verification".

@joestringer
Copy link
Member

Please also drop the merge commits, it's enough to just rebase against the tip of main and force-push your updated PR into this branch.

@joestringer
Copy link
Member

Looking back this took a while to get feedback, probably because (a) it was marked as draft, and generally speaking in Cilium we won't look at drafts unless we're actively collaborating with someone on it, and (b) the PR probably could have been reviewed by anyone in @cilium/sig-datapath , so by marking it as draft and expecting a review from @squeed it meant that if he was busy, on vacation, or so on then it's easy for him to miss that and block the effort. Generally if you don't receive a review within a week or so, I'd encourage you to post a message in the #development channel in Slack to see if someone else can help to unblock it.

@joestringer
Copy link
Member

Other than that, thanks for the submission :) I think there's some value here and we should be able to help you get this in for an upcoming v1.17 snapshot release 🚀

gandro
gandro reviewed Oct 7, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Helm in general looks okay, but there are a few auto-generated files that need to be regenerated. Please run

make -C install/kubernetes
make -C Documentation update-helm-values

Thanks!

@gandro
Copy link
Member

gandro commented Oct 7, 2024

Looks good. Could you also clean up your git history so it only contains one or two commits (no merge commits or follow-up fixes, please). Thank you!

joestringer
joestringer requested changes Oct 7, 2024
View reviewed changes
@joestringer joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Oct 8, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 8, 2024
@aanm aanm requested review from joestringer and gandro October 11, 2024 09:09
@aanm
Copy link
Member

aanm commented Oct 11, 2024

/test

@aanm aanm enabled auto-merge October 11, 2024 09:09
gandro
gandro approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Helm-wise this still looks good. Please squash your commits into one. Thanks!

auto-merge was automatically disabled October 15, 2024 07:40

Head branch was pushed to by a user without write access

@CiraciNicolo CiraciNicolo force-pushed the feat/daemon-configuration branch from f44dcdc to 59190ea Compare October 15, 2024 07:40
@CiraciNicolo
Copy link
Contributor Author

Hi @gandro, commits have been squashed. Do you need a rebase?

@aanm aanm force-pushed the feat/daemon-configuration branch from 59190ea to d31bc59 Compare October 21, 2024 09:51
@aanm aanm requested a review from a team as a code owner October 21, 2024 09:51
@aanm
Copy link
Member

aanm commented Oct 21, 2024

/test

@aanm aanm enabled auto-merge October 21, 2024 09:51
@aanm aanm force-pushed the feat/daemon-configuration branch from d31bc59 to 09ef47e Compare October 21, 2024 10:12
@aanm
Copy link
Member

aanm commented Oct 21, 2024

/test

@gandro
Copy link
Member

gandro commented Oct 22, 2024

The documentation workflow is failing, it seems another run of make -C Documentation update-helm-values is needed. Otherwise this looks good to me

@CiraciNicolo
feat: allow configuration of SourceIPVerification via configuration f…
b24cb0a 
…iles.

This PR implements a way to manage SourceIPVerification flag from Cilium's
config file.

Fixes: cilium#33889

Signed-off-by: Nicolò Ciraci <ciraci.nicolo@gmail.com>
auto-merge was automatically disabled October 22, 2024 13:31

Head branch was pushed to by a user without write access

@CiraciNicolo CiraciNicolo force-pushed the feat/daemon-configuration branch from 09ef47e to b24cb0a Compare October 22, 2024 13:31
@gandro
Copy link
Member

gandro commented Oct 23, 2024

/test

@joestringer joestringer enabled auto-merge October 25, 2024 21:50
@joestringer joestringer changed the title feat: add configuration options helm: Add configuration option for endpoint source IP verification Oct 25, 2024
@joestringer joestringer added this pull request to the merge queue Oct 25, 2024
@joestringer
Copy link
Member

Thanks again for your persistence on working on this @CiraciNicolo, merging :)

Merged via the queue into cilium:main with commit 968116f Oct 25, 2024
62 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@gandro gandro gandro approved these changes

@joestringer joestringer joestringer approved these changes

@joamaki joamaki Awaiting requested review from joamaki joamaki is a code owner automatically assigned from cilium/sig-k8s

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option to disable SourceIPVerification while chaining CNI
7 participants
@CiraciNicolo @squeed @SkalaNetworks @joestringer @gandro @aanm @derailed