Refactor Daemon Identity allocation routines#17724
Closed
joestringer wants to merge 6 commits intocilium:masterfrom
Closed
Refactor Daemon Identity allocation routines#17724joestringer wants to merge 6 commits intocilium:masterfrom
joestringer wants to merge 6 commits intocilium:masterfrom
Conversation
Member
Author
|
/test |
christarazi
reviewed
Oct 28, 2021
c81195f to
6dac5dd
Compare
Member
Author
|
/test |
3d26502 to
94c7e81
Compare
Member
Author
|
/test EDIT: Mostly just unit test failures. The weird one is this: https://jenkins.cilium.io/job/Cilium-PR-Runtime-net-next/477/testReport/junit/(root)/Suite-runtime/RuntimePolicies_TestsEgressToHost_Tests_egress_with_CIDR_L4_policy_to_external_https_service/ |
94c7e81 to
d32dc28
Compare
Member
Author
|
/test-runtime |
The mock identity notifier and identity allocator make sense together under pkg/testutils/identity, and refactoring like this will be required by upcoming commits in order to avoid circular imports. Signed-off-by: Joe Stringer <joe@cilium.io>
This constructor will simplify upcoming commits by performing all of the necessary initialization for the FakeIdentityAllocator in all tests that make use of this structure. Signed-off-by: Joe Stringer <joe@cilium.io>
Upcoming commits will rely on the daemon passing the identityAllocator module down to the fqdn subsystem via the policy repository. For now, adjust the function signatures to pass this through, but avoid actually using the new parameter for now. This commit is a functional no-op but reduces the noise in terms of code churn from upcoming commits. Signed-off-by: Joe Stringer <joe@cilium.io>
In order to test an upcoming bugfix, the internal FQDN handling functions that are triggered from the unit tests must be able to refer to an abstract identity allocator. This will then be mocked out by the tests. This commit extends the existing cache.IdentityAllocator interface to support allocating identities for CIDRs. A future commit will also add Identity release functions to the same abstraction. This also paves the way for some improvements to code structure: Some of the FQDN logic under `daemon/cmd/fqdn.go` may be able to be pushed down into pkg/fqdn now that it doesn't directly reference the globals from the ipcache package. There's another remaining open question around code structure for how the IPcache and identity allocation packages handle the pattern for allocating CIDR identities vs. injecting those identities into the underlying IPcache maps (eg in eBPF). This commit shies away from solving that problem right now in order to keep the changes minimal as this code is likely to be proposed for backport. Signed-off-by: Joe Stringer <joe@cilium.io>
An upcoming commit will introduce a new variation on the release function, so just rename this one here to simplify the upcoming changes. Signed-off-by: Joe Stringer <joe@cilium.io>
Everything that goes up must eventually come down. And so it is also with everything that is allocated; it must be deallocated. Extend the IdentityAllocator interface to also support CIDR release functions and plumb through an initial implementation. This is currently not hooked into the agent itself, this will come with an upcoming bugfix instead. Signed-off-by: Joe Stringer <joe@cilium.io>
d32dc28 to
1af9539
Compare
Member
Author
|
/test |
Member
Author
|
Hit #12756 in Travis, ignoring known flake. |
8 tasks
Member
Author
|
This was reviewed here and also in #17699; I rebased that PR so I think it's sufficient to just merge that version and drop this PR. |
This file contains hidden or 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
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.
See commit messages for more details. No functional changes intended.
Used to implement fix #17699 .