Resolve named ports for DNS policies #29023
Merged
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
release-note/minor
|
/test |
|
/test |
jrajahalme
force-pushed
the
endpoint-resolve-named-ports-for-redirects
branch
from
8bb76fd
to
0e1e44c
Compare
|
rebased for CI fixes |
|
/test |
jrajahalme
force-pushed
the
endpoint-resolve-named-ports-for-redirects
branch
from
0e1e44c
to
6f603da
Compare
jrajahalme
force-pushed
the
endpoint-resolve-named-ports-for-redirects
branch
from
6f603da
to
19c78bc
Compare
|
/test |
aditighag
reviewed
Nov 16, 2023
There was a problem hiding this comment.
I don't have much context. Perhaps it's best reviewed by someone from the policy team. /cc @cilium/sig-policy
Since DNS port is typically 53, it seems likely that named ports would not be used in DNS policies in practice.
I don't quite follow this. K8s mandates named ports when there are more than one port defined in a pod spec.
jrajahalme
changed the title
I was referring to use in CNPs, which for a long time did not support named ports at all, so all examples of DNS policies use the port number |
aanm
approved these changes
Nov 18, 2023
maintainer-s-little-helper
bot
added
ready-to-merge
aanm
removed
the
ready-to-merge
christarazi
approved these changes
Nov 20, 2023
There was a problem hiding this comment.
LGTM, I agree with @aanm's suggestion which simplifies the code.
maintainer-s-little-helper
bot
added
the
ready-to-merge
christarazi
removed
the
ready-to-merge
jrajahalme
force-pushed
the
endpoint-resolve-named-ports-for-redirects
branch
from
19c78bc
to
c9c6a16
Compare
Adding more tests fails due to missing selector cache notifier mock and leaking of endpoint identities in a global manager between tests. Address these and also remove unnecessary logging. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Implement l4Policy wrapper with resolved named port so that proxy package can get the actual resolved named port number when calling GetPort(). Previously, if policy used a named port, GetPort returned 0. GetPort() is only used for DNS proxy function UpdateAllowed(). This means that using a named port for a DNS policy destination port likely has not functioned as intended. Since DNS port is typically 53, it seems likely that named ports are not used in DNS policies. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
force-pushed
the
endpoint-resolve-named-ports-for-redirects
branch
from
c9c6a16
to
fdbca05
Compare
|
/test |
maintainer-s-little-helper
bot
added
the
ready-to-merge
jrajahalme
added a commit
to jrajahalme/cilium
that referenced
this pull request
Dec 1, 2023
Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: cilium#29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
added a commit
to jrajahalme/cilium
that referenced
this pull request
Dec 4, 2023
Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: cilium#29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
added a commit
to jrajahalme/cilium
that referenced
this pull request
Dec 5, 2023
Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: cilium#29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
added a commit
to jrajahalme/cilium
that referenced
this pull request
Dec 6, 2023
Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: cilium#29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
added a commit
to jrajahalme/cilium
that referenced
this pull request
Dec 12, 2023
Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: cilium#29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
added a commit
to jrajahalme/cilium
that referenced
this pull request
Dec 12, 2023
Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: cilium#29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
pippolo84
pushed a commit
to pippolo84/cilium
that referenced
this pull request
Jan 2, 2024
[ upstream commit a573bb4 ] Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: cilium#29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com> Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
dylandreimerink
pushed a commit
that referenced
this pull request
Jan 12, 2024
[ upstream commit a573bb4 ] Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: #29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com> Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
jrajahalme
added
the
needs-backport/1.15
joamaki
added
backport-pending/1.15
sayboras
pushed a commit
that referenced
this pull request
Jun 10, 2024
[ upstream commit a573bb4 ] Commit 10f04fd (endpoint: Resolve named ports for redirects) fixed redirect creation for L7 policies using a named port, but failed to use the resolved destination port also in proxy stats. This commit does that. Fixes: #29023 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com> Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: Gilberto Bertin <jibi@cilium.io>
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.
Implement
l4Policywrapper with resolved named port so that proxy package can get the actual resolved named port number when callingGetPort. Previously, if policy used a named port,GetPortreturned0.GetPortis only used for DNS proxy functionUpdateAllowed. This means that using a named port for a DNS policy destination port likely has not functioned as intended. Since DNS port is typically53, it seems likely that named ports would not be used in DNS policies in practice.Fixes: #11092