Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pkg/k8s: fix all structural issues with CNP validation #10727

Merged
merged 4 commits into from
Apr 2, 2020
Merged

pkg/k8s: fix all structural issues with CNP validation #10727

merged 4 commits into from
Apr 2, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Mar 26, 2020

CNP schema validation was incorrectly formatted for some fields which
could cause badly formatted yaml files to be accepted by kube-apiserver
bypassing the schema validation. This would later cause Cilium to print
errors and potentially avoid it from starting, as the invalid CNPs would
prevent Cilium from fully synchronize with kube-apiserver an operation
that is essential when starting Cilium.

This commit fixes all violations presented by kubernetes for the CCNP
and CNP validation which would then deny bad CNPs and / or CCNPs from
being accepted by kube-apiserver.

Signed-off-by: André Martins [email protected]

Fixes: #10643

Tight CNP and CCNP schema validation for badly formatted policies (yaml or json)

@aanm aanm added kind/bug This is a bug in the Cilium logic. pending-review priority/high This is considered vital to an upcoming release. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Mar 26, 2020
@aanm aanm requested a review from a team as a code owner March 26, 2020 21:18
@aanm aanm added the sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Mar 26, 2020
@aanm aanm force-pushed the pr/fix-policy-validation-issue-10643 branch from 5fa5616 to 8cae230 Compare March 26, 2020 21:18
@aanm
Copy link
Member Author

aanm commented Mar 26, 2020

test-me-please

@coveralls
Copy link

coveralls commented Mar 26, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 45.517% when pulling 6c1835f on pr/fix-policy-validation-issue-10643 into 3de0635 on master.

joestringer
joestringer approved these changes Mar 26, 2020
View reviewed changes
pkg/k8s/apis/cilium.io/v2/register.go
Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
"SecurityGroupsIds": {
"securityGroupsIds": {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any meaningful potential impact with the case change here or are they seen as the same thing when upgrading schemas?

@aanm aanm added wip and removed pending-review labels Mar 27, 2020
@aanm aanm force-pushed the pr/fix-policy-validation-issue-10643 branch from 8cae230 to b37c2b7 Compare March 30, 2020 12:10
@aanm aanm requested review from a team as code owners March 30, 2020 12:10
@aanm aanm requested a review from a team March 30, 2020 12:10
@aanm aanm requested review from a team as code owners March 30, 2020 12:10
@aanm aanm added pending-review and removed wip labels Mar 30, 2020
@aanm
Copy link
Member Author

aanm commented Mar 30, 2020 •
edited
Loading

test-me-please

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18324/

@aanm aanm requested review from joestringer and tgraf March 30, 2020 12:39
@aanm aanm removed the sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Mar 30, 2020
@aanm
Copy link
Member Author

aanm commented Apr 1, 2020

test-me-please

@aanm
Copy link
Member Author

aanm commented Apr 1, 2020

If a user specifies the APIserver in Helm (because of kube-proxy-free), will this tool automatically pick that up and use it?

Good point, it won't. I'll fix this in a follow up PR because this is an existing bug.

Existing users could be deploying with kube-proxy free and be affected by this bug, will this tool work correctly for them during upgrade?

It won't, but to fix this will require more changes that need to be done before we release the next version of 1.7 and 1.6. Which I will take care of it before we do the release.

I've managed to add this suggestion without changing any code. Please check the documentation changes.

@aanm aanm requested a review from joestringer April 1, 2020 21:33
@joestringer joestringer mentioned this pull request Apr 1, 2020
Closed
27 tasks
aanm added 3 commits April 2, 2020 13:36
@aanm
add validation checker for installed CNPs in preflight
8a6f2ba 
As Cilium might require to update its CRD validation schema it is
important for the users to make sure all policies installed in their
cluster are valid in the point of view the new CRD validation schema
before performing an upgrade.

Avoiding doing this validation might cause Cilium from updating its
NodeStatus in those invalid Network Policies as well as in the worst
case scenario it might give a false sense of security to the user if
a policy is badly formatted and Cilium is not inforcing that policy
due a bad validation.

Signed-off-by: André Martins <[email protected]>
@aanm
examples/crds: update CRD with the latest schema
e90c9c5 
Signed-off-by: André Martins <[email protected]>
@aanm
test: fix badly formatted CNP
6c1835f 
Signed-off-by: André Martins <[email protected]>
@aanm aanm force-pushed the pr/fix-policy-validation-issue-10643 branch from 025bd5b to 6c1835f Compare April 2, 2020 11:37
aanm
aanm commented Apr 2, 2020
View reviewed changes
Copy link
Member Author

@aanm aanm left a comment

Choose a reason for hiding this comment

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

There was a typo in the docs:

diff --git b/Documentation/install/upgrade.rst a/Documentation/install/upgrade.rst
index fb35dabae..2d4d02bac 100644
--- b/Documentation/install/upgrade.rst
+++ a/Documentation/install/upgrade.rst
@@ -66,7 +66,7 @@ file.
         --set config.enabled=false \\
         --set operator.enabled=false \\
         --set global.k8sServiceHost=API_SERVER_IP \\
-        --set global.k8sServicePort=API_SERVER_PORT
+        --set global.k8sServicePort=API_SERVER_PORT \\
         > cilium-preflight.yaml
       kubectl create cilium-preflight.yaml

I've fix it. Will merge the PR once the GH check for docs have passed. All other tests have passed before this typo was fixed.

@aanm aanm merged commit bb9c843 into master Apr 2, 2020
@aanm aanm deleted the pr/fix-policy-validation-issue-10643 branch April 2, 2020 11:39
@pchaigno pchaigno mentioned this pull request Apr 2, 2020
Merged
@joestringer joestringer mentioned this pull request Apr 24, 2020
Closed
4 tasks
@christarazi christarazi mentioned this pull request Apr 30, 2020
Merged
@aanm aanm mentioned this pull request May 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgraf tgraf tgraf approved these changes

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
kind/bug This is a bug in the Cilium logic. priority/high This is considered vital to an upcoming release. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Input validation issue with CiliumNetworkPolicy
5 participants
@aanm @coveralls @joestringer @tgraf @pchaigno