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

feat: add v2 endpoints controller #2883

Merged
merged 1 commit into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions control-plane/connect-inject/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,13 @@ import (
"strings"

mapset "github.com/deckarep/golang-set"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/anypb"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1"
)

// DetermineAndValidatePort behaves as follows:
Expand Down Expand Up @@ -116,3 +119,44 @@ func ShouldIgnore(namespace string, denySet, allowSet mapset.Set) bool {
func ConsulNodeNameFromK8sNode(nodeName string) string {
return fmt.Sprintf("%s-virtual", nodeName)
}

// ********************
// V2 Exclusive Common Code
// ********************

// ToProtoAny is a convenience function for converting proto.Message values to anypb.Any without error handling.
// This should _only_ be used in cases where a nil or valid proto.Message value is _guaranteed_, else it will panic.
// If the type of m is *anypb.Any, that value will be returned unmodified.
func ToProtoAny(m proto.Message) *anypb.Any {
switch v := m.(type) {
case nil:
return nil
case *anypb.Any:
return v
}
a, err := anypb.New(m)
if err != nil {
panic(fmt.Errorf("unexpected error: failed to convert proto message to anypb.Any: %w", err))
}
return a
}

// GetPortProtocol matches the Kubernetes EndpointPort.AppProtocol or ServicePort.AppProtocol (*string) to a supported
// Consul catalog port protocol. If nil or unrecognized, the default of `PROTOCOL_UNSPECIFIED` is returned.
func GetPortProtocol(appProtocol *string) pbcatalog.Protocol {
if appProtocol == nil {
return pbcatalog.Protocol_PROTOCOL_UNSPECIFIED
}
switch *appProtocol {
case "tcp":
return pbcatalog.Protocol_PROTOCOL_TCP
case "http":
return pbcatalog.Protocol_PROTOCOL_HTTP
case "http2":
return pbcatalog.Protocol_PROTOCOL_HTTP2
case "grpc":
return pbcatalog.Protocol_PROTOCOL_GRPC
Comment on lines +155 to +158
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we technically need to include prefixes with some of these protocols. I was reading the API docs for appProtocol and it mentions only IANA names can be unprefixed. Only my first time seeing this so I could be mistaken.

I also think an annotation on the service or pod might be easier for customers to support. e.g. the Bitnami Postgres helmchart exposes annotations for services and certain pods but not a change of appProtocol.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if we technically need to include prefixes with some of these protocols. I was reading the API docs for appProtocol and it mentions only IANA names can be unprefixed. Only my first time seeing this so I could be mistaken.

Yeah, I was wondering the same - we may need to iterate on this part. FWIW, Istio's solution (appProtocol and detection via port name) use unprefixed values from what I can see, and Open Service Mesh also includes non-standard tcp and gRPC. I wonder whether the intent in k8s docs was valid Service Name Syntax rather than a registered name - that seems arbitrarily limiting for new protocols, to me, when the purpose appears to be open-ended extension.

I also think an annotation on the service or pod might be easier for customers to support. e.g. the Bitnami Postgres helmchart exposes annotations for services and certain pods but not a change of appProtocol.

Agreed, it seems like annotations could help ease operator pain here rather than plumbing appProtocol through templates - though maybe better for us to enforce "standard" k8s concepts and push that concern up the tooling chain rather than accommodate it ourselves? I can start a 🧵 in the multi-port channel and see whether a follow-up makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this as a bullet in follow-up ticket so we don't lose track but probably don't need to figure out right this second.

Copy link
Member Author

Choose a reason for hiding this comment

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

Circling back to this one to tie up the loose end: @ishustava, do you happen to have perspective on either of Dan's questions above?

My gut is to leave the unprefixed protocols in place for now since there seems to be precedent in other meshes, and wait to see if we need annotations in addition to just appProtocol. If that's not ideal, we can fix now or plan to follow up in 1.18.

}
// If unrecognized or empty string, return default
return pbcatalog.Protocol_PROTOCOL_UNSPECIFIED
}
85 changes: 85 additions & 0 deletions control-plane/connect-inject/common/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,17 @@ import (
"testing"

mapset "github.com/deckarep/golang-set"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/testing/protocmp"
"google.golang.org/protobuf/types/known/anypb"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul-k8s/control-plane/namespaces"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1"
"github.com/hashicorp/consul/proto-public/pbresource"
)

func TestCommonDetermineAndValidatePort(t *testing.T) {
Expand Down Expand Up @@ -314,3 +319,83 @@ func TestShouldIgnore(t *testing.T) {
})
}
}

