CSE-364: Extend cb logdest with TLS and connection-log flags and add update (#193)

Add optional --tls-verify-disabled and --forward-connection-logs to cb logdest add,
and introduce cb logdest update to change an existing destination. Update merges
omitted fields and unset booleans from the current cluster listing so PUT payloads
stay complete.

Extend LogDestination model and list output (TTY labels and extra pipe columns) for
the new fields. Wire Client#update_log_destination and teach completion to suggest
the new flags and true or false after them.

Add specs for add (including invalid boolean strings), update (merge, overrides,
and not-found), list, completion, captured add payloads without mock_client, and
model JSON parsing with and without the new keys. Adjust the spec factory and trim
spec_helper accordingly.

Note by Erik: This was written under my direction by the robot in Cursor with the
model set to Auto. While it wrote some tests as we went, once the new functionality
was working I asked it to do another pass for spec/test coverage on the new
functionality and it found some more issues that it fixed.

* crystal tool format fixes

* Changes for ameba's Lint/NotNil rule

* Limit codegen threads to 1 for crystal spec runs on CI

* Changes from Adam's review.

Author: sfc-gh-erjones

flake.nix

@@ -82,7 +82,9 @@
               name = "specs";
               src = specSrc;
               buildInputs = [ pkgs.libssh2 ];
-              installPhase = "mkdir $out && HOME=$TMP crystal spec --progress";
+              # Cap compiler threads: very high CPU counts on CI can trigger
+              # "BUG: a codegen process failed" during crystal spec (codegen OOM / LLVM flake).
+              installPhase = "mkdir $out && HOME=$TMP crystal spec --progress --threads 1";
               shardsFile = specSrc + "/shards.nix";
               doCheck = false;
               dontPatch = true;

spec/cb/completion_spec.cr

@@ -401,10 +401,14 @@ Spectator.describe CB::Completion do
     expect(result).to have_option "list"
     expect(result).to have_option "destroy"
     expect(result).to have_option "add"
+    expect(result).to have_option "update"
 
     result = parse("cb logdest l")
     expect(result).to have_option "list"
 
+    result = parse("cb logdest u")
+    expect(result).to have_option "update"
+
     result = parse("cb logdest list ")
     expect(result).to have_option "--cluster"
 
@@ -451,6 +455,8 @@ Spectator.describe CB::Completion do
     expect(result).to have_option "--desc"
     expect(result).to have_option "--template"
     expect(result).to have_option "--host"
+    expect(result).to have_option "--tls-verify-disabled"
+    expect(result).to have_option "--forward-connection-logs"
 
     result = parse("cb logdest add --port 3 ")
     expect(result).to_not have_option "--port"
@@ -467,6 +473,39 @@ Spectator.describe CB::Completion do
     result = parse("cb logdest add --host something.com ")
     expect(result).to_not have_option "--host"
     expect(result).to have_option "--cluster"
+
+    result = parse("cb logdest update ")
+    expect(result).to have_option "--cluster"
+    expect(result).to have_option "--logdest"
+    expect(result).to have_option "--port"
+    expect(result).to have_option "--desc"
+    expect(result).to have_option "--template"
+    expect(result).to have_option "--host"
+
+    result = parse("cb logdest update --cluster ")
+    expect(result).to eq expected_cluster_suggestion
+
+    result = parse("cb logdest update --cluster abc ")
+    expect(result).to_not have_option "--cluster"
+    expect(result).to have_option "--logdest"
+
+    result = parse("cb logdest update --cluster abc --logdest ")
+    expect(result).to eq ["logid\tlogdest descr"]
+
+    result = parse("cb logdest update --cluster abc --logdest logid ")
+    expect(result).to have_option "--port"
+    expect(result).to have_option "--host"
+    expect(result).to have_option "--tls-verify-disabled"
+    expect(result).to have_option "--forward-connection-logs"
+
+    result = parse("cb logdest add --cluster abc --tls-verify-disabled ")
+    expect(result).to eq ["false", "true"]
+
+    result = parse("cb logdest update --cluster abc --logdest logid --tls-verify-disabled ")
+    expect(result).to eq ["false", "true"]
+
+    result = parse("cb logdest update --cluster abc --logdest logid --forward-connection-logs ")
+    expect(result).to eq ["false", "true"]
   end
 
   it "completes psql" do

spec/cb/logdest_add_payload_spec.cr

@@ -0,0 +1,58 @@
+require "../spec_helper"
+
+# This file intentionally does not use `mock_client`: Spectator's Client mock
+# is injected into CB::Client and prevents subclasses from overriding API methods.
+
+private class LogDestinationAddCaptureClient < CB::Client
+  getter last_cluster_id : String?
+  getter last_payload_json : String = ""
+
+  def initialize(token : String = TEST_TOKEN)
+    super(host: CB::HOST, bearer_token: token)
+  end
+
+  def add_log_destination(cluster_id, ld)
+    @last_cluster_id = cluster_id.to_s
+    @last_payload_json = ld.to_json
+    "{}"
+  end
+end
+
+Spectator.describe "CB::LogDestinationAdd API payload" do
+  let(cluster) { Factory.cluster }
+
+  it "sends tls and forward-connection flags in the create payload" do
+    cap = LogDestinationAddCaptureClient.new
+    act = CB::LogDestinationAdd.new(client: cap, output: IO::Memory.new)
+    act.cluster_id = cluster.id
+    act.port = 514
+    act.description = "d"
+    act.host = "logs.example.com"
+    act.template = "jsonline"
+    act.tls_verify_disabled = "true"
+    act.forward_connection_logs = "false"
+    act.call
+    cap.last_cluster_id.should eq cluster.id
+    j = JSON.parse(cap.last_payload_json).as_h
+    j["host"].as_s.should eq "logs.example.com"
+    j["port"].as_i.should eq 514
+    j["template"].as_s.should eq "jsonline"
+    j["description"].as_s.should eq "d"
+    j["tls_verify_disabled"].as_bool.should be_true
+    j["forward_connection_logs"].as_bool.should be_false
+  end
+
+  it "defaults tls and forward-connection to false in the payload when unset" do
+    cap = LogDestinationAddCaptureClient.new
+    act = CB::LogDestinationAdd.new(client: cap, output: IO::Memory.new)
+    act.cluster_id = cluster.id
+    act.port = 514
+    act.description = "d"
+    act.host = "logs.example.com"
+    act.template = "jsonline"
+    act.call
+    j = JSON.parse(cap.last_payload_json).as_h
+    j["tls_verify_disabled"].as_bool.should be_false
+    j["forward_connection_logs"].as_bool.should be_false
+  end
+end

spec/cb/logdest_add_spec.cr

@@ -53,4 +53,26 @@ Spectator.describe CB::LogDestinationAdd do
     lda.host = "logs.zam.com"
     lda.description.should eq "already set dont change"
   end
+
+  it "accepts tls and connection log flags as true or false strings" do
+    lda = make_lda
+    lda.tls_verify_disabled = "true"
+    lda.tls_verify_disabled.should be_true
+    lda.tls_verify_disabled = "false"
+    lda.tls_verify_disabled.should be_false
+    lda.forward_connection_logs = "TRUE"
+    lda.forward_connection_logs.should be_true
+    lda.forward_connection_logs = "false"
+    lda.forward_connection_logs.should be_false
+  end
+
+  it "rejects invalid boolean strings for tls_verify_disabled" do
+    lda = make_lda
+    expect { lda.tls_verify_disabled = "yes" }.to raise_error(CB::Program::Error, /Invalid/)
+  end
+
+  it "rejects invalid boolean strings for forward_connection_logs" do
+    lda = make_lda
+    expect { lda.forward_connection_logs = "1" }.to raise_error(CB::Program::Error, /Invalid/)
+  end
 end

spec/cb/logdest_list_spec.cr

@@ -0,0 +1,75 @@
+require "../spec_helper"
+
+private class TtyMemory < IO::Memory
+  def tty?
+    true
+  end
+end
+
+private class LogDestinationListTestClient < CB::Client
+  getter last_cluster : String?
+
+  def get_log_destinations(cluster_id)
+    @last_cluster = cluster_id
+    [
+      Factory.log_destination(
+        tls_verify_disabled: true,
+        forward_connection_logs: false,
+      ),
+      Factory.log_destination(
+        id: "pkdpq6yynjgjbps4otxd7il2u4",
+        description: "second",
+        tls_verify_disabled: false,
+        forward_connection_logs: true,
+      ),
+    ]
+  end
+end
+
+private class LogDestinationListEmptyClient < CB::Client
+  def initialize(token : String = TEST_TOKEN)
+    super(host: CB::HOST, bearer_token: token)
+  end
+
+  def get_log_destinations(cluster_id)
+    [] of CB::Model::LogDestination
+  end
+end
+
+Spectator.describe CB::LogDestinationList do
+  let(client) { LogDestinationListTestClient.new(TEST_TOKEN) }
+  let(cluster) { Factory.cluster }
+  subject(action) { described_class.new client: client, output: IO::Memory.new }
+
+  it "lists flags on tty" do
+    io = TtyMemory.new
+    act = CB::LogDestinationList.new(client: client, output: io)
+    act.cluster_id = cluster.id
+    act.call
+    s = io.to_s
+    s.should contain("TLS verify disabled: yes")
+    s.should contain("Forward connection logs: no")
+    s.should contain("TLS verify disabled: no")
+    s.should contain("Forward connection logs: yes")
+    client.last_cluster.should eq cluster.id
+  end
+
+  it "appends flag columns when piped" do
+    act = CB::LogDestinationList.new(client: client, output: IO::Memory.new)
+    act.cluster_id = cluster.id
+    act.call
+    lines = act.output.to_s.lines.map(&.rstrip("\n"))
+    lines.size.should eq 2
+    lines[0].split('\t').size.should eq 7
+    lines[0].ends_with?("\ttrue\tfalse").should be_true
+    lines[1].ends_with?("\tfalse\ttrue").should be_true
+  end
+
+  it "prints a message when there are no log destinations" do
+    io = TtyMemory.new
+    act = CB::LogDestinationList.new(client: LogDestinationListEmptyClient.new(TEST_TOKEN), output: io)
+    act.cluster_id = Factory.cluster.id
+    act.call
+    io.to_s.should contain("no log destinations")
+  end
+end