Skip to content

option: Require native-routing-cidr only if IPv4 is enabled#12198

Merged
borkmann merged 1 commit intomasterfrom
pr/brb/fix-native-cidr-check
Jun 19, 2020
Merged

option: Require native-routing-cidr only if IPv4 is enabled#12198
borkmann merged 1 commit intomasterfrom
pr/brb/fix-native-cidr-check

Conversation

@brb
Copy link
Copy Markdown
Member

@brb brb commented Jun 19, 2020

Otherwise, when running with IPv6-only the agent fails with the
following:

level=fatal msg="Error while creating daemon" error="invalid daemon
configuration: native routing cidr must be configured with option
--native-routing-cidr in combination with --masquerade --tunnel=disabled
--ipam=hostscope-legacy" subsys=daemon

Also, we currently do not masquerade IPv6.

Fixes: e7d4f5c ("daemon: validate IPv4NativeRoutingCIDR value in DaemonConfig")

@brb brb added kind/bug This is a bug in the Cilium logic. pending-review area/daemon Impacts operation of the Cilium daemon. labels Jun 19, 2020
@brb brb requested a review from a team June 19, 2020 08:49
@brb brb requested a review from a team as a code owner June 19, 2020 08:49
@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Jun 19, 2020

Please set the appropriate release note label.

5 similar comments
@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Jun 19, 2020

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Jun 19, 2020

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Jun 19, 2020

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Jun 19, 2020

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Jun 19, 2020

Please set the appropriate release note label.

@brb brb added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 19, 2020
@brb
Copy link
Copy Markdown
Member Author

brb commented Jun 19, 2020

test-me-please

@rolinh rolinh self-requested a review June 19, 2020 09:06
@rolinh rolinh mentioned this pull request Jun 19, 2020
Closed
rolinh
rolinh approved these changes Jun 19, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

LGTM as tested on an IPv6 only cluster (although had to file #12201).

@borkmann
Copy link
Copy Markdown
Member

borkmann commented Jun 19, 2020

@brb Travis CI failed:

-- PASS: TestBPFMapSizeCalculation (0.00s)
    --- PASS: TestBPFMapSizeCalculation/static_default_sizes (0.00s)
    --- PASS: TestBPFMapSizeCalculation/static,_non-default_sizes_inside_range (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_without_any_static_sizes_(512MB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_without_any_static_sizes_(1GiB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_without_any_static_sizes_(2GiB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_without_any_static_sizes_(7.5GiB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_without_any_static_sizes_(16GiB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_without_any_static_sizes_(120GiB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_without_any_static_sizes_(240GiB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_without_any_static_sizes_(360GiB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/dynamic_size_with_static_CT_TCP_size_(4GiB,_0.25%) (0.00s)
    --- PASS: TestBPFMapSizeCalculation/huge_dynamic_size_ratio_gets_clamped_(8GiB,_98%) (0.00s)
=== RUN   Test
/tmp/check-5577006791947779410/1/test
OK: 26 passed
--- PASS: Test (0.02s)
FAIL
coverage: 5.6% of statements in ./...
FAIL	github.com/cilium/cilium/pkg/option	0.100s
FAIL
Makefile:217: recipe for target 'unit-tests' failed
make: *** [unit-tests] Error 1
The command "./.travis/build.sh" exited with 2.

@brb brb force-pushed the pr/brb/fix-native-cidr-check branch from 9f025c9 to 140c8de Compare June 19, 2020 11:44
@brb
option: Require native-routing-cidr only if IPv4 is enabled
140c8de 
Otherwise, when running with IPv6-only the agent fails with the
following:

    level=fatal msg="Error while creating daemon" error="invalid daemon
    configuration: native routing cidr must be configured with option
    --native-routing-cidr in combination with --masquerade --tunnel=disabled
    --ipam=hostscope-legacy" subsys=daemon

Also, we currently do not masquerade IPv6.

Fixes: e7d4f5c ("daemon: validate IPv4NativeRoutingCIDR value in DaemonConfig")
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Copy Markdown
Member Author

brb commented Jun 19, 2020

All the CI jobs have passed previously, and I've just modified the config_test.go unit test. So, after Travis CI and lint are green, we can merge it.

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 19, 2020

Coverage Status

Coverage decreased (-0.03%) to 37.111% when pulling 140c8de on pr/brb/fix-native-cidr-check into 9a027ee on master.

@borkmann
Copy link
Copy Markdown
Member

borkmann commented Jun 19, 2020

Travis and lint all green now, merging.

@borkmann borkmann merged commit 93d32dd into master Jun 19, 2020
@borkmann borkmann deleted the pr/brb/fix-native-cidr-check branch June 19, 2020 12:32
@borkmann borkmann mentioned this pull request Jun 19, 2020
Merged
@christarazi christarazi mentioned this pull request Jul 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rolinh rolinh rolinh approved these changes

@aanm aanm aanm approved these changes

+1 more reviewer

@tgraf tgraf tgraf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/daemon Impacts operation of the Cilium daemon. kind/bug This is a bug in the Cilium logic. 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.

6 participants

@brb @borkmann @coveralls @rolinh @tgraf @aanm