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

apiext: rewrite to fix CA cert renewal and enhance capabilities #5494

Merged
merged 5 commits into from
Jan 3, 2024

Conversation

LanceEa
Copy link
Contributor

@LanceEa LanceEa commented Dec 29, 2023

Description

This PR looks to fix the long standing issue with CA Certs not auto-renewing and becoming expired thus no longer being able to convert CRD's until a new CA cert is re-created. It expands the capabilities to allow for more flexibility in allowing the CA Cert and CRD Patching to be externally managed by third parties such as CertManager. Also, leverages leader-election to ensure predictability when managed CA Cert and CRD Patching.

APIExt design

The new design is built on top of the controller-runtime using the familiar Runnable and Controller patterns. The following are the key abstractions created.

Entity Description Required Leader Election
CertificateAuthority Internal CA Cert cache that generates Server Certificates Yes No
caCertController Controller watching CA Cert Secret and providing CertificateAuthority the CACert Yes No
crdPatchController Controller patching getambassador.io CRDs to ensure CABundle matches CACert No Yes
CACertManager Runnable watching CA Cert Secret and ensuring it is always valid and non-expired No Yes

Note: crdPatchController and CACertManager can be disabled but the capabilities they provide are required so if disabled then it is up to the user to provide an alternative such as CertManager.

Now that we support LeaderElection the RBAC has been expanded to include the new permissions needed.

One other thing to mention, is that this PR laid the ground work for publishing the apiext as a stand alone container by decoupling its binary from busy ambassador. However, for now the standalone container is only used to simplify the e2e testing (mainly due to keeping scope down, in an already large PR). The standalone binary is still copied into the core container and the deployment in the charts/manifest are still the same.

Related Issues

CA Cert Renewal and support for externally managing CA Cert and CRD Patching.

Testing

New unit tests for rewrite and added automated e2e tests to CI. Pulled into Edge Stack and verified no issues there as well.

Checklist

  • Does my change need to be backported to a previous release?
  • I made sure to update CHANGELOG.md.
  • This is unlikely to impact how Ambassador performs at scale.
  • My change is adequately tested.
  • I updated DEVELOPING.md with any any special dev tricks I had to use to work on this code efficiently.
  • The changes in this PR have been reviewed for security concerns and adherence to security best practices.

Signed-off-by: Lance Austin <laustin@datawire.io>
Lance Austin added 2 commits January 3, 2024 09:33
This is a complete re-write of the apiext internals for managing
and wathcing CA Certs, Patch CRDs and providing a ConversionWebHook
Service for converting the getambassador.io resources.

- Decouples apiext binary from busyambassador binary to make e2e test
simpler and lay framework for extracting from core container (future work)
- Add leader election support to provide predictability when managing CA
and CRD Patching
- New Controller for Patching CRDs with CA bundle
- New Controler for Watching CA Cert
- New CertificateAuthority abstraction for generating Server certs on
conversion webhook requests and cache invalidation on CA Cert changes
- New Controller for Managing CA Cert, creates, updates and auto-renews
when about to expire
- Add ability to run in external managed mode (aka turn off CA and CRD management)
and let external tool like CertManager manage the certs

Signed-off-by: Lance Austin <laustin@datawire.io>
Extends the fix-crds tool so that it can output standalone files for
RBAC, Deployments and CRD's so that we can make easier and
more predicatable e2e testing.

Generally, speaking we should re-think fix-crds and whether it makes
sense but for now this adjusts it to meet the needs for e2e testing
the new apiext server.

- removed uncessary comments that don't play nice with e2e-framework
- added port and path to default Conversion data structure to support
externally Managed mode. Note: when apiext server is patching CRD's it
will override it like it did previously.

Signed-off-by: Lance Austin <laustin@datawire.io>
@LanceEa LanceEa force-pushed the laustin/apiext-renew-cert branch 8 times, most recently from 0f00e7a to 8d22e63 Compare January 3, 2024 19:28
This adds basic e2e tests for the apiext to ensure that it can properly
create, watch and renew expired Certs.

An additional test and example was added on how to run it in an externally
managed mode with CertManager providing the Certificate and Patching
CRD's. Note:  this has limited support because it has only been tested against
the same settings (RSA, PKCS8) as self-managed mode. It may work with
other settings but those are not guaranteed at this time.

A new standalone container is generated locally and pushed to k3d. This
is only for e2e testing but sets up the framework for having it standalone
in the future.

Signed-off-by: Lance Austin <laustin@datawire.io>
@LanceEa LanceEa changed the title WIP: apiext re-work apiext: rewrite to fix CA cert renewal and enhance capabilities Jan 3, 2024
Copy link
Member

@AliceProxy AliceProxy left a comment

Choose a reason for hiding this comment

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

couple questions and a few nits that are pretty low priority, otherwise lgtm

pkg/apiext/internal/ca/authority_test.go Outdated Show resolved Hide resolved
pkg/apiext/internal/controller/cacert/controller.go Outdated Show resolved Hide resolved
test/apiext/testdata/cert.yaml Outdated Show resolved Hide resolved
test/internal/e2e/e2eutils.go Show resolved Hide resolved
Signed-off-by: Lance Austin <laustin@datawire.io>
@LanceEa LanceEa merged commit 14acd8e into master Jan 3, 2024
36 of 38 checks passed
@LanceEa LanceEa deleted the laustin/apiext-renew-cert branch January 3, 2024 22:27
@joshbranham
Copy link
Contributor

Are there plans to cut a release soon with this fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants