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.

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

apiextensions: implement defaulting #77558

Merged
merged 7 commits into from May 29, 2019
Prev

apiextensions: add default integration tests

  • Loading branch information...
sttts committed May 8, 2019
commit b8137809039e293b947e3231aaa7f51f4381a878
@@ -59,14 +59,18 @@ func checks(checkers ...Checker) []Checker {
}

func TestWebhookConverter(t *testing.T) {
testWebhookConverter(t, false)
testWebhookConverter(t, false, false)
}

func TestWebhookConverterWithPruning(t *testing.T) {
testWebhookConverter(t, true)
testWebhookConverter(t, true, false)
}

func testWebhookConverter(t *testing.T, pruning bool) {
func TestWebhookConverterWithDefaulting(t *testing.T) {
testWebhookConverter(t, true, true)
}

func testWebhookConverter(t *testing.T, pruning, defaulting bool) {
tests := []struct {
group string
handler http.Handler
@@ -80,7 +84,7 @@ func testWebhookConverter(t *testing.T, pruning bool) {
{
group: "nontrivial-converter",
handler: NewObjectConverterWebhookHandler(t, nontrivialConverter),
checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2"), validateNonTrivialConverted, validateNonTrivialConvertedList, validateStoragePruning),
checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2"), validateNonTrivialConverted, validateNonTrivialConvertedList, validateStoragePruning, validateDefaulting),
},
{
group: "metadata-mutating-converter",
@@ -110,7 +114,12 @@ func testWebhookConverter(t *testing.T, pruning bool) {
etcd3watcher.TestOnlySetFatalOnDecodeError(false)
defer etcd3watcher.TestOnlySetFatalOnDecodeError(true)

// enable necessary features
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CustomResourceWebhookConversion, true)()
if defaulting {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CustomResourceDefaulting, true)()
}

tearDown, config, options, err := fixtures.StartDefaultServer(t)
if err != nil {
t.Fatal(err)
@@ -132,6 +141,12 @@ func testWebhookConverter(t *testing.T, pruning bool) {
crd := multiVersionFixture.DeepCopy()
crd.Spec.PreserveUnknownFields = pointer.BoolPtr(!pruning)

if !defaulting {
for i := range crd.Spec.Versions {
delete(crd.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties, "defaults")
}
}

RESTOptionsGetter := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd)
restOptions, err := RESTOptionsGetter.GetRESTOptions(schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural})
if err != nil {
@@ -520,6 +535,91 @@ func validateUIDMutation(t *testing.T, ctc *conversionTestContext) {
}
}

func validateDefaulting(t *testing.T, ctc *conversionTestContext) {

This comment has been minimized.

Copy link
@sttts

sttts May 29, 2019

Author Contributor

@liggitt added this: defaulting should happen in the request version when entering the request pipeline, and for the storage version when reading from etcd.

if _, defaulting := ctc.crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["defaults"]; !defaulting {
return
}

ns := ctc.namespace
storageVersion := "v1beta1"

for _, createVersion := range ctc.crd.Spec.Versions {
t.Run(fmt.Sprintf("getting objects created as %s", createVersion.Name), func(t *testing.T) {
name := "defaulting-" + createVersion.Name
client := ctc.versionedClient(ns, createVersion.Name)

fixture := newConversionMultiVersionFixture(ns, name, createVersion.Name)
if err := unstructured.SetNestedField(fixture.Object, map[string]interface{}{}, "defaults"); err != nil {
t.Fatal(err)
}
created, err := client.Create(fixture, metav1.CreateOptions{})
if err != nil {
t.Fatal(err)
}

// check that defaulting happens
// - in the request version when doing no-op conversion when deserializing
// - when reading back from storage in the storage version
// only the first is persisted.
defaults, found, err := unstructured.NestedMap(created.Object, "defaults")
if err != nil {
t.Fatal(err)
} else if !found {
t.Fatalf("expected .defaults to exist")
}
expectedLen := 1
if !createVersion.Storage {
expectedLen++
}
if len(defaults) != expectedLen {
t.Fatalf("after %s create expected .defaults to have %d values, but got: %v", createVersion.Name, expectedLen, defaults)
}
if _, found := defaults[createVersion.Name].(bool); !found {
t.Errorf("after %s create expected .defaults[%s] to be true, but .defaults is: %v", createVersion.Name, createVersion.Name, defaults)
}
if _, found := defaults[storageVersion].(bool); !found {
t.Errorf("after %s create expected .defaults[%s] to be true because it is the storage version, but .defaults is: %v", createVersion.Name, storageVersion, defaults)
}

// verify that only the request version default is persisted
persisted, err := ctc.etcdObjectReader.GetStoredCustomResource(ns, name)
if err != nil {
t.Fatal(err)
}
if _, found, err := unstructured.NestedBool(persisted.Object, "defaults", storageVersion); err != nil {
t.Fatal(err)
} else if createVersion.Name != storageVersion && found {
t.Errorf("after %s create .defaults[storage version %s] not to be persisted, but got in etcd: %v", createVersion.Name, storageVersion, defaults)
}

// check that when reading any other version, we do not default that version, but only the (non-persisted) storage version default
for _, v := range ctc.crd.Spec.Versions {
if v.Name == createVersion.Name {
// create version is persisted anyway, nothing to verify
continue
}

got, err := ctc.versionedClient(ns, v.Name).Get(created.GetName(), metav1.GetOptions{})
if err != nil {
t.Fatal(err)
}

if _, found, err := unstructured.NestedBool(got.Object, "defaults", v.Name); err != nil {
t.Fatal(err)
} else if v.Name != storageVersion && found {
t.Errorf("after %s GET expected .defaults[%s] not to be true because only storage version %s is defaulted on read, but .defaults is: %v", v.Name, v.Name, storageVersion, defaults)
}

if _, found, err := unstructured.NestedBool(got.Object, "defaults", storageVersion); err != nil {
t.Fatal(err)
} else if !found {
t.Errorf("after non-create, non-storage %s GET expected .defaults[storage version %s] to be true, but .defaults is: %v", v.Name, storageVersion, defaults)
}
}
})
}
}

func expectConversionFailureMessage(id, message string) func(t *testing.T, ctc *conversionTestContext) {
return func(t *testing.T, ctc *conversionTestContext) {
ns := ctc.namespace
@@ -918,6 +1018,14 @@ var multiVersionFixture = &apiextensionsv1beta1.CustomResourceDefinition{
"num2": {Type: "integer"},
},
},
"defaults": {
Type: "object",
Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
"v1alpha1": {Type: "boolean"},
"v1beta1": {Type: "boolean", Default: jsonPtr(true)},
"v1beta2": {Type: "boolean"},
},
},
},
},
},
@@ -944,6 +1052,14 @@ var multiVersionFixture = &apiextensionsv1beta1.CustomResourceDefinition{
"num2": {Type: "integer"},
},
},
"defaults": {
Type: "object",
Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
"v1alpha1": {Type: "boolean", Default: jsonPtr(true)},
"v1beta1": {Type: "boolean"},
"v1beta2": {Type: "boolean"},
},
},
},
},
},
@@ -970,6 +1086,14 @@ var multiVersionFixture = &apiextensionsv1beta1.CustomResourceDefinition{
"num2": {Type: "integer"},
},
},
"defaults": {
Type: "object",
Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
"v1alpha1": {Type: "boolean"},
"v1beta1": {Type: "boolean"},
"v1beta2": {Type: "boolean", Default: jsonPtr(true)},
},
},
},
},
},
@@ -1089,3 +1213,12 @@ func closeOnCall(h http.Handler) (chan struct{}, http.Handler) {
h.ServeHTTP(w, r)
})
}

func jsonPtr(x interface{}) *apiextensionsv1beta1.JSON {
bs, err := json.Marshal(x)
if err != nil {
panic(err)
}
ret := apiextensionsv1beta1.JSON{Raw: bs}
return &ret
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.