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

Multiport #1012

Merged
merged 32 commits into from
Feb 22, 2022
Merged

Multiport #1012

merged 32 commits into from
Feb 22, 2022

Conversation

ndhanushkodi
Copy link
Contributor

@ndhanushkodi ndhanushkodi commented Feb 1, 2022

Based off of research and POC by @lkysow #754 Thank you!

Note to reviewers:
I recommend reading the acceptance test first, then looking at the implementation. In the acceptance tests, I've changed the signature for CheckStaticServerConnection* functions, so there's a lot of 1 line changes all over the acceptance tests due to that.

Changes proposed in this PR:

Support a workaround for multi port pods by registering a Consul service per port, and by injecting init containers and envoy sidecars per port.

  • Acceptance test for multiport

    • The multiport app is just http-echo listening on different ports in 2 containers
    • First, the test deploys static-client and multiport, with static-client having multiport and multiport-admin as upstreams. It checks that static-client can make connections to each service-- multiport, and multiport-admin. This is to test inbound connections for a multiport app.
    • Then the test deploys static-server, and checks that multiport can make a connection to static-server. This is to test outbound connections from a multiport app. Note that there is only 1 intention because all upstream connections are configured and go through the 1st service's envoy proxy.
  • Implementation for multiport

    • handler
      • uses the service name annotation to figure out if this is a multiport pod or not
      • inject a connect-init container and an envoy-sidecar container per port/service
      • mount all service account tokens not already on the pod. For any service whose service account is not already attached to the pod, this mounts a volume for that service account token. In container-init, these volumes are added as a container volumeMount so the service account token file can be passed to the connect-init command via -bearer-token-file
    • container_init -- update the template to pass in arguments specific to the multiport service
    • envoy_sidecar -- allow the bootstrap file path to have the service name in it for multiport services, and add --base-id since there could be multiple envoy sidecars
    • endpoints_controller: the listener port should be 20000 for the first service, 20001 for the second, etc.
    • connect_init command: when its a multiport app, add a filter to poll by service name, use the service specific bearertoken, acl-token-sink, and proxyid files

How I've tested this PR:
acceptance tests and unit tests

How I expect reviewers to test this PR:
see note above, and code review.

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

Copy link
Contributor Author

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Adding some comments for reviewers since theres a lot of files

