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.

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

Revert exported NoTrack rule function names. #15505

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Revert exported NoTrack rule function names. #15505

merged 1 commit into from
Apr 6, 2021

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Mar 30, 2021 •
edited

Revert exported NoTrack rule function names.

Google's node-local-dns feature with Cilium depends on these names, revert to unbreak it. Also added comments to explain how these functions interact with flag InstallNoConntrackIptRules.

@Weil0ng Weil0ng requested a review from a team March 30, 2021 01:45
@Weil0ng Weil0ng requested a review from a team as a code owner March 30, 2021 01:45
@Weil0ng Weil0ng requested review from jibi and aditighag March 30, 2021 01:45
@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 Mar 30, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 30, 2021
@Weil0ng Weil0ng added the release-note/misc This PR makes changes that have no direct user impact. label Mar 30, 2021
@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 Mar 30, 2021
@jibi jibi added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Mar 30, 2021
jibi
jibi reviewed Mar 30, 2021
View reviewed changes
Copy link
Member

@jibi jibi left a comment

Choose a reason for hiding this comment

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

OK so these rules are targeting services rather than pod traffic, correct?

pkg/datapath/iptables/iptables.go Show resolved Hide resolved
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Mar 30, 2021

OK so these rules are targeting services rather than pod traffic, correct?

They are targeting pod traffic, it's just we would always want these when using node-local-dns regardless of the flag "InstallNoConntrackIptRules".

@Weil0ng Weil0ng changed the title Force nf NOTRACK rule management when needed. Revert exported NoTrack rule function names. Mar 31, 2021
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Mar 31, 2021

Repurposed this PR, see updated title/PR description/commit msg, PTAL :)

@jibi
Copy link
Member

jibi commented Apr 1, 2021

test-me-please

@jibi
Copy link
Member

jibi commented Apr 1, 2021

retest-gke

@jibi
Copy link
Member

jibi commented Apr 1, 2021

retest-4.9

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 2, 2021 •
edited

hmm, I don't think the CI failure is caused by this change...retesting

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 2, 2021

retest-gke

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 2, 2021

retest-4.9

@Weil0ng
Reverts exported function names and adds comments for context.
043ac7e 
- revert `InstallEndpointNoTrackRules` to `InstallNoTrackRules`
- revert `RemoveEndpointNoTrackRules` to `RemoveNoTrackRules`

This would unbreak google's node-local-dns use case with Cilium.

Signed-off-by: Weilong Cui <cuiwl@google.com>
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 2, 2021

test-me-please

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 3, 2021

CI is green, PTAL

@aditighag
Copy link
Member

Please have Jibi also review it.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 3, 2021
@aditighag
Copy link
Member

Are you planning to add a CI test to catch any regressions in this code?

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 3, 2021

Are you planning to add a CI test to catch any regressions in this code?

This regression is kinda a gap between upstream library function and our internal patch (we call these functions), I don't think a new test in Cilium repo is gonna catch this. I'm open to suggestions as how we might prevent this from happening :)

@pchaigno pchaigno requested a review from jibi April 5, 2021 14:49
@pchaigno pchaigno removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 5, 2021
@aditighag
Copy link
Member

Are you planning to add a CI test to catch any regressions in this code?

This regression is kinda a gap between upstream library function and our internal patch (we call these functions), I don't think a new test in Cilium repo is gonna catch this. I'm open to suggestions as how we might prevent this from happening :)

How about adding a node-local-dns test?

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 5, 2021

Are you planning to add a CI test to catch any regressions in this code?

This regression is kinda a gap between upstream library function and our internal patch (we call these functions), I don't think a new test in Cilium repo is gonna catch this. I'm open to suggestions as how we might prevent this from happening :)

How about adding a node-local-dns test?

I don't think it'll help in this case...the test would have passed because it won't include our caller.

On node-local-dns test, it would basically be the LRP test, or do you have something else in mind?

@pchaigno pchaigno merged commit beceabb into cilium:master Apr 6, 2021
1.10.0 automation moved this from In progress to Done Apr 6, 2021
This was referenced Apr 28, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jibi jibi jibi approved these changes

@aditighag aditighag aditighag approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Weil0ng @jibi @aditighag @pchaigno