OAuth related API Services can be managed/served by an external server/operator by p0lyn0mial · Pull Request #294 · openshift/cluster-openshift-apiserver-operator

p0lyn0mial · 2020-01-10T12:50:46Z

Please see https://github.com/openshift/enhancements/blob/master/enhancements/authentication/separate-oauth-resources.md for details.

p0lyn0mial · 2020-01-10T12:51:13Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+	oauthOperatorMajorMinor.Major = oauthOperatorVersion.Major
+	oauthOperatorMajorMinor.Minor = oauthOperatorVersion.Minor
+
+	return operatorVersionMajorMinor.Compare(oauthOperatorVersion) == 0


this will be slightly different in 4.3

sttts · 2020-01-10T12:52:35Z

pkg/operator/apiservicecontroller/apiservice_controller.go

 type APIServiceController struct {
 	name        string
 	apiServices []*apiregistrationv1.APIService
+	apiGroupsManagedByOAuthServer []schema.GroupVersion


would use more generic names. We will have another split eventually.

like apiGroupsManagedByExternalServer ?

p0lyn0mial · 2020-01-10T13:00:57Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+	filteredApiServices := []*apiregistrationv1.APIService{}
+
+	// the c.apiServices list is authoritative
+	// it can compete with oauth-server in case of errors but it could save us on downgrade (the current coao and an old cao)


if we remove this code from 4.4 then we can hit an issue on downgrade (the current coao and an old cao)

p0lyn0mial · 2020-01-10T14:06:05Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+}
+
+func (c *APIServiceController) isAPIServiceAnnotatedByExternalServer(apiService *apiregistrationv1.APIService) bool {
+	existingApiService, err := c.apiregistrationv1Client.APIServices().Get(apiService.Name, metav1.GetOptions{})


it could be taken from a lister.

p0lyn0mial · 2020-01-10T16:32:55Z

@deads2k I think that our plan is to remove this code and move it to 4.3, I am right?

deads2k · 2020-01-10T17:45:52Z

pkg/operator/apiservicecontroller/apiservice_controller.go

 	}
 }
+
+// TODO: change the name


godoc please

deads2k · 2020-01-10T17:46:15Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+			// TODO: log that this resource is being skipped on behalf of oauth-server
+			continue
+		}
+		filteredApiServices = append(filteredApiServices, apiService)


