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

Cert config option #4069

Merged
merged 6 commits into from Sep 23, 2020
Merged

Cert config option #4069

merged 6 commits into from Sep 23, 2020

Conversation

cchengleo
Copy link
Contributor

What this PR does / why we need it:
This change is the first part of resolving #3648

  • Add flags (virt-handler and virt-api) to use different ConfigMap containing CA certificates, so that users can specify their own root CA.
  • Add flags (virt-handler,virt-controller and virt-api) to allow users to configure the certificate and key file paths.
  • Add flags (virt-handler and virt-api) to allow client's intermediate certs to be used in building up the chain of trust in cert validation.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3648

Special notes for your reviewer:

Release note:

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 27, 2020
@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 27, 2020
@openshift-ci-robot
Copy link
Collaborator

Hi @cchengleo. Thanks for your PR.

I'm waiting for a kubevirt 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.

@kubevirt-bot
Copy link
Contributor

Hi @cchengleo. Thanks for your PR.

I'm waiting for a kubevirt 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.

@mlsorensen
Copy link
Contributor

/ok-to-test

@kubevirt-bot kubevirt-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 28, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 28, 2020
@cchengleo
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-cnao-1.17

@openshift-ci-robot
Copy link
Collaborator

@cchengleo: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test images

Use /test all to run all jobs.

In response to this:

/test pull-kubevirt-e2e-k8s-cnao-1.17

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.

@cchengleo
Copy link
Contributor Author

/retest

@cchengleo cchengleo force-pushed the cert-config-option branch 4 times, most recently from c2b971e to 790d021 Compare September 3, 2020 20:25
Cheng Cheng added 4 commits September 3, 2020 15:54
@cchengleo
Add options to allow users to configure certificate and key file path…
069770a 
…s for virt-handler,

virt-controller and virt-api to accommodate varying rules around certificate
validation.

Signed-off-by: Cheng Cheng <chengcheng@apple.com>
@cchengleo
Add option to allow client's intermediate certs to be used in buildin…
83ea787 
…g up the chain of trust in cert validation for virt-handler and virt-api

Signed-off-by: Cheng Cheng <chengcheng@apple.com>
@cchengleo
Add unit test for cert-manager
ca4e299 
When cert and key file are in different directories, cert-manager should still load the certificate.

Signed-off-by: Cheng Cheng <chengcheng@apple.com>
@cchengleo
Rename option --allow-intermediate-certificates to --externally-name
f88c194 
If the certificates are externally managed, setting this flag will allow
ntermediate certificates to be used in building up the chain of trust in
client certificate validation. In addition, verification of hardcoded CN will not be
skipped.

Signed-off-by: Cheng Cheng <chengcheng@apple.com>
dhiller
dhiller requested changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR! Looks good already in general, some minor things I still have though.

pkg/certificates/bootstrap/cert-manager.go Outdated Show resolved Hide resolved
pkg/certificates/bootstrap/cert-manager.go Show resolved Hide resolved
pkg/util/webhooks/tls.go Outdated Show resolved Hide resolved
pkg/util/webhooks/tls_test.go Show resolved Hide resolved
@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Sep 9, 2020
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Sep 9, 2020
@cchengleo
Fix validation for self-signed cert and addressed comments
00a2f51 
Signed-off-by: Cheng Cheng <chengcheng@apple.com>
@cchengleo
Copy link
Contributor Author

/retest

dhiller
dhiller requested changes Sep 10, 2020
View reviewed changes
Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

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

Hey, two quick suggestions.

pkg/certificates/bootstrap/cert-manager_test.go Show resolved Hide resolved
pkg/util/webhooks/tls_test.go Show resolved Hide resolved
pkg/util/webhooks/tls_test.go Show resolved Hide resolved
pkg/util/webhooks/tls.go Outdated Show resolved Hide resolved
@cchengleo
Addressed comments
d5f7b95 
- Add unit-test for verifing externally-managed certificate TLS setup
- Refactor to remove duplicated blocks.

Signed-off-by: Cheng Cheng <chengcheng@apple.com>
dhiller
dhiller approved these changes Sep 14, 2020
View reviewed changes
Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

/lgtm

Note: I noticed that there is no functest added, I guess that that will be very much work regarding certificate generation and setup, no?

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020
@cchengleo
Copy link
Contributor Author

Thanks for the PR!

/lgtm

Note: I noticed that there is no functest added, I guess that that will be very much work regarding certificate generation and setup, no?

Thanks for the review! I would add more functions tests in the follow-up PR. Generating root certs and mint intermediate certs do take some effort.

@cchengleo
Copy link
Contributor Author

/assign @AlonaKaplan

@rmohr
Copy link
Member

rmohr commented Sep 22, 2020

/approve

Very nice PR. Looking forward to the next PR which will add functional tests.

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2020
@cchengleo
Copy link
Contributor Author

/approve

Very nice PR. Looking forward to the next PR which will add functional tests.

Thanks for the review. I'll add functional tests in the following up PR.

@cchengleo
Copy link
Contributor Author

/unassign @AlonaKaplan

@kubevirt-bot kubevirt-bot merged commit 177364a into kubevirt:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlonaKaplan AlonaKaplan Awaiting requested review from AlonaKaplan

@dhiller dhiller Awaiting requested review from dhiller

Assignees

@dhiller dhiller

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. 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. release-note-none Denotes a PR that doesn't merit a release note. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Externally managed certificates for KubeVirt components
7 participants
@cchengleo @openshift-ci-robot @kubevirt-bot @mlsorensen @rmohr @dhiller @AlonaKaplan