Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OADP-639: Pass custom CA configuration to restic replication source. #198

Merged
merged 6 commits into from Apr 11, 2023
Merged

OADP-639: Pass custom CA configuration to restic replication source. #198

merged 6 commits into from Apr 11, 2023

Conversation

mrnold
Copy link
Contributor

@mrnold mrnold commented Mar 17, 2023

For Add restic custom CA fields from the secret generated in openshift/oadp-operator#918.

Draft pull request to make sure this is the right way to do this, still needs changes for plugin and replication destination.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Mar 17, 2023 •
edited by openshift-ci bot

@mrnold: This pull request references OADP-639 which is a valid jira issue.

In response to this:

For Add restic custom CA fields from the secret generated in openshift/oadp-operator#918.

Draft pull request to make sure this is the right way to do this, still needs changes for plugin and replication destination.

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 kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 17, 2023
@openshift-ci
Copy link

openshift-ci bot commented Mar 17, 2023

Hi @mrnold. Thanks for your PR.

I'm waiting for a migtools member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

@kaovilai
Copy link
Member

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 17, 2023
This was referenced Mar 21, 2023
Closed
Merged
@mrnold
Add optional VSB field for restic custom CA.
7fb93e3 
Signed-off-by: Matthew Arnold <marnold@redhat.com>
@mrnold
Add optional VSR field for restic custom CA.
c40b62b 
Signed-off-by: Matthew Arnold <marnold@redhat.com>
@mrnold
Put custom CA in existing restic secret.
4a5ac9b 
Signed-off-by: Matthew Arnold <marnold@redhat.com>
@mrnold
Include custom CA in final restic secret.
968ebc2 
Signed-off-by: Matthew Arnold <marnold@redhat.com>
@mrnold mrnold marked this pull request as ready for review April 5, 2023 04:50
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2023
@openshift-ci openshift-ci bot requested review from kaovilai and sseago April 5, 2023 04:50
@mrnold
Copy link
Contributor Author

mrnold commented Apr 5, 2023

This should be ready for review now, along with openshift/oadp-operator#918.

kaovilai
kaovilai reviewed Apr 5, 2023
View reviewed changes
controllers/replicationdestination.go Outdated
@@ -347,6 +347,9 @@ func (r *VolumeSnapshotRestoreReconciler) configureRepDestResticVolOptions(vsr *
}
}

repDestResticVolOptions.CustomCA.Key = "RESTIC_CUSTOM_CA"
repDestResticVolOptions.CustomCA.SecretName = resticSecretName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we shouldn't be setting this if customca isn't defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did more testing and volsync does not work when the secret doesn't actually have this set. I will make this open the secret and check first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that's working better now.

@mrnold
Don't include empty custom CA references.
2f12550 
Signed-off-by: Matthew Arnold <marnold@redhat.com>
eemcmullan
eemcmullan reviewed Apr 6, 2023
View reviewed changes
controllers/replicationdestination.go
@@ -120,6 +120,13 @@ func (r *VolumeSnapshotRestoreReconciler) buildReplicationDestination(replicatio
replicationDestination.Spec = replicationDestinationSpec
}

// include custom CA if specified
resticCustomCA := resticSecret.Data[ResticCustomCA]
if len(resticCustomCA) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test case here? https://github.com/migtools/volume-snapshot-mover/blob/master/controllers/replicationdestination_test.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing tests weren't really set up to check this sort of thing, so I tried adding a field to the test input struct to hold a validation function. I didn't see an easier way to do it.

controllers/replicationsource.go
@@ -138,6 +138,13 @@ func (r *VolumeSnapshotBackupReconciler) buildReplicationSource(replicationSourc
replicationSource.Spec = replicationSourceSpec
}

// pass along a custom CA if specified
resticCustomCA := resticSecret.Data[ResticCustomCA]
if len(resticCustomCA) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here as well: https://github.com/migtools/volume-snapshot-mover/blob/master/controllers/replicationsource_test.go

@mrnold
Add tests for custom CA passthrough.
9ac7511 
Signed-off-by: Matthew Arnold <marnold@redhat.com>
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 7, 2023
@openshift-ci
Copy link

openshift-ci bot commented Apr 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eemcmullan, mrnold, shubham-pampattiwar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [eemcmullan,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@shubham-pampattiwar shubham-pampattiwar merged commit b1eedc1 into migtools:master Apr 11, 2023
8 of 9 checks passed
mrnold added a commit to mrnold/volume-snapshot-mover that referenced this pull request Apr 12, 2023
@mrnold
OADP-639: Pass custom CA configuration to restic replication source. (m…
609477a 
…igtools#198)

Signed-off-by: Matthew Arnold <marnold@redhat.com>
@mrnold mrnold mentioned this pull request Apr 12, 2023
Merged
openshift-merge-robot pushed a commit that referenced this pull request Apr 12, 2023
@mrnold
Cherry-pick #198: Pass custom CA configuration to restic replication …
07ede3d 
…source. (#203)

Signed-off-by: Matthew Arnold <marnold@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaovilai kaovilai kaovilai left review comments

@shubham-pampattiwar shubham-pampattiwar shubham-pampattiwar approved these changes

@eemcmullan eemcmullan eemcmullan approved these changes

@sseago sseago Awaiting requested review from sseago

Assignees

@shubham-pampattiwar shubham-pampattiwar

@eemcmullan eemcmullan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mrnold @openshift-ci-robot @kaovilai @shubham-pampattiwar @eemcmullan