deads2k · 2020-01-10T17:47:18Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+	// it can compete with oauth-server in case of errors but it could save us on downgrade (the current coao and an old cao)
+	for _, apiService := range c.apiServices {
+		if c.isAPIServicePotentiallyManagedByExternalServer(apiService) && oauthOperatorPrecondition && c.isAPIServiceAnnotatedByExternalServer(apiService) {
+			// TODO: log that this resource is being skipped on behalf of oauth-server


only when the list has changed from last time. When the list changes, emit an event too.

good idea, thx.

deads2k · 2020-01-10T17:47:58Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+
+	oauthOperatorPrecondition := c.oauthOperatorAvailableAndVersionMatch()
+
+	filteredApiServices := []*apiregistrationv1.APIService{}


track this persistently on the struct so you can inform about deltas, not about the same things every time.

track this persistently on the struct so you can inform about deltas, not about the same things every time.

this still isn't persistent.

deads2k · 2020-01-10T17:49:20Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+// TODO: change the name
+func (c *APIServiceController) filterAPIServices() []*apiregistrationv1.APIService {
+
+	oauthOperatorPrecondition := c.oauthOperatorAvailableAndVersionMatch()


This needs to be a function you can pass in. Make it a WithSharedResponsibility

why ?

This is going to move to library-go so we can re-use it in openshift and oauth server operators

deads2k · 2020-01-10T17:49:43Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+	return filteredApiServices
+}
+
+func (c *APIServiceController) oauthOperatorAvailableAndVersionMatch() bool {


this needs to be a function you pass in during construction.

may I know why :) ?

This is going to move to library-go so we can re-use it in openshift and oauth server operators

deads2k · 2020-01-10T17:50:39Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+
+	oauthOperatorVersion, err := semver.Parse(oauthOperatorVersionString)
+	if err != nil {
+		// TODO: log the err


nah, ripple the error all the way up.

nah, ripple the error all the way up.

Yeah, this won't work well even in CI builds. those don't have real versions.

won't returning an error put the operator into a degraded state?

deads2k · 2020-01-10T17:56:12Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+}
+
+func (c *APIServiceController) oauthOperatorAvailableAndVersionMatch() bool {
+	oauthOperator, err := c.clusterOperatorLister.Get("authentication")


get the authentication.operator.openshift.io resource instead and create a new status field on it that says, "I'm managing this" then backport it to 4.3 and set it to false.

authentication.operator.openshift.io doesn't provide a version information, the KEP explicitly states that we should compare the versions - https://github.com/deads2k/openshift-enhancements/blob/011cb767fb0e769f60f73af8dfb501847e458161/enhancements/authentication/separate-oauth-resources.md

Yeah, turns out that doesn't work when compared against reality :( . Versions don't always exist. We need a non-version dependent mechanism. Sorry, I didn't think about it until I saw it.

deads2k · 2020-01-10T17:56:35Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+		return false
+	}
+
+	operatorVersionString := c.versionRecorder.GetVersions()["operator"]


then all the version stuff is unnecessary.

deads2k · 2020-01-10T17:57:00Z

pkg/operator/apiservicecontroller/apiservice_controller.go

-	"github.com/openshift/library-go/pkg/operator/events"
-	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
-	"github.com/openshift/library-go/pkg/operator/status"
-	"github.com/openshift/library-go/pkg/operator/v1helpers"


don't bother separating. it's a waste of time.

pkg/operator/apiservicecontroller/apiservice_controller.go

p0lyn0mial · 2020-01-13T13:10:37Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+}
+
+// AuthenticationOperatorAvailableAndConditionSetPrecondition lets the authentication operator know that he will manage OAuth API Resources by setting the appropriate condition on its status
+func AuthenticationOperatorAvailableAndConditionSetPrecondition(authOperatorName string, authOperatorGVR schema.GroupVersionResource, authOperatorConditionType string, authOperatorClient v1helpers.OperatorClient) func() error {


I will leave it here, for now, it can be easily moved outside.

pkg/operator/apiservicecontroller/apiservice_controller.go

p0lyn0mial · 2020-01-13T13:33:13Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+		}
+
+		if len(removed) > 0 {
+			c.eventRecorder.Eventf("OAuthAPIResourcesManagement", "The API Services for the following GVs %v will be managed by an external operator/server", removed)


This will have the following format The API Services for the following GVs [v1.oauth.openshift.io v1.user.openshift.io] will be managed by an external operator/server, do we want to make it pretty?

deads2k · 2020-01-13T14:19:29Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+
+// getAPIServicesToManage gets the desired list of API Services that will be managed by this operator
+// note that some services might be managed by an external operators/servers
+func (c *APIServiceController) getAPIServicesToManage() ([]*apiregistrationv1.APIService, error) {


make this function the one to pass through? Doing so allows simple things to be simple and makes hard things possible.

deads2k · 2020-01-13T14:21:08Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+	return c.managedAPIServices, nil
+}
+
+func (c *APIServiceController) isAPIServicePotentiallyManagedByExternalServer(apiService *apiregistrationv1.APIService) bool {


this check looks extraneous

pkg/operator/apiservicecontroller/apiservice_controller.go

p0lyn0mial · 2020-01-13T16:42:28Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+	observedAPIServices := authoritativeAPIServicesList
+	return func() ([]*apiregistrationv1.APIService, error) {
+		if externalOperatorPrecondition := operatorPrecondition(); externalOperatorPrecondition != nil {
+			return nil, externalOperatorPrecondition


we could return authoritativeAPIServicesList when the operator is not available.

we could return authoritativeAPIServicesList when the operator is not available.

yeah, that allows coasting.

deads2k · 2020-01-14T20:51:52Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+		}
+
+		// let know the authentication operator that he will manage OAuth API Resources
+		if !v1helpers.IsOperatorConditionTrue(operatorStatus.Conditions, authOperatorConditionType) {


I would still recommend an API field, but if @sttts is strongly against, I'll let you try this unstructured way first. I don't think it makes the situation easier.

sure, if you don't like the condition I'm going to add a new field.

deads2k · 2020-01-14T20:52:41Z

pkg/operator/apiservicecontroller/apiservice_controller.go

+			return err
+		}
+
+		if !v1helpers.IsOperatorConditionTrue(operatorStatus.Conditions, operatorv1.OperatorStatusTypeAvailable) {


I don't think available enters into it, but this is the spot to put ManagingOAuthAPIServer

this is what you had in your KEP :)

deads2k · 2020-01-14T20:53:40Z

pkg/operator/apiservicecontroller/apiservice_controller_test.go

 				operatorClient:          fakeOperatorClient,
 				apiregistrationv1Client: kubeAggregatorClient.ApiregistrationV1(),
 				versionRecorder:         status.NewVersionGetter(),
+				getAPIServicesToManageFunc: GetAPIServicesToManageWrapper(


Yeah, a separate struct with a NewAPIServicesToManage function is likely to read easier.

deads2k · 2020-01-14T20:54:09Z

pkg/operator/starter.go

+			apiServices(),
+			controllerConfig.EventRecorder,
+			sets.NewString("v1.oauth.openshift.io", "v1.user.openshift.io"),
+			"cluster.authentication.operator.openshift.io/managed",


authentication.operator.openshift.io/managed

p0lyn0mial · 2020-01-15T14:12:05Z

@deads2k ready for final review.

deads2k · 2020-01-15T20:49:52Z

pkg/operator/apiservicecontroller/apiservice_controller.go

 	name string,
-	apiServices []*apiregistrationv1.APIService,
+	versionRecorder status.VersionGetter,
+	getAPIServicesToManageFunc getAPIServicesToMangeFuncType,


super-nit. We generally call the type ***Func and instance **Fn

deads2k · 2020-01-15T21:19:40Z

/lgtm
/approve

I recommend getting someone else to have a look too so they understand what's been done.

openshift-ci-robot · 2020-01-15T21:20:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, p0lyn0mial

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~~ [deads2k]

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

… to an external server

openshift-ci-robot · 2020-01-16T07:52:24Z

New changes are detected. LGTM label has been removed.

p0lyn0mial · 2020-01-16T07:53:07Z

@deads2k please retag, I had to resolve some merge conflicts manually.

p0lyn0mial · 2020-01-16T08:01:15Z

/retest

p0lyn0mial · 2020-01-16T13:07:00Z

/retest

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 10, 2020

p0lyn0mial commented Jan 10, 2020

View reviewed changes

openshift-ci-robot requested review from deads2k and soltysh January 10, 2020 12:51

sttts reviewed Jan 10, 2020

View reviewed changes

p0lyn0mial commented Jan 10, 2020

View reviewed changes

deads2k reviewed Jan 10, 2020

View reviewed changes

pkg/operator/apiservicecontroller/apiservice_controller.go Outdated

}

}

// TODO: change the name

Copy link

Contributor

deads2k Jan 10, 2020

Choose a reason for hiding this comment

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

godoc please

deads2k reviewed Jan 10, 2020

View reviewed changes

p0lyn0mial force-pushed the donot-manage-apiservice-cao branch from 1125cd8 to c060ad5 Compare January 13, 2020 12:41

p0lyn0mial commented Jan 13, 2020

View reviewed changes

pkg/operator/apiservicecontroller/apiservice_controller.go Outdated Show resolved Hide resolved

p0lyn0mial commented Jan 13, 2020

View reviewed changes

pkg/operator/apiservicecontroller/apiservice_controller.go Outdated Show resolved Hide resolved

p0lyn0mial commented Jan 13, 2020

View reviewed changes

deads2k reviewed Jan 13, 2020

View reviewed changes

pkg/operator/apiservicecontroller/apiservice_controller.go Show resolved Hide resolved

p0lyn0mial force-pushed the donot-manage-apiservice-cao branch from c060ad5 to ce31686 Compare January 13, 2020 16:39

p0lyn0mial changed the title ~~wip~~ OAuth related API Services can be managed/served by an external server/operator Jan 13, 2020

p0lyn0mial commented Jan 13, 2020

View reviewed changes

openshift-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 13, 2020

deads2k reviewed Jan 14, 2020

View reviewed changes

p0lyn0mial force-pushed the donot-manage-apiservice-cao branch from 81600df to 7899798 Compare January 15, 2020 10:38

p0lyn0mial mentioned this pull request Jan 15, 2020

adds a new ManagingOAuthAPIServer field to authentication's operator status field openshift/api#567

Merged

p0lyn0mial force-pushed the donot-manage-apiservice-cao branch 2 times, most recently from 230d23a to 21c9e03 Compare January 15, 2020 14:10

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 15, 2020

deads2k reviewed Jan 15, 2020

View reviewed changes

openshift-ci-robot assigned deads2k Jan 15, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 15, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2020

p0lyn0mial added 2 commits January 16, 2020 08:50

adds the ability to delegate management of OAuth related API Services…

e0ba29c

… to an external server

bump (api)

713cd09

p0lyn0mial force-pushed the donot-manage-apiservice-cao branch from 21c9e03 to 713cd09 Compare January 16, 2020 07:51

openshift-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm Indicates that a PR is ready to be merged. labels Jan 16, 2020

deads2k added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020

openshift-merge-robot merged commit 5ca7887 into openshift:master Jan 16, 2020

p0lyn0mial mentioned this pull request Jan 20, 2020

Add controllerset for controlling an apiserver deployment openshift/library-go#667

Merged


		oauthOperatorPrecondition := c.oauthOperatorAvailableAndVersionMatch()

		filteredApiServices := []*apiregistrationv1.APIService{}

Conversation

p0lyn0mial commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial commented Jan 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

p0lyn0mial commented Jan 10, 2020 •

edited

Loading

p0lyn0mial Jan 10, 2020 •

edited

Loading