Add custom branding to config map #220

zherman0 · 2019-05-01T20:31:02Z

Add custom product name and logo to the config map and update unit/e2e tests.

For testing:
Creating config map with image (image needs to be less than 1MB)
oc create configmap myconfig --from-file=/mypic.jpg -n openshift-config
Creating operator config:

apiVersion: operator.openshift.io/v1
kind: Console
metadata:
  annotations:
    release.openshift.io/create-only: "true"
  name: cluster
spec:
  customization:
    brand: azure
    customProductName: my-custom-name
    customLogoFile:
      name:  myconfig
      key: mypic.jpg
  managementState: Managed

Update Operator Config:
oc apply -f custom.yaml

Verify Changes in Deployment:
oc get deployment console -n openshift-console -o yaml

If testing this locally before merge, you will need to make sure console code is the latest along with the console-operator code.
Additionally, you will need to manually run the oc apply -f manifests/03-rbac-role-ns-openshift-config.yaml and oc apply -f manifests/04-rbac-rolebinding.yaml

zherman0 · 2019-05-01T20:35:50Z

Everything expected to fail until api changes are in place
/hold

zherman0 · 2019-05-01T20:43:26Z

@spadgett @benjaminapetersen
/assign @benjaminapetersen

pkg/console/subresource/deployment/deployment.go

pkg/console/subresource/configmap/configmap.go

pkg/console/subresource/configmap/configmap_test.go

pkg/console/subresource/deployment/deployment.go

pkg/console/subresource/deployment/deployment_test.go

benjaminapetersen · 2019-05-23T15:23:03Z

Gonna add e2e for this feature?

pkg/console/subresource/configmap/configmap.go

benjaminapetersen · 2019-06-14T21:03:55Z

Users will set the CustomLogoFile on the operator config, if it doesn't work, they will prob oc get it. So having a .Ststus.Conditions[CustomLogoInvalid] with an error message is prob good UX. That's assuming a bad config could cause the console pods to crash loop due to a file read air.

zherman0 · 2019-06-14T22:49:45Z

Users will set the CustomLogoFile on the operator config, if it doesn't work, they will prob oc get it. So having a .Ststus.Conditions[CustomLogoInvalid] with an error message is prob good UX. That's assuming a bad config could cause the console pods to crash loop due to a file read air.

So here are the errors I have been able to generate:

User sets the operator config but does not create the configmap
Result: Console Pod stays in ContainerCreating and Event message on pod says FailedMount
There is no obvious error in the Console deployment status
User sets the operator config and makes a config map but the key file is not there or invalid
Result: Console Pod has CrashLoopBackoff, nothing in Events but the console pod log file shows:
cmd/main: could not read logo file: stat /var/logo/small-tractor.jpg: no such file or directory
There is no obvious error in the Console deployment status

zherman0 · 2019-06-15T01:34:56Z

@benjaminapetersen - I added logic in the operator.go function CheckCustomLogoImageStatus that will see if the file exists and if it does not I set a Status Condition. It does not work correctly but let me know if this is a reasonable way to inform user why the console pods might be crashing if there is a logo set.

enj · 2019-06-15T07:11:17Z

Some comments about your API:

...
spec:
  customization:
    ...
    customLogoFile:        # this really should be customLogo
      name:  myconfig      # validate using the logic stated below
      key: mypic.jpg       # there is no value is allowing a user to pick the key

must be no more than 253 characters

