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

endpoint: Fix the initialization of endpoints' datapath configs #15228

Merged
merged 4 commits into from
Mar 10, 2021
Merged

endpoint: Fix the initialization of endpoints' datapath configs #15228

merged 4 commits into from
Mar 10, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Mar 5, 2021 •
edited
Loading

See commits for details.

Fixes: #13773.

Fix bug where `enable-endpoint-routes` change required all pods to restart to take effect

@pchaigno pchaigno added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.9 labels Mar 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.5 Mar 5, 2021
@pchaigno pchaigno marked this pull request as ready for review March 5, 2021 22:24
@pchaigno pchaigno requested a review from a team as a code owner March 5, 2021 22:24
@pchaigno pchaigno requested review from a team, aditighag and gandro March 5, 2021 22:24
@pchaigno pchaigno requested review from a team and jrfastab March 8, 2021 09:56
gandro
gandro approved these changes Mar 8, 2021
View reviewed changes
Copy link
Member

@gandro gandro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The health related change looks good to me!

I also didn't spot any obvious defects in the rest of the code, but I'm not familiar enough with the datapath.

@pchaigno pchaigno mentioned this pull request Mar 8, 2021
Merged
@aditighag aditighag removed their assignment Mar 8, 2021
@pchaigno pchaigno mentioned this pull request Mar 9, 2021
Closed
qmonnet
qmonnet approved these changes Mar 9, 2021
View reviewed changes
pkg/datapath/loader/loader.go Show resolved Hide resolved
joestringer
joestringer requested changes Mar 9, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I think that this is potentially breaking modes where the cilium-cni specifies datapath configuration options to the cilium-agent, thread below.

joestringer
joestringer reviewed Mar 9, 2021
View reviewed changes
daemon/cmd/endpoint.go Outdated Show resolved Hide resolved
@pchaigno pchaigno mentioned this pull request Mar 9, 2021
Closed
@pchaigno
Copy link
Member Author

Runtime failed with know-flake #14676, other builds are passing. Reviews are in. Marking as ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 10, 2021
@kkourt kkourt merged commit a9ecab1 into cilium:master Mar 10, 2021
1.10.0 automation moved this from In progress to Done Mar 10, 2021
@pchaigno pchaigno deleted the fix-init-dp-config branch March 10, 2021 08:47
@pchaigno pchaigno mentioned this pull request Mar 10, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.5 Mar 10, 2021
@christarazi christarazi added this to Backport pending to v1.9 in 1.9.6 Mar 10, 2021
@christarazi christarazi removed this from Backport pending to v1.9 in 1.9.5 Mar 10, 2021
@joestringer joestringer moved this from Backport pending to v1.9 to Needs backport from master in 1.9.6 Mar 10, 2021
@christarazi christarazi mentioned this pull request Mar 10, 2021
Merged
@kaworu kaworu mentioned this pull request Mar 12, 2021
Merged
@jrajahalme jrajahalme mentioned this pull request Mar 19, 2021
Merged
joestringer
joestringer reviewed Apr 14, 2021
View reviewed changes
daemon/cmd/endpoint.go
Comment on lines +309 to +312
if epTemplate.DatapathConfiguration == nil {
dpConfig := endpoint.NewDatapathConfiguration()
epTemplate.DatapathConfiguration = &dpConfig
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this mean that endpoint routes mode is only ever applied if the caller specifies the epTemplate.DatapathConfiguration? Why isn't this broken?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's my understanding as well 🤔

Not sure why it doesn't break in tests. I've set a reminder to look tomorrow (or at least open an issue if I don't get time to debug).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still unsure why it's not breaking anything in CI. I suspected that maybe we never received nil epTemplate.DatapathConfigurations but checked the client's code and we should receive it. I even went so far as to reproduce the marshalling bits to double check it doesn't turn nil into an empty structure. It doesn't.

If we were hitting this, the agent would consider endpoint routes to be disabled for all new endpoints added via the API AFAICS. Looking at a couple tests with endpoint routes enabled, that's clearly not the case (I see the expected routes and BPF programs).

It's still a bug so I'll send a PR to fix, but we're missing something here 🤔

@joestringer joestringer moved this from Needs backport from master to Backport done to v1.9 in 1.9.6 Apr 15, 2021
@joestringer joestringer mentioned this pull request Apr 20, 2021
Merged
@pchaigno pchaigno mentioned this pull request May 7, 2021
Merged
@pchaigno pchaigno mentioned this pull request May 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@gandro gandro gandro approved these changes

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

@qmonnet qmonnet qmonnet approved these changes

@aditighag aditighag Awaiting requested review from aditighag

@jrfastab jrfastab Awaiting requested review from jrfastab

Assignees

@jrfastab jrfastab

@aditighag aditighag

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.0
Done
1.9.6
Backport done to v1.9
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI: K8sDatapathConfig Encapsulation Check iptables masquerading with random-fully fails on k8s-all
10 participants
@pchaigno @gandro @joestringer @christarazi @aanm @qmonnet @kkourt @jrfastab @jrajahalme @aditighag