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: Remove the duplicate ENABLE_SIP_VERIFICATION in ep_config.h #25533

Merged
merged 3 commits into from Jun 18, 2023
Merged

endpoint: Remove the duplicate ENABLE_SIP_VERIFICATION in ep_config.h #25533

merged 3 commits into from Jun 18, 2023

Conversation

oblazek
Copy link
Contributor

@oblazek oblazek commented May 18, 2023

Currently ENABLE_SIP_VERIFICATION runtime option can define ENABLE_SIP_VERIFICATION macro just like DisableSIPVerification endpoint datapath option can. If DatapathConfiguration.DisableSipVerification value is not inline with SourceIPVerification, the two macros in ep_config.h may conflict.

Related slack thread https://cilium.slack.com/archives/C2B917YHE/p1680518197957369

Fix a bug where datapath option DisableSipVerification can no longer be used.

This completes the PR #24150 (i.e. 24150 can be closed, original author is honored here...)

@oblazek oblazek requested review from a team as code owners May 18, 2023 20:17
@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 May 18, 2023
@oblazek oblazek changed the title fix: Remove the duplicate ENABLE_SIP_VERIFICATION in ep_config.h endpoint: Remove the duplicate ENABLE_SIP_VERIFICATION in ep_config.h May 18, 2023
@oblazek oblazek mentioned this pull request May 18, 2023
Closed
@oblazek
Copy link
Contributor Author

oblazek commented May 18, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "http://[fd04::12]:32531" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2395/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

ti-mo
ti-mo reviewed May 22, 2023
View reviewed changes
pkg/endpoint/endpoint.go Outdated
@@ -721,6 +721,12 @@ func (e *Endpoint) SetDefaultOpts(opts *option.IntOptions) {
e.Options.Opts = option.OptionMap{}
}

// We need to make sure DatapathConfiguration.DisableSipVerification value is same as
// the value of SourceIPVerification option in the endpoint.
if e.DatapathConfiguration.DisableSipVerification {
Copy link
Contributor

@ti-mo ti-mo May 22, 2023 •
edited

Choose a reason for hiding this comment

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

This looks like a code smell to me. This function is supposed to set the Endpoint's default options. All other code in this function looks rather generic, while this if statement is specific to a feature.

Is this code strictly necessary? Shouldn't this value make it into e.Options.Opts at an earlier stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be possible, yeah.. it's not strictly necessary, I only need to make sure this is also correctly set on endpoint restoration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, ptal :)
I also added a unit test to make sure this change is covered.
Btw this was a bit tricky as I found out later that these settings are overriden by calling SetDefaultConfiguration in AddEndpoint(). I believe the removal of it is safe as all references where this is called (the AddEndpoint()) are in the end calling SetDefaultOpts.

@tommyp1ckles
Copy link
Contributor

Changes make sense to me - just waiting for resolution to ti-mo's point.

@oblazek oblazek force-pushed the sip_verification branch 2 times, most recently from 68027ef to f8af249 Compare May 25, 2023 10:33
@oblazek
Copy link
Contributor Author

oblazek commented May 25, 2023

/test

@oblazek
Copy link
Contributor Author

oblazek commented May 25, 2023

/ci-multicluster

@oblazek oblazek requested a review from ti-mo May 26, 2023 04:51
@tommyp1ckles
Copy link
Contributor

Playing around with this locally, I notice there is a small different between the old way and using IntOptions.

The new way sets #undef FOO when it's disabled. I'm not as familiar with the C code side of things @ti-mo is there any concerns between the change in behaviour here?

tommyp1ckles
tommyp1ckles reviewed May 30, 2023
View reviewed changes
pkg/datapath/linux/config/config.go
@@ -901,10 +901,6 @@ func (h *HeaderfileWriter) writeTemplateConfig(fw *bufio.Writer, e datapath.Endp
fmt.Fprintf(fw, "#define ENABLE_ROUTING 1\n")
}

if !e.DisableSIPVerification() {
Copy link
Contributor

Choose a reason for hiding this comment

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

With this, I think we can just remove this from the EndpointConfiguration interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, let me try to fix that

@oblazek oblazek requested review from a team as code owners June 2, 2023 06:32
@oblazek oblazek requested a review from nebril June 2, 2023 06:32
nebril
nebril approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

Tests lgtm

@oblazek
Copy link
Contributor Author

oblazek commented Jun 12, 2023

rebased with latest main branch.. retrigering tests

@oblazek
Copy link
Contributor Author

oblazek commented Jun 12, 2023

/test

ChengyuanLiCY and others added 3 commits June 15, 2023 10:35
@ChengyuanLiCY @oblazek
fix: Remove the duplicate ENABLE_SIP_VERIFICATION in ep_config.h
7512959 
Currently ENABLE_SIP_VERIFICATION runtime option can define ENABLE_SIP_VERIFICATION macro just like
DisableSIPVerification endpoint datapath option can.
If DatapathConfiguration.DisableSipVerification value is not inline with SourceIPVerification,
the two macros in ep_config.h may conflict.

Secondly calling of SetDefaultConfiguration() func is removed from AddEndpoint() as that
is being called for the second time - although not using this wrapper
but via SetDefaultOpts() directly.

Related slack thread https://cilium.slack.com/archives/C2B917YHE/p1680518197957369

Signed-off-by: Li Chengyuan <chengyuanli@hotmail.com>
Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
@oblazek
endpoint: add endpoint datapath options unit test
1f0dd47 
Add unit test to test SourceIPVerification datapath option
can always be overridden by endpoint specific DisableSipVerification
datapath configuration.

Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
@oblazek
endpoint: remove DisableSIPVerification func from Endpoint iface
20aee91 
Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
@oblazek
Copy link
Contributor Author

oblazek commented Jun 15, 2023

/test

@oblazek
Copy link
Contributor Author

oblazek commented Jun 15, 2023

/ci-gke

@ti-mo ti-mo added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 18, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 18, 2023
@ti-mo ti-mo added the kind/bug This is a bug in the Cilium logic. label Jun 18, 2023
@ti-mo ti-mo merged commit bfa8697 into cilium:main Jun 18, 2023
60 of 61 checks passed
@joestringer joestringer mentioned this pull request Jun 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nebril nebril nebril approved these changes

@ti-mo ti-mo ti-mo approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
kind/bug This is a bug in the Cilium logic. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@oblazek @tommyp1ckles @nebril @ti-mo @ChengyuanLiCY