Skip to content

Conversation

bimmlerd
Copy link
Member

@bimmlerd bimmlerd commented Oct 2, 2023

The first commit converts the MapState type from a newtype an interface (mostly Nate's work, rebased on newer main)

The second commit then splits the map in the struct into two - to track allows and denies seperately.

This allows for an optimization in the third commit: denyPreferredInserts for the allow case, now doesn't have to loop through all other allows (which we don't care about).

Commit msgs for convenience:

policy: Make MapState an interface

`MapState` has many methods, some of which are public.
In order to keep state to accomplish creating a cohesive
map state that can keep state besides what will be put into
an ebpf map, `MapState` can no longer be a type declaration
for a golang map, but must be a struct that keeps state.

The endpoint package must also be refactored to use `MapState`
as an interface rather than a instantiated type of a map.

mapstate: split allows and denies

Now that mapState is a struct, we can split the tracking of allows and
denies. This commit should not introduce functional changes, but
prepares us for an optimization in a later commit.

mapstate: optimize denyPreferredInsert

Since the denyPreferredInsert logic cleanly splits on whether the to be
inserted entry is a deny entry, we can avoid iterating over some
entries: If we are inserting a deny, we are mostly interested in
iteration over existing allows, and vice versa.

In the case where we have a FQDN policy for a hostname which resolves to
many IPs (for example with a fqdn policy covering Amazon's S3), this
becomes a hot function (without performing a lot of useful work). The S3
workload is expected to be heavily skewed towards allow entries for the
many CIDR identities allocated for S3. When we insert such an entry, we
only need to iterate deny entries, of which there are expected to be
much fewer. Now that the tracking is split, we can employ this fact and,
instead of iterating all entries, iterate only the deny entries.

@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 2, 2023
@bimmlerd bimmlerd changed the title Pr/bimmlerd/split mapstate allow deny Split mapstate keys into allow and deny Oct 2, 2023
@bimmlerd bimmlerd added area/daemon Impacts operation of the Cilium daemon. kind/performance There is a performance impact of this. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. area/agent Cilium agent related. labels Oct 2, 2023
@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 2, 2023
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/split-mapstate-allow-deny branch 4 times, most recently from 714daaa to 2fb53b8 Compare October 2, 2023 13:05
@bimmlerd
Copy link
Member Author

bimmlerd commented Oct 2, 2023

/test

@bimmlerd bimmlerd force-pushed the pr/bimmlerd/split-mapstate-allow-deny branch 4 times, most recently from 4bd82fd to 7e771e7 Compare October 3, 2023 07:17
@bimmlerd
Copy link
Member Author

bimmlerd commented Oct 3, 2023

/test

@bimmlerd bimmlerd force-pushed the pr/bimmlerd/split-mapstate-allow-deny branch from 7e771e7 to 477f805 Compare October 3, 2023 13:07
@bimmlerd
Copy link
Member Author

bimmlerd commented Oct 3, 2023

/ci-ginkgo

@bimmlerd
Copy link
Member Author

bimmlerd commented Oct 3, 2023

/test

@bimmlerd
Copy link
Member Author

bimmlerd commented Oct 3, 2023

/ci-gateway-api

Rerunning. Filed #28374 for the issue which occurred.

@bimmlerd bimmlerd marked this pull request as ready for review October 3, 2023 14:49
@bimmlerd bimmlerd requested review from a team as code owners October 3, 2023 14:49
@bimmlerd bimmlerd requested a review from nathanjsweet October 3, 2023 14:49
@jrajahalme
Copy link
Member

@bimmlerd Changed v1.12 backport label to needs-backport/1.12 to be able to complete the release process. Looks like we can't have any pending backports when doing the release.

@tklauser tklauser added the backport/author The backport will be carried out by the author of the PR. label Oct 24, 2023
@tklauser
Copy link
Member

@bimmlerd Added backport/author label because I assume you'll take care of manually backporting this. Automated backports are leading to merge conflicts.

@bimmlerd
Copy link
Member Author

removing backport-pending labels here, as the project has decided against backporting this change -- it's too invasive for extensive backporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent Cilium agent related. area/daemon Impacts operation of the Cilium daemon. backport/author The backport will be carried out by the author of the PR. kind/performance There is a performance impact of this. 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. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants