Skip to content

Commit 52c018f

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #290 from DirectXMan12/bug/collpase-common-version-info
🐛 Collapse Common CRD Version Info
2 parents a0b2e93 + 4cc480f commit 52c018f

File tree

3 files changed

+300
-1
lines changed

3 files changed

+300
-1
lines changed

‎Gopkg.lock

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/crd/crd_spec_test.go

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
/*
2+
Copyright 2019 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package crd_test
18+
19+
import (
20+
. "github.com/onsi/ginkgo"
21+
. "github.com/onsi/gomega"
22+
apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
23+
24+
"sigs.k8s.io/controller-tools/pkg/crd"
25+
)
26+
27+
var _ = Describe("CRD Generation", func() {
28+
Describe("Utilities", func() {
29+
Describe("MergeIdenticalVersionInfo", func() {
30+
It("should replace per-version schemata with a top-level schema if all are identical", func() {
31+
spec := &apiext.CustomResourceDefinition{
32+
Spec: apiext.CustomResourceDefinitionSpec{
33+
Versions: []apiext.CustomResourceDefinitionVersion{
34+
{
35+
Name: "v1",
36+
Schema: &apiext.CustomResourceValidation{
37+
OpenAPIV3Schema: &apiext.JSONSchemaProps{
38+
Required: []string{"foo"},
39+
Type: "object",
40+
Properties: map[string]apiext.JSONSchemaProps{"foo": apiext.JSONSchemaProps{Type: "string"}},
41+
},
42+
},
43+
},
44+
{
45+
Name: "v2",
46+
Storage: true,
47+
Schema: &apiext.CustomResourceValidation{
48+
OpenAPIV3Schema: &apiext.JSONSchemaProps{
49+
Required: []string{"foo"},
50+
Type: "object",
51+
Properties: map[string]apiext.JSONSchemaProps{"foo": apiext.JSONSchemaProps{Type: "string"}},
52+
},
53+
},
54+
},
55+
},
56+
},
57+
}
58+
crd.MergeIdenticalVersionInfo(spec)
59+
Expect(spec.Spec.Validation).To(Equal(&apiext.CustomResourceValidation{
60+
OpenAPIV3Schema: &apiext.JSONSchemaProps{
61+
Required: []string{"foo"},
62+
Type: "object",
63+
Properties: map[string]apiext.JSONSchemaProps{"foo": apiext.JSONSchemaProps{Type: "string"}},
64+
},
65+
}))
66+
Expect(spec.Spec.Versions).To(Equal([]apiext.CustomResourceDefinitionVersion{
67+
{Name: "v1"}, {Name: "v2", Storage: true},
68+
}))
69+
})
70+
71+
It("shouldn't merge different schemata", func() {
72+
spec := &apiext.CustomResourceDefinition{
73+
Spec: apiext.CustomResourceDefinitionSpec{
74+
Versions: []apiext.CustomResourceDefinitionVersion{
75+
{
76+
Name: "v1",
77+
Schema: &apiext.CustomResourceValidation{
78+
OpenAPIV3Schema: &apiext.JSONSchemaProps{
79+
Type: "object",
80+
Properties: map[string]apiext.JSONSchemaProps{"foo": apiext.JSONSchemaProps{Type: "string"}},
81+
},
82+
},
83+
},
84+
{
85+
Name: "v2",
86+
Storage: true,
87+
Schema: &apiext.CustomResourceValidation{
88+
OpenAPIV3Schema: &apiext.JSONSchemaProps{
89+
Required: []string{"foo"},
90+
Type: "object",
91+
Properties: map[string]apiext.JSONSchemaProps{"foo": apiext.JSONSchemaProps{Type: "string"}},
92+
},
93+
},
94+
},
95+
},
96+
},
97+
}
98+
orig := spec.DeepCopy()
99+
crd.MergeIdenticalVersionInfo(spec)
100+
Expect(spec).To(Equal(orig))
101+
})
102+
103+
It("should replace per-version subresources with top-level subresources if all are identical", func() {
104+
spec := &apiext.CustomResourceDefinition{
105+
Spec: apiext.CustomResourceDefinitionSpec{
106+
Versions: []apiext.CustomResourceDefinitionVersion{
107+
{
108+
Name: "v1",
109+
Subresources: &apiext.CustomResourceSubresources{
110+
Status: &apiext.CustomResourceSubresourceStatus{},
111+
},
112+
},
113+
{
114+
Name: "v2",
115+
Storage: true,
116+
Subresources: &apiext.CustomResourceSubresources{
117+
Status: &apiext.CustomResourceSubresourceStatus{},
118+
},
119+
},
120+
},
121+
},
122+
}
123+
124+
crd.MergeIdenticalVersionInfo(spec)
125+
Expect(spec.Spec.Subresources).To(Equal(&apiext.CustomResourceSubresources{
126+
Status: &apiext.CustomResourceSubresourceStatus{},
127+
}))
128+
Expect(spec.Spec.Versions).To(Equal([]apiext.CustomResourceDefinitionVersion{
129+
{Name: "v1"}, {Name: "v2", Storage: true},
130+
}))
131+
})
132+
133+
It("shouldn't merge different subresources", func() {
134+
spec := &apiext.CustomResourceDefinition{
135+
Spec: apiext.CustomResourceDefinitionSpec{
136+
Versions: []apiext.CustomResourceDefinitionVersion{
137+
{
138+
Name: "v1",
139+
Subresources: &apiext.CustomResourceSubresources{
140+
Status: &apiext.CustomResourceSubresourceStatus{},
141+
},
142+
},
143+
{
144+
Name: "v2",
145+
Storage: true,
146+
},
147+
},
148+
},
149+
}
150+
orig := spec.DeepCopy()
151+
crd.MergeIdenticalVersionInfo(spec)
152+
Expect(spec).To(Equal(orig))
153+
})
154+
155+
It("should replace per-version printer columns with top-level printer columns if all are identical", func() {
156+
spec := &apiext.CustomResourceDefinition{
157+
Spec: apiext.CustomResourceDefinitionSpec{
158+
Versions: []apiext.CustomResourceDefinitionVersion{
159+
{
160+
Name: "v1",
161+
AdditionalPrinterColumns: []apiext.CustomResourceColumnDefinition{
162+
{Name: "Cheddar", JSONPath: ".spec.cheddar"},
163+
{Name: "Parmesan", JSONPath: ".status.parmesan"},
164+
},
165+
},
166+
{
167+
Name: "v2",
168+
Storage: true,
169+
AdditionalPrinterColumns: []apiext.CustomResourceColumnDefinition{
170+
{Name: "Cheddar", JSONPath: ".spec.cheddar"},
171+
{Name: "Parmesan", JSONPath: ".status.parmesan"},
172+
},
173+
},
174+
},
175+
},
176+
}
177+
178+
crd.MergeIdenticalVersionInfo(spec)
179+
Expect(spec.Spec.AdditionalPrinterColumns).To(Equal([]apiext.CustomResourceColumnDefinition{
180+
{Name: "Cheddar", JSONPath: ".spec.cheddar"},
181+
{Name: "Parmesan", JSONPath: ".status.parmesan"},
182+
}))
183+
Expect(spec.Spec.Versions).To(Equal([]apiext.CustomResourceDefinitionVersion{
184+
{Name: "v1"}, {Name: "v2", Storage: true},
185+
}))
186+
})
187+
188+
It("shouldn't merge different printer columns", func() {
189+
spec := &apiext.CustomResourceDefinition{
190+
Spec: apiext.CustomResourceDefinitionSpec{
191+
Versions: []apiext.CustomResourceDefinitionVersion{
192+
{
193+
Name: "v1",
194+
AdditionalPrinterColumns: []apiext.CustomResourceColumnDefinition{
195+
{Name: "Cheddar", JSONPath: ".spec.cheddar"},
196+
{Name: "Parmesan", JSONPath: ".status.parmesan"},
197+
},
198+
},
199+
{
200+
Name: "v2",
201+
Storage: true,
202+
},
203+
},
204+
},
205+
}
206+
orig := spec.DeepCopy()
207+
crd.MergeIdenticalVersionInfo(spec)
208+
Expect(spec).To(Equal(orig))
209+
})
210+
})
211+
})
212+
})

‎pkg/crd/spec.go

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/gobuffalo/flect"
2424

2525
apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
26+
"k8s.io/apimachinery/pkg/api/equality"
2627
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2728
"k8s.io/apimachinery/pkg/runtime/schema"
2829

@@ -37,6 +38,86 @@ type SpecMarker interface {
3738
ApplyToCRD(crd *apiext.CustomResourceDefinitionSpec, version string) error
3839
}
3940

41+
// mergeIdenticalSubresources checks to see if subresources are identical across
42+
// all versions, and if so, merges them into a top-level version.
43+
//
44+
// This assumes you're not using trivial versions.
45+
func mergeIdenticalSubresources(crd *apiext.CustomResourceDefinition) {
46+
subres := crd.Spec.Versions[0].Subresources
47+
for _, ver := range crd.Spec.Versions {
48+
if ver.Subresources == nil || !equality.Semantic.DeepEqual(subres, ver.Subresources) {
49+
// either all nil, or not identical
50+
return
51+
}
52+
}
53+
54+
// things are identical if we've gotten this far, so move the subresources up
55+
// and discard the identical per-version ones
56+
crd.Spec.Subresources = subres
57+
for i := range crd.Spec.Versions {
58+
crd.Spec.Versions[i].Subresources = nil
59+
}
60+
}
61+
62+
// mergeIdenticalSchemata checks to see if schemata are identical across
63+
// all versions, and if so, merges them into a top-level version.
64+
//
65+
// This assumes you're not using trivial versions.
66+
func mergeIdenticalSchemata(crd *apiext.CustomResourceDefinition) {
67+
schema := crd.Spec.Versions[0].Schema
68+
for _, ver := range crd.Spec.Versions {
69+
if ver.Schema == nil || !equality.Semantic.DeepEqual(schema, ver.Schema) {
70+
// either all nil, or not identical
71+
return
72+
}
73+
}
74+
75+
// things are identical if we've gotten this far, so move the schemata up
76+
// to a single schema and discard the identical per-version ones
77+
crd.Spec.Validation = schema
78+
for i := range crd.Spec.Versions {
79+
crd.Spec.Versions[i].Schema = nil
80+
}
81+
}
82+
83+
// mergeIdenticalPrinterColumns checks to see if schemata are identical across
84+
// all versions, and if so, merges them into a top-level version.
85+
//
86+
// This assumes you're not using trivial versions.
87+
func mergeIdenticalPrinterColumns(crd *apiext.CustomResourceDefinition) {
88+
cols := crd.Spec.Versions[0].AdditionalPrinterColumns
89+
for _, ver := range crd.Spec.Versions {
90+
if len(ver.AdditionalPrinterColumns) == 0 || !equality.Semantic.DeepEqual(cols, ver.AdditionalPrinterColumns) {
91+
// either all nil, or not identical
92+
return
93+
}
94+
}
95+
96+
// things are identical if we've gotten this far, so move the printer columns up
97+
// and discard the identical per-version ones
98+
crd.Spec.AdditionalPrinterColumns = cols
99+
for i := range crd.Spec.Versions {
100+
crd.Spec.Versions[i].AdditionalPrinterColumns = nil
101+
}
102+
}
103+
104+
// MergeIdenticalVersionInfo makes sure that components of the Versions field that are identical
105+
// across all versions get merged into the top-level fields in v1beta1.
106+
//
107+
// This is required by the Kubernetes API server validation.
108+
//
109+
// The reason is that a v1beta1 -> v1 -> v1beta1 conversion cycle would need to
110+
// round-trip identically, v1 doesn't have top-level subresources, and without
111+
// this restriction it would be ambiguous how a v1-with-identical-subresources
112+
// converts into a v1beta1).
113+
func MergeIdenticalVersionInfo(crd *apiext.CustomResourceDefinition) {
114+
if len(crd.Spec.Versions) > 1 {
115+
mergeIdenticalSubresources(crd)
116+
mergeIdenticalSchemata(crd)
117+
mergeIdenticalPrinterColumns(crd)
118+
}
119+
}
120+
40121
// NeedCRDFor requests the full CRD for the given group-kind. It requires
41122
// that the packages containing the Go structs for that CRD have already
42123
// been loaded with NeedPackage.
@@ -150,5 +231,9 @@ func (p *Parser) NeedCRDFor(groupKind schema.GroupKind) {
150231
crd.Status.Conditions = []apiext.CustomResourceDefinitionCondition{}
151232
crd.Status.StoredVersions = []string{}
152233

234+
// make sure we merge identical per-version parts, to avoid validation errors
235+
// (see the reasoning near the top of the file).
236+
MergeIdenticalVersionInfo(&crd)
237+
153238
p.CustomResourceDefinitions[groupKind] = crd
154239
}

0 commit comments

Comments
 (0)