Honor configs & report public hostname on status #134

benjaminapetersen · 2019-02-12T04:39:22Z

handles values set on console config
handle values set on operator config
report public hostname on status
- oauth team dependent on this update

benjaminapetersen · 2019-02-12T04:40:20Z

/hold

This is a pretty quick stub in so far, needs some work to guard/check/validate/etc.

@jhadvig @zherman0 fyi, we will need this in this week.

benjaminapetersen · 2019-02-12T15:48:28Z

pkg/console/subresource/configmap/configmap.go

+	defaultDocumentationBaseURL = "https://docs.okd.io/4.0/"
+	defaultBranding             = "okd"
+)
+


These 3 functions may belong in util.go or somewhere else...

benjaminapetersen · 2019-02-12T15:48:49Z

pkg/console/subresource/configmap/configmap.go

+
+//operatorConfig.Spec.Customization.Brand
+func getBrand(operatorConfig *operatorv1.Console) operatorv1.Brand {
+	// TODO: handle if not provided


@zherman0 this is related to your brand work PR.

#126
Just want to ensure they function together.

So, if brand is not EXPLICITLY set, it should just fallback to the default which is provided in #126 (baked into the image). Either okd or ocp.

jhadvig · 2019-02-12T17:09:34Z

/test unit

jhadvig

Just a few quick comments. Also need to check SyncConsoleConfig() cause I have a feeling that that might mess things up.

jhadvig · 2019-02-12T18:27:15Z

pkg/console/subresource/configmap/configmap.go

-func DefaultConfigMap(cr *operatorv1.Console, rt *routev1.Route) *corev1.ConfigMap {
+// overridden by console config
+const (
+	defaultLogoutURL = ""


dont we have a default URL for logout ?

Nope, there should not be a default (empty string). See console auth/logout handling:

https://github.com/openshift/console/blob/e267fcf4e177da5858c5c63d021d4f19f71646b8/frontend/public/module/auth.js#L56

jhadvig · 2019-02-12T18:27:43Z

pkg/console/subresource/configmap/configmap.go

+//consoleConfig.Spec.Authentication.LogoutRedirect
+func getLogoutRedirect(consoleConfig *configv1.Console) string {
+	// TODO: handle if not provided
+	// return consoleConfig.Spec.Authentication.LogoutRedirect || defaultLogoutURL


Lets uncomment this and comment the below ? or am I missing something ?

Nope, you aren't missing anything. This is why its still WIP. I didn't have time to test everything before opening last night, but wanted to raise awareness early.

jhadvig · 2019-02-12T20:38:44Z

@benjaminapeterse we probrably need to update the tests

[INFO] Testing github.com/openshift/console-operator/pkg/console/subresource/configmap
# github.com/openshift/console-operator/pkg/console/subresource/configmap
pkg/console/subresource/configmap/configmap_test.go:100:30: not enough arguments in call to DefaultConfigMap
	have (*"github.com/openshift/console-operator/vendor/github.com/openshift/api/operator/v1".Console, *"github.com/openshift/console-operator/vendor/github.com/openshift/api/route/v1".Route)
	want (*"github.com/openshift/console-operator/vendor/github.com/openshift/api/operator/v1".Console, *"github.com/openshift/console-operator/vendor/github.com/openshift/api/config/v1".Console, *"github.com/openshift/console-operator/vendor/github.com/openshift/api/route/v1".Route)
pkg/console/subresource/configmap/configmap_test.go:170:14: undefined: NewYamlConfigString
FAIL	github.com/openshift/console-operator/pkg/console/subresource/configmap [build failed]

benjaminapetersen · 2019-02-13T05:07:57Z

pkg/console/subresource/configmap/configmap.go

+)
+
+func getLogoutRedirect(consoleConfig *configv1.Console) string {
+	if len(consoleConfig.Spec.Authentication.LogoutRedirect) > 0 {


I believe this is all we need to do. The config structs are not pointers, so when the YAML is instantiated, nested structs should be created with empty string values. Therefore if empty string is provided, we can return the default value instead.

@jhadvig @zherman0 work? Same treatment for all three of these.

benjaminapetersen · 2019-02-13T05:08:54Z

pkg/console/subresource/configmap/configmap.go

+
 	host := rt.Spec.Host
-	config := NewYamlConfigString(host)
+	config := string(NewYamlConfig(host, logoutRedirect, brand, docURL))


I eliminated the one wrapper func NewYamlConfigString(), was unnecessary additional plumbing for passing these values down, when all it did was call string().

benjaminapetersen · 2019-02-13T05:10:32Z

Updated the tests & filled out the defaults.

jhadvig · 2019-02-13T15:25:54Z

/test e2e-aws

benjaminapetersen · 2019-02-13T18:55:06Z

/retest

one success. interesting.

…ith public hostname

benjaminapetersen · 2019-02-13T21:28:54Z

So at present you should be able to update the console.config.openshift.io:

apiVersion: config.openshift.io/v1
kind: Console
spec:
  authentication:
    logoutRedirect: https://www.redhat.com/i/should/logout/i/think

and it will update the web console.

In addition, when the Route.Host is provided, this will be reported on the console.config.openshift.io:

oc get console.config.openshift.io console -o yaml
apiVersion: config.openshift.io/v1
kind: Console
metadata:
  name: console
  resourceVersion: "1542289"
  selfLink: /apis/config.openshift.io/v1/consoles/console
  uid: 9a327206-2fce-11e9-92db-021245e32db2
spec:
  authentication:
    logoutRedirect: https://www.redhat.com/i/should/logout/i/think
status:
  publicHostname: https://console-openshift-console.apps.betersen.devcluster.openshift.com

Also, the console.operator.openshift.io can be configured with:

oc get console.operator.openshift.io console -o yaml
apiVersion: operator.openshift.io/v1
kind: Console
metadata:
  name: console
  resourceVersion: "1483224"  
spec:
  customization:
    brand: online
    documentationBaseURL: https://www.redhat.com/someplace/for/docs/give/it/a/try/
  logLevel: ""
  managementState: Managed
  observedConfig: null
  operandSpecs: null
  unsupportedConfigOverrides: null

The customizations on branding & documentationBaseURL will be set on the console configmap, and will be updated & visible in the console.

benjaminapetersen · 2019-02-13T21:34:06Z

/hold cancel

benjaminapetersen · 2019-02-13T22:05:31Z

/retest

flake

jhadvig · 2019-02-13T22:56:57Z

Last failure:

time="2019-02-13T21:57:19Z" level=info msg="finished syncing operator \"console\" (50.148µs) \n\n"
E0213 21:57:19.063215       1 controller.go:130] {🐼 🐼} failed with: Operation cannot be fulfilled on consoles.config.openshift.io "console": the object has been modified; please apply your changes to the latest version and try again
I0213 21:57:19.063575       1 option.go:62] Console: handling update openshift-console/console: /apis/apps/v1/namespaces/openshift-console/deployments/console

jhadvig · 2019-02-13T22:57:16Z

/test e2e-aws

benjaminapetersen · 2019-02-14T00:06:40Z

That fail is interesting. Will check in a short bit.

jhadvig · 2019-02-14T00:19:27Z

flake
/test e2e-aws

benjaminapetersen · 2019-02-14T01:32:23Z

all succeed. @jhadvig or @zherman0 just need a lgtm 👍

jhadvig

/lgtm

openshift-ci-robot · 2019-02-14T01:51:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, jhadvig

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

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

Console publishes its config under the name 'console' now. Ref: openshift/console-operator#134

benjaminapetersen changed the title ~~Honor configs & report public hostname on status~~ [WIP] Honor configs & report public hostname on status Feb 12, 2019

openshift-ci-robot requested review from enj and spadgett February 12, 2019 04:40

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2019

benjaminapetersen commented Feb 12, 2019

View reviewed changes

jhadvig reviewed Feb 12, 2019

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2019

benjaminapetersen commented Feb 13, 2019

View reviewed changes

benjaminapetersen force-pushed the honor-configs branch from 8201e13 to f347cab Compare February 13, 2019 05:10

benjaminapetersen added 4 commits February 13, 2019 13:55

Honor console config, operator config, update console config status w…

5c2a74a

…ith public hostname

Handle default values for config getter funcs

8c6e269

Update unit tests

1721f4b

rbac update

8e62a40

benjaminapetersen force-pushed the honor-configs branch from f347cab to 8e62a40 Compare February 13, 2019 21:25

benjaminapetersen changed the title ~~[WIP] Honor configs & report public hostname on status~~ Honor configs & report public hostname on status Feb 13, 2019

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 Feb 13, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2019

openshift-ci-robot assigned jhadvig Feb 14, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019

jhadvig approved these changes Feb 14, 2019

View reviewed changes

openshift-merge-robot merged commit 5c1ecb8 into openshift:master Feb 14, 2019

ericavonb pushed a commit to ericavonb/cluster-authentication-operator that referenced this pull request Feb 15, 2019

fix(operator2/console): update console config resource name

6df69c9

Console publishes its config under the name 'console' now. Ref: openshift/console-operator#134

ericavonb mentioned this pull request Feb 15, 2019

fix(operator2/console): update console config resource name openshift/cluster-authentication-operator#73

Closed

Honor configs & report public hostname on status #134

Honor configs & report public hostname on status #134

Uh oh!

Conversation

benjaminapetersen commented Feb 12, 2019

Uh oh!

benjaminapetersen commented Feb 12, 2019 • 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!

benjaminapetersen Feb 12, 2019 • 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!

jhadvig commented Feb 12, 2019

Uh oh!

jhadvig 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!

jhadvig commented Feb 12, 2019 • 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!

benjaminapetersen commented Feb 13, 2019

Uh oh!

jhadvig commented Feb 13, 2019

Uh oh!

benjaminapetersen commented Feb 13, 2019

Uh oh!

benjaminapetersen commented Feb 13, 2019

Uh oh!

benjaminapetersen commented Feb 13, 2019

Uh oh!

benjaminapetersen commented Feb 13, 2019

Uh oh!

jhadvig commented Feb 13, 2019

Uh oh!

jhadvig commented Feb 13, 2019

Uh oh!

benjaminapetersen commented Feb 14, 2019

Uh oh!

jhadvig commented Feb 14, 2019

Uh oh!

benjaminapetersen commented Feb 14, 2019

Uh oh!

jhadvig left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Feb 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benjaminapetersen commented Feb 12, 2019 •

edited

Loading

benjaminapetersen Feb 12, 2019 •

edited

Loading

jhadvig commented Feb 12, 2019 •

edited

Loading