policy: Adjust existing policy for visibility annotations #16258
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
jrajahalme
added
kind/enhancement
maintainer-s-little-helper
bot
added
dont-merge/needs-release-note-label
|
@aanm This has critical interaction with deny policies, your review would be highly valued :-) |
|
May need to add back the sidecar exception, as we likely get visibility for all HTTP that goes through a Cilium version of a sidecar already. |
|
Only tested with a version of cilium CLI that adds a HTTP visibility annotation to one of the echo pods. In a situation where an L3-only ingress policy is applied, Hubble shows also the HTTP level events: |
jrajahalme
added
the
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
jrajahalme
force-pushed
the
visibility-policy-fix-up
branch
from
963c746
to
a4b6944
Compare
|
test-me-please |
|
Runtime hit by known flake #16221 |
jrajahalme
force-pushed
the
visibility-policy-fix-up
branch
from
a4b6944
to
495e965
Compare
|
Fixed CI test verifying visibility annotation interaction with policy Add/Delete. |
|
test-me-please |
jrajahalme
force-pushed
the
visibility-policy-fix-up
branch
from
495e965
to
3b40862
Compare
|
test-me-please |
|
test-1.20-4.19 |
|
test-1.19-5.4 got 403 error from Vagrant: |
|
test-1.19-5.4 |
|
test-me-please Job 'Cilium-PR-Runtime-4.9' failed and has not been observed before, so may be related to your PR: Click to show.Test NameFailure OutputIf it is a flake, comment Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR: Click to show.Test NameFailure OutputIf it is a flake, comment |
|
/mlh new-flake Cilium-PR-Runtime-4.9 👍 created #17104 |
joestringer
reviewed
Aug 9, 2021
jrajahalme
force-pushed
the
visibility-policy-fix-up
branch
from
b155993
to
daeefd5
Compare
|
test-me-please Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR: Click to show.Test NameFailure OutputIf it is a flake, comment |
|
CI fails seem like unrelated flakes to me.. |
|
net-next hit by #16971 |
jrajahalme
force-pushed
the
visibility-policy-fix-up
branch
from
daeefd5
to
b1a81ea
Compare
|
rebased to pick up CI fixes. |
|
test-me-please |
christarazi
added
the
dont-merge/needs-rebase
Allow other types in addition to CachedSelectors to own MapState entries. This is needed to properly track entires added due to deny rules so that incremental map updates can be performed correctly. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
MapState internal state could be modified by callers via internal maps. Nil them on entries stored outside of the MapState itself. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
MapState entries that should be deleted together with some other entry are "dependent entries". These are created for some deny entries. Keep track of them, and delete them when the main entry is deleted. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Adjust and expand eBPF policy map to redirect for visibility on the
port of the visibility annotation while still denying traffic on this
port for identities for which the traffic is denied.
Datapath lookup order is, from highest to lowest precedence:
1. L3/L4
2. L4-only (wildcard L3)
3. L3-only (wildcard L4)
4. Allow-all
This means that the L4-only allow visibility key can only be added if
there is an allow-all key, and all L3-only deny keys are expanded to
L3/L4 keys. If no L4-only key is added then also the L3-only allow
keys need to be expanded to L3/L4 keys for visibility redirection. In
addition the existing L3/L4 and L4-only allow keys need to be
redirected to the proxy port, if not already redirected.
The above can be accomplished by:
1. Change existing L4-only ALLOW key on matching port that does not already
redirect to redirect.
- e.g., 0:80=allow,0 -> 0:80=allow,<proxyport>
2. If allow-all key exists, add L4-only visibility redirect key if the L4-only
key does not already exist.
- e.g., 0:0=allow,0 -> add 0:80=allow,<proxyport> if 0:80 key does not exist
- this allows all traffic on port 80, but see step 5 below
3. Change all L3/L4 ALLOW keys on matching port that do not already redirect to
redirect.
- e.g, <ID1>:80=allow,0 -> <ID1>:80=allow,<proxyport>
4. For each L3-only ALLOW key add the corresponding L3/L4 ALLOW redirect if no
L3/L4 key already exists and no L4-only key already exists and one is not added.
- e.g., <ID2>:0=allow,0 -> add <ID2>:80=allow,<proxyport> if <ID2>:80
and 0:80 do not exist and 0:80 was not added
5. If a new L4-only key was added: For each L3-only DENY key add the
corresponding L3/L4 DENY key if no L3/L4 key already exists
- e.g., <ID3>:0=deny,0 -> add <ID3>:80=deny,0 if <ID3>:80 does not exist
With the above we only change/expand existing allow keys to redirect, and
expand existing drop keys to also drop on the port of interest, if a new
L4-only key allowing the port is added.
Adjust the unit test to mimic the real behavior a bit better by
regenerating endpoint policy after updating visibility policy. This is
needed due to the test using same port for HTTP and Kafka, and without
regenerating visibility redirects no longer overwrite existing
redirects.
Visibility redirects need to be re-applied after applying incremental
policy map changes. This is due to incremental changes possibly adding
keys for new identities that need visibility redirections or
additional deny keys as defined above.
Testing: New unit tests in policy package cover all the cases
described above.
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
force-pushed
the
visibility-policy-fix-up
branch
from
b1a81ea
to
0afc0c0
Compare
|
Rebased. |
|
test-me-please |
tklauser
removed
the
dont-merge/needs-rebase
|
Unrelated Travis-CI integration test flake #11560 on amd64, arm64 OK. |
jrajahalme
added
the
ready-to-merge
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.
Adjust and expand eBPF policy map keys and values to redirect for
visibility on the port of the visibility annotation while still
denying traffic on this port for identities for which the traffic is
denied.
Datapath lookup order is, from highest to lowest precedence:
This means that the L4-only allow visibility key can only be added if
there is an allow-all key, and all L3-only deny keys are expanded to
L3/L4 keys. If no L4-only key is added then also the L3-only allow
keys need to be expanded to L3/L4 keys for visibility redirection. In
addition the existing L3/L4 and L4-only allow keys need to be
redirected to the proxy port, if not already redirected.
The above can be accomplished by:
redirect to redirect.
key does not already exist.
redirect.
L3/L4 key already exists and no L4-only key already exists and one is not added.
and 0:80 do not exist and 0:80 was not added
corresponding L3/L4 DENY key if no L3/L4 key already exists
With the above we only change/expand existing allow keys to redirect, and
expand existing drop keys to also drop on the port of interest, if a new
L4-only key allowing the port is added.
Adjust the unit test to mimic the real behavior a bit better by
regenerating endpoint policy after updating visibility policy. This is
needed due to the test using same port for HTTP and Kafka, and without
regenerating visibility redirects no longer overwrite existing
redirects.
Visibility redirects need to be re-applied after applying incremental
policy map changes. This is due to incremental changes possibly adding
keys for new identities that need visibility redirections or
additional deny keys as defined above.
Testing: New unit tests in policy package cover all the cases
described above.
Fixes: #9088