Add require-crl-on-client-cert flag to certificate-authority commands#3274
Open
Aravind Khasibhatla (akhasibhatla) wants to merge 11 commits intomainfrom
Open
Add require-crl-on-client-cert flag to certificate-authority commands#3274Aravind Khasibhatla (akhasibhatla) wants to merge 11 commits intomainfrom
Aravind Khasibhatla (akhasibhatla) wants to merge 11 commits intomainfrom
Conversation
Aravind Khasibhatla (akhasibhatla)
requested a review
from a team
as a code owner
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
Copilot started reviewing on behalf of
Aravind Khasibhatla (akhasibhatla)
March 3, 2026 20:52
View session
There was a problem hiding this comment.
Pull request overview
Adds support in the IAM certificate-authority CLI commands for configuring and displaying the new “require CRL validation on client certificates” setting, aligning CLI behavior/output with the upstream API/SDK changes.
Changes:
- Added
--require-crl-on-client-certificatetocertificate-authority create(required) andcertificate-authority update. - Extended
describe/listoutput (human + JSON) to includeRequire CRL On Client Certificate. - Updated test server handlers and golden fixtures to reflect the new field.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/iam/command_certificate_authority_create.go |
Adds the new boolean flag and sends it in the create request (currently marked required). |
internal/iam/command_certificate_authority_update.go |
Adds the new boolean flag and allows updating it in the update request. |
internal/iam/command_certificate_authority.go |
Extends the output model + describe/create/update table output to include the new field. |
internal/iam/command_certificate_authority_list.go |
Extends list output rows to include the new field. |
test/test-server/iam_handlers.go |
Test server returns/echoes the new field in CA responses. |
test/fixtures/output/iam/certificate-authority/*.golden |
Updates expected CLI output for create/update/describe/list (human + JSON). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+98
to
+104
| if cmd.Flags().Changed("require-crl-on-client-certificate") { | ||
| requireCrlOnClientCertificate, err := cmd.Flags().GetBool("require-crl-on-client-certificate") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| update.RequireCrlOnClientCertificate = certificateauthorityv2.PtrBool(requireCrlOnClientCertificate) | ||
| } |
There was a problem hiding this comment.
The new --require-crl-on-client-certificate update path isn’t covered by the existing IAM certificate-authority integration fixtures (current tests cover other update flags like certificate-chain/crl-url, but not toggling this boolean). Add a test case + golden fixture that runs certificate-authority update ... --require-crl-on-client-certificate[=false|true] and asserts the field changes in the output (and request handling) so regressions in the Flags().Changed(...) logic are caught.
There was a problem hiding this comment.
Only update require-crl-on-client-certificate when user explicitly do it for keep backward compatible. .
There was a problem hiding this comment.
Confirmed - the update command only updates the field when explicitly specified via cmd.Flags().Changed(), maintaining backward compatibility.
There was a problem hiding this comment.
Jeff Huang (@jeffhuang26) PTAL when you have a minute today.
2 tasks
Author
Integration Test UpdateIntegration tests for the certificate-authority commands have been verified. Test ResultsTest Coverage
Golden FilesGolden files have been updated to reflect the current table formatting behavior:
|
…ommands Add support for the new require_crl_on_client_certificate attribute in the Certificate Authority API: - Add --require-crl-on-client-certificate flag to create command (required) - Add --require-crl-on-client-certificate flag to update command - Display the field in describe and list output - Update test fixtures and handlers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update certificate-authority create test cases to include the new required flag. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Regenerate golden files to match current table formatting behavior. The auto-wrap feature wraps long labels like "Require CRL On Client Certificate" across multiple lines. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ibility The flag defaults to false when not specified, maintaining backward compatibility with existing CLI usage. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Author
Final Verification After RebaseBranch has been rebased onto latest main and all integration tests pass. Test Results (post-rebase)Changes Verified
Ready for final review and merge once SDK PR is merged. |
Author
Exact Commands Run for Verification1. Rebase onto maincd ~/code/cli
git fetch origin main
git stash
git rebase origin/main
git stash pop2. Run integration testscd ~/code/cli/test
mkdir -p /tmp/coverage
GOCOVERDIR=/tmp/coverage go test -v -run TestCLI/TestIamCertificateAuthority3. Full test outputAll 12 certificate-authority integration tests pass after rebase. |
Author
Update SummaryWhat was done:
Remaining Steps
Once SDK PR #337 is merged and released, I'll update the CLI with the released SDK version. |
Member
Cynthia Qin (cqin-confluent)
left a comment
Cynthia Qin (cqin-confluent)
left a comment
There was a problem hiding this comment.
Looks good overall! Left a few comments.
Quick question:
I noticed that all the CRL-related fields (CrlSource, CrlUrl, CrlUpdatedAt) use omitempty so they’re hidden when empty, but RequireCrlOnClientCertificate doesn’t and always shows false even for CAs without CRL.
CrlSource string `human:"CRL Source,omitempty" serialized:"crl_source,omitempty"`
CrlUrl string `human:"CRL URL,omitempty" serialized:"crl_url,omitempty"`
RequireCrlOnClientCertificate bool `human:"Require CRL On Client Certificate"
serialized:"require_crl_on_client_certificate"`
Was it intentional to always show this field? For CAs without CRL, displaying false might be noisy. Maybe omitempty is worth considering here (though note that on a bool it hides false, which may or may not be what we want).
Jeff Huang (jeffhuang26)
previously approved these changes
Mar 4, 2026
Adds test case to verify the update operation with the new flag, as requested in review feedback. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
airlock-confluentinc
bot
dismissed
Jeff Huang (jeffhuang26)’s stale review
via
720a39f
airlock-confluentinc
bot
force-pushed
the
akk-add-require-crl-on-client-certificate
branch
from
8040b95 to
720a39f
Compare
Shortened the flag name from --require-crl-on-client-certificate to --require-crl-on-client-cert based on review feedback. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Author
|
CynthiaQin - Following up on review feedback, I've made the following changes: Changes Made1. Renamed Flag for Brevity
2. Added New Test Case for UPDATE Operation{args: "iam certificate-authority update op-12345 --require-crl-on-client-cert=false", fixture: "iam/certificate-authority/update-require-crl.golden"},Test ResultsAll 13 certificate-authority integration tests pass: cd ~/code/cli/test
mkdir -p /tmp/coverage
GOCOVERDIR=/tmp/coverage go test -v -run TestCLI/TestIamCertificateAuthority
Ready for re-review! |
Updated to SDK version v0.0.3-0.20260304220235-a27d679482e3 from master which includes the new RequireCrlOnClientCertificate field. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Author
SDK PR Merged - CLI UpdatedSDK PR #337 has been merged. Changes Made
Test ResultsAll 13 certificate-authority integration tests pass with the released SDK: cd ~/code/cli
go get github.com/confluentinc/ccloud-sdk-go-v2/certificate-authority@master
go mod tidy
cd test
GOCOVERDIR=/tmp/coverage go test -v -run TestCLI/TestIamCertificateAuthorityCI should pass now. Ready for final review! |
Aravind Khasibhatla (akhasibhatla)
changed the title
Author
|
CynthiaQin Apologies for the premature SDK merge - I should have waited for the field rename to be finalized before merging PR #337. To address this, I've created a follow-up SDK PR to rename the field: This rename aligns the JSON field name ( Once the SDK rename PR is merged, I'll update this CLI PR to use the renamed field and ensure everything stays consistent. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release Notes
New Features
--require-crl-on-client-certificateflag toconfluent iam certificate-authority [ create | update | describe | list ]command to enable CRL validation on client certificatesChecklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.What
This PR adds support for the new
require_crl_on_client_certificateattribute in the Certificate Authority API. This attribute allows users to specify whether CRL (Certificate Revocation List) validation should be required on client certificates.Applies to: Confluent Cloud only
Blast Radius
Should be minimal. This change is additive only:
false, maintaining backward compatibilityReferences
Test & Review
Integration Tests
All 12 certificate-authority integration tests pass:
Manual testing -- default behaviour (
require-crl-on-client-certis not specified and defaulted to be false for backward compatibility)create
describe
update
list
delete
Manual testing -- updated behaviour (
require-crl-on-client-certflag added)create
describe
update
delete