@@ -16,8 +16,6 @@ import (
"k8s.io/apimachinery/pkg/util/yaml"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the function signature for CheckStaticServer* to allow passing in an app which is the app you're connecting from, and an expectedSuccessOutput which is the expected message back from the server. I needed to be able to make connections from the multiport app and not just static client, and each of the multiport services had a different success output.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add this note to a comment for this function(s)🙏

@@ -19,6 +19,7 @@ import (

const staticClientName = "static-client"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is function signature changes

@@ -177,9 +177,9 @@ func TestConnectInjectNamespaces(t *testing.T) {
if c.secure {
logger.Log(t, "checking that the connection is not successful because there's no intention")
if cfg.EnableTransparentProxy {
k8s.CheckStaticServerConnectionFailing(t, staticClientOpts, fmt.Sprintf("http://static-server.%s", staticServerNamespace))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

function signature changes

k8s.CheckStaticServerConnectionSuccessful(t, ctx.KubectlOptions(t), staticClientName, "http://localhost:1234")
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds the multiport acceptance test

@@ -0,0 +1,2 @@
FROM kschoche/http-echo:latest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds curl to the http-echo image, I plan to swap out this for the actual http-echo image, so this can be ignored for now in review-- the only change would be to delete this file and swap the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swapped and deleted

@@ -415,11 +415,11 @@ func TestPartitions(t *testing.T) {
if c.ACLsAndAutoEncryptEnabled {
logger.Log(t, "checking that the connection is not successful because there's no intention")
if cfg.EnableTransparentProxy {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function signature changes

@@ -121,7 +121,7 @@ func TestTerminatingGatewaySingleNamespace(t *testing.T) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function signature changes

@@ -93,7 +93,7 @@ func TestTerminatingGateway(t *testing.T) {

// Test that we can make a call to the terminating gateway.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function signature changes

@@ -15,7 +15,8 @@ import (
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function signature changes

})
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept this TestReconcileCreateEndpoint_MultiportService test separate from the existing create endpoint test because of the changes to the case structure that wouldn't be needed by the non-multiport tests. Also because this is a workaround, it might be nice to keep the code as separate as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really appreciate the comments that you are leaving in the PR!

Copy link
Contributor

Choose a reason for hiding this comment

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

+1! Love all the comments!

@ndhanushkodi ndhanushkodi requested review from a team, curtbushko and ishustava and removed request for a team February 11, 2022 22:48
@ndhanushkodi ndhanushkodi marked this pull request as ready for review February 11, 2022 22:48
@@ -90,19 +88,24 @@ func CheckStaticServerConnection(t *testing.T, options *k8s.KubectlOptions, expe
// If expectSuccess is true, it will expect connection to succeed,
// otherwise it will expect failure due to intentions. If multiple failureMessages are provided it will assert
// on the existence of any of them.
func CheckStaticServerConnectionMultipleFailureMessages(t *testing.T, options *k8s.KubectlOptions, expectSuccess bool, failureMessages []string, curlArgs ...string) {
func CheckStaticServerConnectionMultipleFailureMessages(t *testing.T, options *k8s.KubectlOptions, app string, expectSuccess bool, failureMessages []string, expectedSuccessOutput string, curlArgs ...string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

RE: expectedSuccessOutput. I am not 100% sure if you need to change the function signature here as every call to CheckStaticServerConnectionMultipleFailureMessages is setting "" and using the default. Maybe it is good for future proofing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nearly every call is setting "" and using the default, except for CheckStaticServerConnectionSuccessfulWithMessage, which is what we're using in the multiport test

})
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I really appreciate the comments that you are leaving in the PR!

control-plane/connect-inject/envoy_sidecar.go Outdated Show resolved Hide resolved
control-plane/connect-inject/envoy_sidecar.go Show resolved Hide resolved
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

This looks fantastic!!! Thanks you so much for comments throughout the code and the PR - this made it so easy to review ❤️

I left some comments and questions inline, mostly to clarify my own understanding and correct some minor things.

Great work on this!!

acceptance/framework/k8s/deploy.go Outdated Show resolved Hide resolved
acceptance/framework/k8s/deploy.go Outdated Show resolved Hide resolved
@@ -16,8 +16,6 @@ import (
"k8s.io/apimachinery/pkg/util/yaml"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add this note to a comment for this function(s)🙏

acceptance/tests/connect/connect_inject_test.go Outdated Show resolved Hide resolved
control-plane/connect-inject/container_init.go Outdated Show resolved Hide resolved
control-plane/connect-inject/container_init.go Outdated Show resolved Hide resolved
})
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

+1! Love all the comments!

control-plane/connect-inject/endpoints_controller_test.go Outdated Show resolved Hide resolved
control-plane/connect-inject/handler.go Outdated Show resolved Hide resolved
@ndhanushkodi
Copy link
Contributor Author

ndhanushkodi commented Feb 18, 2022

WANFed tests for vault are failing but it might be due to my image? I'll try rebasing and using a new image for acceptance tests, but since everything else is passing and this doesn't touch vault this is ready for a second round of review cc @ishustava @curtbushko

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

A couple of minor comments, but they're not blocking!

control-plane/connect-inject/container_init.go Outdated Show resolved Hide resolved
return i
}
}
return -1
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we could also return an error here (it feels a bit more idiomatic to go) but definitely not a blocker!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started making this change, but then I realized that in a case where you may have a non-multiport pod, you need to know to ignore the error (for example, in the processUpstreams function). I like the way it is now, because it implies that it's not an error if you don't find a multiport index in the service list, and rather you should handle the -1 by not following the multiport case.

@@ -62,7 +68,7 @@ func (h *Handler) envoySidecar(namespace corev1.Namespace, pod corev1.Pod) (core
// has only injected init containers so all containers defined in pod.Spec.Containers are from the user.
for _, c := range pod.Spec.Containers {
// User container and Envoy container cannot have the same UID.
if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil && *c.SecurityContext.RunAsUser == envoyUserAndGroupID {
if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil && *c.SecurityContext.RunAsUser == envoyUserAndGroupID && c.Image != h.ImageEnvoy {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why do we need that extra image check at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we're running multiple sidecars in the pod, they clash with the same uid/gid so this check allows using the same uid/gid for 2 envoy sidecars. Since tproxy isn't supported it seems ok to let them run with the same uid/gid.

control-plane/subcommand/connect-init/command.go Outdated Show resolved Hide resolved
control-plane/subcommand/connect-init/command.go Outdated Show resolved Hide resolved
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.

3 participants