From bb59042ab9fc340468f2f006255b682bcafe2ddd Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Wed, 17 Mar 2021 00:26:23 +0000 Subject: [PATCH 1/7] Add ability to skip OpenAPI handler installation --- cmd/kube-apiserver/app/aggregator.go | 3 +++ staging/src/k8s.io/apiserver/pkg/server/config.go | 5 ++++- .../k8s.io/apiserver/pkg/server/genericapiserver.go | 10 ++++++++-- .../src/k8s.io/kube-aggregator/pkg/cmd/server/start.go | 3 +++ 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/cmd/kube-apiserver/app/aggregator.go b/cmd/kube-apiserver/app/aggregator.go index 4289ce48d8fe..d93b73295f1f 100644 --- a/cmd/kube-apiserver/app/aggregator.go +++ b/cmd/kube-apiserver/app/aggregator.go @@ -67,6 +67,9 @@ func createAggregatorConfig( genericConfig := kubeAPIServerConfig genericConfig.PostStartHooks = map[string]genericapiserver.PostStartHookConfigEntry{} genericConfig.RESTOptionsGetter = nil + // prevent generic API server from installing the OpenAPI handler. Aggregator server + // has its own customized OpenAPI handler. + genericConfig.SkipOpenAPIInstallation = true if utilfeature.DefaultFeatureGate.Enabled(genericfeatures.StorageVersionAPI) && utilfeature.DefaultFeatureGate.Enabled(genericfeatures.APIServerIdentity) { diff --git a/staging/src/k8s.io/apiserver/pkg/server/config.go b/staging/src/k8s.io/apiserver/pkg/server/config.go index 9ac85792401e..1d872cf444c4 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/config.go +++ b/staging/src/k8s.io/apiserver/pkg/server/config.go @@ -165,6 +165,8 @@ type Config struct { Serializer runtime.NegotiatedSerializer // OpenAPIConfig will be used in generating OpenAPI spec. This is nil by default. Use DefaultOpenAPIConfig for "working" defaults. OpenAPIConfig *openapicommon.Config + // SkipOpenAPIInstallation avoids installing the OpenAPI handler if set to true. + SkipOpenAPIInstallation bool // RESTOptionsGetter is used to construct RESTStorage types via the generic registry. RESTOptionsGetter genericregistry.RESTOptionsGetter @@ -571,7 +573,8 @@ func (c completedConfig) New(name string, delegationTarget DelegationTarget) (*G listedPathProvider: apiServerHandler, - openAPIConfig: c.OpenAPIConfig, + openAPIConfig: c.OpenAPIConfig, + skipOpenAPIInstallation: c.SkipOpenAPIInstallation, postStartHooks: map[string]postStartHookEntry{}, preShutdownHooks: map[string]preShutdownHookEntry{}, diff --git a/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go b/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go index d7d60b213de8..cc1979dba068 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go +++ b/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go @@ -133,8 +133,14 @@ type GenericAPIServer struct { // Enable swagger and/or OpenAPI if these configs are non-nil. openAPIConfig *openapicommon.Config + // SkipOpenAPIInstallation indicates not to install the OpenAPI handler + // during PrepareRun. + // Set this to true when the specific API Server has its own OpenAPI handler + // (e.g. kube-aggregator) + skipOpenAPIInstallation bool + // OpenAPIVersionedService controls the /openapi/v2 endpoint, and can be used to update the served spec. - // It is set during PrepareRun. + // It is set during PrepareRun if `openAPIConfig` is non-nil unless `skipOpenAPIInstallation` is true. OpenAPIVersionedService *handler.OpenAPIService // StaticOpenAPISpec is the spec derived from the restful container endpoints. @@ -289,7 +295,7 @@ type preparedGenericAPIServer struct { func (s *GenericAPIServer) PrepareRun() preparedGenericAPIServer { s.delegationTarget.PrepareRun() - if s.openAPIConfig != nil { + if s.openAPIConfig != nil && !s.skipOpenAPIInstallation { s.OpenAPIVersionedService, s.StaticOpenAPISpec = routes.OpenAPI{ Config: s.openAPIConfig, }.Install(s.Handler.GoRestfulContainer, s.Handler.NonGoRestfulMux) diff --git a/staging/src/k8s.io/kube-aggregator/pkg/cmd/server/start.go b/staging/src/k8s.io/kube-aggregator/pkg/cmd/server/start.go index 31ccdcf337ea..88abac45418c 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/cmd/server/start.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/cmd/server/start.go @@ -142,6 +142,9 @@ func (o AggregatorOptions) RunAggregator(stopCh <-chan struct{}) error { ) serverConfig.OpenAPIConfig = genericapiserver.DefaultOpenAPIConfig(openapi.GetOpenAPIDefinitions, openapinamer.NewDefinitionNamer(aggregatorscheme.Scheme)) serverConfig.OpenAPIConfig.Info.Title = "kube-aggregator" + // prevent generic API server from installing the OpenAPI handler. Aggregator server + // has its own customized OpenAPI handler. + serverConfig.SkipOpenAPIInstallation = true serviceResolver := apiserver.NewClusterIPServiceResolver(serverConfig.SharedInformerFactory.Core().V1().Services().Lister()) From c85828aed7b5edfeb65299050e4087d5cb6841cc Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 28 Jan 2021 17:45:24 -0500 Subject: [PATCH 2/7] Declare TCP default for service port protocol --- pkg/apis/core/v1/zz_generated.defaults.go | 6 ++++++ staging/src/k8s.io/api/core/v1/generated.proto | 1 + staging/src/k8s.io/api/core/v1/types.go | 1 + 3 files changed, 8 insertions(+) diff --git a/pkg/apis/core/v1/zz_generated.defaults.go b/pkg/apis/core/v1/zz_generated.defaults.go index 89f45a026b69..b895109c011d 100644 --- a/pkg/apis/core/v1/zz_generated.defaults.go +++ b/pkg/apis/core/v1/zz_generated.defaults.go @@ -915,6 +915,12 @@ func SetObjectDefaults_SecretList(in *v1.SecretList) { func SetObjectDefaults_Service(in *v1.Service) { SetDefaults_Service(in) + for i := range in.Spec.Ports { + a := &in.Spec.Ports[i] + if a.Protocol == "" { + a.Protocol = "TCP" + } + } } func SetObjectDefaults_ServiceList(in *v1.ServiceList) { diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index 3a13c53fa385..14c733e9fa07 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -4753,6 +4753,7 @@ message ServicePort { // The IP protocol for this port. Supports "TCP", "UDP", and "SCTP". // Default is TCP. + // +default="TCP" // +optional optional string protocol = 2; diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 2bba97251925..769b70a95129 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -4264,6 +4264,7 @@ type ServicePort struct { // The IP protocol for this port. Supports "TCP", "UDP", and "SCTP". // Default is TCP. + // +default="TCP" // +optional Protocol Protocol `json:"protocol,omitempty" protobuf:"bytes,2,opt,name=protocol,casttype=Protocol"` From 02c3a6373fc98cc4bd94d93701de461945564d5b Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 28 Jan 2021 17:45:38 -0500 Subject: [PATCH 3/7] Stop clearing OpenAPIConfig for kube-aggregator --- .../src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go index d0ab3186a9a5..25027835c0b9 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go @@ -163,11 +163,6 @@ func (cfg *Config) Complete() CompletedConfig { // NewWithDelegate returns a new instance of APIAggregator from the given config. func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.DelegationTarget) (*APIAggregator, error) { - // Prevent generic API server to install OpenAPI handler. Aggregator server - // has its own customized OpenAPI handler. - openAPIConfig := c.GenericConfig.OpenAPIConfig - c.GenericConfig.OpenAPIConfig = nil - genericServer, err := c.GenericConfig.New("kube-aggregator", delegationTarget) if err != nil { return nil, err @@ -191,7 +186,7 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg lister: informerFactory.Apiregistration().V1().APIServices().Lister(), APIRegistrationInformers: informerFactory, serviceResolver: c.ExtraConfig.ServiceResolver, - openAPIConfig: openAPIConfig, + openAPIConfig: c.GenericConfig.OpenAPIConfig, egressSelector: c.GenericConfig.EgressSelector, proxyCurrentCertKeyContent: func() (bytes []byte, bytes2 []byte) { return nil, nil }, } From beeeb1a8f0d6183630f47d60ed3328f23f06e52c Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 28 Jan 2021 17:45:55 -0500 Subject: [PATCH 4/7] Stop skipping APIService in apply test --- test/integration/apiserver/apply/status_test.go | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/test/integration/apiserver/apply/status_test.go b/test/integration/apiserver/apply/status_test.go index 32b82735a3b8..751643a7f97b 100644 --- a/test/integration/apiserver/apply/status_test.go +++ b/test/integration/apiserver/apply/status_test.go @@ -60,15 +60,7 @@ var statusData = map[schema.GroupVersionResource]string{ gvr("internal.apiserver.k8s.io", "v1alpha1", "storageversions"): `{"status": {"commonEncodingVersion":"v1","storageVersions":[{"apiServerID":"1","decodableVersions":["v1","v2"],"encodingVersion":"v1"}],"conditions":[{"type":"AllEncodingVersionsEqual","status":"True","lastTransitionTime":"2020-01-01T00:00:00Z","reason":"allEncodingVersionsEqual","message":"all encoding versions are set to v1"}]}}`, } -const statusDefault = `{"status": {"conditions": [{"type": "MyStatus", "status":"true"}]}}` - -// DO NOT ADD TO THIS LIST. -// This list is used to ignore known bugs. We shouldn't introduce new bugs. -var ignoreList = map[schema.GroupVersionResource]struct{}{ - // TODO(#89264): apiservices doesn't work because the openapi is not routed properly. - gvr("apiregistration.k8s.io", "v1beta1", "apiservices"): {}, - gvr("apiregistration.k8s.io", "v1", "apiservices"): {}, -} +const statusDefault = `{"status": {"conditions": [{"type": "MyStatus", "status":"True"}]}}` // Some status-only APIs have empty object on creation. Therefore we don't expect create_test // managedFields for these APIs @@ -146,9 +138,6 @@ func TestApplyStatus(t *testing.T) { t.Fatal(err) } t.Run(mapping.Resource.String(), func(t *testing.T) { - if _, ok := ignoreList[mapping.Resource]; ok { - t.Skip() - } status, ok := statusData[mapping.Resource] if !ok { status = statusDefault From 95714c2fe69d511dc41cad744e8aa0ab2af66e86 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 28 Jan 2021 17:46:09 -0500 Subject: [PATCH 5/7] Use apply to create objects in TestApplyStatus --- test/integration/apiserver/apply/status_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/integration/apiserver/apply/status_test.go b/test/integration/apiserver/apply/status_test.go index 751643a7f97b..864bf0bbf6e4 100644 --- a/test/integration/apiserver/apply/status_test.go +++ b/test/integration/apiserver/apply/status_test.go @@ -156,8 +156,17 @@ func TestApplyStatus(t *testing.T) { namespace = "" } name := newObj.GetName() + + // etcd test stub data doesn't contain apiVersion/kind (!), but apply requires it + newObj.SetGroupVersionKind(mapping.GroupVersionKind) + createData, err := json.Marshal(newObj.Object) + if err != nil { + t.Fatal(err) + } + rsc := dynamicClient.Resource(mapping.Resource).Namespace(namespace) - _, err := rsc.Create(context.TODO(), &newObj, metav1.CreateOptions{FieldManager: "create_test"}) + // apply to create + _, err = rsc.Patch(context.TODO(), name, types.ApplyPatchType, []byte(createData), metav1.PatchOptions{FieldManager: "create_test"}) if err != nil { t.Fatal(err) } From 593cd4db7a315dd5bac83832b36f403a249b2619 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Thu, 1 Apr 2021 03:55:03 +0000 Subject: [PATCH 6/7] make generated_files --- pkg/apis/core/v1/zz_generated.defaults.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/core/v1/zz_generated.defaults.go b/pkg/apis/core/v1/zz_generated.defaults.go index b895109c011d..8751929ed0d9 100644 --- a/pkg/apis/core/v1/zz_generated.defaults.go +++ b/pkg/apis/core/v1/zz_generated.defaults.go @@ -917,7 +917,7 @@ func SetObjectDefaults_Service(in *v1.Service) { SetDefaults_Service(in) for i := range in.Spec.Ports { a := &in.Spec.Ports[i] - if a.Protocol == "" { + if reflect.ValueOf(a.Protocol).IsZero() { a.Protocol = "TCP" } } From b8f7e215eaaec772f6b3f4bd16682dbf670cab0e Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Fri, 12 Feb 2021 10:36:15 -0800 Subject: [PATCH 7/7] Fix test now that empty struct are tracked in mangaed fields --- test/integration/apiserver/apply/status_test.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/test/integration/apiserver/apply/status_test.go b/test/integration/apiserver/apply/status_test.go index 864bf0bbf6e4..045f3052e644 100644 --- a/test/integration/apiserver/apply/status_test.go +++ b/test/integration/apiserver/apply/status_test.go @@ -62,12 +62,6 @@ var statusData = map[schema.GroupVersionResource]string{ const statusDefault = `{"status": {"conditions": [{"type": "MyStatus", "status":"True"}]}}` -// Some status-only APIs have empty object on creation. Therefore we don't expect create_test -// managedFields for these APIs -var ignoreCreateManagementList = map[schema.GroupVersionResource]struct{}{ - gvr("internal.apiserver.k8s.io", "v1alpha1", "storageversions"): {}, -} - func gvr(g, v, r string) schema.GroupVersionResource { return schema.GroupVersionResource{Group: g, Version: v, Resource: r} } @@ -205,11 +199,7 @@ func TestApplyStatus(t *testing.T) { t.Fatalf("Couldn't find apply_status_test: %v", managedFields) } if !findManager(managedFields, "create_test") { - if _, ok := ignoreCreateManagementList[mapping.Resource]; !ok { - t.Fatalf("Couldn't find create_test: %v", managedFields) - } - } else if _, ok := ignoreCreateManagementList[mapping.Resource]; ok { - t.Fatalf("found create_test in ignoreCreateManagementList resource: %v", managedFields) + t.Fatalf("Couldn't find create_test: %v", managedFields) } if err := rsc.Delete(context.TODO(), name, *metav1.NewDeleteOptions(0)); err != nil {