WRKLDS-728: Disable apiservers #366

ingvagabund · 2023-05-02T18:11:34Z

Extend the OA init code to explicitly disable individual apiservers. The change is important for explicitly disabling DeploymentConfigs and Builds API through capabilities. The controlplane configuration on-disk data type gets rendered by OA operator in openshift/cluster-openshift-apiserver-operator#532.

openshift-ci-robot · 2023-05-02T18:11:37Z

@ingvagabund: This pull request references WRKLDS-728 which is a valid jira issue.

Details

In response to this:

TBD

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ingvagabund · 2023-07-14T11:46:02Z

/retest-required

openshift-ci-robot · 2023-07-17T12:54:03Z

@ingvagabund: This pull request references WRKLDS-728 which is a valid jira issue.

Details

In response to this:

Extend the OA init code to explicitly disable individual apiservers. The change is important for explicitly disabling DeploymentConfigs and Builds API through capabilities. The controlplane configuration on-disk data type gets rendered by OA operator in openshift/cluster-openshift-apiserver-operator#532.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ingvagabund · 2023-07-17T13:00:53Z

pkg/route/apis/route/types.go

+	// chain. Do not include a CA certificate. The secret referenced should
+	// be present in the same namespace as that of the Route.
+	// Forbidden when `certificate` is set.
+	ExternalCertificate LocalObjectReference `json:"externalCertificate,omitempty" protobuf:"bytes,7,opt,name=externalCertificate"`


Internal types do not need the json tags

dgrisonnet · 2023-07-17T13:32:19Z

I think we will need to touch some other parts of the codebase to truly be able to enable/disable APIs. The current change only stop serving the APIs, but the underneath logic will still be executed even when the APIs are disabled.

The first one that I can see is the post start hooks: https://github.com/openshift/openshift-apiserver/blob/master/pkg/cmd/openshift-apiserver/openshiftapiserver/openshift_apiserver.go#L444-L473. Some of these will not be needed if their API is disabled.

Another part that will need to be gated on the config are the various caches configurations in https://github.com/openshift/openshift-apiserver/blob/master/pkg/cmd/openshift-apiserver/openshiftapiserver/config.go. I don't think we should create the cache of an API that is disabled. Same goes for the controllers that are normally used to fill these cache and respond to the apiserver.

There might be some other parts of the code that will need to be tweaked, but I generally think we should also try to enable/disable the logic behind the APIs.

ingvagabund · 2023-07-19T08:47:41Z

When trying to disable an unsupported group:

$ oc logs -n openshift-apiserver apiserver-db6f9dff8-5hq5q
Defaulted container "openshift-apiserver" out of: openshift-apiserver, openshift-apiserver-check-endpoints, fix-audit-permissions (init)
Copying system trust bundle
I0719 08:46:03.241688       1 dynamic_serving_content.go:113] "Loaded a new cert/key pair" name="serving-cert::/var/run/secrets/serving-cert/tls.crt::/var/run/secrets/serving-cert/tls.key"
I0719 08:46:03.749924       1 requestheader_controller.go:244] Loaded a new request header values for RequestHeaderAuthRequestController
I0719 08:46:03.752332       1 plugins.go:84] "Registered admission plugin" plugin="NamespaceLifecycle"
I0719 08:46:03.752347       1 plugins.go:84] "Registered admission plugin" plugin="ValidatingAdmissionWebhook"
I0719 08:46:03.752352       1 plugins.go:84] "Registered admission plugin" plugin="MutatingAdmissionWebhook"
I0719 08:46:03.752356       1 plugins.go:84] "Registered admission plugin" plugin="ValidatingAdmissionPolicy"
I0719 08:46:03.752670       1 admission.go:48] Admission plugin "project.openshift.io/ProjectRequestLimit" is not configured so it will be disabled.
I0719 08:46:03.753088       1 plugins.go:158] Loaded 5 mutating admission controller(s) successfully in the following order: NamespaceLifecycle,build.openshift.io/BuildConfigSecretInjector,image.openshift.io/ImageLimitRange,image.openshift.io/ImagePolicy,MutatingAdmissionWebhook.
I0719 08:46:03.753098       1 plugins.go:161] Loaded 9 validating admission controller(s) successfully in the following order: OwnerReferencesPermissionEnforcement,build.openshift.io/BuildConfigSecretInjector,build.openshift.io/BuildByStrategy,image.openshift.io/ImageLimitRange,image.openshift.io/ImagePolicy,quota.openshift.io/ClusterResourceQuota,route.openshift.io/RequiredRouteAnnotations,ValidatingAdmissionWebhook,ResourceQuota.
F0719 08:46:03.753366       1 cmd.go:72] only [apps.openshift.io build.openshift.io] APIs can be configured, "route.openshift.io" is not supported

