Skip to content

Refactor Daemon Identity allocation routines#17724

Closed
joestringer wants to merge 6 commits intocilium:masterfrom
joestringer:submit/testutils-identity-refactor
Closed

Refactor Daemon Identity allocation routines#17724
joestringer wants to merge 6 commits intocilium:masterfrom
joestringer:submit/testutils-identity-refactor

Conversation

@joestringer
Copy link
Copy Markdown
Member

@joestringer joestringer commented Oct 27, 2021

See commit messages for more details. No functional changes intended.

Used to implement fix #17699 .

@joestringer joestringer requested a review from a team October 27, 2021 21:52
@joestringer joestringer requested review from a team as code owners October 27, 2021 21:52
@joestringer joestringer requested review from a team October 27, 2021 21:52
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Oct 27, 2021
@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 Oct 27, 2021
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 27, 2021
@joestringer joestringer requested review from a team as code owners October 27, 2021 23:03
@joestringer joestringer marked this pull request as draft October 27, 2021 23:04
@joestringer joestringer changed the title testutils: Refactor FakeIdentityAllocator definition and usage testutils: Refactor Daemon Identity allocation routines Oct 27, 2021
@joestringer joestringer changed the title testutils: Refactor Daemon Identity allocation routines Refactor Daemon Identity allocation routines Oct 27, 2021
@joestringer
Copy link
Copy Markdown
Member Author

/test

Comment thread daemon/cmd/daemon.go Outdated
@joestringer joestringer force-pushed the submit/testutils-identity-refactor branch from c81195f to 6dac5dd Compare October 28, 2021 21:46
@joestringer
Copy link
Copy Markdown
Member Author

/test

@joestringer joestringer force-pushed the submit/testutils-identity-refactor branch 2 times, most recently from 3d26502 to 94c7e81 Compare October 28, 2021 22:39
@joestringer
Copy link
Copy Markdown
Member Author

joestringer commented Oct 28, 2021

@joestringer joestringer force-pushed the submit/testutils-identity-refactor branch from 94c7e81 to d32dc28 Compare October 29, 2021 00:36
@joestringer
Copy link
Copy Markdown
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>
@joestringer joestringer force-pushed the submit/testutils-identity-refactor branch from d32dc28 to 1af9539 Compare November 1, 2021 21:44
@joestringer
Copy link
Copy Markdown
Member Author

/test

@joestringer
Copy link
Copy Markdown
Member Author

Hit #12756 in Travis, ignoring known flake.

@joestringer joestringer marked this pull request as ready for review November 1, 2021 23:51
Copy link
Copy Markdown
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Copy Markdown
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

Nice refactor 👍

Copy link
Copy Markdown
Member

@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.

LGTM

@joestringer
Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants