CNTRLPLANE-2678: Add fetch-etcd-certs CPO subcommand for HCPEtcdBackup - #8010
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
📝 WalkthroughWalkthroughThis pull request adds a new Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as fetch-etcd-certs CLI
participant K8s as Kubernetes API
participant FS as File System
User->>CLI: Invoke with hcp-namespace, output-dir flags
CLI->>CLI: Parse and validate flags
CLI->>CLI: Create controller-runtime client (kubeconfig)
CLI->>K8s: Get etcd-client Secret from namespace
K8s-->>CLI: Return Secret with cert/key data
CLI->>K8s: Get etcd CA ConfigMap from namespace
K8s-->>CLI: Return ConfigMap with CA cert data
CLI->>CLI: Validate required data keys exist
CLI->>FS: Create output directory if needed
FS-->>CLI: Directory created/exists
CLI->>FS: Write client cert file (0600)
FS-->>CLI: Cert file written
CLI->>FS: Write client key file (0600)
FS-->>CLI: Key file written
CLI->>FS: Write CA cert file (0600)
FS-->>CLI: CA cert file written
CLI-->>User: Success logged with file paths and byte counts
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/lgtm Hold for other reviewers |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Scheduling tests matching the |
Test Resultse2e-aws
Failed TestsTotal failed tests: 3
e2e-aks
|
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws |
Adapt the etcd-backup command to work when the backup Job runs from the HyperShift Operator namespace. Update default cert paths to /etc/etcd-certs/ to align with the fetch-etcd-certs init container output introduced in openshift#8010. The command focuses on executing etcdctl snapshot save and writing the snapshot to disk. Add new manifest functions for running etcd backup Jobs from the HO namespace: - EtcdBackupJob: Job manifest with HCP labels - EtcdBackupJobServiceAccount: SA for the HO namespace - EtcdBackupJobRole: Role in HCP namespace for etcd TLS access - EtcdBackupJobRoleBinding: cross-namespace RoleBinding - EtcdBackupNetworkPolicy: temporary NetworkPolicy for etcd access Existing EtcdBackupCronJob and EtcdBackupServiceAccount are preserved for backward compatibility. JIRA: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…lows Add integration tests that validate the etcd backup process against a live management cluster. Two test scenarios are covered: - HO namespace flow: creates cross-namespace RBAC, NetworkPolicy, and a Job using fetch-etcd-certs (PR openshift#8010) + etcdctl snapshot + etcdutl verify as a mock of the future HCPEtcdBackup controller. - HCP namespace flow (legacy): creates a Job directly in the HCP namespace mounting etcd-client-tls and etcd-ca as volumes. Both tests verify snapshot integrity via etcdutl snapshot status. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
Adapt the etcd-backup subcommand to work when the backup Job runs from the HO namespace, aligning default cert paths with the fetch-etcd-certs subcommand (PR openshift#8010). Remove S3 upload logic (upload responsibility moves to a separate etcd-upload step). Add new manifest functions for HO namespace deployment: EtcdBackupJob, EtcdBackupJobServiceAccount, EtcdBackupJobRole, EtcdBackupJobRoleBinding, EtcdBackupNetworkPolicy. Preserve backward compatibility with existing EtcdBackupCronJob and EtcdBackupServiceAccount. Update vendor to remove unused AWS S3 transfermanager dependency. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…lows Add integration tests that validate the etcd backup process against a live management cluster. Two test scenarios are covered: - HO namespace flow: creates cross-namespace RBAC, NetworkPolicy, and a Job using fetch-etcd-certs (PR openshift#8010) + etcdctl snapshot + etcdutl verify as a mock of the future HCPEtcdBackup controller. - HCP namespace flow (legacy): creates a Job directly in the HCP namespace mounting etcd-client-tls and etcd-ca as volumes. Both tests verify snapshot integrity via etcdutl snapshot status. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
… manifests Update the etcd-backup subcommand default cert paths to align with the fetch-etcd-certs output directory (PR openshift#8010). Add new manifest functions for HO namespace deployment: EtcdBackupJob, EtcdBackupJobServiceAccount, EtcdBackupJobRole, EtcdBackupJobRoleBinding, EtcdBackupNetworkPolicy. Preserve backward compatibility with existing EtcdBackupCronJob and EtcdBackupServiceAccount. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
ea9836a to
0acaf88
Compare
| type certFile struct { | ||
| name string | ||
| data []byte | ||
| } |
| Short: "Fetch etcd TLS certificates from an HCP namespace and write them to disk", | ||
| SilenceUsage: true, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| ctx, cancel := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill, sdminonne The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@jparrill: This pull request references CNTRLPLANE-2678 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
etcd-backup/fetchcerts.go (1)
111-113: Use a tighter directory mode for cert/key storageLine 111 creates the cert directory as
0755. Since this directory stores private key material, prefer0700unless broader traversal is explicitly required.Proposed fix
- if err := os.MkdirAll(opts.outputDir, 0755); err != nil { + if err := os.MkdirAll(opts.outputDir, 0700); err != nil { return fmt.Errorf("failed to create output directory %s: %w", opts.outputDir, err) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etcd-backup/fetchcerts.go` around lines 111 - 113, The directory creation for certificate storage uses os.MkdirAll(opts.outputDir, 0755), which is too permissive for private keys; change the mode to 0700 to restrict access (update the os.MkdirAll call that references opts.outputDir in fetchcerts.go to use 0700) so only the owner can traverse/read the cert/key directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@etcd-backup/fetchcerts.go`:
- Around line 43-47: The current signal.NotifyContext yields a cancel-only
context that can allow API Get calls in runFetchCerts to hang indefinitely;
update the flow to apply a deadline (context.WithTimeout) so API operations time
out: either wrap the NotifyContext result with a reasonable timeout before
calling runFetchCerts (e.g., ctx, cancel := context.WithTimeout(ctx,
<duration>)) or, inside runFetchCerts, create per-API-call contexts with
timeouts when performing the client Get calls (the methods invoking Get) so each
network/API call uses a context with deadline and is properly cancelled;
reference runFetchCerts, signal.NotifyContext, and the API Get calls to locate
where to add context.WithTimeout and ensure all derived contexts are cancelled
via defer cancel().
---
Nitpick comments:
In `@etcd-backup/fetchcerts.go`:
- Around line 111-113: The directory creation for certificate storage uses
os.MkdirAll(opts.outputDir, 0755), which is too permissive for private keys;
change the mode to 0700 to restrict access (update the os.MkdirAll call that
references opts.outputDir in fetchcerts.go to use 0700) so only the owner can
traverse/read the cert/key directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e0deb307-ace2-46d8-87a0-5acd7463257c
📒 Files selected for processing (4)
Dockerfile.devcontrol-plane-operator/main.goetcd-backup/fetchcerts.goetcd-backup/fetchcerts_test.go
✅ Files skipped from review due to trivial changes (3)
- Dockerfile.dev
- control-plane-operator/main.go
- etcd-backup/fetchcerts_test.go
| ctx, cancel := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) | ||
| defer cancel() | ||
|
|
||
| return runFetchCerts(ctx, opts) | ||
| }, |
There was a problem hiding this comment.
Add a deadline to API operations to avoid hanging the init container
Line 43 creates a cancel-only context. If the API server/network stalls, the Get calls at Line 86 and Line 92 can block indefinitely and hold the backup Job.
Proposed fix
import (
"context"
"fmt"
"log"
"os"
"os/signal"
"path/filepath"
"syscall"
+ "time"
@@
RunE: func(cmd *cobra.Command, args []string) error {
ctx, cancel := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM)
defer cancel()
+ ctx, timeoutCancel := context.WithTimeout(ctx, 30*time.Second)
+ defer timeoutCancel()
return runFetchCerts(ctx, opts)
},
}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 86-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@etcd-backup/fetchcerts.go` around lines 43 - 47, The current
signal.NotifyContext yields a cancel-only context that can allow API Get calls
in runFetchCerts to hang indefinitely; update the flow to apply a deadline
(context.WithTimeout) so API operations time out: either wrap the NotifyContext
result with a reasonable timeout before calling runFetchCerts (e.g., ctx, cancel
:= context.WithTimeout(ctx, <duration>)) or, inside runFetchCerts, create
per-API-call contexts with timeouts when performing the client Get calls (the
methods invoking Get) so each network/API call uses a context with deadline and
is properly cancelled; reference runFetchCerts, signal.NotifyContext, and the
API Get calls to locate where to add context.WithTimeout and ensure all derived
contexts are cancelled via defer cancel().
|
/retest-required |
|
/retest |
|
I just did a simple verification against the running cluster: Everything looks ok. This will be part of the bigger feature which will be verified later. |
|
/verified by @mgencur |
openshift-ci-robot
added
the
verified
|
@mgencur: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
@jparrill: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
… manifests Update the etcd-backup subcommand default cert paths to align with the fetch-etcd-certs output directory (PR openshift#8010). Add new manifest functions for HO namespace deployment: EtcdBackupJob, EtcdBackupJobServiceAccount, EtcdBackupJobRole, EtcdBackupJobRoleBinding, EtcdBackupNetworkPolicy. Preserve backward compatibility with existing EtcdBackupCronJob and EtcdBackupServiceAccount. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…lows Add integration tests that validate the etcd backup process against a live management cluster. Two test scenarios are covered: - HO namespace flow: creates cross-namespace RBAC, NetworkPolicy, and a Job using fetch-etcd-certs (PR openshift#8010) + etcdctl snapshot + etcdutl verify as a mock of the future HCPEtcdBackup controller. - HCP namespace flow (legacy): creates a Job directly in the HCP namespace mounting etcd-client-tls and etcd-ca as volumes. Both tests verify snapshot integrity via etcdutl snapshot status. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…lows Add integration tests that validate the etcd backup process against a live management cluster. Two test scenarios are covered: - HO namespace flow: creates cross-namespace RBAC, NetworkPolicy, and a Job using fetch-etcd-certs (PR openshift#8010) + etcdctl snapshot + etcdutl verify as a mock of the future HCPEtcdBackup controller. - HCP namespace flow (legacy): creates a Job directly in the HCP namespace mounting etcd-client-tls and etcd-ca as volumes. Both tests verify snapshot integrity via etcdutl snapshot status. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…lows Add integration tests that validate the etcd backup process against a live management cluster. Two test scenarios are covered: - HO namespace flow: creates cross-namespace RBAC, NetworkPolicy, and a Job using fetch-etcd-certs (PR openshift#8010) + etcdctl snapshot + etcdutl verify as a mock of the future HCPEtcdBackup controller. - HCP namespace flow (legacy): creates a Job directly in the HCP namespace mounting etcd-client-tls and etcd-ca as volumes. Both tests verify snapshot integrity via etcdutl snapshot status. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
… manifests Update the etcd-backup subcommand default cert paths to align with the fetch-etcd-certs output directory (PR openshift#8010). Add new manifest functions for HO namespace deployment: EtcdBackupJob, EtcdBackupJobServiceAccount, EtcdBackupJobRole, EtcdBackupJobRoleBinding, EtcdBackupNetworkPolicy. Preserve backward compatibility with existing EtcdBackupCronJob and EtcdBackupServiceAccount. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…lows Add integration tests that validate the etcd backup process against a live management cluster. Two test scenarios are covered: - HO namespace flow: creates cross-namespace RBAC, NetworkPolicy, and a Job using fetch-etcd-certs (PR openshift#8010) + etcdctl snapshot + etcdutl verify as a mock of the future HCPEtcdBackup controller. - HCP namespace flow (legacy): creates a Job directly in the HCP namespace mounting etcd-client-tls and etcd-ca as volumes. Both tests verify snapshot integrity via etcdutl snapshot status. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
… manifests Update the etcd-backup subcommand default cert paths to align with the fetch-etcd-certs output directory (PR openshift#8010). Add new manifest functions for HO namespace deployment: EtcdBackupJob, EtcdBackupJobServiceAccount, EtcdBackupJobRole, EtcdBackupJobRoleBinding, EtcdBackupNetworkPolicy. Preserve backward compatibility with existing EtcdBackupCronJob and EtcdBackupServiceAccount. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…lows Add integration tests that validate the etcd backup process against a live management cluster. Two test scenarios are covered: - HO namespace flow: creates cross-namespace RBAC, NetworkPolicy, and a Job using fetch-etcd-certs (PR openshift#8010) + etcdctl snapshot + etcdutl verify as a mock of the future HCPEtcdBackup controller. - HCP namespace flow (legacy): creates a Job directly in the HCP namespace mounting etcd-client-tls and etcd-ca as volumes. Both tests verify snapshot integrity via etcdutl snapshot status. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
/jira backport release-4.21,release-4.20 |
|
@jparrill: Missing required branches for backport chain:
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
… manifests Update the etcd-backup subcommand default cert paths to align with the fetch-etcd-certs output directory (PR openshift#8010). Add new manifest functions for HO namespace deployment: EtcdBackupJob, EtcdBackupJobServiceAccount, EtcdBackupJobRole, EtcdBackupJobRoleBinding, EtcdBackupNetworkPolicy. Preserve backward compatibility with existing EtcdBackupCronJob and EtcdBackupServiceAccount. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…lows Add integration tests that validate the etcd backup process against a live management cluster. Two test scenarios are covered: - HO namespace flow: creates cross-namespace RBAC, NetworkPolicy, and a Job using fetch-etcd-certs (PR openshift#8010) + etcdctl snapshot + etcdutl verify as a mock of the future HCPEtcdBackup controller. - HCP namespace flow (legacy): creates a Job directly in the HCP namespace mounting etcd-client-tls and etcd-ca as volumes. Both tests verify snapshot integrity via etcdutl snapshot status. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
Adjust code from PRs openshift#8010, openshift#8017, openshift#8040 to compile on release-4.21: - etcd-upload: migrate S3TransferAPI from transfermanager v0.1 API (PutObject/PutObjectInput) to v0.2 API (UploadObject/UploadObjectInput) resolved by go mod tidy on this branch. Regenerate mock accordingly. - etcd-backup: fix mapToTags to use url.Values for proper URL-encoding of S3 object tag keys/values (bug exposed by backported tests). - go.mod/vendor: add aws-sdk-go-v2 direct dependencies (config, s3, transfermanager) and Azure azblob SDK required by etcd-upload, then run go mod tidy + go mod vendor. Ref: CNTRLPLANE-2678 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
5 participants
Summary
fetch-etcd-certssubcommand to the CPO binary that fetchesetcd-client-tlsSecret andetcd-caConfigMap from an HCP namespace and writes TLS certificates to diskcommandFor()(symlink dispatch) anddefaultCommand()(subcommand dispatch)Dockerfile.devfor dev image buildsThis is the OCP payload component of CNTRLPLANE-2678 (HCPEtcdBackup Controller). The full controller implementation (reconciler, Job construction, NetworkPolicy, retention) will follow in subsequent PRs. This is shipped first as it affects the CPO binary in the OCP release payload.
Related PRs
Jira
Test plan
opensslgo build ./etcd-backup/... ./control-plane-operator/...passesHow to test manually
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
fetch-etcd-certsCLI command (runnable from the control-plane-operator) to retrieve etcd TLS artifacts from a HostedControlPlane namespace and write them to disk with secure (0600) permissions; flags for HCP namespace, output directory, and credential resource names.Tests