Add Authentication and OAuth crds #948

sallyom · 2018-12-19T20:49:35Z

This PR adds CRDs for Authentication and OAuth and a default CR for each.
@enj

openshift-ci-robot · 2018-12-19T20:49:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sallyom
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: abhinavdahiya

If they are not already assigned, you can assign the PR to them by writing /assign @abhinavdahiya in a comment when ready.

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

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

abhinavdahiya · 2018-12-19T21:07:17Z

why am i seeing these changes https://github.com/openshift/installer/pull/948/files#diff-6883245a42348c108776e47496bbf7c7

staebler

Comments in pkg/asset/manifests/authentication.go apply to pkg/asset/manifests/oauth.go as well.

See #943 for a discussion about whether these manifests should be written out by the manifest-templates target.

staebler · 2018-12-19T21:24:28Z

pkg/asset/manifests/authentication.go

Better error message.

staebler · 2018-12-19T21:24:47Z

pkg/asset/manifests/authentication.go

Wrap error in a meaningful message.

staebler · 2018-12-19T21:25:47Z

pkg/asset/manifests/authentication.go

This asset should not be loadable. Its files will be loaded by the Common Manifests asset. For more context, see #877.

👍 got it, thanks

staebler · 2018-12-19T21:26:20Z

pkg/asset/manifests/authentication.go

Remove this field. It is not needed.

sallyom · 2018-12-19T21:49:57Z

@abhinavdahiya I updated the commit in Gopkg.tomlfor openshift/api, then ran dep ensure, commited those changes in a single commit. I don't know why this file pkg/terraform/exec/vendor/github.com/masterzen/winrm/ntlm.go showed that diff, was not my direct doing. I over gofmt-ed, whoops

staebler · 2018-12-19T21:56:49Z

@abhinavdahiya I updated the commit in Gopkg.tomlfor openshift/api, then ran dep ensure, commited those changes in a single commit. I don't know why this file pkg/terraform/exec/vendor/github.com/masterzen/winrm/ntlm.go showed that diff, was not my direct doing.

@sallyom It looks like the result of running go fmt on the files.

abhinavdahiya · 2018-12-19T21:56:50Z

I updated the commit in Gopkg.tomlfor openshift/api, then ran dep ensure, commited those changes in a single commit. I don't know why this file pkg/terraform/exec/vendor/github.com/masterzen/winrm/ntlm.go showed that diff, was not my direct doing.

the seem formatting changes... maybe gofmt issues?

enj · 2018-12-19T23:07:58Z

data/data/manifests/openshift/cluster-authentication-crd.yaml

Set spec.subresources.status to the empty object for both resources.

enj · 2018-12-19T23:51:02Z

pkg/asset/manifests/authentication.go

All we need for this is:

configv1.Authentication{ TypeMeta: metav1.TypeMeta{ Kind: "Authentication", APIVersion: configv1.GroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ Name: "cluster", }, }

enj · 2018-12-19T23:55:22Z

pkg/asset/manifests/oauth.go

