search

rancher / dashboard

The Rancher UI
https://rancher.com
Apache License 2.0
462 stars 261 forks source link

Differences in behaviour of conditional logic in questions.yaml between Cluster Manager Apps and Cluster Explorer Apps & Marketplace #4706

Closed gaktive closed 2 years ago

gaktive commented 2 years ago

Issue description: There is a difference in the behaviour of show_subquestion_if and  show_if in a chart's question.yaml, between the older Apps functionality in the Cluster Manager and the Apps & Marketplace in the Cluster Explorer

Business impact: Unable to directly port chart catalog from old Apps feature to new Apps & Marketplace

Root cause

The root cause was that the Ember UI and the Dashboard UI had different ways of rendering the forms that were configured in a helm chart app's questions.yaml. Ember used custom code for checking the conditional logic in the questions.yaml, whereas the Dashboard UI used a library called Jexl for this purpose. It turns out Jexl has slightly different behavior for evaluating whether to show and hide form inputs based on other form inputs. For example, in Ember, turning something on might cause more config options to be displayed. In the dashboard those extra options just weren't showing up.

What was fixed, or what changes have occurred

I made a PR that makes it so that the dashboard reverts back to using the custom code from the Ember UI for an edge case where the conditional logic in questions.yaml has a hyphen in it. (Jexl doesn't support hyphens because it converts the condition string into javascript, and in javascript the hyphen means a minus sign so it couldn't process it as part of a variable name as Ember did.) It only reverts to the custom ember logic when there's a hyphen to avoid breaking the logic in other places.

Vince said that we shouldn't completely remove Jexl because Jexl is probably better/safer overall and we already told users that we are using this library.

Areas or cases that should be tested

I would say only this very specific customer's use case should be tested because this PR is really about supporting existing customers and making it so they don't have to do annoying manual work to migrate legacy apps. If we didn't make this change, they would have to manually remove the hyphens from their variable names in questions.yaml. I would expect that in most cases, variable names wouldn't have hyphens so this issue wouldn't come up.

What areas could experience regressions?

The PR affects how custom forms are rendered from Questions.yaml, so if it caused a regression it would be there.

Are the repro steps accurate/minimal?

The repro steps show how to see the expected behavior in the Ember UI. The desired behavior is to replicate the same behavior in the Dashboard UI.

Repro steps for the expected behavior

Repro steps for the actual behavior

Actual behavior: Behaviour differs between Cluster Manager Apps and Cluster Explorer Apps & Marketplace

Expected behavior: Behaviour the same

Additional notes: Support did narrow down the situation to a failure to process the conditional logic as expected when the variable contains a hyphen.

Create a chart with the questions.yaml below and observe the show_subquestion_if: true is processed as expected in both Apps v1 in the Cluster Manager and Apps v2 in the Cluster Explorer. Update the variable name to contain a hyphen (e.g. se-rabbitmq.rabbitmq.ingress.enabled or rabbitmq.se-rabbitmq.ingress.enabled) and observe the subquestions are not displayed in the Cluster Explorer.

Global For All charts

questions:
  - variable: rabbitmq.rabbitmq.ingress.enabled
    default: true
    description: "Use default Datadog image or specify a custom one"
    label: Use Default Datadog Image
    type: boolean
    show_subquestion_if: true
    group: "rabbitmq Ingress Settings"
    subquestions:
    - variable: agents.image.repository
      default: "datadog/agent"
      description: "Datadog image name"
      type: string
      label: Datadog Image Name
    - variable: agents.image.tag
      default: "7.21.1"
      description: "Datadog Image Tag"
      type: string
      label: Datadog Image Tag

Internal reference: SURE-3300

catherineluse commented 2 years ago

It seems that in the Questions component, this Jexl expression evaluator always returns false for values with dashes.

function evalExpr(expr, values) {
  try {
    // For values with hyphens, this always evaluates false
    const out = Jexl.evalSync(expr, values);

    return out;
  } catch (err) {
    console.error('Error evaluating expression:', expr, values); // eslint-disable-line no-console

    return true;
  }
}

According to Jexl documentation it doesn't support variables with hyphens https://people.apache.org/~henrib/jexl-3.0/reference/syntax.html

catherineluse commented 2 years ago

The Ember UI didn't use Jexl as it had basically used its own custom-made in-house system for evaluating Javascript expressions. The logic for those evaluations is found here: https://github.com/rancher/ui/blob/master/lib/shared/addon/utils/evaluate.js

Since there have been no other problems with Jexl so far, I'm re-adding Ember's custom evaluation logic, but only using it for evaluating expressions that contain hyphens. Everything else should use Jexl, so these changes will only apply to this narrowly defined case where hyphens are used.

ronhorton commented 2 years ago

Pass Verified in v2.6.4-rc10

  1. added repo to downstream cluster
  2. confirmed repo
  3. toggled rabbitmq Ingress enabled
  4. toggled Influxdb Application Version

toggling changes the values displayed in the UI