uswitch / yggdrasil

Envoy Control Plane for Kubernetes Multi-cluster Ingress
Apache License 2.0
192 stars 17 forks source link

Add TLS certificates synchronization from ingresses' secrets #68

Closed Aluxima closed 1 year ago

Aluxima commented 1 year ago

This feature allows yggdrasil to dynamically synchronize downstream TLS certificates from Kubernetes secrets configured under ingresses' spec.tls by setting syncSecrets true in Yggdrasil configuration (false by default).

This has been running fine in production for over 3 months at Numberly.

No breaking change as this mode is disabled by default.

Closes https://github.com/uswitch/yggdrasil/issues/54

Thanks in advance for the review!

DewaldV commented 1 year ago

Hi @Aluxima this looks like a very cool feature. We've not had a need for it internally but it's definitely a great addition for the wider community!

Glad to hear you've been running it successfully for the last three months as well, that provides a lot of good confidence that it's solid.

I'll take some time to review this later this week so I can get a feel for the changes.

Once again, thanks for the contribution :smile:

Aluxima commented 1 year ago

Hello @DewaldV, I hope you're doing well.

Do you have any feedback regarding this PR?

I have other contributions waiting for this one to be merged, like envoy 1.24 support, upstream and downstream http2 support, circuit breaker configuration tuning

I noticed some of the work is being done for envoy 1.24 support in #72 and #73, it would be nice not to do the same work twice :smile:

DewaldV commented 1 year ago

Hi @Aluxima. Apologies for the delays on this, we've actually just started to pick up Yggdrasil changes in the new year.

We ran into a bug in production where some uploads with sizes > 40MB or so were getting interrupted and traced it to an old Envoy version so we've just done some work on our side to get Envoy 1.24 support done too. Apologies for the double work there!

I'd be keen to bring in your changes as we haven't swapped over to anypb.New or bumped the Control plane version yet so those are good changes to get in.

Let me get back on this as we've been a bit behind on getting Yggdrasil up to date!

DewaldV commented 1 year ago

@Aluxima I pulled your branch down and did a cursory rebase and there's quite a bit to work through but it all looks well contained. We'll have to sort through the merge conflicts but otherwise it looks great. :+1:

Are you able to rebase the change on latest master?

Alternatively I can give it a go but I'll have to push to a branch on our repo and re-open the PR and I don't want to snipe any kudo's for this excellent new feature. :smile:

DewaldV commented 1 year ago

Alternatively-alternatively we could merge in your Envoy 1.24 branch (and to fix a few differences in error handling) and you could then base this one off of that once merged?

That way we can get both of those in in sequence, first 1.24 then TLS changes given our 1.24 support changes are very nearly identical to yours.

What do you think, how would you like to play this? :smiley:

Aluxima commented 1 year ago

@DewaldV Thanks for the review. I'll try preparing the PR for 1.24 support this week and then I'll fix the conflicts here based on that, as suggested.

I'll keep you posted!

Aluxima commented 1 year ago

Hi @DewaldV So it's been a very busy week haha

I just posted a PR for envoy 1.26 support: https://github.com/uswitch/yggdrasil/pull/74 This will unlock the other next PRs to come with all the features we've implemented at Numberly.

DewaldV commented 1 year ago

Aces @Aluxima! I've merged #74, thanks so much!

Aluxima commented 1 year ago

Here we go, ready to be merged again

Solvik commented 1 year ago

thanks @DewaldV !

we'll open the remaining MR now (weight, http2 upstream/downstream, a fix for Host with port, Circuit Breaker Limit tuning and better timeout tuning, etc)

DewaldV commented 1 year ago

Thanks @Solvik :smile:

Looking forward to it!