ingvagabund · 2023-07-19T10:57:43Z

/retest-required

ingvagabund · 2023-07-20T07:45:38Z

/retest-required

dgrisonnet · 2023-07-27T12:39:32Z

I like the change to have an allow list with the API that can be disabled today. That said we still have the same problem as before from #366 (comment) where the inner logic (controller, cache, ...) will still be executed even if an API is disabled no?

Also, I think it would be a great addition to add a log line and maybe even a metric telling which API is enabled and which one is disabled.

ingvagabund · 2023-07-31T13:09:08Z

I like the change to have an allow list with the API that can be disabled today. That said we still have the same problem as before from #366 (comment) where the inner logic (controller, cache, ...) will still be executed even if an API is disabled no?

Checking the PostStart hooks I don't see anything related to DCs/Builds under https://github.com/openshift/openshift-apiserver/blob/master/pkg/cmd/openshift-apiserver/openshiftapiserver/openshift_apiserver.go#L453-L484. There's also no DCs/Builds informer mentioned in https://github.com/openshift/openshift-apiserver/blob/master/pkg/cmd/openshift-apiserver/openshiftapiserver/openshift_apiserver.go#L98-L142 or https://github.com/openshift/openshift-apiserver/blob/master/pkg/cmd/openshift-apiserver/openshiftapiserver/informers.go#L90-L99.

dgrisonnet · 2023-08-02T13:51:38Z

This looks good to me overall, but we might also want to disable these admission plugins https://github.com/openshift/openshift-apiserver/blob/master/pkg/cmd/openshift-apiserver/openshiftadmission/register.go#L37-L38 no?

ingvagabund · 2023-08-02T14:01:24Z

Those are registering handlers, which should not get invoked at all. Plus, RegisterOpenshiftAdmissionPlugins is invoked in RegisterAllAdmissionPlugins which is invoked in init() function. Too late to read the config and disable the handler.

dgrisonnet · 2023-08-02T14:08:11Z

We could maybe leverage the admission options to disable them? https://github.com/openshift/openshift-apiserver/blob/master/pkg/cmd/openshift-apiserver/openshiftapiserver/config.go#L212

But as you mentioned it should be impossible to invoke them so disabling them doesn't bring much value

dgrisonnet · 2023-08-02T14:10:24Z

pkg/cmd/openshift-apiserver/openshiftapiserver/openshift_apiserver.go

+	apiServers := make(map[openshiftcontrolplanev1.OpenShiftAPIserverName]openshiftcontrolplanev1.PerGroupOptions)
+
+	// At the moment only Builds and DeploymentConfig API can be disabled.
+	// Other APIs will be added to the list as needed.
+	for _, group := range c.ExtraConfig.APIServers.PerGroupOptions {
+		if !configurableAPIList.Has(group.Name) {
+			return nil, fmt.Errorf("only %v APIs can be configured, %q is not supported", sets.List[openshiftcontrolplanev1.OpenShiftAPIserverName](configurableAPIList), group.Name)
+		}
+		if _, exists := apiServers[group.Name]; exists {
+			return nil, fmt.Errorf("list of enabled/disabled API servers contains a duplicated entry for %v", group.Name)
+		}
+		enabledVersions := sets.NewString(group.EnabledVersions...)
+		disabledVersions := sets.NewString(group.DisabledVersions...)
+
+		if enabledVersions.Intersection(disabledVersions).Len() > 0 {
+			return nil, fmt.Errorf("list of enabled and disabled versions for %q is not allowed to intersect: %v are in both lists", group.Name, enabledVersions.Intersection(disabledVersions).List())
+		}
+		// Only v1 version is supported
+		for _, version := range enabledVersions.List() {
+			if version != "v1" {
+				return nil, fmt.Errorf("only v1 version is currently supported for %q: %v is not", group.Name, version)
+			}
+		}
+		for _, version := range disabledVersions.List() {
+			if version != "v1" {
+				return nil, fmt.Errorf("only v1 version is currently supported for %q: %v is not", group.Name, version)
+			}
+		}
+		apiServers[group.Name] = group
+	}
+
+	// All API servers are enabled by default (nothing new to enable -> ignore the list of enabled versions)
+	for name, initFnc := range apiServerInitializers {
+		if _, exists := apiServers[name]; exists {
+			// All API servers are serving v1 resources
+			if sets.NewString(apiServers[name].DisabledVersions...).Has("v1") {
+				continue
+			}
+		}
+		delegateAPIServer = addAPIServerOrDie(delegateAPIServer, initFnc)
+	}


