feat(kuma-cp) refactor SDS to go-control-plane building blocks #721
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lobkovilya
requested changes
May 7, 2020
|
|
||
| parts := strings.Split(currentSnapshot.GetVersion(envoy_resource.SecretType), "-") | ||
| if len(parts) != 2 { | ||
| return false, errors.New(`invalid snapshot version format. Format should be "UnixNano-NameOfTheCA"`) |
There was a problem hiding this comment.
Is there any way cache contains snapshots with invalid version format? From what I see we generate it ourselves in generateSnapshot. That might simplify signature of shouldGenerateSnapshot and get rid of error in return values
There was a problem hiding this comment.
Not really, unless someone connects to the control plane and explicitly set the version. Or we change it in the future and it won't be compatible with current format.
But let's say it should not happen. What is the alternative? Panic or ignore the error?
There was a problem hiding this comment.
If we want to protect ourselves from our future changes in the format, then it feels more like panic. But that's really minor comment, so up to you :)
nickolaev
reviewed
May 7, 2020
jakubdyszkiewicz
force-pushed
the
sds-cache
branch
from
02dd9ef
to
ed50690
Compare
lobkovilya
approved these changes
May 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors SDS to use the same patterns as our XDS implementation.
Until now, SDS was custom implementation operating on raw Envoy protos.
The problem with this implementation is that
DP has 1000 outbound listeners. Since it is mTLS, we have to configure every outbound listener with DP cert and CA cert. Every DP will open 2000 streams (one for CA, one for DP cert) and send a request on every stream. This will generate 1000 certs for one DP instead of one.
Given that there can be thousands of such dataplanes and CA can be external system, we can overload the system with generating millions of certs.
In this implementation we use SnapshotCache from go-control-plane which will store the same cert for subsequent requests, so we only generate 1 certificate per Dataplane.
This will also helps us to solve #719