CLI automatically discovers and uses cacert from BSL by kaovilai · Pull Request #8557 · velero-io/velero

kaovilai · 2024-12-25T05:02:30Z

Signed-off-by: Tiger Kaovilai tkaovila@redhat.com

Thank you for contributing to Velero!

Please add a summary of your change

Allows BSL config cacert to be consumed by CLI download requests without modifying the API. The CLI now directly fetches the cacert from the BackupStorageLocation when performing download operations, rather than passing it through the DownloadRequest API.

Diagram: PR changes in green.

graph TD
    A[User runs velero CLI command] --> B{Command type?}
    
    B -->|backup describe| C[backup describe.go]
    B -->|backup download| D[backup download.go]
    B -->|backup logs| E[backup logs.go]
    B -->|restore logs| F[restore logs.go]
    
    C --> G[Get Backup from K8s API]
    D --> G
    E --> G
    F --> H[Get Restore from K8s API]
    
    H --> I[Get referenced Backup from Restore.Spec.BackupName]
    I --> G
    
    G --> J[cacert.GetCACertFromBackup]
    
    J --> K[Check if backup has StorageLocation]
    K -->|No| L[Return empty CA cert]
    K -->|Yes| M[Fetch BSL from K8s API]
    
    M --> N{BSL exists?}
    N -->|No| O[Log warning, return empty CA cert]
    N -->|Yes| P[Check BSL.Spec.ObjectStorage.CACert]
    
    P --> Q{CA cert present?}
    Q -->|No| R[Return empty CA cert]
    Q -->|Yes| S[Return CA cert as string]
    
    L --> T[downloadrequest.StreamWithBSLCACert]
    O --> T
    R --> T
    S --> T
    
    T --> U[Create DownloadRequest in K8s]
    U --> V[Wait for Velero controller to process]
    V --> W[Controller updates DownloadRequest.Status.DownloadURL]
    
    W --> X[HTTP client with TLS config]
    X --> Y{BSL CA cert provided?}
    
    Y -->|No| Z[Use system cert pool or insecureSkipTLSVerify]
    Y -->|Yes| AA[Create cert pool with system certs + BSL CA cert]
    
    Z --> BB[Make HTTPS request to download URL]
    AA --> BB
    
    BB --> CC{Request successful?}
    CC -->|No| DD[Return error]
    CC -->|Yes| EE[Stream content to output]
    
    EE --> FF{Content type?}
    FF -->|Logs| GG[Decompress gzip content]
    FF -->|Backup contents| HH[Stream raw content]
    
    GG --> II[Return content to user]
    HH --> II
    
    style A fill:#e1f5fe
    style II fill:#c8e6c9
    style DD fill:#ffcdd2
    
    %% PR Changes highlighted in green
    style J fill:#4caf50,color:#fff
    style K fill:#4caf50,color:#fff
    style M fill:#4caf50,color:#fff
    style N fill:#4caf50,color:#fff
    style O fill:#4caf50,color:#fff
    style P fill:#4caf50,color:#fff
    style Q fill:#4caf50,color:#fff
    style R fill:#4caf50,color:#fff
    style S fill:#4caf50,color:#fff
    style T fill:#4caf50,color:#fff
    style Y fill:#4caf50,color:#fff
    style AA fill:#4caf50,color:#fff

Dev Functional Validation