nit: could you move that code inside a function to make New more readable?

dgrisonnet

/lgtm
/approve

openshift-ci · 2023-08-02T15:10:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, ingvagabund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dgrisonnet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-08-02T16:29:17Z

/retest-required

Remaining retests: 0 against base HEAD 9801454 and 2 for PR HEAD 2af0149 in total

openshift-ci · 2023-08-02T18:36:55Z

@ingvagabund: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn-cmd	`2af0149`	link	false	`/test e2e-aws-ovn-cmd`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 2, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 2, 2023

openshift-ci bot requested review from dgrisonnet and mfojtik May 2, 2023 18:12

ingvagabund force-pushed the disable-apiservers branch from 5320731 to 5899405 Compare May 21, 2023 08:24

ingvagabund mentioned this pull request May 21, 2023

WRKLDS-728: Make Build and DeploymentConfig API optional through capabilities openshift/api#1462

Merged

ingvagabund mentioned this pull request May 30, 2023

WRKLDS-728: OpenShiftAPIServerConfig: new ApiServers field to specify enabled/disabled API servers openshift/api#1475

Merged

ingvagabund force-pushed the disable-apiservers branch from 5899405 to e713c26 Compare June 20, 2023 10:57

ingvagabund force-pushed the disable-apiservers branch from e713c26 to 1f0c8c3 Compare July 14, 2023 07:39

ingvagabund force-pushed the disable-apiservers branch from 1f0c8c3 to bf1d15a Compare July 17, 2023 11:11

ingvagabund changed the title ~~wip: WRKLDS-728: Disable apiservers~~ WRKLDS-728: Disable apiservers Jul 17, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2023

ingvagabund commented Jul 17, 2023

View reviewed changes

Allow to disable individual API servers

c7d3b21

ingvagabund force-pushed the disable-apiservers branch from bf1d15a to c7d3b21 Compare July 19, 2023 08:46

dgrisonnet reviewed Aug 2, 2023

View reviewed changes

Move the delegateAPIServer initialization code under a new method

2af0149

dgrisonnet approved these changes Aug 2, 2023

View reviewed changes

openshift-ci bot assigned dgrisonnet Aug 2, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2023

openshift-merge-robot merged commit 18c41ee into openshift:master Aug 2, 2023

ingvagabund deleted the disable-apiservers branch August 3, 2023 08:15

WRKLDS-728: Disable apiservers #366

WRKLDS-728: Disable apiservers #366

Uh oh!

Conversation

ingvagabund commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 2, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingvagabund commented Jul 14, 2023

Uh oh!

openshift-ci-robot commented Jul 17, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingvagabund Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

dgrisonnet commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingvagabund commented Jul 19, 2023

Uh oh!

ingvagabund commented Jul 19, 2023

Uh oh!

ingvagabund commented Jul 20, 2023

Uh oh!

dgrisonnet commented Jul 27, 2023

Uh oh!

ingvagabund commented Jul 31, 2023

Uh oh!

dgrisonnet commented Aug 2, 2023

Uh oh!

ingvagabund commented Aug 2, 2023

Uh oh!

dgrisonnet commented Aug 2, 2023

Uh oh!

dgrisonnet Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

dgrisonnet left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 2, 2023

Uh oh!

openshift-ci-robot commented Aug 2, 2023

Uh oh!

openshift-ci bot commented Aug 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ingvagabund commented May 2, 2023 •

edited

Loading

openshift-ci-robot commented May 2, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jul 17, 2023 •

edited by openshift-ci bot

Loading

dgrisonnet commented Jul 17, 2023 •

edited

Loading