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

auth: Use authmap for auth_required policies #24410

Merged
merged 5 commits into from Apr 10, 2023
Merged

auth: Use authmap for auth_required policies #24410

merged 5 commits into from Apr 10, 2023

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Mar 16, 2023 •
edited

Use the auth map (added in PR #24218) for auth_required policy instead of the CT map.

@jrajahalme jrajahalme added release-note/misc This PR makes changes that have no direct user impact. area/servicemesh GH issues or PRs regarding servicemesh labels Mar 16, 2023
@jrajahalme jrajahalme requested review from a team as code owners March 16, 2023 11:30
@jrajahalme jrajahalme requested a review from a team as a code owner March 16, 2023 12:41
@jrajahalme jrajahalme requested a review from jibi March 16, 2023 12:41
@mhofstetter mhofstetter mentioned this pull request Mar 16, 2023
Merged
squeed
squeed approved these changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

I only reviewed my files; looks fine.

youngnick
youngnick approved these changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM for the Go bits.

@jrajahalme
Copy link
Member Author

/test

joestringer
joestringer reviewed Mar 20, 2023
View reviewed changes
bpf/lib/host_firewall.h Outdated Show resolved Hide resolved
bpf/lib/host_firewall.h Show resolved Hide resolved
pkg/auth/authmap_authenticator.go Outdated Show resolved Hide resolved
pkg/auth/authmap_authenticator.go Outdated Show resolved Hide resolved
bpf/lib/auth.h Outdated Show resolved Hide resolved
bpf/bpf_lxc.c Outdated Show resolved Hide resolved
pkg/auth/manager.go Show resolved Hide resolved
@meyskens
Copy link
Member

I did run this to test in a e2e test and I got a nil pointer that seems to be related to this

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x211c76b]

goroutine 225 [running]:
github.com/cilium/cilium/pkg/maps/authmap.(*Map).Update(0x0, 0xdbacfb33?, 0xe?, 0x0?, 0x0?, 0x10ce03058200)
	/go/src/github.com/cilium/cilium/pkg/maps/authmap/auth_map.go:87 +0x6b
github.com/cilium/cilium/pkg/auth.(*authMapAuthenticator).markAuthenticated(0xc0010eaa20, 0xc00248ae00)
	/go/src/github.com/cilium/cilium/pkg/auth/authmap_authenticator.go:38 +0x6f
github.com/cilium/cilium/pkg/auth.(*authManager).authRequired(0xc0010dfcb0, 0xc002488c30, 0xc0013994a0)
	/go/src/github.com/cilium/cilium/pkg/auth/manager.go:120 +0x3f3
github.com/cilium/cilium/pkg/auth.(*authManager).AuthRequired(0xc000d74090?, 0x84?, 0x84?)
	/go/src/github.com/cilium/cilium/pkg/auth/manager.go:81 +0x2e
github.com/cilium/cilium/pkg/auth/monitor.(*dropMonitor).startEventProcessing.func1()
	/go/src/github.com/cilium/cilium/pkg/auth/monitor/monitor.go:93 +0x4c
created by github.com/cilium/cilium/pkg/auth/monitor.(*dropMonitor).startEventProcessing
	/go/src/github.com/cilium/cilium/pkg/auth/monitor/monitor.go:84 +0x56

@pchaigno pchaigno mentioned this pull request Mar 22, 2023
Closed
@jrajahalme jrajahalme force-pushed the cilium-mtls-poc branch 3 times, most recently from 030d83b to 65cf37e Compare March 23, 2023 12:17
@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

bpf test fails are due to master breakage, not by this PR. There is a separate PR #24534 for the fix.

@jrajahalme
Copy link
Member Author

Rebased to pick up master fixes.

@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme requested a review from joamaki April 5, 2023 20:59
@jrajahalme
Copy link
Member Author

jrajahalme commented Apr 5, 2023 •
edited by maintainer-s-little-helper bot

/test

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

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN and endpoint routes

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testclient-host-llw5n

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

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.

@jrajahalme
Copy link
Member Author

Restarted ci-eks due to timeout waiting for pods to become ready on install.

@jrajahalme
Copy link
Member Author

ci-eks fail due to:

2023-04-06T01:39:22.882453745Z level=warning msg="Unable to synchronize EC2 VPC list" error="operation error EC2: DescribeVpcs, exceeded maximum number of attempts, 3, https response error StatusCode: 0, RequestID: , request send failed, Post \"https://ec2.us-east-2.amazonaws.com/\": dial tcp: lookup ec2.us-east-2.amazonaws.com on 10.100.0.10:53: write udp 192.168.143.93:42454->10.100.0.10:53: write: operation not permitted" subsys=eni