must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character
regex used for validation is:
[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*

enj · 2019-06-15T07:19:21Z

pkg/console/operator/operator.go

 		// recorder
-		recorder: recorder,
+		recorder:       recorder,
+		resourceSyncer: resourceSyncer,


I would expect a config informer name addition below.

I do not have another config informer. The informer is part of the resourceSyncer.

@enj - Can you clarify this further? We do have the config informer set just as it is in the cluster auth op.

This will probably clarify:
https://github.com/openshift/cluster-authentication-operator/blob/master/pkg/operator2/operator.go#L230

Further this line:

operator.WithInformer(configMapInformer, operator.FilterByNames(api.OpenShiftConsoleConfigMapName, api.ServiceCAConfigMapName)),

Shows that right now in theopenshift-console namespace, the informers only fire on changes to console-config & service-ca. If you sync in my-logo, my-logo2, etc, these will not trigger the informer. In addition, if my-logo is already sync'd into the openshift-console namespace, but the user changes the value of the logo within the configmap, this will also NOT be noticed.

It's possible you are seeing sync because you are triggering the loop in other ways. By the time a typical user is getting to the point of configuring a custom logo, the system will be very idle. There won't be other events to piggy-back on. If you happen to be testing with some bash scripts, I'd be cautious about what they are doing that might be triggering the loop in unexpected ways.

So, we are assuming a "non-happy" path where the user changes the configmap directly in the openshift-console namespace? We should only expect a change when the user changes the configmap in openshift-config but I agree we should cover this just in case.
As a side question, if the user can manipulate the openshift-console namespace, why did we bother putting the custom logo config map in the openshift-config in the first place?

No, we are not saying the user changes a configmap directly in openshift-console. I think you are misunderstanding the design here. You have 2 sync loops running with the syncer.

When the resource syncer finally decides "yes, this is a logo, copy it over"... our main sync needs to know that the new custom-logo configmap even exists & watches is so that it updates (and you cannot trust an accidental update because the sync loop ran for another reason).

Make sure you understand how the sync loops communicate. It is a very small overlap (precisely to ensure our main loop is not triggered too much).

pkg/console/operator/operator.go

enj · 2019-06-15T07:23:35Z

pkg/console/operator/operator.go

+	// Get Operator Config here
+	logoName := operatorConfig.Spec.Customization.CustomLogoFile.Name
+	// add syncing for the custom logo config map
+	if logoName != "" {


If this is empty, you need to set the source to ""/""

I am skipping the sync code completely if there is no logoName set so I don't know what good it would be to set the source to "/". Currently, the code is working as intended.

""/"" meant "namespace / name"

Still not following here, if the logoName is empty (the value has not been set on the operator config), then I would end up with "namespace / ", plus this code would not get executed.

Ensure it deletes? I was able to easily get about a dozen new configmaps in theopenshift-console namespace. That shouldn't happen.

enj · 2019-06-15T07:23:55Z

pkg/console/operator/operator.go

+	// add syncing for the custom logo config map
+	if logoName != "" {
+		err := c.resourceSyncer.SyncConfigMap(
+			resourcesynccontroller.ResourceLocation{Namespace: api.OpenShiftConsoleNamespace, Name: logoName},


The destination must be static.

I wish I could make the destination name static but that value is set by the user. The SyncConfigMap does not have an issue with me passing a dynamic name into the ResourceLocations.

Try oc apply -f on this operator config:

apiVersion: operator.openshift.io/v1 kind: Console metadata: name: cluster spec: customization: customProductName: Muh Clusta customLogoFile: name: console-config # uh oh! We gonna blow up.... key: fake-logo.png

oc apply -f

apiVersion: operator.openshift.io/v1 kind: Console metadata: name: cluster spec: customization: customProductName: Muh Clusta customLogoFile: name: service-ca # also bad things will happen. key: fake-logo.png

I am glad you changed your mind on having this dynamic. I will set the destination name to a set name and that will prevent existing configmaps from being clobbered.

I am glad you changed your mind on having this dynamic.

I dont know what this means?

We haven't changed our minds. The user SHOULD be able to do:

customLogoFile: name: console-config # or service-ca or whatever they want to call it, we don't care... key: fake-logo.png

And it SHOULD NOT destroy console-config or service-ca. This should be copied into a configmap named custom-logo in openshift-console with no risk of destroying any console config.

zherman0 · 2019-06-15T01:31:46Z

pkg/console/operator/operator.go

+		if err != nil {
+			klog.Errorf("customLogo configmap not valid, %v", err)
+			// Set Operator Status to CustomLogoInvalid
+			c.SetStatusCondition(operatorConfig, reasonCustomLogoInvalid, operatorsv1.ConditionFalse, reasonCustomLogoInvalid, "custom logo config map does not exist or has error")


While I see these in the conditions array programmatically, the oc get console.operator.openshift.io cluster -o yaml status.conditions does not show them

Aborted at your call point, before a sync of status.

pkg/console/operator/operator.go

benjaminapetersen · 2019-06-18T19:01:17Z

So I pulled this branch, built and deployed the operator. Then I did the following:

# create a configmap with a logo
oc create configmap my-logo-file --from-file ~/Desktop/fake-logo.png -n openshift-config
# update the operator to use it 
oc apply -f console.operator.with.my-logo-file.yaml

My operator config file looks like this:

apiVersion: operator.openshift.io/v1
kind: Console
metadata:
  name: cluster
spec:
  customization:
    customProductName: My Super Fancy Cluster
    customLogoFile:
      name: my-logo-file
      key: fake-logo.png

I don't actually see a customLogo or a customProductName in the console, it still shows Red Hat OpenShift Online.
The console operator pod is hot looping. It isn't looping fast, but it never settles into an idle state. The sync loop should settle out after a couple spins, every time.
The operator status remains in a Progressing state (oc get console.operator.openshift.io -o yaml):

    - lastTransitionTime: 2019-06-18T18:13:04Z
      message: Changes made during sync updates, additional sync expected.
      reason: SyncLoopProgressing
      status: "True"
      type: Progressing

Every time I change update the configmap, I get additional configs in the openshift-console namespace:

oc get configmap -n openshift-console
NAME            DATA   AGE
my-logo-file    0      11m
my-logo-file2   0      11m
my-logo-file3   0      11m
my-logo-file4   0      11s
my-logo-file5   0      7s
my-logo-file6   0      2s
service-ca      1      27h
# uh oh... console-config got clobbered & isn't coming back?

I should only have 1 configmap, just the most recent.

If I use an existing name console-config or service-ca, it seems I permanently eliminate them. See:

oc get configmap -n openshift-console
NAME            DATA   AGE
my-logo-file    0      11m
my-logo-file2   0      11m
my-logo-file3   0      11m
my-logo-file4   0      64s
my-logo-file5   0      60s
my-logo-file6   0      55s
# service-ca is gone
# console-config is gone

The operator is in a permanent progressing state. See oc get console.operator.openshift.io -o yaml or oc get clusteroperator console -o yaml
I can't undo this state, If I try to apply the following clean operator config:

apiVersion: operator.openshift.io/v1
kind: Console
metadata:
  name: cluster
spec:
  managementState: Managed

The operator remains in a hot loop (see oc logs -f console-operator-55bffddbdf-bkswr) and is stuck in SyncLoopProgressing (see oc get console.operator.openshift.io -o yaml)

Recommendations from the above:

sync configmap whatever-the-user-named-it in openshift-config to one name, custom-logo, in openshift-console. Ensures we cannot trample our own configs, and should not have dozens of configs pile up.
Be sure to update the informer watching openshift-console for configmaps. As above, if you stick to a single named configmap like custom-logo, then this should be pretty easy (operator.WithInformer(configMapInformer, operator.FilterByNames(api.OpenShiftConsoleConfigMapName, api.ServiceCAConfigMapName)), )
With the syncer, if the source is gone, the destination will be removed. Pass empty source to explictly delete the destination (destination being custom-logo in openshift-console).
Operator should settle to Available:True, Progressing: False, Degraded:False, unless the operator is in a bad state. (currently this PR can leave the operator in a permanent Progressing state)
Operator pods should settle & idle fairly quick after any change. The sync loop should run once for your update (a customization of the logo), again to sync any secondary changes (volume mounts for the deployment), maybe once or twice more as it settles, but it should settle quickly and full-stop.

benjaminapetersen · 2019-06-18T18:17:10Z

pkg/console/operator/operator.go

+	// add syncing for the custom logo config map
+	if logoName != "" {
+		err := c.resourceSyncer.SyncConfigMap(
+			resourcesynccontroller.ResourceLocation{Namespace: api.OpenShiftConsoleNamespace, Name: logoName},


oc apply -f

apiVersion: operator.openshift.io/v1 kind: Console metadata: name: cluster spec: customization: customProductName: Muh Clusta customLogoFile: name: service-ca # also bad things will happen. key: fake-logo.png

benjaminapetersen · 2019-06-18T19:35:06Z

/hold

Until bugs addressed.

openshift-ci-robot · 2019-06-19T17:24:08Z

@zherman0: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-operator	`a882f4e`	link	`/test e2e-aws-operator`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2019-06-19T19:19:23Z

@zherman0: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

benjaminapetersen · 2019-06-21T20:04:28Z

Closing this one in favor of #245
#245 includes this PR + some additional commits on top to help move us to completion since we have so much vacation time happening right now. Original authorship of commits will be retained!

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 1, 2019

openshift-ci-robot requested review from enj and spadgett May 1, 2019 20:31

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

openshift-ci-robot assigned benjaminapetersen May 1, 2019

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 May 1, 2019

zherman0 commented May 1, 2019

View reviewed changes

pkg/console/subresource/deployment/deployment.go Outdated Show resolved Hide resolved

zherman0 force-pushed the custom-brand branch from 095b4c2 to aae5b50 Compare May 8, 2019 20:14

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2019

zherman0 force-pushed the custom-brand branch from 8c07c82 to d5e8a63 Compare May 17, 2019 03:23

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2019

benjaminapetersen reviewed May 23, 2019

View reviewed changes

pkg/console/subresource/configmap/configmap.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed May 23, 2019

View reviewed changes

pkg/console/subresource/configmap/configmap.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed May 23, 2019

View reviewed changes

pkg/console/subresource/configmap/configmap.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed May 23, 2019

View reviewed changes

pkg/console/subresource/configmap/configmap_test.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed May 23, 2019

View reviewed changes

pkg/console/subresource/deployment/deployment.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed May 23, 2019

View reviewed changes

pkg/console/subresource/deployment/deployment_test.go Outdated Show resolved Hide resolved

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 25, 2019

zherman0 force-pushed the custom-brand branch from b41862c to 8b29f6a Compare May 31, 2019 14:35

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2019

zherman0 changed the title ~~[WIP] Add custom branding to config map~~ Add custom branding to config map May 31, 2019

benjaminapetersen reviewed Jun 14, 2019

View reviewed changes

pkg/console/subresource/configmap/configmap.go Outdated Show resolved Hide resolved

zherman0 force-pushed the custom-brand branch from aeeec20 to 25de69d Compare June 14, 2019 19:26

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 14, 2019

zherman0 force-pushed the custom-brand branch from 25de69d to 56f2483 Compare June 14, 2019 20:19

benjaminapetersen reviewed Jun 14, 2019

View reviewed changes

pkg/console/subresource/configmap/configmap.go Outdated Show resolved Hide resolved

zherman0 force-pushed the custom-brand branch from 0b46efd to c048e4e Compare June 14, 2019 21:43

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 15, 2019

enj suggested changes Jun 15, 2019

View reviewed changes

zherman0 commented Jun 17, 2019

View reviewed changes

benjaminapetersen reviewed Jun 17, 2019

View reviewed changes

pkg/console/operator/operator.go Show resolved Hide resolved

benjaminapetersen reviewed Jun 17, 2019

View reviewed changes

pkg/console/operator/operator.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed Jun 17, 2019

View reviewed changes

pkg/console/operator/operator.go Outdated Show resolved Hide resolved

benjaminapetersen suggested changes Jun 18, 2019

View reviewed changes

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

Add custom branding to config map

a882f4e

zherman0 force-pushed the custom-brand branch from 77ecdb3 to a882f4e Compare June 19, 2019 16:28

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019

benjaminapetersen mentioned this pull request Jun 21, 2019

Custom brand revisions #245

Merged

benjaminapetersen closed this Jun 21, 2019

Add custom branding to config map #220

Add custom branding to config map #220

Uh oh!

Conversation

zherman0 commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherman0 commented May 1, 2019

Uh oh!

zherman0 commented May 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjaminapetersen commented May 23, 2019

Uh oh!

Uh oh!

Uh oh!

benjaminapetersen commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherman0 commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherman0 commented Jun 15, 2019

Uh oh!

enj commented Jun 15, 2019

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!

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!

Uh oh!

enj Jun 15, 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!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen Jun 18, 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!

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!

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 Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

zherman0 commented May 1, 2019 •

edited

Loading

benjaminapetersen commented Jun 14, 2019 •

edited

Loading

zherman0 commented Jun 14, 2019 •

edited

Loading

enj Jun 15, 2019 •

edited

Loading

benjaminapetersen Jun 18, 2019 •

edited

Loading

benjaminapetersen Jun 19, 2019 •

edited

Loading

benjaminapetersen Jun 18, 2019 •

edited

Loading

benjaminapetersen commented Jun 18, 2019 •

edited

Loading