configv1.OAuth{ TypeMeta: metav1.TypeMeta{ Kind: "OAuth", APIVersion: configv1.GroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ Name: "cluster", }, Spec: configv1.OAuthSpec{ TokenConfig: configv1.TokenConfig{ AuthorizeTokenMaxAgeSeconds: 5 * 60, // 5 minutes AccessTokenMaxAgeSeconds: 24 * 60 * 60, // 1 day }, }, }

sallyom · 2018-12-20T01:14:36Z

@abhinavdahiya @staebler yes, was gofmt, duh! I was wondering why gofmt -w pkg/. took so much longer than usual , I would've noticed it eventually.. thanks

sallyom · 2018-12-20T02:28:28Z

@enj from cluster installed w/ this PR:

$ oc get authentications -o yaml
apiVersion: v1
items:
- apiVersion: config.openshift.io/v1
  kind: Authentication
  metadata:
    creationTimestamp: 2018-12-20T02:13:49Z
    generation: 1
    name: cluster
    namespace: ""
    resourceVersion: "225"
    selfLink: /apis/config.openshift.io/v1/authentications/cluster
    uid: e341ed0d-03fc-11e9-9f58-0256a9d73dc6
  spec:
    oauthMetadata:
      name: ""
    webhookTokenAuthenticators: null
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

and

$ oc get oauths -o yaml
apiVersion: v1
items:
- apiVersion: config.openshift.io/v1
  kind: OAuth
  metadata:
    creationTimestamp: 2018-12-20T02:13:49Z
    generation: 1
    name: cluster
    namespace: ""
    resourceVersion: "232"
    selfLink: /apis/config.openshift.io/v1/oauths/cluster
    uid: e3464dbd-03fc-11e9-9f58-0256a9d73dc6
  spec:
    identityProviders: null
    templates:
      error:
        name: ""
      login:
        name: ""
      providerSelection:
        name: ""
    tokenConfig:
      accessTokenMaxAgeSeconds: 86400
      authorizeTokenMaxAgeSeconds: 300
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

sallyom · 2018-12-20T02:46:59Z

@staebler addressed your feedback, thanks!
@abhinavdahiya un-gofmt-ed files unrelated to this PR

wking · 2018-12-20T03:33:37Z

Why does the installer need to be involved here? These look like static objects that could be injected following the standard approach.

deads2k · 2018-12-20T14:34:42Z

Why does the installer need to be involved here? These look like static objects that could be injected following the standard approach.

@wking we have a unusual situation that we need to kick loose. Our operators are based on binaries, but our configuration is explicitly built to group features not binaries. This makes it easy for admins to describe a unified configuration without worrying about a deployment topology, but it leaves us in a spot where there isn't an obvious owning operator to produce the CRDs themselves.

In addition, this feature based configuration is logically an object that cluster-admins will want access to when they render out from the installer. I suspect that long term we will produce a "fake" operator that's only job is to provide CRD manifests for rending. The one that occurs to me is cluster-bootstrap (bootkube replacement), but the parallel efforts have not met. That would give you a CRD resource, but not the CR, which I think would be cleaner to render from the installer itself because it would never be later produced or managed by any component we have.

sallyom · 2018-12-20T16:16:14Z

/test e2e-aws

wking · 2018-12-20T16:33:58Z

Our operators are based on binaries, but our configuration is explicitly built to group features not binaries.

Putting these under a parent operator makes sense to me.

The one that occurs to me is cluster-bootstrap (bootkube replacement), but the parallel efforts have not met.

When the CVO rolls out an update, will it push out updated objects from cluster-boptstrap's manifests?

That would give you a CRD resource, but not the CR, which I think would be cleaner to render from the installer itself because it would never be later produced or managed by any component we have.

Anything injected only by the installer will make 3.x -> 4 transition more complicated. Why can't the CRD-managine operator set the default, tell the CVO it's happy, and exit?

ericavonb · 2018-12-20T22:14:06Z

I agree with @wking. I don't think this approach makes sense. The purpose of the operator-driven architecture is specifically to decouple components into individual control loops focusing on specific concerns. This is important for automatic updates, faster development by teams, more resilient clusters, etc.

Our operators are based on binaries, but our configuration is explicitly built to group features not binaries.
This is the wrong framing. Our operators should be built around features. The operators are not the thing we want users to concentrate on configuring but rather we want user interacting with the custom resources managed by the operators that abstract away underlying services. Yes, a binary often needs to be run to achieve that functionality but that's not driving factor.

Operators do need to be installed, upgraded, scaled. To automate that process, I'd prefer creating an abstraction to represent the operators (ideally the same resource types for all operators, like ClusterOperators or ClusterServiceVersions). Then a meta-operator can react to changes to desired operator state and such.

sallyom · 2018-12-21T18:24:01Z

closing, in favor of this: openshift/cluster-authentication-operator#7

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 19, 2018

openshift-ci-robot requested review from crawford and staebler December 19, 2018 20:49

sallyom force-pushed the add-auth-crd branch from 0731c82 to 63d511d Compare December 19, 2018 21:04

staebler suggested changes Dec 19, 2018

View reviewed changes

enj suggested changes Dec 19, 2018

View reviewed changes

sallyom force-pushed the add-auth-crd branch 2 times, most recently from ae9f7b9 to e8f0993 Compare December 20, 2018 02:01

sallyom added 2 commits December 19, 2018 21:03

add authentication and oauth crd

9a2f9d0

dep: update version of openshift/api

23ff464

sallyom force-pushed the add-auth-crd branch from e8f0993 to 23ff464 Compare December 20, 2018 02:03

sallyom changed the title ~~[WIP] Add Authentication and OAuth crds~~ Add Authentication and OAuth crds Dec 20, 2018

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 Dec 20, 2018

sallyom closed this Dec 21, 2018

Add Authentication and OAuth crds #948

Add Authentication and OAuth crds #948

Uh oh!

Conversation

sallyom commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Dec 19, 2018

Uh oh!

abhinavdahiya commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staebler left a comment

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!

sallyom Dec 20, 2018 • 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!

sallyom commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staebler commented Dec 19, 2018

Uh oh!

abhinavdahiya commented Dec 19, 2018

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!

sallyom commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sallyom commented Dec 20, 2018

Uh oh!

sallyom commented Dec 20, 2018

Uh oh!

wking commented Dec 20, 2018

Uh oh!

deads2k commented Dec 20, 2018

Uh oh!

sallyom commented Dec 20, 2018

Uh oh!

wking commented Dec 20, 2018

Uh oh!

ericavonb commented Dec 20, 2018

Uh oh!

sallyom commented Dec 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sallyom commented Dec 19, 2018 •

edited

Loading

abhinavdahiya commented Dec 19, 2018 •

edited

Loading

sallyom Dec 20, 2018 •

edited

Loading

sallyom commented Dec 19, 2018 •

edited

Loading

sallyom commented Dec 20, 2018 •

edited

Loading