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.

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

Enable IPsec #886

Merged
merged 4 commits into from Dec 4, 2020
Merged

Enable IPsec #886

merged 4 commits into from Dec 4, 2020

Conversation

markdgray
Copy link
Contributor

@markdgray markdgray commented Nov 20, 2020

This PR enables IPsec for the Cluster Network Operator. There are a number of components to this:

  • Re-vendor in order to pull in the latest changes to openshift/api which include updates to the CNO crd.
  • Addition of a CertificateSigningRequest controller that watches for CertificateSigningRequest resources and signs them using a CA key pair that has been created by the OperatorPKI.
  • Modification of the ovnkube-master and ovs-nodes manifests and creation of a new ovn-ipsec Daemonset that generate necessary keys and configure OVN and OVS to enable IPsec across the network. This change can only be made at cluster installation time.

This PR is dependent on a number changes in other projects that have not been merged:

This PR has been minimally tested with IPsec enabled but will require the kernel patch for full testing by the CI (which may throw up some issues). However, this should pass the current CI as IPsec is disabled by default and cannot be enabled after cluster initialization.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2020
@markdgray
Copy link
Contributor Author

@squeed @rcarrillocruz @trozet @alexanderConstantinescu @danwinship @abhat : @dcbw suggested adding you for review

@markdgray markdgray changed the title [WIP] Enable IPsec Enable IPsec Nov 21, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2020
@abhat
Copy link
Contributor

abhat commented Nov 23, 2020

As per https://github.com/openshift/api/pull/783/files#r528769454, consider the Enable flag based on the IPsecConfig being present. If the IPsecConfig is empty struct, it can be interpreted as the feature is disabled.

enableIPsec.sh Outdated Show resolved Hide resolved
@squeed
Copy link
Contributor

squeed commented Nov 23, 2020

/hold
because this is still a work in progress.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2020
@squeed
Copy link
Contributor

squeed commented Nov 23, 2020

How do we handle certificate rotation? What happens if a node misses its rotation deadline?

@markdgray
Copy link
Contributor Author

How do we handle certificate rotation? What happens if a node misses its rotation deadline?

We don't. I think we will need this but we can't use the cert rotation controller for this. I also don't think this was specified as part of the enhancement proposal. I could make the certificates have a long lifespan initially? Restarting the container will, of course, recreate and resign but as you mentioned, that Daemonset will be going away.

@markdgray
Copy link
Contributor Author

@squeed Thanks for the quick review!

@markdgray
Copy link
Contributor Author

As per https://github.com/openshift/api/pull/783/files#r528769454, consider the Enable flag based on the IPsecConfig being present. If the IPsecConfig is empty struct, it can be interpreted as the feature is disabled.

@abhat openshift/api#794 I will update this PR when/if this gets accepted

@markdgray
Copy link
Contributor Author

@squeed @abhat Updated as per your comments. Any further issues?

@squeed
Copy link
Contributor

squeed commented Dec 3, 2020

Handling all controller states is tricky - and it doesn't need to be perfect, since this isn't a user-facing API. In other words, we don't really need to worry about every possible invalid state. But we should handle the case where we find an approved, unsigned certificate.

In order to keep private keys private to nodes, this patch
introduces a basic CertificateSigningRequest controller to CNO. This
allows a client to generate a cert/private key pair locally
and request for the certificate to be signed by a CNO CA.

This signer used the OperatorPKI to generate a new CA (signer-ca)
that is rotated.

Signed-off-by: Mark Gray <mark.d.gray@redhat.com>
This patch introduces IPsec enablement for the Cluster Network
Operator using the OVN IPsec functionality.

A new Daemonset is created that hosts libreswan and ovs-monitor-ipsec
which watch for changes in OVN/OVS configuration and update the
Linux XFRM framework appropriately.

This patch also modifies the ovnkube-master Daemonset to enable IPsec across
the cluster. This is done by writing a configuration option to the NB DB.

The patch also modifies the the ovs-node Daemonset to generate
a key pair on initialization, and request for that keypair to be
signed by the signer-ca.

IPsec should only be configurable at cluster installation time.

Signed-off-by: Mark Gray <mark.d.gray@redhat.com>
@markdgray
Copy link
Contributor Author

/retest

1 similar comment
@markdgray
Copy link
Contributor Author

/retest

@markdgray
Copy link
Contributor Author

Handling all controller states is tricky - and it doesn't need to be perfect, since this isn't a user-facing API. In other words, we don't really need to worry about every possible invalid state. But we should handle the case where we find an approved, unsigned certificate.

Also done

@markdgray
Copy link
Contributor Author

As per https://github.com/openshift/api/pull/783/files#r528769454, consider the Enable flag based on the IPsecConfig being present. If the IPsecConfig is empty struct, it can be interpreted as the feature is disabled.

Done

@dcbw dcbw removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2020
@dcbw
Copy link
Member

dcbw commented Dec 3, 2020

/test e2e-aws-ovn-windows
msg=Error: Error creating VPC: VpcLimitExceeded: The maximum number of VPCs has been reached.

@markdgray
Copy link
Contributor Author

/test e2e-aws-ovn-windows

@dcbw
Copy link
Member

dcbw commented Dec 3, 2020

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, markdgray

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@markdgray
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@dcbw
Copy link
Member

dcbw commented Dec 4, 2020

/override ci/prow/e2e-ovn-ipsec-step-registry
not supposed to be required yet openshift/release#14068

@openshift-ci-robot
Copy link
Contributor

@dcbw: Overrode contexts on behalf of dcbw: ci/prow/e2e-ovn-ipsec-step-registry

In response to this:

/override ci/prow/e2e-ovn-ipsec-step-registry
not supposed to be required yet openshift/release#14068

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.

@dcbw
Copy link
Member

dcbw commented Dec 4, 2020

/test e2e-openstack-ovn

@dcbw
Copy link
Member

dcbw commented Dec 4, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

@markdgray: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-openstack-ovn f8a0217 link /test e2e-openstack-ovn

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 8fcfb7f into openshift:master Dec 4, 2020
@markdgray markdgray deleted the ipsec branch February 9, 2021 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants