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

Validation for CRD custom resources: feature gate promotion alpha->beta #54647

Merged
merged 1 commit into from Nov 16, 2017
+4 −37
Merged

Validation for CRD custom resources: feature gate promotion alpha->beta #54647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/features/kube_features.go
View file
Open in desktop
Expand Up @@ -212,6 +212,6 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS

// inherited features from apiextensions-apiserver, relisted here to get a conflict if it is changed
// unintentionally on either side:
apiextensionsfeatures.CustomResourceValidation: {Default: false, PreRelease: utilfeature.Alpha},
apiextensionsfeatures.CustomResourceValidation: {Default: true, PreRelease: utilfeature.Beta},
SupportIPVSProxyMode: {Default: false, PreRelease: utilfeature.Alpha},
}
1 change: 0 additions & 1 deletion staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/generated.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go
View file
Open in desktop
Expand Up @@ -29,7 +29,6 @@ type CustomResourceDefinitionSpec struct {
// Scope indicates whether this resource is cluster or namespace scoped. Default is namespaced
Scope ResourceScope `json:"scope" protobuf:"bytes,4,opt,name=scope,casttype=ResourceScope"`
// Validation describes the validation methods for CustomResources
// This field is alpha-level and should only be sent to servers that enable the CustomResourceValidation feature.
Copy link
Member

Choose a reason for hiding this comment

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

This needs update in the generated proto file too:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/generated.proto

Lines 107 to 111 in 4eadfbb

// Validation describes the validation methods for CustomResources
// This field is alpha-level and should only be sent to servers that enable the CustomResourceValidation feature.
// +optional
optional CustomResourceValidation validation = 5;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// +optional
Validation *CustomResourceValidation `json:"validation,omitempty" protobuf:"bytes,5,opt,name=validation"`
Copy link
Member

Choose a reason for hiding this comment

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

Was this cleared before storage if the flag was off?

If not, you have a bunch of clusters out there with time-bombs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To check my understanding, this is a time-bomb if someone has submitted resources with validation enabled, but without having the feature actually enabled? Hence if we activate it by default in 1.8, they could have custom resources in the API that do not actually validate?

If that understanding is not correct then I'm not sure I'm groking the problem.

I can investigate whether or not the field was being cleared. If it weren't being cleared, what are our options for moving forward and/or minimizing breakage short of changing the field name?

Copy link
Member

Choose a reason for hiding this comment

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

Was this cleared before storage if the flag was off?

Yes, it was cleared if the feature was disabled. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go#L57

}
Expand Down
3 changes: 2 additions & 1 deletion staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ const (

// owner: @sttts, @nikhita
// alpha: v1.8
// beta: v1.9
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure of the expected/desired syntax here; let me know what's appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

seems right as per:

kubernetes/staging/src/k8s.io/apiserver/pkg/features/kube_features.go

Lines 59 to 65 in 7914aed

// owner: @smarterclayton
// alpha: v1.8
// beta: v1.9
//
// Allow API clients to retrieve resource lists in chunks rather than
// all at once.
APIListChunking utilfeature.Feature = "APIListChunking"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

//
// CustomResourceValidation is a list of validation methods for CustomResources
CustomResourceValidation utilfeature.Feature = "CustomResourceValidation"
Expand All @@ -42,5 +43,5 @@ func init() {
// To add a new feature, define a key for it above and add it here. The features will be
// available throughout Kubernetes binaries.
var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureSpec{
CustomResourceValidation: {Default: false, PreRelease: utilfeature.Alpha},
CustomResourceValidation: {Default: true, PreRelease: utilfeature.Beta},
}
1 change: 0 additions & 1 deletion staging/src/k8s.io/apiextensions-apiserver/test/integration/BUILD
View file
Open in desktop
Expand Up @@ -29,7 +29,6 @@ go_test(
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/watch:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//vendor/k8s.io/client-go/dynamic:go_default_library",
],
)
Expand Down
31 changes: 0 additions & 31 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go
View file
Open in desktop
Expand Up @@ -25,7 +25,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/util/wait"
utilfeature "k8s.io/apiserver/pkg/util/feature"

apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
"k8s.io/apiextensions-apiserver/test/integration/testserver"
Expand Down Expand Up @@ -176,12 +175,6 @@ func TestCustomResourceValidation(t *testing.T) {
}
defer close(stopCh)

// enable alpha feature CustomResourceValidation
err = utilfeature.DefaultFeatureGate.Set("CustomResourceValidation=true")
if err != nil {
t.Errorf("failed to enable feature gate for CustomResourceValidation: %v", err)
}

noxuDefinition := newNoxuValidationCRD(apiextensionsv1beta1.NamespaceScoped)
noxuVersionClient, err := testserver.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, clientPool)
if err != nil {
Expand All @@ -203,12 +196,6 @@ func TestCustomResourceUpdateValidation(t *testing.T) {
}
defer close(stopCh)

// enable alpha feature CustomResourceValidation
err = utilfeature.DefaultFeatureGate.Set("CustomResourceValidation=true")
if err != nil {
t.Errorf("failed to enable feature gate for CustomResourceValidation: %v", err)
}

noxuDefinition := newNoxuValidationCRD(apiextensionsv1beta1.NamespaceScoped)
noxuVersionClient, err := testserver.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, clientPool)
if err != nil {
Expand Down Expand Up @@ -252,12 +239,6 @@ func TestCustomResourceValidationErrors(t *testing.T) {
}
defer close(stopCh)

// enable alpha feature CustomResourceValidation
err = utilfeature.DefaultFeatureGate.Set("CustomResourceValidation=true")
if err != nil {
t.Errorf("failed to enable feature gate for CustomResourceValidation: %v", err)
}

noxuDefinition := newNoxuValidationCRD(apiextensionsv1beta1.NamespaceScoped)
noxuVersionClient, err := testserver.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, clientPool)
if err != nil {
Expand Down Expand Up @@ -349,12 +330,6 @@ func TestCRValidationOnCRDUpdate(t *testing.T) {
}
defer close(stopCh)

// enable alpha feature CustomResourceValidation
err = utilfeature.DefaultFeatureGate.Set("CustomResourceValidation=true")
if err != nil {
t.Errorf("failed to enable feature gate for CustomResourceValidation: %v", err)
}

noxuDefinition := newNoxuValidationCRD(apiextensionsv1beta1.NamespaceScoped)

// set stricter schema
Expand Down Expand Up @@ -409,12 +384,6 @@ func TestForbiddenFieldsInSchema(t *testing.T) {
}
defer close(stopCh)

// enable alpha feature CustomResourceValidation
err = utilfeature.DefaultFeatureGate.Set("CustomResourceValidation=true")
if err != nil {
t.Errorf("failed to enable feature gate for CustomResourceValidation: %v", err)
}

noxuDefinition := newNoxuValidationCRD(apiextensionsv1beta1.NamespaceScoped)
noxuDefinition.Spec.Validation.OpenAPIV3Schema.AdditionalProperties.Allows = false

Expand Down