[v1.8] k8s: Fix non-structural schema with CiliumNode #13196

christarazi · 2020-09-17T04:47:51Z

This is a direct backport to 1.8.

Fix bug in EKS environments where Cilium agents never become ready due to a missing CiliumNode CRD schema property

christarazi · 2020-09-17T04:49:12Z

test-backport-1.8

sayboras · 2020-09-17T05:58:30Z

Just a question on how we decide to do direct backport instead of backporting from upstream PR :). Thanks.

aanm

Also, please write a good release note for our users

pkg/k8s/apis/cilium.io/v2/client/register.go

aanm · 2020-09-17T06:05:41Z

pkg/k8s/apis/cilium.io/v2/register.go

@@ -31,7 +31,7 @@ const (

 	// CustomResourceDefinitionSchemaVersion is semver-conformant version of CRD schema
 	// Used to determine if CRD needs to be updated in cluster
-	CustomResourceDefinitionSchemaVersion = "1.21.1"
+	CustomResourceDefinitionSchemaVersion = "1.21.2"


This versioning is only used for Cilium [C]CNP. Lines 261-263 are missing this annotation so the "upgrade" of this CRD won't work.

Ah good catch. How should we workaround this then?

We just need to add the annotation in the Cilium Node. AFAIK, the update logic will update a CRD if the old one does not contain the annotation but the new one does.

Presumably we'll need another fix on master to propagate the schema version for 1.9 or later.

master already attaches the schema versions to all CRDs:

cilium/pkg/k8s/apis/cilium.io/v2/client/register.go

Line 270 in a7451f8

Labels: map[string]string{

christarazi · 2020-09-17T06:17:38Z

Just a question on how we decide to do direct backport instead of backporting from upstream PR :). Thanks.

@sayboras We usually do this when the issue is not on master. In this case, this bug exists on the 1.8.x line.

christarazi · 2020-09-17T06:24:36Z

Also, please write a good release note for our users

@aanm I updated it, but unsure how to best word this one. Feel free to update the PR description to your liking.

joestringer · 2020-09-17T17:51:33Z

@christarazi The question is typically "If I'm a user without deep understanding of Cilium internals, how do I know if this bug might be fixing the issue I'm hitting?". From this perspective:

Fix CiliumNode CRD schema for Kubernetes 1.16 or greater

This says OK there's some problem with the schema, but the symptoms and consequences aren't clear.

I would suggest instead something like:

Fix bug in EKS environments where Cilium agents never become ready due to a missing CiliumNode CRD schema property

joestringer

I've fixed up the release note. Just need to address the schema version issue Andre pointed out below.

joestringer · 2020-09-17T17:54:09Z

pkg/k8s/apis/cilium.io/v2/register.go

@@ -31,7 +31,7 @@ const (

 	// CustomResourceDefinitionSchemaVersion is semver-conformant version of CRD schema
 	// Used to determine if CRD needs to be updated in cluster
-	CustomResourceDefinitionSchemaVersion = "1.21.1"
+	CustomResourceDefinitionSchemaVersion = "1.21.2"


Presumably we'll need another fix on master to propagate the schema version for 1.9 or later.

christarazi · 2020-09-17T18:03:58Z

@christarazi The question is typically "If I'm a user without deep understanding of Cilium internals, how do I know if this bug might be fixing the issue I'm hitting?".

Thanks! This is a helpful way to frame the release notes. The new release note sounds much better to me 👍

The schema was missing a `type` field on the `spec` property. ``` conditions: - lastTransitionTime: "2020-09-16T21:08:18Z" message: '[spec.validation.openAPIV3Schema.properties[spec].type: Required value: must not be empty for specified object fields, spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root]' reason: Violations status: "True" type: NonStructuralSchema ``` Signed-off-by: Chris Tarazi <chris@isovalent.com>

This commit allows the CiliumNode CRD to be versioned. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi · 2020-09-17T18:11:40Z

test-backport-1.8

joestringer

LGTM, though @aanm would know the nuance of these specifications more clearly

christarazi · 2020-09-18T00:42:10Z

test-missed-k8s

christarazi added area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Sep 17, 2020

maintainer-s-little-helper bot added backport/1.8 kind/backports This PR provides functionality previously merged into master. labels Sep 17, 2020

christarazi marked this pull request as ready for review September 17, 2020 04:49

christarazi requested a review from a team as a code owner September 17, 2020 04:49

christarazi requested review from aanm and a team September 17, 2020 04:49

aanm requested changes Sep 17, 2020

View reviewed changes

christarazi changed the title ~~[v1.8] k8s: Fix non-strucural schema with CiliumNode~~ [v1.8] k8s: Fix non-structural schema with CiliumNode Sep 17, 2020

joestringer requested changes Sep 17, 2020

View reviewed changes

christarazi added 3 commits September 17, 2020 11:08

k8s: Attach CRD schema version to CiliumNode

92a88f0

This commit allows the CiliumNode CRD to be versioned. Signed-off-by: Chris Tarazi <chris@isovalent.com>

k8s: Bump CRD schema version to 1.21.2

b5d16f9

Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/fix-non-structural-schema-cilium-node branch from f90c9d6 to b5d16f9 Compare September 17, 2020 18:09

christarazi requested review from aanm and joestringer September 17, 2020 18:11

joestringer approved these changes Sep 17, 2020

View reviewed changes

aanm approved these changes Sep 17, 2020

View reviewed changes

aanm merged commit f842660 into cilium:v1.8 Sep 18, 2020

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 18, 2020

christarazi mentioned this pull request Sep 18, 2020

kube-apiserver 1.16+ complains that CiliumNode CRD is non-strucutural #13192

Closed

joestringer mentioned this pull request Oct 1, 2020

Prepare for release v1.8.4 #13361

Merged

christarazi deleted the pr/christarazi/fix-non-structural-schema-cilium-node branch November 6, 2020 05:31

[v1.8] k8s: Fix non-structural schema with CiliumNode #13196

[v1.8] k8s: Fix non-structural schema with CiliumNode #13196

Uh oh!

Conversation

christarazi commented Sep 17, 2020 • edited by joestringer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christarazi commented Sep 17, 2020

Uh oh!

sayboras commented Sep 17, 2020

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aanm Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

aanm Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi commented Sep 17, 2020

Uh oh!

christarazi commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Sep 17, 2020

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi commented Sep 17, 2020

Uh oh!

christarazi commented Sep 17, 2020

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

christarazi commented Sep 18, 2020

Uh oh!

Uh oh!

christarazi commented Sep 17, 2020 •

edited by joestringer

Loading

christarazi commented Sep 17, 2020 •

edited

Loading