add an observer for webhookTokenAuthenticator #890

stlaz · 2020-06-22T14:58:00Z

Adds an observer that's supposed to observe the authentication.Spec.WebhookTokenAuthenticator field and sync a potential referenced secret to openshift-kube-apiserver NS

stlaz · 2020-06-24T08:19:31Z

/retest

stlaz · 2020-06-29T07:23:04Z

/retest

pkg/operator/configobservation/auth/webhook_authenticator.go

sttts · 2020-07-01T09:56:59Z

pkg/operator/configobservation/auth/webhook_authenticator.go

+
+		if secretErrors := validateKubeconfigSecret(kubeconfigSecret); len(secretErrors) > 0 {
+			return existingConfig, append(errs,
+				fmt.Errorf("secret openshift-config/%s: %w", webhookSecretName, utilerrors.NewAggregate(secretErrors)))


secret openshift-config/%s is not a real error message. Am not sure that appending on aggregate error will look good at the end.

I turned it into an actual error message

sttts · 2020-07-01T09:58:42Z

pkg/operator/configobservation/auth/webhook_authenticator.go

+		resourceSyncer.SyncSecret(
+			resourcesynccontroller.ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: "webhook-authenticator"},
+			resourcesynccontroller.ResourceLocation{Namespace: operatorclient.GlobalUserSpecifiedConfigNamespace, Name: webhookSecretName},
+		)


syncing from an observer is an unexpected side-effect. How do we do this elsewhere?

This is being done in other observers as well:

cluster-kube-apiserver-operator/pkg/operator/configobservation/apiserver/observe_apiserver.go

Line 226 in f73bebb

errs = append(errs, syncObservedResources(resourceSync, resourceSyncRules)...)

cluster-kube-apiserver-operator/pkg/operator/configobservation/auth/auth_metadata.go

Lines 84 to 93 in f73bebb

err = listers.ResourceSyncer().SyncConfigMap(

resourcesynccontroller.ResourceLocation{

Namespace: targetNamespaceName,

Name: "oauth-metadata",

},

resourcesynccontroller.ResourceLocation{

Namespace: sourceNamespace,

Name: sourceConfigMap,

},

)

We're handling the field that's a reference to a secret here, it makes sense to me to handle the sync as well.

This is backed by the resource sync controller, which then essential does this:

c.configMapSyncRules[destination] = source

So this sync is ongoing, the call only registers a rule, does not do the a synchronous sync. And a new, different source overrides the old source.

So, I think this is fine.

mfojtik · 2020-07-10T10:14:51Z

pkg/operator/configobservation/auth/webhook_authenticator.go

+
+var (
+	webhookTokenAuthenticatorPath = []string{"apiServerArguments", "authentication-token-webhook-config-file"}
+	webhookTokenAuthenticatorFile = []interface{}{"/etc/kubernetes/static-pod-resources/secrets/webhook-authenticator/kubeConfig"}


is kubeConfig right? (usually it is kubeconfig)

as per API doc, this is expected

pkg/operator/configobservation/auth/webhook_authenticator.go

mfojtik · 2020-07-10T10:17:16Z

pkg/operator/configobservation/auth/webhook_authenticator.go

+		)
+	}
+
+	if observedWebhookConfigured != existingWebhookConfigured {


we usually use reflect.DeepEqual()

this is a comparison of two booleans

mfojtik · 2020-07-10T10:18:56Z

couple nits, LGTM otherwise

mfojtik · 2020-07-10T10:29:15Z

/lgtm
/approve

openshift-ci-robot · 2020-07-10T10:29:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, stlaz

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mfojtik]

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

stlaz · 2020-07-10T12:18:26Z

/retest

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2020

openshift-ci-robot requested review from mfojtik and soltysh June 22, 2020 14:58

stlaz force-pushed the webhook-authenticators branch 2 times, most recently from b66e007 to 753ec77 Compare June 23, 2020 14:29

stlaz force-pushed the webhook-authenticators branch 2 times, most recently from b6dbace to e40b56b Compare June 26, 2020 10:14

bump api

fce4f37

stlaz force-pushed the webhook-authenticators branch from e40b56b to ee4171b Compare June 26, 2020 13:47

stlaz changed the title ~~[wip] add an observer for webhookTokenAuthenticator~~ add an observer for webhookTokenAuthenticator Jun 29, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2020

sttts reviewed Jul 1, 2020

View reviewed changes

pkg/operator/configobservation/auth/webhook_authenticator.go Show resolved Hide resolved

sttts reviewed Jul 1, 2020

View reviewed changes

pkg/operator/configobservation/auth/webhook_authenticator.go Outdated Show resolved Hide resolved

sttts reviewed Jul 1, 2020

View reviewed changes

pkg/operator/configobservation/auth/webhook_authenticator.go Outdated Show resolved Hide resolved

sttts reviewed Jul 1, 2020

View reviewed changes

stlaz force-pushed the webhook-authenticators branch from ee4171b to 8bf9741 Compare July 1, 2020 10:33

mfojtik reviewed Jul 10, 2020

View reviewed changes

pkg/operator/configobservation/auth/webhook_authenticator.go Show resolved Hide resolved

mfojtik reviewed Jul 10, 2020

View reviewed changes

stlaz force-pushed the webhook-authenticators branch from 8bf9741 to 4078b00 Compare July 10, 2020 10:22

add observer for webhook token authenticator

5f86326

stlaz force-pushed the webhook-authenticators branch from 4078b00 to 5f86326 Compare July 10, 2020 10:23

openshift-ci-robot assigned mfojtik Jul 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2020

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

openshift-merge-robot merged commit 53f0782 into openshift:master Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add an observer for webhookTokenAuthenticator #890

add an observer for webhookTokenAuthenticator #890

stlaz commented Jun 22, 2020 •

edited

stlaz commented Jun 24, 2020

stlaz commented Jun 29, 2020

sttts Jul 1, 2020

stlaz Jul 1, 2020

sttts Jul 1, 2020

stlaz Jul 1, 2020

sttts Jul 1, 2020

mfojtik Jul 10, 2020

stlaz Jul 10, 2020

mfojtik Jul 10, 2020

stlaz Jul 10, 2020

mfojtik commented Jul 10, 2020

mfojtik commented Jul 10, 2020

openshift-ci-robot commented Jul 10, 2020

stlaz commented Jul 10, 2020

	err = listers.ResourceSyncer().SyncConfigMap(
	resourcesynccontroller.ResourceLocation{
	Namespace: targetNamespaceName,
	Name: "oauth-metadata",
	},
	resourcesynccontroller.ResourceLocation{
	Namespace: sourceNamespace,
	Name: sourceConfigMap,
	},
	)

add an observer for webhookTokenAuthenticator #890

add an observer for webhookTokenAuthenticator #890

Conversation

stlaz commented Jun 22, 2020 • edited

stlaz commented Jun 24, 2020

stlaz commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Jul 10, 2020

mfojtik commented Jul 10, 2020

openshift-ci-robot commented Jul 10, 2020

stlaz commented Jul 10, 2020

stlaz commented Jun 22, 2020 •

edited