Merge pull request #12059 from rabbitmq/mergify/bp/v3.13.x/pr-12058

Make it hard to accidentally enable an experimental feature flag (backport #11998) (backport #12058)

Author: michaelklishin

deps/rabbitmq_cli/lib/rabbitmq/cli/ctl/commands/enable_feature_flag_command.ex

@@ -7,54 +7,68 @@
 defmodule RabbitMQ.CLI.Ctl.Commands.EnableFeatureFlagCommand do
   @behaviour RabbitMQ.CLI.CommandBehaviour
 
-  def merge_defaults(args, opts), do: {args, opts}
+  def switches(), do: [experimental: :boolean]
+  def aliases(), do: [e: :experimental]
 
-  def validate([], _), do: {:validation_failure, :not_enough_args}
-  def validate([_ | _] = args, _) when length(args) > 1, do: {:validation_failure, :too_many_args}
+  def merge_defaults(args, opts), do: { args, Map.merge(%{experimental: false}, opts) }
 
-  def validate([""], _),
+  def validate([], _opts), do: {:validation_failure, :not_enough_args}
+  def validate([_ | _] = args, _opts) when length(args) > 1, do: {:validation_failure, :too_many_args}
+
+  def validate([""], _opts),
     do: {:validation_failure, {:bad_argument, "feature_flag cannot be an empty string."}}
 
-  def validate([_], _), do: :ok
+  def validate([_], _opts), do: :ok
 
   use RabbitMQ.CLI.Core.RequiresRabbitAppRunning
 
-  def run(["all"], %{node: node_name}) do
-    case :rabbit_misc.rpc_call(node_name, :rabbit_feature_flags, :enable_all, []) do
-      # Server does not support feature flags, consider none are available.
-      # See rabbitmq/rabbitmq-cli#344 for context. MK.
-      {:badrpc, {:EXIT, {:undef, _}}} -> {:error, :unsupported}
-      {:badrpc, _} = err -> err
-      other -> other
+  def run(["all"], %{node: node_name, experimental: experimental}) do
+    case experimental do
+      true ->
+        {:error, RabbitMQ.CLI.Core.ExitCodes.exit_usage(), "`--experiemntal` flag is not allowed when enabling all feature flags.\nUse --experimental with a specific feature flag if you want to enable an experimental feature."}
+      false ->
+        case :rabbit_misc.rpc_call(node_name, :rabbit_feature_flags, :enable_all, []) do
+          {:badrpc, _} = err -> err
+          other -> other
+        end
     end
   end
 
-  def run([feature_flag], %{node: node_name}) do
-    case :rabbit_misc.rpc_call(node_name, :rabbit_feature_flags, :enable, [
-           String.to_atom(feature_flag)
-         ]) do
-      # Server does not support feature flags, consider none are available.
-      # See rabbitmq/rabbitmq-cli#344 for context. MK.
-      {:badrpc, {:EXIT, {:undef, _}}} -> {:error, :unsupported}
-      {:badrpc, _} = err -> err
-      other -> other
+  def run([feature_flag], %{node: node_name, experimental: experimental}) do
+    case {experimental, :rabbit_misc.rpc_call(node_name, :rabbit_feature_flags, :get_stability, [
+      String.to_atom(feature_flag)
+    ])} do
+          {_, {:badrpc, _} = err} -> err
+          {false, :experimental} ->
+              IO.puts("Feature flag #{feature_flag} is experimental. If you understand the risk, use --experimental to enable it.")
+          _ ->
+                case :rabbit_misc.rpc_call(node_name, :rabbit_feature_flags, :enable, [
+                  String.to_atom(feature_flag)
+                ]) do
+                  {:badrpc, _} = err -> err
+                  other -> other
+                end
     end
   end
 
   def output({:error, :unsupported}, %{node: node_name}) do
     {:error, RabbitMQ.CLI.Core.ExitCodes.exit_usage(),
-     "This feature flag is not supported by node #{node_name}"}
+      "This feature flag is not supported by node #{node_name}"}
   end
 
   use RabbitMQ.CLI.DefaultOutput
 
-  def usage, do: "enable_feature_flag <all | feature_flag>"
+  def usage, do: "enable_feature_flag [--experimental] <all | feature_flag>"
 
   def usage_additional() do
     [
       [
         "<feature_flag>",
         "name of the feature flag to enable, or \"all\" to enable all supported flags"
+      ],
+      [
+        "--experimental",
+        "required to enable experimental feature flags (make sure you understand the risks!)"
       ]
     ]
   end

