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

fix(kuma-cp) use EnvoyGRPC to fix DNS resolving #1740

Merged
merged 2 commits into from
Apr 2, 2021

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

Use EnvoyGRPC instead of GoogleGRPC for XDS so it fixes #1620
The cost of this fix is that now the dataplane token is visible in config dump. It's not that bad since we don't expose config_dump ourselves.
You need to explicitly ssh into host to fetch it and Envoy admin binds only to localhost.

We may switch back to GoogleGRPC once the bug with DNS resolving is fixed.

Full changelog

  • Pass Dataplane Token instead of Dataplane Token Path for Bootstrap request. This way we detect that we should use EnvoyGRPC. If dataplane token path is send then it means we still use GoogleGRPC.
    This has to be done for backwards compatibility.
  • Pass CA to BootstrapRequest that was passed to the kuma-dp. Previously we were just using cert of dp server and it somehow worked for GoogleGRPC whether it was CA or not.
    EnvoyGRPC requires real CA, therefore we use CA (trusted_ca.inline_bytes) that was passed to kuma-dp run if possible. If not we are trying to use DP server cert which is most likely selfsigned cert (is CA).
    If it is not a CA we throw an error.

Issues resolved

Fix #1620

Documentation

  • No docs, internal changes

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner March 31, 2021 16:09
@subnetmarco
Copy link
Contributor

This is okay because the config dump cannot be performed without SSH access to the machine, correct? @jakubdyszkiewicz

func isNotCA(cert []byte) (bool, error) {
pemCert, _ := pem.Decode(cert)
if pemCert == nil {
return false, errors.New("could not parse certificate")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think errors could not parse certificate should be better wrapped somewhere up the stack trace. If these are validation errors then I believe it makes sense to mention the field .CaCert

@@ -307,14 +348,27 @@ func (s SANSet) slice() []string {
return sans
}

func isNotCA(cert []byte) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd better change it to

func validateCA(cert []byte) error {
...
}

and if it's not CA then return NotCA straight away.

It will simplify the usage of this method:

func (b *bootstrapGenerator) validateCaCert(cert []byte, request types.BootstrapRequest) error {
	if request.DataplaneTokenPath != "" {
		// when using GoogleGRPC it is valid to put non-CA certificate therefore we should only verify this for EnvoyGRPC
		// EnvoyGRPC is used when DataplaneToken is passed via request as inline value, not as a file.
		return nil
	}

	return validateCA(cert)
}

And probably, in that case, we don't need a separate validateCA method, I guess its content can be inlined

@jakubdyszkiewicz
Copy link
Contributor Author

This is okay because the config dump cannot be performed without SSH access to the machine, correct? @jakubdyszkiewicz

@subnetmarco correct

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz merged commit 48cac88 into master Apr 2, 2021
@jakubdyszkiewicz jakubdyszkiewicz deleted the fix/use-envoy-grpc branch April 2, 2021 07:45
mergify bot pushed a commit that referenced this pull request Apr 2, 2021
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
(cherry picked from commit 48cac88)
jakubdyszkiewicz pushed a commit that referenced this pull request Apr 6, 2021
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
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.

Dataplane doesn't reconnect to changing control plane IP
4 participants