From 29e8982f82478931ed2cf82e3bbcb53e86fab8f3 Mon Sep 17 00:00:00 2001 From: Erik Jones Date: Thu, 14 May 2026 15:14:32 -0700 Subject: [PATCH 1/5] CSE-364: Extend log destinations with TLS and connection-log flags and add update 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. --- spec/cb/completion_spec.cr | 39 ++++++ spec/cb/logdest_add_payload_spec.cr | 58 +++++++++ spec/cb/logdest_add_spec.cr | 22 ++++ spec/cb/logdest_list_spec.cr | 75 +++++++++++ spec/cb/logdest_update_spec.cr | 187 ++++++++++++++++++++++++++++ spec/models/log_destination_spec.cr | 19 +++ spec/spec_helper.cr | 4 - spec/support/factory.cr | 12 +- src/cb/completion.cr | 42 ++++++- src/cb/logdest_add.cr | 12 +- src/cb/logdest_list.cr | 10 +- src/cb/logdest_update.cr | 66 ++++++++++ src/cli.cr | 19 ++- src/client/log_destination.cr | 8 ++ src/models/log_destination.cr | 4 +- 15 files changed, 559 insertions(+), 18 deletions(-) create mode 100644 spec/cb/logdest_add_payload_spec.cr create mode 100644 spec/cb/logdest_list_spec.cr create mode 100644 spec/cb/logdest_update_spec.cr create mode 100644 spec/models/log_destination_spec.cr create mode 100644 src/cb/logdest_update.cr diff --git a/spec/cb/completion_spec.cr b/spec/cb/completion_spec.cr index cf4f256..9c88eac 100644 --- a/spec/cb/completion_spec.cr +++ b/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 diff --git a/spec/cb/logdest_add_payload_spec.cr b/spec/cb/logdest_add_payload_spec.cr new file mode 100644 index 0000000..4600e4a --- /dev/null +++ b/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 diff --git a/spec/cb/logdest_add_spec.cr b/spec/cb/logdest_add_spec.cr index 40cd805..c0681b1 100644 --- a/spec/cb/logdest_add_spec.cr +++ b/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 diff --git a/spec/cb/logdest_list_spec.cr b/spec/cb/logdest_list_spec.cr new file mode 100644 index 0000000..f8962af --- /dev/null +++ b/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 diff --git a/spec/cb/logdest_update_spec.cr b/spec/cb/logdest_update_spec.cr new file mode 100644 index 0000000..a9fa9b8 --- /dev/null +++ b/spec/cb/logdest_update_spec.cr @@ -0,0 +1,187 @@ +require "../spec_helper" + +private class LogDestinationUpdateTestClient < CB::Client +end + +private class LogDestinationUpdateTrackingClient < CB::Client + getter body_sent : Hash(String, String | Int32 | Bool)? + + def initialize(@list_rows : Array(CB::Model::LogDestination), token : String = TEST_TOKEN) + super(host: CB::HOST, bearer_token: token) + end + + def get_log_destinations(cluster_id) + @list_rows + end + + def update_log_destination(cluster_id, logdest_id, body) + @body_sent = body + "{}" + end +end + +private def make_ldu + CB::LogDestinationUpdate.new(LogDestinationUpdateTestClient.new(TEST_TOKEN)) +end + +Spectator.describe CB::LogDestinationUpdate do + subject(action) { described_class.new client: client, output: IO::Memory.new } + + mock_client + + let(cluster) { Factory.cluster } + let(logdest) { Factory.log_destination } + + it "ensures required arguments are present" do + expect_missing_arg_error + action.cluster_id = cluster.id + + expect_missing_arg_error + action.logdest_id = logdest.id + + expect(&.validate).to be_true + end + + it "sets a default description based on the host if missing" do + ldu = make_ldu + ldu.description.should be_nil + + ldu.host = "foo" + ldu.host.should eq "foo" + ldu.description.should eq "foo" + + ldu.description = nil + ldu.host = "bar.com" + ldu.description.should eq "bar" + + ldu.description = nil + ldu.host = "logs.baz.com" + ldu.description.should eq "baz" + + ldu.description = "already set dont change" + ldu.host = "logs.zam.com" + ldu.description.should eq "already set dont change" + end + + it "rejects invalid boolean strings for optional tls flag" do + ldu = make_ldu + expect { ldu.tls_verify_disabled = "maybe" }.to raise_error(CB::Program::Error, /Invalid/) + end + + it "rejects invalid boolean strings for optional forward-connection flag" do + ldu = make_ldu + expect { ldu.forward_connection_logs = "on" }.to raise_error(CB::Program::Error, /Invalid/) + end + + describe "#run" do + let(row) { Factory.log_destination } + + it "fills host, port, template, and description from list when omitted" do + tracking = LogDestinationUpdateTrackingClient.new([row]) + act = CB::LogDestinationUpdate.new(client: tracking, output: IO::Memory.new) + act.cluster_id = cluster.id + act.logdest_id = row.id + act.call + tracking.body_sent.should eq({ + "host" => "host", + "port" => 2020, + "template" => "template", + "description" => "logdest descr", + "tls_verify_disabled" => false, + "forward_connection_logs" => false, + }) + end + + it "merges optional flags with list defaults" do + tracking = LogDestinationUpdateTrackingClient.new([row]) + act = CB::LogDestinationUpdate.new(client: tracking, output: IO::Memory.new) + act.cluster_id = cluster.id + act.logdest_id = row.id + act.forward_connection_logs = true + act.call + tracking.body_sent.should eq({ + "host" => "host", + "port" => 2020, + "template" => "template", + "description" => "logdest descr", + "tls_verify_disabled" => false, + "forward_connection_logs" => true, + }) + end + + it "keeps the other boolean from the list when only one flag is set" do + row_with_flags = Factory.log_destination( + tls_verify_disabled: true, + forward_connection_logs: false, + ) + tracking = LogDestinationUpdateTrackingClient.new([row_with_flags]) + act = CB::LogDestinationUpdate.new(client: tracking, output: IO::Memory.new) + act.cluster_id = cluster.id + act.logdest_id = row_with_flags.id + act.forward_connection_logs = true + act.call + tracking.body_sent.try &.["tls_verify_disabled"].should eq true + tracking.body_sent.try &.["forward_connection_logs"].should eq true + end + + it "applies tls_verify_disabled from a string argument" do + row_with_flags = Factory.log_destination( + tls_verify_disabled: true, + forward_connection_logs: true, + ) + tracking = LogDestinationUpdateTrackingClient.new([row_with_flags]) + act = CB::LogDestinationUpdate.new(client: tracking, output: IO::Memory.new) + act.cluster_id = cluster.id + act.logdest_id = row_with_flags.id + act.tls_verify_disabled = "false" + act.call + tracking.body_sent.try &.["tls_verify_disabled"].should eq false + tracking.body_sent.try &.["forward_connection_logs"].should eq true + end + + it "applies forward_connection_logs from a string argument" do + row_with_flags = Factory.log_destination( + tls_verify_disabled: false, + forward_connection_logs: true, + ) + tracking = LogDestinationUpdateTrackingClient.new([row_with_flags]) + act = CB::LogDestinationUpdate.new(client: tracking, output: IO::Memory.new) + act.cluster_id = cluster.id + act.logdest_id = row_with_flags.id + act.forward_connection_logs = "false" + act.call + tracking.body_sent.try &.["tls_verify_disabled"].should eq false + tracking.body_sent.try &.["forward_connection_logs"].should eq false + end + + it "overrides port from the list when given" do + tracking = LogDestinationUpdateTrackingClient.new([row]) + act = CB::LogDestinationUpdate.new(client: tracking, output: IO::Memory.new) + act.cluster_id = cluster.id + act.logdest_id = row.id + act.port = 9999 + act.call + tracking.body_sent.try &.["port"].should eq 9999 + tracking.body_sent.try &.["host"].should eq "host" + end + + it "raises when the log destination is not on the list" do + tracking = LogDestinationUpdateTrackingClient.new([] of CB::Model::LogDestination) + act = CB::LogDestinationUpdate.new(client: tracking, output: IO::Memory.new) + act.cluster_id = cluster.id + act.logdest_id = row.id + expect_raises(CB::Program::Error, /not found/) { act.call } + end + + it "raises when the list has no matching log destination id" do + other = Factory.log_destination( + id: "pkdpq6yynjgjbps4otxd7il2u4", + ) + tracking = LogDestinationUpdateTrackingClient.new([other]) + act = CB::LogDestinationUpdate.new(client: tracking, output: IO::Memory.new) + act.cluster_id = cluster.id + act.logdest_id = row.id + expect_raises(CB::Program::Error, /not found/) { act.call } + end + end +end diff --git a/spec/models/log_destination_spec.cr b/spec/models/log_destination_spec.cr new file mode 100644 index 0000000..9d06870 --- /dev/null +++ b/spec/models/log_destination_spec.cr @@ -0,0 +1,19 @@ +require "../spec_helper" + +Spectator.describe CB::Model::LogDestination do + it "parses tls flags from list JSON" do + json = %({"loggers":[{"id":"pxbcigcufjdqje6drled4rj6p4","host":"h","port":1,"template":"t","description":"d","tls_verify_disabled":true,"forward_connection_logs":false}]}) + list = Array(CB::Model::LogDestination).from_json json, root: "loggers" + list.size.should eq 1 + d = list.first + d.tls_verify_disabled.should be_true + d.forward_connection_logs.should be_false + end + + it "defaults tls flags when keys are missing" do + json = %({"loggers":[{"id":"pxbcigcufjdqje6drled4rj6p4","host":"h","port":1,"template":"t","description":"d"}]}) + d = Array(CB::Model::LogDestination).from_json(json, root: "loggers").first + d.tls_verify_disabled.should be_false + d.forward_connection_logs.should be_false + end +end diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index 425632b..084d019 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -16,10 +16,6 @@ macro expect_missing_arg_error expect(&.validate).to raise_error(CB::Program::Error, /Missing required argument/) end -macro expect_invalid_arg_error - expect({{yield}}).to raise_error(CB::Program::Error, /Invalid/) -end - macro mock_client mock Client do def initialize(@token : Token = TEST_TOKEN) diff --git a/spec/support/factory.cr b/spec/support/factory.cr index 87b2854..414ff9d 100644 --- a/spec/support/factory.cr +++ b/spec/support/factory.cr @@ -128,11 +128,13 @@ module Factory def log_destination(**params) params = { - id: "pxbcigcufjdqje6drled4rj6p4", - host: "host", - port: 2020, - template: "template", - description: "logdest descr", + id: "pxbcigcufjdqje6drled4rj6p4", + host: "host", + port: 2020, + template: "template", + description: "logdest descr", + tls_verify_disabled: false, + forward_connection_logs: false, }.merge(params) CB::Model::LogDestination.new **params diff --git a/src/cb/completion.cr b/src/cb/completion.cr index fffa09a..7391d0b 100644 --- a/src/cb/completion.cr +++ b/src/cb/completion.cr @@ -353,11 +353,14 @@ class CB::Completion logdest_destroy when "add" logdest_add + when "update" + logdest_update else [ "list\tlist all log destinations for a cluster", "add\tadd a new log destination to a cluster", - "destroy\tremove a new destination from a cluster", + "update\tupdate an existing log destination on a cluster", + "destroy\tremove an existing log destination from a cluster", ] end end @@ -896,6 +899,9 @@ class CB::Completion return cluster_suggestions end + suggest_bool if last_arg?("--tls-verify-disabled") + suggest_bool if last_arg?("--forward-connection-logs") + # return missing args suggest = [] of String suggest << "--help\tshow help" if args.size == 3 @@ -904,6 +910,37 @@ class CB::Completion suggest << "--port\tport number" unless has_full_flag? :port suggest << "--desc\tdescription" unless has_full_flag? :desc suggest << "--template\ttemplate" unless has_full_flag? :template + suggest << "--tls-verify-disabled\ttrue or false (default false)" unless has_full_flag? :tls_verify_disabled + suggest << "--forward-connection-logs\ttrue or false (default false)" unless has_full_flag? :forward_connection_logs + suggest + end + + def logdest_update + cluster = find_arg_value "--cluster" + logdest = find_arg_value "--logdest" + + if last_arg?("--cluster") + return cluster.nil? ? cluster_suggestions : [] of String + end + + if last_arg?("--logdest") + return [] of String unless logdest.nil? && cluster + return client.get_log_destinations(cluster).map { |d| "#{d.id}\t#{d.description}" } + end + + suggest_bool if last_arg?("--tls-verify-disabled") + suggest_bool if last_arg?("--forward-connection-logs") + + suggest = [] of String + suggest << "--help\tshow help" if args.size == 3 + suggest << "--cluster\tcluster id" unless has_full_flag? :cluster + suggest << "--logdest\tlog destination id" unless has_full_flag? :logdest + suggest << "--host\thostname" unless has_full_flag? :host + suggest << "--port\tport number" unless has_full_flag? :port + suggest << "--desc\tdescription" unless has_full_flag? :desc + suggest << "--template\ttemplate" unless has_full_flag? :template + suggest << "--tls-verify-disabled\ttrue or false (default false)" unless has_full_flag? :tls_verify_disabled + suggest << "--forward-connection-logs\ttrue or false (default false)" unless has_full_flag? :forward_connection_logs suggest end @@ -1359,6 +1396,7 @@ class CB::Completion full << :storage if has_full_flag? "--storage", "-s" full << :platform if has_full_flag? "--platform", "-p" full << :port if has_full_flag? "--port" + full << :logdest if has_full_flag? "--logdest" full << :desc if has_full_flag? "--desc" full << :template if has_full_flag? "--template" full << :header if has_full_flag? "-H" @@ -1386,6 +1424,8 @@ class CB::Completion full << :no_header if has_full_flag? "--no-header" full << :peering if has_full_flag? "--peering" full << :maintenance_window_start if has_full_flag? "--maintenance-window-start" + full << :tls_verify_disabled if has_full_flag? "--tls-verify-disabled" + full << :forward_connection_logs if has_full_flag? "--forward-connection-logs" full end diff --git a/src/cb/logdest_add.cr b/src/cb/logdest_add.cr index c131e3f..0606997 100644 --- a/src/cb/logdest_add.cr +++ b/src/cb/logdest_add.cr @@ -7,14 +7,18 @@ module CB property host : String? property template : String? property description : String? + bool_setter tls_verify_disabled + bool_setter forward_connection_logs def run validate client.add_log_destination(cluster_id, { - "host": host, - "port": port, - "template": template, - "description": description, + "host": host, + "port": port, + "template": template, + "description": description, + "tls_verify_disabled": tls_verify_disabled, + "forward_connection_logs": forward_connection_logs, }) output << "added new log destination for " diff --git a/src/cb/logdest_list.cr b/src/cb/logdest_list.cr index a77a6bb..7cd9401 100644 --- a/src/cb/logdest_list.cr +++ b/src/cb/logdest_list.cr @@ -22,6 +22,8 @@ module CB output << " " << d.description.colorize.t_name output << " (" << d.host << ":" << d.port << ")\n" output << " " << d.template << "\n" + output << " " << "TLS verify disabled: " << yes_no(d.tls_verify_disabled) << "\n" + output << " " << "Forward connection logs: " << yes_no(d.forward_connection_logs) << "\n" end end @@ -31,8 +33,14 @@ module CB output << d.description << "\t" output << d.host << "\t" output << d.port << "\t" - output << d.template << "\n" + output << d.template << "\t" + output << d.tls_verify_disabled << "\t" + output << d.forward_connection_logs << "\n" end end + + private def yes_no(value : Bool) : String + value ? "yes" : "no" + end end end diff --git a/src/cb/logdest_update.cr b/src/cb/logdest_update.cr new file mode 100644 index 0000000..1accca0 --- /dev/null +++ b/src/cb/logdest_update.cr @@ -0,0 +1,66 @@ +require "./action" + +module CB + class LogDestinationUpdate < APIAction + eid_setter cluster_id + eid_setter logdest_id + i32_setter port + property host : String? + property template : String? + property description : String? + bool_setter? tls_verify_disabled + bool_setter? forward_connection_logs + + def run + validate + cid = cluster_id.not_nil! + lid = logdest_id.not_nil! + + existing = client.get_log_destinations(cid).find { |d| d.id == lid } + unless existing + raise Program::Error.new "log destination #{lid.colorize.t_id} was not found for this cluster." + end + + client.update_log_destination(cid, lid, update_body(existing)) + + output << "updated log destination " + output << "#{logdest_id}".colorize.t_id << " for " + output << "#{cluster_id}".colorize.t_id << '\n' + end + + def validate + check_required_args do |missing| + missing << "cluster" unless cluster_id + missing << "logdest" unless logdest_id + end + end + + private def update_body(existing : CB::Model::LogDestination) : Hash(String, String | Int32 | Bool) + eff_host = host || existing.host + eff_port = port || existing.port + eff_template = template || existing.template + eff_description = description || existing.description + + body = {} of String => String | Int32 | Bool + body["host"] = eff_host + body["port"] = eff_port + body["template"] = eff_template + body["description"] = eff_description + body["tls_verify_disabled"] = tls_verify_disabled.nil? ? existing.tls_verify_disabled : tls_verify_disabled.not_nil! + body["forward_connection_logs"] = forward_connection_logs.nil? ? existing.forward_connection_logs : forward_connection_logs.not_nil! + body + end + + def port=(i : Int32) + raise_arg_error "port", i unless 1 <= i < 65_535 + @port = i + end + + def host=(str : String) + unless @description + @description = str.split('.').last(2).first + end + @host = str + end + end +end diff --git a/src/cli.cr b/src/cli.cr index 8754027..2606fcf 100755 --- a/src/cli.cr +++ b/src/cli.cr @@ -390,7 +390,7 @@ op = OptionParser.new do |parser| end parser.on("logdest", "Manage log destinations") do - parser.banner = "cb logdest " + parser.banner = "cb logdest " parser.on("list", "List log destinations for a cluster") do list = set_action LogDestinationList @@ -400,12 +400,27 @@ op = OptionParser.new do |parser| parser.on("add", "Add a new log destination to a cluster") do add = set_action LogDestinationAdd - parser.banner = "cb logdest add <--cluster> <--host> <--port> <--template> [--desc]" + parser.banner = "cb logdest add <--cluster> <--host> <--port> <--template> [--desc] [--tls-verify-disabled (default false)] [--forward-connection-logs (default false)]" parser.on("--cluster ID", "Choose cluster") { |arg| add.cluster_id = arg } parser.on("--host HOST", "Hostname") { |arg| add.host = arg } parser.on("--port PORT", "Port number") { |arg| add.port = arg } parser.on("--template STR", "Log template") { |arg| add.template = arg } parser.on("--desc STR", "Description") { |arg| add.description = arg } + parser.on("--tls-verify-disabled BOOL", "Whether TLS certificate verification is disabled (true|false). Defaults to false if omitted.") { |arg| add.tls_verify_disabled = arg } + parser.on("--forward-connection-logs BOOL", "Whether to forward connection logs (true|false). Defaults to false if omitted.") { |arg| add.forward_connection_logs = arg } + end + + parser.on("update", "Update an existing log destination on a cluster") do + update = set_action LogDestinationUpdate + parser.banner = "cb logdest update <--cluster> <--logdest> [--host HOST] [--port PORT] [--template STR] [--desc STR] [--tls-verify-disabled ] [--forward-connection-logs )]\n (Optional options keep the destination's current value when omitted.)" + parser.on("--cluster ID", "Cluster ID (required)") { |arg| update.cluster_id = arg } + parser.on("--logdest ID", "Log destination ID (required)") { |arg| update.logdest_id = arg } + parser.on("--host HOST", "Hostname (optional)") { |arg| update.host = arg } + parser.on("--port PORT", "Port number (optional)") { |arg| update.port = arg } + parser.on("--template STR", "Log template (optional)") { |arg| update.template = arg } + parser.on("--desc STR", "Description (optional)") { |arg| update.description = arg } + parser.on("--tls-verify-disabled BOOL", "Whether TLS certificate verification is disabled (optional)") { |arg| update.tls_verify_disabled = arg } + parser.on("--forward-connection-logs BOOL", "Whether to forward connection logs (optional)") { |arg| update.forward_connection_logs = arg } end parser.on("destroy", "Remove an existing log destination from a cluster") do diff --git a/src/client/log_destination.cr b/src/client/log_destination.cr index 87dfa33..ec174dc 100644 --- a/src/client/log_destination.cr +++ b/src/client/log_destination.cr @@ -18,6 +18,14 @@ module CB resp.body end + # Update an existing logger on a cluster. + # + # https://crunchybridgeapi.docs.apiary.io/#reference/0/clustersclusteridloggersloggerid/update-logger + def update_log_destination(cluster_id, logdest_id, ld) + resp = put "clusters/#{cluster_id}/loggers/#{logdest_id}", ld + resp.body + end + # Remove a logger from a cluster. # # https://crunchybridgeapi.docs.apiary.io/#reference/0/clustersclusteridloggersloggerid/destroy-logger diff --git a/src/models/log_destination.cr b/src/models/log_destination.cr index 19170b9..7918eeb 100644 --- a/src/models/log_destination.cr +++ b/src/models/log_destination.cr @@ -4,5 +4,7 @@ module CB::Model host : String, port : Int32, template : String, - description : String + description : String, + tls_verify_disabled : Bool = false, + forward_connection_logs : Bool = false end From 653542b2aea180fe615067b775a5c6f19e99790a Mon Sep 17 00:00:00 2001 From: Erik Jones Date: Fri, 15 May 2026 12:03:14 -0700 Subject: [PATCH 2/5] crystal tool format fixes --- spec/cb/logdest_update_spec.cr | 24 ++++++++++++------------ spec/support/factory.cr | 14 +++++++------- src/cb/logdest_add.cr | 12 ++++++------ 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/spec/cb/logdest_update_spec.cr b/spec/cb/logdest_update_spec.cr index a9fa9b8..75243df 100644 --- a/spec/cb/logdest_update_spec.cr +++ b/spec/cb/logdest_update_spec.cr @@ -83,12 +83,12 @@ Spectator.describe CB::LogDestinationUpdate do act.logdest_id = row.id act.call tracking.body_sent.should eq({ - "host" => "host", - "port" => 2020, - "template" => "template", - "description" => "logdest descr", - "tls_verify_disabled" => false, - "forward_connection_logs" => false, + "host" => "host", + "port" => 2020, + "template" => "template", + "description" => "logdest descr", + "tls_verify_disabled" => false, + "forward_connection_logs" => false, }) end @@ -100,12 +100,12 @@ Spectator.describe CB::LogDestinationUpdate do act.forward_connection_logs = true act.call tracking.body_sent.should eq({ - "host" => "host", - "port" => 2020, - "template" => "template", - "description" => "logdest descr", - "tls_verify_disabled" => false, - "forward_connection_logs" => true, + "host" => "host", + "port" => 2020, + "template" => "template", + "description" => "logdest descr", + "tls_verify_disabled" => false, + "forward_connection_logs" => true, }) end diff --git a/spec/support/factory.cr b/spec/support/factory.cr index 414ff9d..5dd0459 100644 --- a/spec/support/factory.cr +++ b/spec/support/factory.cr @@ -128,13 +128,13 @@ module Factory def log_destination(**params) params = { - id: "pxbcigcufjdqje6drled4rj6p4", - host: "host", - port: 2020, - template: "template", - description: "logdest descr", - tls_verify_disabled: false, - forward_connection_logs: false, + id: "pxbcigcufjdqje6drled4rj6p4", + host: "host", + port: 2020, + template: "template", + description: "logdest descr", + tls_verify_disabled: false, + forward_connection_logs: false, }.merge(params) CB::Model::LogDestination.new **params diff --git a/src/cb/logdest_add.cr b/src/cb/logdest_add.cr index 0606997..2bd14d9 100644 --- a/src/cb/logdest_add.cr +++ b/src/cb/logdest_add.cr @@ -13,12 +13,12 @@ module CB def run validate client.add_log_destination(cluster_id, { - "host": host, - "port": port, - "template": template, - "description": description, - "tls_verify_disabled": tls_verify_disabled, - "forward_connection_logs": forward_connection_logs, + "host": host, + "port": port, + "template": template, + "description": description, + "tls_verify_disabled": tls_verify_disabled, + "forward_connection_logs": forward_connection_logs, }) output << "added new log destination for " From f88c79744fa5c23b7747a50d6adc62ae84b41fb9 Mon Sep 17 00:00:00 2001 From: Erik Jones Date: Fri, 15 May 2026 12:23:06 -0700 Subject: [PATCH 3/5] Changes for ameba's Lint/NotNil rule --- src/cb/logdest_update.cr | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/cb/logdest_update.cr b/src/cb/logdest_update.cr index 1accca0..b2731a2 100644 --- a/src/cb/logdest_update.cr +++ b/src/cb/logdest_update.cr @@ -13,8 +13,9 @@ module CB def run validate - cid = cluster_id.not_nil! - lid = logdest_id.not_nil! + cid = cluster_id + lid = logdest_id + raise Error.new "Missing required arguments: cluster, logdest" if cid.nil? || lid.nil? existing = client.get_log_destinations(cid).find { |d| d.id == lid } unless existing @@ -35,6 +36,10 @@ module CB end end + private def merged_bool(override : Bool?, existing : Bool) : Bool + override.nil? ? existing : override + end + private def update_body(existing : CB::Model::LogDestination) : Hash(String, String | Int32 | Bool) eff_host = host || existing.host eff_port = port || existing.port @@ -46,8 +51,8 @@ module CB body["port"] = eff_port body["template"] = eff_template body["description"] = eff_description - body["tls_verify_disabled"] = tls_verify_disabled.nil? ? existing.tls_verify_disabled : tls_verify_disabled.not_nil! - body["forward_connection_logs"] = forward_connection_logs.nil? ? existing.forward_connection_logs : forward_connection_logs.not_nil! + body["tls_verify_disabled"] = merged_bool(tls_verify_disabled, existing.tls_verify_disabled) + body["forward_connection_logs"] = merged_bool(forward_connection_logs, existing.forward_connection_logs) body end From b9ee73280b648ae85bd98f1168003af4416a4879 Mon Sep 17 00:00:00 2001 From: Erik Jones Date: Fri, 15 May 2026 12:56:01 -0700 Subject: [PATCH 4/5] Limit codegen threads to 1 for crystal spec runs on CI --- flake.nix | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/flake.nix b/flake.nix index 193829b..a8b6106 100644 --- a/flake.nix +++ b/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; From 90d5cefcc3b585d5d208e79d6d8f3d27cff7b0d0 Mon Sep 17 00:00:00 2001 From: Erik Jones Date: Wed, 20 May 2026 15:35:26 -0700 Subject: [PATCH 5/5] Changes from Adam's review. --- src/cb/logdest_update.cr | 9 +++------ src/cli.cr | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/cb/logdest_update.cr b/src/cb/logdest_update.cr index b2731a2..2c79455 100644 --- a/src/cb/logdest_update.cr +++ b/src/cb/logdest_update.cr @@ -13,16 +13,13 @@ module CB def run validate - cid = cluster_id - lid = logdest_id - raise Error.new "Missing required arguments: cluster, logdest" if cid.nil? || lid.nil? - existing = client.get_log_destinations(cid).find { |d| d.id == lid } + existing = client.get_log_destinations(cluster_id).find { |d| d.id == logdest_id } unless existing - raise Program::Error.new "log destination #{lid.colorize.t_id} was not found for this cluster." + raise Program::Error.new "log destination #{logdest_id.colorize.t_id} was not found for this cluster." end - client.update_log_destination(cid, lid, update_body(existing)) + client.update_log_destination(cluster_id, logdest_id, update_body(existing)) output << "updated log destination " output << "#{logdest_id}".colorize.t_id << " for " diff --git a/src/cli.cr b/src/cli.cr index 2606fcf..83ac2ad 100755 --- a/src/cli.cr +++ b/src/cli.cr @@ -412,7 +412,7 @@ op = OptionParser.new do |parser| parser.on("update", "Update an existing log destination on a cluster") do update = set_action LogDestinationUpdate - parser.banner = "cb logdest update <--cluster> <--logdest> [--host HOST] [--port PORT] [--template STR] [--desc STR] [--tls-verify-disabled ] [--forward-connection-logs )]\n (Optional options keep the destination's current value when omitted.)" + parser.banner = "cb logdest update <--cluster> <--logdest> [--host HOST] [--port PORT] [--template STR] [--desc STR] [--tls-verify-disabled ] [--forward-connection-logs ]\n (Optional options keep the destination's current value when omitted.)" parser.on("--cluster ID", "Cluster ID (required)") { |arg| update.cluster_id = arg } parser.on("--logdest ID", "Log destination ID (required)") { |arg| update.logdest_id = arg } parser.on("--host HOST", "Hostname (optional)") { |arg| update.host = arg }