Bug 1847921 - Form view for creating Operands does not allow creating empty JSON objects
Summary: Form view for creating Operands does not allow creating empty JSON objects
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: Management Console
Version: 4.5
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: ---
: 4.6.0
Assignee: Jon Jackson
QA Contact: Yadan Pei
URL:
Whiteboard:
: 1854152 (view as bug list)
Depends On:
Blocks: 1856867
TreeView+ depends on / blocked
 
Reported: 2020-06-17 10:56 UTC by Simon Woodman
Modified: 2020-10-27 16:07 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Cause: When creating an operand, the form data was pruned when submitting or switching between form and yaml view. Consequence: If the template data included any empty objects, they would be pruned when submitting or switching. Fix: Use the template data as a "mask" when pruning empty structures from the form data. Only prune values that are not defined in the template. Result: Empty values that were defined in a template do not get pruned.
Clone Of:
: 1856867 (view as bug list)
Environment:
Last Closed: 2020-10-27 16:07:36 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)
YAML with empty data structures (9.38 KB, text/plain)
2020-06-23 19:20 UTC, Jon Jackson
no flags Details
Verification screenshot - form view (143.55 KB, image/png)
2020-07-14 06:50 UTC, XiaochuanWang
no flags Details
Verification screenshot - yaml view (178.93 KB, image/png)
2020-07-14 06:52 UTC, XiaochuanWang
no flags Details
YAML view in which the `spec.listeners.plain:{}` is still missing (215.10 KB, image/png)
2020-07-14 08:40 UTC, Simon Woodman
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Github openshift console pull 5966 0 None closed Bug 1847921: Do not prune empty values from sample data on create operand … 2021-01-13 13:29:43 UTC
Red Hat Product Errata RHBA-2020:4196 0 None None None 2020-10-27 16:07:56 UTC

Description Simon Woodman 2020-06-17 10:56:56 UTC
Description of problem:

It is not possible to create an empty JSON object to be used as part of the spec of an Operand. For example, the Kafka CR in AMQ Streams includes `spec.kafka.listeners.plain: {}`. It is not possible to generate this from the Form View when creating an Operand. The same applies to other fields in the CR such as `spec.entityOperator.topicOperator: {}`

Also, the default YAML view does not display the correct alm-example from the CSV which contain these empty objects.

Version-Release number of selected component (if applicable):

OpenShift 4.5-rc1 and AMQ Streams 1.4.1

How reproducible:


Steps to Reproduce:
1. Install AMQ Streams Operator
2. From the OpenShift console try to create an instance of Kafka resource
3. The YAML view will be missing the `spec.kafka.listeners.plain:{}` object
4. The form view is unable to create this object

Actual results:

Operand installation fails because required fields aren't set. 

Expected results:

Operand installation succeeds.

The Form View for creating an Operand can generate empty objects. The YAML view will include these empty objects when they exist in the alm-examples section of the CSV

Additional info:

Comment 1 Jon Jackson 2020-06-18 19:26:15 UTC
Didn't get a chance to look into this yet. It's next on my queue.

Comment 2 Jon Jackson 2020-06-23 19:20:06 UTC
Created attachment 1698491 [details]
YAML with empty data structures

Comment 3 Jon Jackson 2020-06-23 19:21:38 UTC
IMO, this is not a console bug. It was an intentional decision to prune empty data structures when toggling between editors and before submitting. I've attached a yaml file that shows what happens if we don't. (spoiler, you end up with a 400+ line YAML file that only defines 10 properties). I think the root of this bug is actually in the CRD validation schema.

Why it's a schema validation issue:
From what I gather, resource creation is failing validation because spec.kafka.listeners is defined as a required property in the schema. This means that even if the CR doesn't define any kafka listeners, an empty object has to be defined in order to pass validation. If the CRD schema is designed to allow spec.kafka.listeners to be empty, it probably shouldn't require that field. If, on the other hand, spec.kafka.listeners SHOULD be a required property, then at least one of its properties should also be required. If that required property is object, it should have a required property, and repeat recursively until you reach a primitive-type property.

I can't think of a case where an empty object property would need to be required, but that's not to say that it couldn't exist. If this is such a case, then there will need to be some further discussion about how we allow users to define empty data structures from the form view, and how we selectively sync empty data structures between the form and yaml views.

