Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
gaktive opened this issue Dec 4, 2021 · 3 comments

Comments

@gaktive
Copy link
Member

gaktive commented Dec 4, 2021

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

  • Provision a Rancher v2.5.9 instance with single all role node downstream
  • Add catalog [https://github.com/axeal/rancher-catalog.git] branch case-00311126 in Cluster Manager and same as Chart Repository in Cluster Explorer
  • Cluster Manager -> Apps -> Launch -> json-exporter
  • Toggle rabbitmq Ingress enabled between True/False and observe rabbitmq Ingress TLS enabled, rabbitmq secretName and rabbitmq certManager enabled fields displayed or hidden 

Screen Shot 2022-01-27 at 1 56 32 PM

Screen Shot 2022-01-27 at 1 56 14 PM

  • Toggle Influxdb Application Version between v1/v2 and observe Init Admin Token, Init Bucket Name, Init Mode Name, Init Organization Name and Init Retention Day fields displayed when v2 selected and HTTP Auth Enabled ? when v1 selected 

Screen Shot 2022-01-27 at 1 57 37 PM

Screen Shot 2022-01-27 at 1 57 25 PM

Repro steps for the actual behavior

  • Cluster Explorer -> Apps & Marketplace -> Charts -> json-exporter
  • Under rabbitmq Ingress Settings Tab observe toggling rabbitmq Ingress enabled has no effect on fields displayed  !cluster-explorer-ingress.png!
  • Under InfluxDB Configuration Settings Tab observe toggling Influxdb Application Version has now effect on fields displayed !cluster-explorer-influxdb-version.png!

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
Copy link
Contributor

catherineluse commented Jan 28, 2022

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
Copy link
Contributor

catherineluse commented Jan 28, 2022

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
Copy link

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

@zube zube bot closed this as completed Mar 21, 2022
@zube zube bot removed the [zube]: Done label Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants