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

k8s.io/apiserver: remove skewed completion from EtcdOptions #118416

Merged
merged 2 commits into from Jun 6, 2023
Merged

k8s.io/apiserver: remove skewed completion from EtcdOptions #118416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f351c6d
k8s.io/apiserver: apply storage object count tracker implicitly
sttts Jun 2, 2023
e9e4acb
k8s.io/apiserver: remove skewed completion from EtcdOptions
sttts Jun 2, 2023
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
6 changes: 1 addition & 5 deletions cmd/kube-apiserver/app/apiextensions.go
View file
Open in desktop
Expand Up @@ -71,17 +71,13 @@ func createAPIExtensionsConfig(
apiextensionsapiserver.Scheme); err != nil {
return nil, err
}
crdRESTOptionsGetter, err := apiextensionsoptions.NewCRDRESTOptionsGetter(etcdOptions)
if err != nil {
return nil, err
}
apiextensionsConfig := &apiextensionsapiserver.Config{
GenericConfig: &genericapiserver.RecommendedConfig{
Config: genericConfig,
SharedInformerFactory: externalInformers,
},
ExtraConfig: apiextensionsapiserver.ExtraConfig{
CRDRESTOptionsGetter: crdRESTOptionsGetter,
CRDRESTOptionsGetter: apiextensionsoptions.NewCRDRESTOptionsGetter(etcdOptions, genericConfig.ResourceTransformers, genericConfig.StorageObjectCountTracker),
MasterCount: masterCount,
AuthResolverWrapper: authResolverWrapper,
ServiceResolver: serviceResolver,
Expand Down
4 changes: 1 addition & 3 deletions cmd/kube-apiserver/app/server.go
View file
Open in desktop
Expand Up @@ -402,12 +402,10 @@ func buildGenericConfig(
} else {
s.Etcd.StorageConfig.Transport.TracerProvider = oteltrace.NewNoopTracerProvider()
}
if lastErr = s.Etcd.Complete(genericConfig.StorageObjectCountTracker, genericConfig.DrainedNotify(), genericConfig.AddPostStartHook); lastErr != nil {
return
}

storageFactoryConfig := kubeapiserver.NewStorageFactoryConfig()
storageFactoryConfig.APIResourceConfig = genericConfig.MergedResourceConfig
storageFactoryConfig.StorageConfig.StorageObjectCountTracker = genericConfig.StorageObjectCountTracker
storageFactory, lastErr = storageFactoryConfig.Complete(s.Etcd).New()
if lastErr != nil {
return
Expand Down
5 changes: 1 addition & 4 deletions pkg/controlplane/instance_test.go
View file
Open in desktop
Expand Up @@ -88,15 +88,12 @@ func setUp(t *testing.T) (*etcd3testing.EtcdTestServer, Config, *assert.Assertio
}

storageFactoryConfig := kubeapiserver.NewStorageFactoryConfig()
storageConfig.StorageObjectCountTracker = config.GenericConfig.StorageObjectCountTracker
resourceEncoding := resourceconfig.MergeResourceEncodingConfigs(storageFactoryConfig.DefaultResourceEncoding, storageFactoryConfig.ResourceEncodingOverrides)
storageFactory := serverstorage.NewDefaultStorageFactory(*storageConfig, "application/vnd.kubernetes.protobuf", storageFactoryConfig.Serializer, resourceEncoding, DefaultAPIResourceConfigSource(), nil)

etcdOptions := options.NewEtcdOptions(storageConfig)
// unit tests don't need watch cache and it leaks lots of goroutines with etcd testing functions during unit tests
etcdOptions.EnableWatchCache = false
if err := etcdOptions.Complete(config.GenericConfig.StorageObjectCountTracker, config.GenericConfig.DrainedNotify(), config.GenericConfig.AddPostStartHook); err != nil {
t.Fatal(err)
}
err := etcdOptions.ApplyWithStorageFactoryTo(storageFactory, config.GenericConfig)
if err != nil {
t.Fatal(err)
Expand Down
39 changes: 11 additions & 28 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/cmd/server/options/options.go
View file
Open in desktop
Expand Up @@ -21,7 +21,6 @@ import (
"io"
"net"
"net/url"
"reflect"

"github.com/spf13/pflag"
oteltrace "go.opentelemetry.io/otel/trace"
Expand All @@ -36,6 +35,8 @@ import (
genericregistry "k8s.io/apiserver/pkg/registry/generic"
genericapiserver "k8s.io/apiserver/pkg/server"
genericoptions "k8s.io/apiserver/pkg/server/options"
storagevalue "k8s.io/apiserver/pkg/storage/value"
flowcontrolrequest "k8s.io/apiserver/pkg/util/flowcontrol/request"
"k8s.io/apiserver/pkg/util/openapi"
"k8s.io/apiserver/pkg/util/proxy"
"k8s.io/apiserver/pkg/util/webhook"
Expand Down Expand Up @@ -111,16 +112,12 @@ func (o CustomResourceDefinitionsServerOptions) Config() (*apiserver.Config, err
if err := o.APIEnablement.ApplyTo(&serverConfig.Config, apiserver.DefaultAPIResourceConfigSource(), apiserver.Scheme); err != nil {
return nil, err
}
crdRESTOptionsGetter, err := NewCRDRESTOptionsGetter(*o.RecommendedOptions.Etcd)
if err != nil {
return nil, err
}

serverConfig.OpenAPIV3Config = genericapiserver.DefaultOpenAPIV3Config(openapi.GetOpenAPIDefinitionsWithoutDisabledFeatures(generatedopenapi.GetOpenAPIDefinitions), openapinamer.NewDefinitionNamer(apiserver.Scheme, scheme.Scheme))
config := &apiserver.Config{
GenericConfig: serverConfig,
ExtraConfig: apiserver.ExtraConfig{
CRDRESTOptionsGetter: crdRESTOptionsGetter,
CRDRESTOptionsGetter: NewCRDRESTOptionsGetter(*o.RecommendedOptions.Etcd, serverConfig.ResourceTransformers, serverConfig.StorageObjectCountTracker),
ServiceResolver: &serviceResolver{serverConfig.SharedInformerFactory.Core().V1().Services().Lister()},
AuthResolverWrapper: webhook.NewDefaultAuthenticationInfoResolverWrapper(nil, nil, serverConfig.LoopbackClientConfig, oteltrace.NewNoopTracerProvider()),
},
Expand All @@ -129,30 +126,16 @@ func (o CustomResourceDefinitionsServerOptions) Config() (*apiserver.Config, err
}

// NewCRDRESTOptionsGetter create a RESTOptionsGetter for CustomResources.
// This works on a copy of the etcd options so we don't mutate originals.
// We assume that the input etcd options have been completed already.
//
// Avoid messing with anything outside of changes to StorageConfig as that
// may lead to unexpected behavior when the options are applied.
func NewCRDRESTOptionsGetter(etcdOptions genericoptions.EtcdOptions) (genericregistry.RESTOptionsGetter, error) {
etcdOptions.StorageConfig.Codec = unstructured.UnstructuredJSONScheme
etcdOptions.WatchCacheSizes = nil // this control is not provided for custom resources
etcdOptions.SkipHealthEndpoints = true // avoid double wiring of health checks

// creates a generic apiserver config for etcdOptions to mutate
c := genericapiserver.Config{}
if err := etcdOptions.ApplyTo(&c); err != nil {
return nil, err
}
restOptionsGetter := c.RESTOptionsGetter
if restOptionsGetter == nil {
return nil, fmt.Errorf("server.Config RESTOptionsGetter should not be nil")
}
// sanity check that no other fields are set
c.RESTOptionsGetter = nil
if !reflect.DeepEqual(c, genericapiserver.Config{}) {
return nil, fmt.Errorf("only RESTOptionsGetter should have been mutated in server.Config")
}
return restOptionsGetter, nil
Comment on lines -141 to -155
Copy link
Member

Choose a reason for hiding this comment

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

I know this code isn't pretty but you could easily re-use it as-is by just setting resourceTransformers on the genericapiserver.Config. It would force the code paths to stay in sync with each other, which was one of the reasons we chose to use ApplyTo here.

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 I can follow. This path is explicitly non-standard for CRs, and it is simple, calling the options package. So no risk to diverge.

What is still a little strange IMO is that the tracker is not set on the StorageConfig persistently. But that's another skew for another PR.

Copy link
Member

Choose a reason for hiding this comment

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

If the Apply funcs add further logic beyond what is covered by the tracker/transformer/etc, then CRs would diverge from standard resources again (in the same way they did for transformers which broke encryption for them). This is a slow moving part of the codebase so maybe it is fine.

func NewCRDRESTOptionsGetter(etcdOptions genericoptions.EtcdOptions, resourceTransformers storagevalue.ResourceTransformers, tracker flowcontrolrequest.StorageObjectCountTracker) genericregistry.RESTOptionsGetter {
etcdOptionsCopy := etcdOptions
etcdOptionsCopy.StorageConfig.Codec = unstructured.UnstructuredJSONScheme
etcdOptionsCopy.StorageConfig.StorageObjectCountTracker = tracker
etcdOptionsCopy.WatchCacheSizes = nil // this control is not provided for custom resources

return etcdOptions.CreateRESTOptionsGetter(&genericoptions.SimpleStorageFactory{StorageConfig: etcdOptionsCopy.StorageConfig}, resourceTransformers)
}

type serviceResolver struct {
Expand Down
5 changes: 1 addition & 4 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go
View file
Open in desktop
Expand Up @@ -182,10 +182,7 @@ func testWebhookConverter(t *testing.T, watchCache bool) {

crd := multiVersionFixture.DeepCopy()

RESTOptionsGetter, err := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd)
if err != nil {
t.Fatal(err)
}
RESTOptionsGetter := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd, nil, nil)
restOptions, err := RESTOptionsGetter.GetRESTOptions(schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural})
if err != nil {
t.Fatal(err)
Expand Down
5 changes: 1 addition & 4 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go
View file
Open in desktop
Expand Up @@ -658,10 +658,7 @@ func TestCustomResourceDefaultingOfMetaFields(t *testing.T) {
t.Logf("CR created: %#v", returnedFoo.UnstructuredContent())

// get persisted object
RESTOptionsGetter, err := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd)
if err != nil {
t.Fatal(err)
}
RESTOptionsGetter := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd, nil, nil)
restOptions, err := RESTOptionsGetter.GetRESTOptions(schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural})
if err != nil {
t.Fatal(err)
Expand Down
13 changes: 10 additions & 3 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go
View file
Open in desktop
Expand Up @@ -31,6 +31,8 @@ import (
serveroptions "k8s.io/apiextensions-apiserver/pkg/cmd/server/options"
servertesting "k8s.io/apiextensions-apiserver/pkg/cmd/server/testing"
"k8s.io/apimachinery/pkg/runtime/schema"
genericapiserver "k8s.io/apiserver/pkg/server"
storagevalue "k8s.io/apiserver/pkg/storage/value"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/rest"
)
Expand Down Expand Up @@ -145,10 +147,15 @@ func StartDefaultServerWithClientsAndEtcd(t servertesting.Logger, extraFlags ...
return nil, nil, nil, nil, "", err
}

RESTOptionsGetter, err := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd)
if err != nil {
return nil, nil, nil, nil, "", err
var resourceTransformers storagevalue.ResourceTransformers
if len(options.RecommendedOptions.Etcd.EncryptionProviderConfigFilepath) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition looks redundant to me given the behavior of ApplyTo.

// be clever in tests to reconstruct the transformers, for encryption integration tests
config := genericapiserver.Config{}
options.RecommendedOptions.Etcd.ApplyTo(&config)
resourceTransformers = config.ResourceTransformers
}

RESTOptionsGetter := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd, resourceTransformers, nil)
restOptions, err := RESTOptionsGetter.GetRESTOptions(schema.GroupResource{Group: "hopefully-ignored-group", Resource: "hopefully-ignored-resources"})
if err != nil {
return nil, nil, nil, nil, "", err
Expand Down
5 changes: 1 addition & 4 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/objectmeta_test.go
View file
Open in desktop
Expand Up @@ -132,10 +132,7 @@ func TestInvalidObjectMetaInStorage(t *testing.T) {
t.Fatal(err)
}

RESTOptionsGetter, err := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd)
if err != nil {
t.Fatal(err)
}
RESTOptionsGetter := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd, nil, nil)
restOptions, err := RESTOptionsGetter.GetRESTOptions(schema.GroupResource{Group: noxuDefinition.Spec.Group, Resource: noxuDefinition.Spec.Names.Plural})
if err != nil {
t.Fatal(err)
Expand Down
3 changes: 3 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/server/config.go
View file
Open in desktop
Expand Up @@ -65,6 +65,7 @@ import (
"k8s.io/apiserver/pkg/server/healthz"
"k8s.io/apiserver/pkg/server/routes"
serverstore "k8s.io/apiserver/pkg/server/storage"
storagevalue "k8s.io/apiserver/pkg/storage/value"
"k8s.io/apiserver/pkg/storageversion"
utilfeature "k8s.io/apiserver/pkg/util/feature"
utilflowcontrol "k8s.io/apiserver/pkg/util/flowcontrol"
Expand Down Expand Up @@ -190,6 +191,8 @@ type Config struct {
// SkipOpenAPIInstallation avoids installing the OpenAPI handler if set to true.
SkipOpenAPIInstallation bool

// ResourceTransformers are used to transform resources from and to etcd, e.g. encryption.
ResourceTransformers storagevalue.ResourceTransformers
// RESTOptionsGetter is used to construct RESTStorage types via the generic registry.
RESTOptionsGetter genericregistry.RESTOptionsGetter

Expand Down