k8s: Decouple CNP embedding inside CCNP #14557
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
christarazi
added
sig/k8s
maintainer-s-little-helper
bot
added
dont-merge/needs-release-note-label
maintainer-s-little-helper
bot
assigned nathanjsweet and fristonio and unassigned fristonio
|
test-me-please |
christarazi
force-pushed
the
pr/christarazi/decouple-ccnp-from-cnp
branch
from
93ffc20
to
ba83b6c
Compare
|
test-me-please |
aanm
approved these changes
Jan 8, 2021
fristonio
requested changes
Jan 8, 2021
There was a problem hiding this comment.
The changes looks good to me. Thanks for the Pull Request 🙏 🚀
There is some documentation around ClusterwideNetworkPolicies that needs an update. For example here - https://github.com/cilium/cilium/blob/master/Documentation/concepts/kubernetes/policy.rst#ciliumclusterwidenetworkpolicy
Due to cilium#14526, we should test against full YAMLs representing CNPs and CCNPs for more realistic results. Signed-off-by: Chris Tarazi <chris@isovalent.com>
Previously, embedding CNP inside the CCNP object was meant to simplify the code as a CCNP is an identical structure to CNP. However, due to a bug reported by a user (cilium#14526), we've uncovered that CCNP validation in the preflight command was completely broken. Upon further investigation, it was due to an unmarshalling issue with the CCNP object (that embeds a CNP). Throughout the code, we have instances where we must treat the CCNP object with special care to avoid this unmarshalling issue (see b5c04ee and c813a15). In the above reported issue, yet again we can apply the same workarounds as the aforementioned commits do. However, we should avoid papering over this again with the same workaround, because in the future someone may not be aware of this gotcha when they make a new change. It is better to simply move away from embedding CNP inside CCNP and deal with the code duplication that results from it, than to rely on the mental energy and worst of all, waste time trying to debug very unintuitive errors within K8s infrastructure code. This commit moves away from embedding CNP inside CCNP. Signed-off-by: Chris Tarazi <chris@isovalent.com>
Following the previous commit, update the references in the docs for CCNP. Signed-off-by: Chris Tarazi <chris@isovalent.com>
Bring this up-to-date with what's currently in the codebase. Signed-off-by: Chris Tarazi <chris@isovalent.com>
christarazi
force-pushed
the
pr/christarazi/decouple-ccnp-from-cnp
branch
from
b680840
to
89eab48
Compare
|
test-me-please |
qmonnet
approved these changes
Jan 11, 2021
|
|
||
| // Status is the status of the Cilium policy rule. | ||
| // | ||
| // The reason this field exists in this structure is due a bug in the k8s |
There was a problem hiding this comment.
Suggested change
| // The reason this field exists in this structure is due a bug in the k8s | |
| // The reason this field exists in this structure is due to a bug in the k8s |
|
test-gke |
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.9
in 1.9.2
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.9
in 1.9.2
maintainer-s-little-helper
bot
moved this from Backport pending to v1.9
to Backport done to v1.9
in 1.9.2
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 msgs.
Fixes: #14526