deps/rabbitmq_cli/test/ctl/enable_feature_flag_test.exs

@@ -35,7 +35,7 @@ defmodule EnableFeatureFlagCommandTest do
 
     {
       :ok,
-      opts: %{node: get_rabbit_hostname()}, feature_flag: @feature_flag
+      opts: %{node: get_rabbit_hostname(), experimental: false}, feature_flag: @feature_flag
     }
   end
 
@@ -59,7 +59,7 @@ defmodule EnableFeatureFlagCommandTest do
   end
 
   test "run: attempt to use an unreachable node returns a nodedown" do
-    opts = %{node: :jake@thedog, timeout: 200}
+    opts = %{node: :jake@thedog, timeout: 200, experimental: false}
     assert match?({:badrpc, _}, @command.run(["na"], opts))
   end
 

deps/rabbitmq_management/priv/www/js/tmpl/feature-flags.ejs

@@ -13,7 +13,7 @@
      </p>
   <% } %>
 <div class="section">
-  <h2>All Feature Flags</h2>
+  <h2>Feature Flags</h2>
   <div class="hider">
 <%= filter_ui(feature_flags) %>
   <div class="updatable">
@@ -30,6 +30,9 @@
     <%
        for (var i = 0; i < feature_flags.length; i++) {
          var feature_flag = feature_flags[i];
+	 if (feature_flag.stability == "experimental") {
+	   continue;
+	 }
          var state_color = "grey";
          if (feature_flag.state == "enabled") {
            state_color = "green";
@@ -76,3 +79,74 @@
   </div>
   </div>
 </div>
+
+
+
+<div class="section">
+  <h2>Experimental Feature Flags</h2>
+  <div class="hider">
+<% if (feature_flags.length > 0) { %>
+<p class="warning">
+  Feature flags listed below are experimental. They should not be enabled in a production deployment.
+</p>
+<table class="list">
+  <thead>
+    <tr>
+      <th><%= fmt_sort('Name', 'name') %></th>
+      <th class="c"><%= fmt_sort('State', 'state') %></th>
+      <th>Description</th>
+    </tr>
+  </thead>
+  <tbody>
+    <%
+       for (var i = 0; i < feature_flags.length; i++) {
+         var feature_flag = feature_flags[i];
+	 if (feature_flag.stability != "experimental") {
+	   continue;
+	 }
+         var state_color = "grey";
+         if (feature_flag.state == "enabled") {
+           state_color = "green";
+         } else if (feature_flag.state == "disabled") {
+           state_color = "yellow";
+         } else if (feature_flag.state == "unsupported") {
+           state_color = "red";
+         }
+    %>
+       <tr<%= alt_rows(i)%>>
+         <td><%= fmt_string(feature_flag.name) %></td>
+         <td class="c">
+           <% if (feature_flag.state == "disabled") { %>
+	      <div>
+              <input id="<%= feature_flag.name %>" type="checkbox" class="riskCheckbox" onclick="this.parentNode.querySelector('.enable-feature-flag input[type=submit]').disabled = !this.checked;">
+              <label for="<%= feature_flag.name %>"> I understand the risk</label><br>
+	      <br>
+              <form action="#/feature-flags-enable" method="put" style="display: inline-block" class="enable-feature-flag">
+              <input type="hidden" name="name" value="<%= fmt_string(feature_flag.name) %>"/>
+              <input type="submit" value="Enable" class="c" disabled="disabled"/>
+	      </div>
+           </form>
+           <% } else { %>
+           <abbr class="status-<%= fmt_string(state_color) %>"
+             style="text-transform: capitalize"
+             title="Feature flag state: <%= fmt_string(feature_flag.state) %>">
+           <%= fmt_string(feature_flag.state) %>
+           </abbr>
+           <% } %>
+         </td>
+         <td>
+         <p><%= fmt_string(feature_flag.desc) %></p>
+         <% if (feature_flag.doc_url) { %>
+         <p><a href="<%= fmt_string(feature_flag.doc_url) %>">[Learn more]</a></p>
+         <% } %>
+         </td>
+       </tr>
+    <% } %>
+  </tbody>
+</table>
+<% } else { %>
+    <p>... no feature_flags ...</p>
+<% } %>
+  </div>
+  </div>
+</div>