Auth use signalmap #25284
Merged
Auth use signalmap #25284
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
jrajahalme
added
area/servicemesh
jrajahalme
force-pushed
the
auth-use-signalmap
branch
from
1645664
to
90d5e49
Compare
jrajahalme
changed the title
jrajahalme
force-pushed
the
auth-use-signalmap
branch
from
90d5e49
to
1b1d397
Compare
jrajahalme
added
the
release-note/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
jrajahalme
force-pushed
the
auth-use-signalmap
branch
from
1b1d397
to
a4e65b5
Compare
|
Commit a4e65b5e26948f65e5c887e4e5ec58face823bbb does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
maintainer-s-little-helper
bot
added
the
dont-merge/needs-sign-off
jrajahalme
force-pushed
the
auth-use-signalmap
branch
from
a4e65b5
to
b1b563d
Compare
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-sign-off
jrajahalme
force-pushed
the
auth-use-signalmap
branch
from
b1b563d
to
22ef3c6
Compare
|
/test-1.26-net-next |
jrajahalme
commented
May 16, 2023
|
|
||
| var ( | ||
| log = logging.DefaultLogger.WithField(logfields.LogSubsys, "signalmap") | ||
| ) |
There was a problem hiding this comment.
Will drop this log in favour of logrus.FieldLogger provided by Hive in a forthcoming update.
|
/test-1.26-net-next |
|
net-next did not trigger, tried again. |
|
/test-1.26-net-next |
1 similar comment
|
/test-1.26-net-next |
pippolo84
approved these changes
May 16, 2023
Comment on lines
+20
to
+31
| const ( | ||
| // SignalProtoV4 denotes IPv4 protocol | ||
| SignalProtoV4 SignalData = iota | ||
| // SignalProtoV6 denotes IPv6 protocol | ||
| SignalProtoV6 | ||
| SignalProtoMax | ||
| ) | ||
|
|
||
| var signalProto = [SignalProtoMax]string{ | ||
| SignalProtoV4: "ipv4", | ||
| SignalProtoV6: "ipv6", | ||
| } |
There was a problem hiding this comment.
Nit: we can get rid of the SignalProtoMax constant by letting the compiler calculate the size of the signalProto array with
var signalProto = [...]string{
SignalProtoV4: "ipv4",
SignalProtoV6: "ipv6",
}
Comment on lines
+200
to
+205
| if ch == nil { | ||
| return "", ErrNilChannel | ||
| } |
There was a problem hiding this comment.
I suggest to move this out of the returned func, since ch is not gonna change after the call to ChannelHandler. Doing that we can also avoid generating a func that always returns "", ErrNilChannel.
|
|
||
| func newManager(params authManagerParams) (Manager, error) { | ||
| mgr, err := newAuthManager(params.AuthHandlers, params.DatapathAuthenticator, params.IPCache) | ||
| func newManager(params authManagerParams) error { |
There was a problem hiding this comment.
Nit: we usually call functions to be invoked as registerX, leaving newX for the ones that provide API to the hive.
Add GetNodeIP(nodeID uint16) that returns the node IP for which the given nodeID was allocated for. This will be used for authentication handshake in later commits. Check for 0 (== local node) explicitly and return local node address from pkg/node in that case, as local node IPs are not in the maps. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Maximum nunber of entries in both eventsmap and signalmap is the number of possible CPUs, which never changes, is a low number, and is not adding any real information to the status report. Remove them to reduce direct dependencies to these maps. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Move definition of the signal data to the signal handlers to limit go dependencies. Signal delivery channel is now buffered so that signal manager will not get blocked by any one of the signal handlers. Pause/Unpause is generalized so that the perf events are paused only when all the signals have been paused. Current implementation uses bit mask (64 bits) which limits the max number of signals to 64. Signal registration is to be done only from init or cell OnStart functions, so that no locking is needed for managing signal handlers. Mute/Unmute calls can happen concurrently. Manage dependencies via Hive/Cell. Add a fake signalmap so that controlplane tests may depend on it. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Use signalmap instead of monitor events to notify userspace of missing authentication. Execute authentications concurrently and check for pending or completed authentications before starting a new one with the same key. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
force-pushed
the
auth-use-signalmap
branch
from
050d639
to
b4d31dc
Compare
|
Rebased to resolve conflict |
|
/ci-e2e |
|
/ci-l4lb |
|
/ci-multicluster |
|
/ci-awscni |
|
/ci-eks |
|
/ci-verifier |
|
/ci-external-workloads |
nebril
approved these changes
May 23, 2023
maintainer-s-little-helper
bot
added
the
ready-to-merge
bimmlerd
added a commit
that referenced
this pull request
Jun 6, 2023
[ upstream commit 8294624 ] [ backporter notes: conflicts due to moved files, #25284 not yet backported, hence no GetNodeIP() to move.] The NodeHandler interface is too large, as can be seen in various implementations which are only implementing a subset of methods. This patch splits off the NodeID handling part, and removes the stub methods from noop implementations. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
dylandreimerink
pushed a commit
that referenced
this pull request
Jun 9, 2023
[ upstream commit 8294624 ] [ backporter notes: conflicts due to moved files, #25284 not yet backported, hence no GetNodeIP() to move.] The NodeHandler interface is too large, as can be seen in various implementations which are only implementing a subset of methods. This patch splits off the NodeID handling part, and removes the stub methods from noop implementations. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
jrajahalme
added a commit
to jrajahalme/image-tools
that referenced
this pull request
Jun 13, 2023
cilium/cilium has ignored MACRO_ARG_REUSE since cilium/cilium#25284, update the default ignores so that this does not need to be explicitly added in cilium/cilium any more. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
pchaigno
pushed a commit
that referenced
this pull request
Jan 8, 2024
[ upstream commit 8294624 ] [ backporter notes: conflicts due to moved files, #25284 not yet backported, hence no GetNodeIP() to move.] The NodeHandler interface is too large, as can be seen in various implementations which are only implementing a subset of methods. This patch splits off the NodeID handling part, and removes the stub methods from noop implementations. Signed-off-by: David Bimmler <david.bimmler@isovalent.com> Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Agent implementation of
pkg/signalmapis generalized from a single target (conntrack GC) to allow for multiple targets. This is then used for passing policy notifications for required authentication from datapath to the agent instead of using the monitor perf ring buffer. This should reduce CPU overhead when monitoring is otherwise not used.