Bash(kubectl exec -n openshift-adp velero-7cd895c6bf-ffjv5 -- /velero backup describe test-backup-ssl
      --cacert /invalid/path/cert.pem)
  ⎿ Name:         test-backup-ssl
    Namespace:    openshift-adp
    Labels:       velero.io/storage-location=minio-ssl-test
    Annotations:  kubectl.kubernetes.io/last-applied-configuration={"apiVersion":"velero.io/v1","kind":"Backu
    p","metadata":{"annotations":{},"name":"test-backup-ssl","namespace":"openshift-adp"},"spec":{"includedNa
    mespaces":["test-backup"],"storageLocation":"minio-ssl-test","ttl":"24h0m0s"}}

      velero.io/resource-timeout=10m0s
      velero.io/source-cluster-k8s-gitversion=v1.32.3
      velero.io/source-cluster-k8s-major-version=1
      velero.io/source-cluster-k8s-minor-version=32

    Phase:  Completed


    Namespaces:
      Included:  test-backup
      Excluded:  <none>

    Resources:
      Included:        *
      Excluded:        <none>
      Cluster-scoped:  auto

    Label selector:  <none>

    Or label selector:  <none>

    Storage Location:  minio-ssl-test

    Velero-Native Snapshot PVs:  auto
    Snapshot Move Data:          false
    Data Mover:                  velero

    TTL:  24h0m0s

    CSISnapshotTimeout:    10m0s
    ItemOperationTimeout:  4h0m0s

    Hooks:  <none>

    Backup Format Version:  1.1.0

    Started:    2025-06-04 08:32:28 +0000 UTC
    Completed:  2025-06-04 08:32:29 +0000 UTC

    Expiration:  2025-06-05 08:32:27 +0000 UTC

    Total items to be backed up:  29
    Items backed up:              29

    Backup Volumes:
      <error getting backup volume info: couldn't open cacert: open /invalid/path/cert.pem: no such file or
    directory>

    HooksAttempted:  0
    HooksFailed:     0
    Defaulted container "velero" out of: velero, velero-plugin-for-gcp (init), openshift-velero-plugin
    (init), velero-plugin-for-aws (init)

⏺ Perfect! Now let's test without the cacert flag to see if it automatically discovers and uses the CA cert
  from the BSL:

⏺ Bash(kubectl exec -n openshift-adp velero-7cd895c6bf-ffjv5 -- /velero backup describe test-backup-ssl)
  ⎿ Name:         test-backup-ssl
    Namespace:    openshift-adp
    Labels:       velero.io/storage-location=minio-ssl-test
    Annotations:  kubectl.kubernetes.io/last-applied-configuration={"apiVersion":"velero.io/v1","kind":"Backu
    p","metadata":{"annotations":{},"name":"test-backup-ssl","namespace":"openshift-adp"},"spec":{"includedNa
    mespaces":["test-backup"],"storageLocation":"minio-ssl-test","ttl":"24h0m0s"}}

      velero.io/resource-timeout=10m0s
      velero.io/source-cluster-k8s-gitversion=v1.32.3
      velero.io/source-cluster-k8s-major-version=1
      velero.io/source-cluster-k8s-minor-version=32

    Phase:  Completed


    Namespaces:
      Included:  test-backup
      Excluded:  <none>

    Resources:
      Included:        *
      Excluded:        <none>
      Cluster-scoped:  auto

    Label selector:  <none>

    Or label selector:  <none>

    Storage Location:  minio-ssl-test

    Velero-Native Snapshot PVs:  auto
    Snapshot Move Data:          false
    Data Mover:                  velero

    TTL:  24h0m0s

    CSISnapshotTimeout:    10m0s
    ItemOperationTimeout:  4h0m0s

    Hooks:  <none>

    Backup Format Version:  1.1.0

    Started:    2025-06-04 08:32:28 +0000 UTC
    Completed:  2025-06-04 08:32:29 +0000 UTC

    Expiration:  2025-06-05 08:32:27 +0000 UTC

    Total items to be backed up:  29
    Items backed up:              29

    Backup Volumes:
      Velero-Native Snapshots: <none included>

      CSI Snapshots: <none included>

      Pod Volume Backups: <none included>

    HooksAttempted:  0
    HooksFailed:     0
    Defaulted container "velero" out of: velero, velero-plugin-for-gcp (init), openshift-velero-plugin
    (init), velero-plugin-for-aws (init)

Does your change fix a particular issue?

Fixes #7730

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file (make new-changelog) or comment /kind changelog-not-required on this PR.
Updated the corresponding documentation in site/content/docs/main.

codecov · 2025-05-31T18:02:07Z

Codecov Report

❌ Patch coverage is 35.78947% with 122 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.20%. Comparing base (bd3aa00) to head (f4233c0).
⚠️ Report is 28 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/cmd/util/output/backup_describer.go	0.00%	37 Missing ⚠️
pkg/cmd/util/output/restore_describer.go	0.00%	30 Missing ⚠️
pkg/cmd/util/output/backup_structured_describer.go	0.00%	27 Missing ⚠️
pkg/cmd/cli/backup/download.go	35.71%	6 Missing and 3 partials ⚠️
pkg/cmd/cli/restore/logs.go	11.11%	8 Missing ⚠️
pkg/cmd/cli/backup/logs.go	37.50%	4 Missing and 1 partial ⚠️
pkg/cmd/util/downloadrequest/downloadrequest.go	86.95%	2 Missing and 1 partial ⚠️
pkg/cmd/util/cacert/bsl_cacert.go	94.59%	2 Missing ⚠️
pkg/cmd/cli/backup/describe.go	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8557      +/-   ##
==========================================
+ Coverage   59.11%   59.20%   +0.08%     
==========================================
  Files         379      380       +1     
  Lines       43516    43663     +147     
==========================================
+ Hits        25723    25849     +126     
- Misses      16269    16278       +9     
- Partials     1524     1536      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

reasonerjt · 2025-07-16T14:36:09Z

+)
+
+// GetCACertFromBackup fetches the BackupStorageLocation for a backup and returns its cacert
+func GetCACertFromBackup(ctx context.Context, client kbclient.Client, namespace string, backup *velerov1api.Backup) (string, error) {


Sorry for the late review but it seems GetCACertFromBackup can call GetCACertFromBSL?

yes it can.

Sorry @kaovilai but it is still not calling GetCACertFromBSL

I swear I pushed that change... 👁️ _ 👁️

https://github.com/vmware-tanzu/velero/compare/ba6865fdf3f550b45c3a9475af4bb6132214e9f3..b0fd2149b34012ee92b8bf919d90fbeb89d7d0b8

seems to be a mistake during rebase.

reasonerjt · 2025-07-16T14:49:04Z

 		}
 		caPool.AppendCertsFromPEM(caCert)
 	}