@jrajahalme
Copy link
Member Author

net-next failed on k8s auth error:

error: unable to upgrade connection: Authorization error (user=kube-apiserver-kubelet-client, verb=create, resource=nodes, subresource=proxy)

@jrajahalme
Copy link
Member Author

/test-1.26-net-next

@jrajahalme
Copy link
Member Author

jrajahalme commented Apr 6, 2023 •
edited by maintainer-s-little-helper bot

/test-1.26-net-next

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

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing Tests with XDP, direct routing, DSR and Maglev

Failure Output

FAIL: Can not connect to service "tftp://[fd04::11]:30411/hello" from outside cluster (10/10)

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

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.

@jrajahalme
Copy link
Member Author

/ci-eks

lmb
lmb approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@lmb lmb left a comment

Choose a reason for hiding this comment

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

configmap changes LGTM! Thanks for reworking :)

bpf/lib/maps.h Show resolved Hide resolved
@jrajahalme
maps/auth: rename package to authmap
da63d4d 
Rename pkg/maps/auth as pkg/maps/authmap to avoid requiring explicitly
importing it as "authmap" when also importing the pkg/auth.

Most of the other maps packages have the "map" suffix, so adds to this
consistency in package naming.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
maps: Add datapath runtime config map (configmap)
4283aa7 
Add a new config map for passing runtime configuration values that may
change often enough to not warrant recompiling the affected bpf programs,
and whose changes will not have an effect to the structure of the bpf
programs.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
utime: Add runtime config for time syncing with the datapath
faac8ec 
Define "utime" as positive Unix epoch time, in 512 nanosecod units. This
unit is chosen as the largest exponentian of base 2 that yields a whole
integer multiplier to get from seconds to utime units without any loss of
accuracy, and to gain more future time range. "utime" is an unsigned 64
bit integer, so the range is from the Unix epoch (1.1.1970) to September
3rd of year 301261.

With this formulation, the result of ktime_get_ns() can be shifted right
by 9 bits to gain the current monotonic clock in "utime" units.

Finally, this commit adds an entry to the configmap, the offset
to add to the shifted monotonic clock to get the current time in Unix
epoch time. This offset is periodically updated by Cilium agent to
account for the boot time of the system, any NTP time jumps, and the
difference between boottime and monotonic clocks (such as suspend time).

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
datapath: Use auth map for policy
db36e55 
Use the new auth map for auth_required policy instead of the CT map.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
Revert "bpf: Add CT map flag for auth required"
8cd3c52 
This reverts commit 1f7ed72.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
jrajahalme commented Apr 6, 2023
View reviewed changes
Copy link
Member Author

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Updated to not move from_tunnel bit in conntrack flags around.

bpf/lib/maps.h Show resolved Hide resolved
bpf/lib/common.h Show resolved Hide resolved
@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

Travis test had failed on lbipam and inctimer, which were unrelated flakes, restarted.

@jrajahalme
Copy link
Member Author

ci-eks is failing on almost every run (last successful run is from yesterday).

@jrajahalme
Copy link
Member Author

ci-eks is broken, so disregarding it and marking this as ready-to-merge.

@jrajahalme jrajahalme added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 7, 2023
@joestringer
Copy link
Member

@jrajahalme is there an issue filed for the ci-eks breakage? Is someone following up on that, do we need to mark the job as "not required"? We shouldn't be marking ready-to-merge without a plan on how to address CI failures, that is a recipe for beginning to ignore every failure in CI.

@dylandreimerink
Copy link
Member

The ci-eks breakage is being tracked in #24774. Lets continue the discussion about temporary fixes there. I believe it should not hold back this PR at this time.

@dylandreimerink dylandreimerink merged commit 2216111 into cilium:master Apr 10, 2023
56 of 57 checks passed
@dylandreimerink dylandreimerink mentioned this pull request Apr 10, 2023
Closed
@jrajahalme jrajahalme mentioned this pull request Apr 11, 2023
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

@ti-mo ti-mo ti-mo left review comments

@mhofstetter mhofstetter mhofstetter approved these changes

@lmb lmb lmb approved these changes

@squeed squeed squeed approved these changes

@joamaki joamaki joamaki approved these changes

@ldelossa ldelossa ldelossa approved these changes

@aanm aanm aanm approved these changes

@youngnick youngnick youngnick approved these changes

@jibi jibi Awaiting requested review from jibi jibi was automatically assigned from cilium/build

Assignees
No one assigned
Labels
area/servicemesh GH issues or PRs regarding servicemesh 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet