add auth schema with auth providers order #499

aborilov · 2020-07-02T07:59:17Z

What this PR does / why we need it:
configure auth provider ordering explicitly by the user

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #470

Special notes for your reviewer:

Documentation:

Does this PR introduce a user-facing change?:

NONE

kubermatic-bot · 2020-07-02T07:59:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aborilov

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

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

pkg/apis/operator/v1alpha1/apiserver_types.go

nmiculinic

See my comments about schema/order in the associated Issue.

pkg/apis/operator/v1alpha1/apiserver_types.go

pkg/operator/internal/webhooks/kubecarrier_webhook.go

aborilov · 2020-07-07T06:37:46Z

/retest

aborilov · 2020-07-07T07:49:53Z

/retest

jiachengxu · 2020-07-07T07:58:53Z

pkg/operator/internal/webhooks/kubecarrier_webhook.go

@@ -70,5 +71,40 @@ func (r *KubeCarrierWebhookHandler) validateCreate(kubeCarrier *operatorv1alpha1
 	if kubeCarrier.Name != constants.KubeCarrierDefaultName {
 		return fmt.Errorf("KubeCarrier object name should be 'kubecarrier', found: %s", kubeCarrier.Name)
 	}
+	auth := map[string]bool{}


I think in go, we use map[string]struct{} as set in common cases

it looks much nicer with map[string]bool{} then map[string]strunct{}{}.
also with bool as a value we can do if auth['anonymous'] but with struct you will have to do if _, ok := auth['anonymous']; ok{

nmiculinic

I've gone over the whole PR again. Please address the comment, I've left a few of my own. Afterward, I'll lgtm it; it's almost done except for a few nits/legibility here and there.

pkg/internal/resources/apiserver/apiserver.go

nmiculinic · 2020-07-07T10:53:29Z

pkg/internal/resources/apiserver/apiserver.go

-				)
-				supportedAuth = append(supportedAuth, "Htpasswd")
+	var supportedAuth []string
+	for _, auth := range c.Spec.Authentication {


Suggested change

for _, auth := range c.Spec.Authentication {

var supportedAuth []string

present := make(map[string]struct{})

for i, auth := range c.Spec.Authentication {

if auth.StaticUsers != nil {

if _, ok := present["statik"]; ok {

return nil, fmt.Errorf("statik config redefined")

}

present["statik"] = struct{}{}

if err := addStatikConfig(auth.StaticUsers, &deploymentPatch.Spec.Template.Spec); err != nil {

return nil, err

}

supportedAuth = append(supportedAuth, "Statik")

}

if len(supportedAuth) != (i + 1) {

return nil, fmt.Errorf("only a single auth provider can be defined per statement. got %v", supportedAuth[i + 1:])

}

...

}

perhaps extract each auth provider to its own method; it's easier to read and follow the main function. Also, you're missing the checks if only a single one is non-nil.

Also, you're missing the checks if only a single one is non-nil.

we have this check in the kubecarrier_webhook

defense in depth.

add we have if-else here, so only the first one will be added

aborilov · 2020-07-07T11:13:26Z

I've gone over the whole PR again. Please address the comment, I've left a few of my own. Afterward, I'll lgtm it; it's almost done except for a few nits/legibility here and there.

I think I've done all of them except defaulting webhook, as for now, we have only validation webhook? do you think it's a good idea to change it to defaulting webhook in this PR?

aborilov · 2020-07-07T11:43:40Z

/retest

aborilov · 2020-07-08T07:58:41Z

/retest

aborilov · 2020-07-08T08:28:13Z

/retest

aborilov · 2020-07-08T08:56:32Z

/retest

nmiculinic · 2020-07-08T09:17:06Z

I'm still worried about validation and error handling. Currently, only in the kubecarrier CRD, the API server validation is being checked. Not in the API Server CRD, which the user can create, nor in the apiserver.Manifests resource generation. If no/multiple auth options are specified per line in any one of those the failure shall be silent and ignored --> the most bite you in the ass type.

What I'm suggesting is creating: func validateAPIServerSpec(*operatorv1alpha1.APIServerSpec) error and defaultAPIServerSpec(*operatorv1alpha1.APIServerSpec) error helper functions which can and should be used in 3 places:

(maybe placing those functions close to the API definitions, or even make them member function for the operatorv1alpha1.APIServerSpec object? .Validate() error .Default() error)

Kubecarrier CRD webooks
API Server CRD webhooks (this doesn't exists for now)
apiserver.Manifests function (especially here, since it's the last point before silent failure)

What do you think?

Otherwise, this is a great PR, well written, and tested!

jiachengxu · 2020-07-09T05:02:42Z

I'm still worried about validation and error handling. Currently, only in the kubecarrier CRD, the API server validation is being checked. Not in the API Server CRD, which the user can create, nor in the apiserver.Manifests resource generation. If no/multiple auth options are specified per line in any one of those the failure shall be silent and ignored --> the most bite you in the ass type.

@nmiculinic The worrying is completely valid, and I agree that this may become an issue in the future, but for now I think it is fine. The reason are as following:

In the usecase that we offer KubeCarrier, users don't have permission to interact with operator API group, so this should be fine.
In the usercase that someone install KubeCarrier, and play around with it. Mistakes in APIServerCRD and also in apiserver.Manifests will be kind of "ignored" , this is because KubeCarrier operator controller will reconcile the object with the default KubeCarrer config from CLI, and for that one we have a webhook to reject all mistakes.

aborilov · 2020-07-09T06:48:19Z

I don't like to overcomplicate things, in our case users don't interact with APIserver configuration directly, so I don't think we need to add webhooks to apiserver operator. I will add defaulting webhook for KubeCarrier operator and I like the idea of Default and Validate methods for APIServiceSpec, will also do that. Later, if we decide to add webhooks for APIserver operator, we can easily use them

nmiculinic · 2020-07-09T07:47:07Z

@aborilov , that's what I meant, adding APIserver webhooks is too much for this PR. Adding checks in apiserver.Manifests should be all I ask for. Those validate functions shall be called in two places -- kubecarrier webook and apiserver.Manifests and all is good!

aborilov · 2020-07-09T07:48:12Z

@aborilov , that's what I meant, adding APIserver webhooks is too much for this PR. Adding checks in apiserver.Manifests should be all I ask for. Those validate functions shall be called in two places -- kubecarrier webook and apiserver.Manifests and all is good!

yep, that's done

aborilov · 2020-07-09T07:56:45Z

/retest

aborilov · 2020-07-09T08:04:15Z

/retest

nmiculinic · 2020-07-10T07:14:46Z

/lgtm

kubermatic-bot · 2020-07-10T07:14:49Z

LGTM label has been added.

Git tree hash: 1d3f7dbb1bee08642bd23348761c2d31829d6a69

add auth schema with auth providers order

7c27999

kubermatic-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. labels Jul 2, 2020

kubermatic-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 2, 2020

jiachengxu reviewed Jul 2, 2020

View reviewed changes

pkg/apis/operator/v1alpha1/apiserver_types.go Outdated Show resolved Hide resolved

pkg/apis/operator/v1alpha1/apiserver_types.go Outdated Show resolved Hide resolved

nmiculinic suggested changes Jul 3, 2020

View reviewed changes

kubermatic-bot assigned nmiculinic Jul 3, 2020

nmiculinic suggested changes Jul 6, 2020

View reviewed changes

pkg/apis/operator/v1alpha1/apiserver_types.go Outdated Show resolved Hide resolved

replace order list with list of auth configs

842a7bb

kubermatic-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Jul 6, 2020

nmiculinic suggested changes Jul 6, 2020

View reviewed changes

pkg/operator/internal/webhooks/kubecarrier_webhook.go Outdated Show resolved Hide resolved

aborilov added 2 commits July 7, 2020 09:11

Merge branch 'master' into auth-schema

1f327cd

make generate

9b9a241

kubermatic-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2020

only one auth config per item enabled

ae5ac56

jiachengxu reviewed Jul 7, 2020

View reviewed changes

nmiculinic suggested changes Jul 7, 2020

View reviewed changes

remove broken test

5cc86b3

kubermatic-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2020

aborilov added 3 commits July 8, 2020 10:24

add provider constants

13e2581

Merge branch 'master' into auth-schema

1e331c8

make generate

f6fcf48

kubermatic-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2020

fix types

654b813

kubermatic-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2020

aborilov added 2 commits July 9, 2020 10:44

add mutatic webhook for kubecarrier operator

0e9dfeb

Merge branch 'master' into auth-schema

f0a0fff

make generate

00595d7

kubermatic-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2020

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2020

kubermatic-bot merged commit 88784df into master Jul 10, 2020

kubermatic-bot deleted the auth-schema branch July 10, 2020 07:15

-	for _, auth := range c.Spec.Authentication {
+	var supportedAuth []string
+	present := make(map[string]struct{})
+	for i, auth := range c.Spec.Authentication {
+		if auth.StaticUsers != nil {
+			if _, ok := present["statik"]; ok {
+				return nil, fmt.Errorf("statik config redefined")
+			}
+			present["statik"] = struct{}{}
+			if err := addStatikConfig(auth.StaticUsers, &deploymentPatch.Spec.Template.Spec); err != nil {
+				return nil, err
+			}
+			supportedAuth = append(supportedAuth, "Statik")
+		}
+		if len(supportedAuth) != (i + 1) {
+			return nil, fmt.Errorf("only a single auth provider can be defined per statement. got %v", supportedAuth[i + 1:])
+		}
+		...
+	}

add auth schema with auth providers order #499

add auth schema with auth providers order #499

Uh oh!

Conversation

aborilov commented Jul 2, 2020

Uh oh!

kubermatic-bot commented Jul 2, 2020

Uh oh!

Uh oh!

Uh oh!

nmiculinic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aborilov commented Jul 7, 2020

Uh oh!

aborilov commented Jul 7, 2020

Uh oh!

jiachengxu Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

aborilov Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

nmiculinic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nmiculinic Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

aborilov Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

nmiculinic Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

aborilov Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

aborilov commented Jul 7, 2020

Uh oh!

aborilov commented Jul 7, 2020

Uh oh!

aborilov commented Jul 8, 2020

Uh oh!

aborilov commented Jul 8, 2020

Uh oh!

aborilov commented Jul 8, 2020

Uh oh!

nmiculinic commented Jul 8, 2020

Uh oh!

jiachengxu commented Jul 9, 2020

Uh oh!

aborilov commented Jul 9, 2020

Uh oh!

nmiculinic commented Jul 9, 2020

Uh oh!

aborilov commented Jul 9, 2020

Uh oh!

aborilov commented Jul 9, 2020

Uh oh!

aborilov commented Jul 9, 2020

Uh oh!

nmiculinic commented Jul 10, 2020

Uh oh!

kubermatic-bot commented Jul 10, 2020

Uh oh!

Uh oh!