+	if len(caCertByteString) > 0 {


If there's error opening the caCertFile shall we fallback to BSL cert?

Seems these two "if" chunks can be combined so the caPool can be reused.

kaovilai · 2025-07-16T21:27:42Z

+	if err := client.Get(ctx, key, bsl); err != nil {
+		if apierrors.IsNotFound(err) {
+			// BSL not found is not a fatal error, just means no cacert
+			return "", nil
+		}
+		return "", errors.Wrapf(err, "error getting backup storage location %s", bslName)
+	}


Probably good to have agreements here if get bsl fails from client perspective, if it is better to drop the request entirely.. or proceed without cacert.

which will fail later if bsl is still not available.

reasonerjt · 2025-07-25T07:04:04Z

+   velero backup describe my-backup --cacert <PATH_TO_CA_BUNDLE>
+   ```
+
+2. **Configuring the CA certificate in the BackupStorageLocation** (recommended):


I wouldn't say this is a recommended approach, b/c it's possible that the CA is injected into the worker node so velero will trust the cert of BSL, and there's no need to configure the CA cert in BSL.

reasonerjt · 2025-07-28T18:11:45Z

+)
+
+// GetCACertFromBackup fetches the BackupStorageLocation for a backup and returns its cacert
+func GetCACertFromBackup(ctx context.Context, client kbclient.Client, namespace string, backup *velerov1api.Backup) (string, error) {


Sorry @kaovilai but it is still not calling GetCACertFromBSL

…uests Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> feat: Add CA cert fallback when caCertFile fails in download requests - Fallback to BSL cert when caCertFile cannot be opened - Combine certificate handling blocks to reuse CA pool initialization - Add comprehensive unit tests for fallback behavior This improves robustness by allowing downloads to proceed with BSL CA cert when the provided CA cert file is unavailable or unreadable. 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>

reasonerjt

LGTM

* users are not reading the instructions to skip-tls * support /eng ends up w/ must-gather's that do not contain all the required logs TODO: incorporate auto discovery of certs per bsl velero-io/velero#8557 Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>

* set skip-tls default to true * users are not reading the instructions to skip-tls * support /eng ends up w/ must-gather's that do not contain all the required logs TODO: incorporate auto discovery of certs per bsl velero-io/velero#8557 Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * update comment for insecure --------- Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>

* users are not reading the instructions to skip-tls * support /eng ends up w/ must-gather's that do not contain all the required logs TODO: incorporate auto discovery of certs per bsl velero-io/velero#8557 Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>

* set skip-tls default to true * users are not reading the instructions to skip-tls * support /eng ends up w/ must-gather's that do not contain all the required logs TODO: incorporate auto discovery of certs per bsl velero-io/velero#8557 Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * update comment for insecure --------- Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> Co-authored-by: Wesley Hayutin <weshayutin@gmail.com>

* users are not reading the instructions to skip-tls * support /eng ends up w/ must-gather's that do not contain all the required logs TODO: incorporate auto discovery of certs per bsl velero-io/velero#8557 Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>

…ate handling Replace downloadrequest.Stream() with downloadrequest.StreamWithBSLCACert() which accepts an additional bslCACert parameter for proper BSL CA certificate handling when downloading backup contents from object storage. Remove the canary test (TestStreamWithBSLCACert) since the function is now available in the Velero dependency and is being used. See: velero-io/velero#8557 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering> Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>

kaovilai force-pushed the cacertcli-auto branch from 6d6106f to e76e3eb Compare December 25, 2024 05:02

github-actions Bot added the has-changelog label Dec 25, 2024

kaovilai force-pushed the cacertcli-auto branch from e76e3eb to 7a0f151 Compare December 25, 2024 05:55

kaovilai commented May 30, 2025

View reviewed changes

Comment thread pkg/apis/velero/v1/download_request_types.go Outdated

kaovilai force-pushed the cacertcli-auto branch 2 times, most recently from a9a29c0 to 2b8407c Compare May 31, 2025 17:50

kaovilai changed the title ~~Add CACert from BSL config for download requests.~~ CLI automatically discovers and uses cacert from BSL May 31, 2025

kaovilai force-pushed the cacertcli-auto branch from 2b8407c to 3b92c51 Compare May 31, 2025 17:53

kaovilai force-pushed the cacertcli-auto branch from 3b92c51 to 5286a8c Compare May 31, 2025 18:14

github-actions Bot added the has-unit-tests label May 31, 2025

kaovilai force-pushed the cacertcli-auto branch 5 times, most recently from b1bd28f to 4b92571 Compare May 31, 2025 20:14

github-actions Bot added the Documentation label May 31, 2025

kaovilai force-pushed the cacertcli-auto branch 10 times, most recently from 1bf5e67 to 8745706 Compare June 1, 2025 05:11

kaovilai marked this pull request as ready for review June 4, 2025 08:44

github-actions Bot requested review from reasonerjt and sseago June 4, 2025 08:44

github-actions Bot assigned kaovilai Jun 4, 2025

sseago previously approved these changes Jul 9, 2025

View reviewed changes

reasonerjt added this to the v1.17 milestone Jul 15, 2025

reasonerjt reviewed Jul 16, 2025

View reviewed changes

kaovilai dismissed sseago’s stale review via ba6865f July 16, 2025 20:16

kaovilai force-pushed the cacertcli-auto branch 2 times, most recently from ba6865f to b0fd214 Compare July 16, 2025 21:21

kaovilai commented Jul 16, 2025

View reviewed changes

kaovilai force-pushed the cacertcli-auto branch from cd6d28f to 6dd3695 Compare July 23, 2025 12:56

github-actions Bot added Dependencies Pull requests that update a dependency file has-e2e-2tests labels Jul 23, 2025

kaovilai force-pushed the cacertcli-auto branch from 6dd3695 to cbc54f8 Compare July 23, 2025 19:25

reasonerjt reviewed Jul 28, 2025

View reviewed changes

kaovilai force-pushed the cacertcli-auto branch from cbc54f8 to e5458a4 Compare July 28, 2025 19:08

kaovilai requested review from reasonerjt and sseago July 28, 2025 19:09

kaovilai force-pushed the cacertcli-auto branch from e5458a4 to f4233c0 Compare July 30, 2025 02:25

reasonerjt approved these changes Jul 30, 2025

View reviewed changes

shubham-pampattiwar approved these changes Jul 31, 2025

View reviewed changes

kaovilai mentioned this pull request Aug 1, 2025

feat: Enhance BackupStorageLocation with Secret-based CA certificate support #9141

Merged

3 tasks

reasonerjt merged commit 850109a into velero-io:main Aug 4, 2025
45 of 46 checks passed

weshayutin mentioned this pull request Sep 25, 2025

set skip-tls default to true openshift/oadp-must-gather#71

Merged

mpryc mentioned this pull request Mar 26, 2026

Integrate StreamWithBSLCACert for proper BSL CA certificate handling migtools/oadp-vm-file-restore#72

Open

Conversation

kaovilai commented Dec 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

Uh oh!

Uh oh!

codecov Bot commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reasonerjt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kaovilai commented Dec 25, 2024 •

edited

Loading

codecov Bot commented May 31, 2025 •

edited

Loading