storage/kubernetes: add CRD support #1062

rithujohn191 · 2017-09-13T18:02:08Z

This PR makes the following changes:

Add CRD support for kubernetes storage. In order to enable CRDs a Boolean flag has been introduced in the kubernetes storage config (useCRD).
The apiVersion i.e the API Group and version can be configured via the ConfigMap.

ericchiang · 2017-09-13T20:09:03Z

storage/kubernetes/storage.go

+		// they'll immediately be available, but ensures that the client will actually try
+		// once.
+		logger.Errorf("failed creating custom resource definitions: %v", err)
+		go func() {


This go statement needs to be inside the if block above.

ericchiang · 2017-09-13T20:10:40Z

storage/kubernetes/storage.go

 	if err != nil {
 		return nil, fmt.Errorf("create client: %v", err)
 	}

 	ctx, cancel := context.WithCancel(context.Background())

+	if c.UseCRD {
+		if !cli.createCustomResourceDefinitions() {


Instead of reproducing this logic, I'd recommend just replacing the createThirdPartyResources call bellow with something that conditionally creates either CRDs or TPRs. E.g.

// Create either TPRs or CRDs. cli.registerCustomResoruces(c.UseCRD)

And not having this block be reproduced.

ericchiang · 2017-09-13T20:12:29Z

storage/kubernetes/types.go

+			Group:   apiGroup,
+			Version: "v1",
+			Names: k8sapi.CustomResourceDefinitionNames{
+				Plural:   "signingkeies",


Can you add a comment why we chose not to change this? E.g.

// An artifact from the old TPR pluralization. Since users don't directly interact with this value, it'd be more of // a pain to correct this than to leave as is.

ericchiang · 2017-09-13T20:12:56Z

storage/kubernetes/storage.go

@@ -38,6 +38,8 @@ const (
 type Config struct {
 	InCluster      bool   `json:"inCluster"`
 	KubeConfigFile string `json:"kubeConfigFile"`
+	APIVersion     string `json:"apiVersion"` // API Group and version


I'd rather not expose this. It's always implied by "useCRD"

but how else would users configure the APIVersion?

ericchiang · 2017-09-13T20:13:26Z

storage/kubernetes/storage.go

@@ -38,6 +38,8 @@ const (
 type Config struct {
 	InCluster      bool   `json:"inCluster"`
 	KubeConfigFile string `json:"kubeConfigFile"`
+	APIVersion     string `json:"apiVersion"` // API Group and version
+	UseCRD         bool   `json:"useCRD"`     // Flag option to use CRDs instead of TPRs


At some point we need to leave a comment that this will default to true.

ericchiang

few nits, but overall looks good. thanks!

ericchiang · 2017-09-13T20:14:31Z

Also some lint checks are failing

/home/travis/gopath/src/github.com/coreos/dex/storage/kubernetes/k8sapi/crd_extensions.go:50:2: exported const ClusterScoped should have comment (or a comment on this block) or be unexported
/home/travis/gopath/src/github.com/coreos/dex/storage/kubernetes/k8sapi/crd_extensions.go:54:6: exported type ConditionStatus should have comment or be unexported

rithujohn191 · 2017-09-13T23:00:55Z

@ericchiang addressed all feedback

rithujohn191 · 2017-09-13T23:04:28Z

Testing:
Manually tested with a kubernetes cluster. All CRDs got created.

kubectl get customresourcedefinition
NAME                                    KIND
authcodes.dex.coreos.com                CustomResourceDefinition.v1beta1.apiextensions.k8s.io
authrequests.dex.coreos.com             CustomResourceDefinition.v1beta1.apiextensions.k8s.io
connectors.dex.coreos.com               CustomResourceDefinition.v1beta1.apiextensions.k8s.io
oauth2clients.dex.coreos.com            CustomResourceDefinition.v1beta1.apiextensions.k8s.io
offlinesessionses.dex.coreos.com        CustomResourceDefinition.v1beta1.apiextensions.k8s.io
passwords.dex.coreos.com                CustomResourceDefinition.v1beta1.apiextensions.k8s.io
refreshtokens.dex.coreos.com            CustomResourceDefinition.v1beta1.apiextensions.k8s.io
signingkeies.dex.coreos.com             CustomResourceDefinition.v1beta1.apiextensions.k8s.io

TODO: still need to add docs regarding migration

rithujohn191 · 2017-09-13T23:05:48Z

Sample CRD that gets created:

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  creationTimestamp: 2017-09-13T19:56:28Z
  name: authcodes.dex.coreos.com
  resourceVersion: "288893"
  selfLink: /apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions/authcodes.dex.coreos.com
  uid: a1cb72dc-98bd-11e7-8f6a-02d13336a01e
spec:
  group: dex.coreos.com
  names:
    kind: AuthCode
    listKind: AuthCodeList
    plural: authcodes
    singular: authcode
  scope: Namespaced
  version: v1
status:
  acceptedNames:
    kind: AuthCode
    listKind: AuthCodeList
    plural: authcodes
    singular: authcode
  conditions:
  - lastTransitionTime: null
    message: no conflicts found
    reason: NoConflicts
    status: "True"
    type: NamesAccepted
  - lastTransitionTime: 2017-09-13T19:56:28Z
    message: the initial names have been accepted
    reason: InitialNamesAccepted
    status: "True"
    type: Established

ericchiang · 2017-09-13T23:07:55Z

code lgtm.

Can you add a doc and switch the conformance tests to use CRDs?

diegs

Very clean, just one kind of annoying overall comment.

diegs · 2017-09-13T23:12:46Z

storage/kubernetes/storage.go

 type Config struct {
 	InCluster      bool   `json:"inCluster"`
 	KubeConfigFile string `json:"kubeConfigFile"`
+	UseCRD         bool   `json:"useCRD"` // Flag option to use CRDs instead of TPRs


I know this sounds kind of silly, but I would make the flag UseTPR instead of UseCRD. You can bump to a new major version for this, so clients have to explicitly set UseTPR if they want to. Then removing the flag later will be much cleaner and require no changes from conforming clients.

(Note that this applies to all the boolean vars you plumb through everywhere).

rithujohn191 · 2017-09-14T18:30:56Z

Addressed all feedback. Added basic CRD documentation for now. Will handle CRD migration docs and script in a separate PR

diegs

LGTM

ericchiang · 2017-09-14T18:35:29Z

Documentation/storage.md

+  type: kubernetes
+  config:
+    kubeConfigFile: kubeconfig
+    useCRD: true


nit: this is "useTPR"

thanks for catching that

storage/kubernetes: add CRD support

[WIP]: add CRD support

1464813

rithujohn191 requested a review from ericchiang September 13, 2017 18:02

ericchiang reviewed Sep 13, 2017

View reviewed changes

rithujohn191 force-pushed the crd-migration branch 3 times, most recently from 336929f to 3ff69a6 Compare September 13, 2017 22:58

rithujohn191 changed the title ~~[WIP]: add CRD support~~ storage/kubernetes: add CRD support Sep 13, 2017

rithujohn191 requested a review from diegs September 13, 2017 23:08

diegs reviewed Sep 13, 2017

View reviewed changes

rithujohn191 force-pushed the crd-migration branch 3 times, most recently from b0f721b to 6dccfa9 Compare September 14, 2017 18:30

diegs approved these changes Sep 14, 2017

View reviewed changes

ericchiang reviewed Sep 14, 2017

View reviewed changes

ericchiang mentioned this pull request Sep 14, 2017

Migrate from TPRs to CRDs #993

Closed

storage/kubernetes: add CRD support

1311caf

rithujohn191 force-pushed the crd-migration branch from 6dccfa9 to 1311caf Compare September 14, 2017 18:48

ericchiang approved these changes Sep 14, 2017

View reviewed changes

rithujohn191 merged commit 03de0ec into dexidp:master Sep 14, 2017

rithujohn191 deleted the crd-migration branch September 14, 2017 20:22

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019

Merge pull request dexidp#1062 from rithujohn191/crd-migration

bb54077

storage/kubernetes: add CRD support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage/kubernetes: add CRD support #1062

storage/kubernetes: add CRD support #1062

rithujohn191 commented Sep 13, 2017

ericchiang Sep 13, 2017

ericchiang Sep 13, 2017 •

edited

Loading

ericchiang Sep 13, 2017

ericchiang Sep 13, 2017

rithujohn191 Sep 13, 2017

ericchiang Sep 13, 2017

ericchiang left a comment

ericchiang commented Sep 13, 2017

rithujohn191 commented Sep 13, 2017

rithujohn191 commented Sep 13, 2017

rithujohn191 commented Sep 13, 2017

ericchiang commented Sep 13, 2017

diegs left a comment

diegs Sep 13, 2017

rithujohn191 commented Sep 14, 2017

diegs left a comment

ericchiang Sep 14, 2017

rithujohn191 Sep 14, 2017

storage/kubernetes: add CRD support #1062

storage/kubernetes: add CRD support #1062

Conversation

rithujohn191 commented Sep 13, 2017

Choose a reason for hiding this comment

ericchiang Sep 13, 2017 • edited Loading

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

ericchiang left a comment

Choose a reason for hiding this comment

ericchiang commented Sep 13, 2017

rithujohn191 commented Sep 13, 2017

rithujohn191 commented Sep 13, 2017

rithujohn191 commented Sep 13, 2017

ericchiang commented Sep 13, 2017

diegs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rithujohn191 commented Sep 14, 2017

diegs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang Sep 13, 2017 •

edited

Loading