Why the schema issue wasn't caught before:
This behavior is only now manifesting in 4.5 because prior to this release, we landed on the YAML view and did not support nested fields in the form view. So these empty values were populated in the YAML view, but could not be edited in the form view, and therefore remained untouched when switching from YAML to form and back. In 4.5, we land on the form view and have added support for nested fields. So spec.kafka.listeners and all of its nested properties are now editable in the form view. With nested fields, we have to assume that when a user doesn't change anything within an empty or undefined nested structure, it will remain undefined. If instead we automatically assign an empty data structure to that field, then the YAML editor would become filled with empty data structures, which clutters up the YAML view for CRs with large schemas. For example, this is what spec.kafka.listeners looks like in the YAML view if we leave empty object definitions instead of pruning them:

listeners:
  external:
    authentication:
      clientSecret: {}
    configuration:
      bootstrap:
        dnsAnnotations: {}
      brokerCertChainAndKey: {}
    overrides:
      bootstrap:
        dnsAnnotations: {}
  plain:
    authentication:
      clientSecret: {}
  tls:
    authentication:
      clientSecret: {}
    configuration:
      brokerCertChainAndKey: {}

Comment 4 Jakub Scholz 2020-06-24 08:26:54 UTC
I think you are missing the point. There is a big difference between a defined but empty property and a property which is not defined in the CR YAML / JSON. So having empty listeners is not the same as having listeners with some properties defined but empty. You are right that the CRD definition could maybe use the anyOf keyword for the objects inside listener to indicate that at least one of the properties inside is required. But that would still leave them valid as empty objects - so I guess that would not solve the UI issue. Or would it?

I also do not think we are asking you to generate empty objects for every field in the schema. I think it would be good if the user can specify empty objects from the form (e.g. using some checkbox or something) but mainly the UI should respect the examples which are valid and which worked completely fine with the previous version of OCP.

Comment 5 Jon Jackson 2020-06-24 14:35:14 UTC
Jakub, yeah, I totally understand that empty is not the same as undefined. I guess I was wondering if in this specific case those two things actually convey different information. Regardless, I see your point about respecting the example if it is valid. Regardless of whether there is a logical flaw in the schema, if the example is valid, we should respect it. Let me look a little more into this.

Comment 6 Jon Jackson 2020-07-07 14:09:46 UTC
Fix is in progress.

Comment 9 XiaochuanWang 2020-07-14 06:50:40 UTC
Created attachment 1700969 [details]
Verification screenshot - form view

Comment 10 XiaochuanWang 2020-07-14 06:52:28 UTC
Created attachment 1700970 [details]
Verification screenshot - yaml view

Comment 11 XiaochuanWang 2020-07-14 07:30:33 UTC
Attached the screenshots, both yaml view and form view have the fields for `spec.kafka.listeners.plain:{}` object.
This could be Verified on 4.6.0-0.nightly-2020-07-14-004201

Comment 12 Simon Woodman 2020-07-14 08:38:53 UTC
This is not fixed. When I changed the `alm-example` section of the CSV to use empty objects for the `listeners.plain` and `listeners.tls` these empty objects do not appear in the YAML view for creating the Kafka Operand. Here is the section of the CSV

--
   alm-examples: |-
      [
        {
          "apiVersion":"kafka.strimzi.io/v1beta1",
          "kind":"Kafka",
          "metadata":{
            "name":"my-cluster"
          },
          "spec":{
            "kafka":{
                "version":"2.5.0",
                "replicas":3,
                "listeners":{
                  "plain":{
                  },
                  "tls":{
                  }
                },
--

Tested on 4.6.0-0.nightly-2020-07-14-004201

Comment 13 Simon Woodman 2020-07-14 08:40:15 UTC
Created attachment 1700986 [details]
YAML view in which the `spec.listeners.plain:{}` is still missing

Comment 14 Samuel Padgett 2020-07-14 13:47:16 UTC
Simon, that nightly doesn't have the fix. You need at least 4.6.0-0.nightly-2020-07-14-035247

Comment 15 Simon Woodman 2020-07-15 09:31:34 UTC
Thanks, I misinterpreted the fix version - tested on 4.6.0-0.nightly-2020-07-15-031221 and it looks good.

Comment 16 Bruno Andrade 2020-07-20 15:21:37 UTC
*** Bug 1854152 has been marked as a duplicate of this bug. ***

Comment 18 errata-xmlrpc 2020-10-27 16:07:36 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (OpenShift Container Platform 4.6 GA Images), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2020:4196


Note You need to log in before you can comment on or make changes to this bug.