func TestToProtoAny(t *testing.T) {
zalimeni marked this conversation as resolved.
Show resolved Hide resolved
t.Parallel()

t.Run("nil gets nil", func(t *testing.T) {
require.Nil(t, ToProtoAny(nil))
})

t.Run("anypb.Any gets same value", func(t *testing.T) {
testMsg := &pbresource.Resource{Id: &pbresource.ID{Name: "foo"}}
testAny, err := anypb.New(testMsg)
require.NoError(t, err)

require.Equal(t, testAny, ToProtoAny(testAny))
})

t.Run("valid proto is successfully serialized", func(t *testing.T) {
testMsg := &pbresource.Resource{Id: &pbresource.ID{Name: "foo"}}
testAny, err := anypb.New(testMsg)
require.NoError(t, err)

if diff := cmp.Diff(testAny, ToProtoAny(testMsg), protocmp.Transform()); diff != "" {
t.Errorf("unexpected difference:\n%v", diff)
}
})
}

func TestGetPortProtocol(t *testing.T) {
t.Parallel()
toStringPtr := func(s string) *string {
return &s
}
cases := []struct {
name string
input *string
expected pbcatalog.Protocol
}{
{
name: "nil gets UNSPECIFIED",
input: nil,
expected: pbcatalog.Protocol_PROTOCOL_UNSPECIFIED,
},
{
name: "tcp gets TCP",
input: toStringPtr("tcp"),
expected: pbcatalog.Protocol_PROTOCOL_TCP,
},
{
name: "http gets HTTP",
input: toStringPtr("http"),
expected: pbcatalog.Protocol_PROTOCOL_HTTP,
},
{
name: "http2 gets HTTP2",
input: toStringPtr("http2"),
expected: pbcatalog.Protocol_PROTOCOL_HTTP2,
},
{
name: "grpc gets GRPC",
input: toStringPtr("grpc"),
expected: pbcatalog.Protocol_PROTOCOL_GRPC,
},
{
name: "case sensitive",
input: toStringPtr("gRPC"),
expected: pbcatalog.Protocol_PROTOCOL_UNSPECIFIED,
},
{
zalimeni marked this conversation as resolved.
Show resolved Hide resolved
name: "unknown gets UNSPECIFIED",
input: toStringPtr("foo"),
expected: pbcatalog.Protocol_PROTOCOL_UNSPECIFIED,
},
}
for _, tt := range cases {
t.Run(tt.name, func(t *testing.T) {
actual := GetPortProtocol(tt.input)
require.Equal(t, tt.expected, actual)
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ const (
Enabled = "enabled"

// ManagedByValue is the value for keyManagedBy.
//TODO(zalimeni) rename this to ManagedByLegacyEndpointsValue.
ManagedByValue = "consul-k8s-endpoints-controller"
)

Expand All @@ -220,8 +221,13 @@ const (
// a pod after an injection is done.
KeyMeshInjectStatus = "consul.hashicorp.com/mesh-inject-status"

// ManagedByEndpointsValue is used in Consul metadata to identify the manager
// of resources. The 'v2' suffix is used to differentiate from the legacy
// endpoints controller of the same name.
ManagedByEndpointsValue = "consul-k8s-endpoints-controller-v2"

// ManagedByPodValue is used in Consul metadata to identify the manager
// of this resource.
// of resources.
ManagedByPodValue = "consul-k8s-pod-controller"
)

Expand Down
Loading