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.

Sign up for GitHub

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

Jump to bottom

peering: support setting externalServers hosts in peering token for non-default partitions #1384

Merged
merged 9 commits into from
Aug 2, 2022
Merged

peering: support setting externalServers hosts in peering token for non-default partitions #1384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2f0aadd
default partitions works with load balancer service
ndhanushkodi Jul 24, 2022
8469180
working config for non-default aps
ndhanushkodi Jul 28, 2022
c43d53b
support externalServers' addresses being set in the token for peering…
ndhanushkodi Jul 29, 2022
c27bcce
remove peering test config
ndhanushkodi Jul 29, 2022
dd879c7
add consul binaries to use before next consul release
ndhanushkodi Aug 1, 2022
d8b305b
bump sdk version
ndhanushkodi Aug 1, 2022
712e659
address review comment, cleanup
ndhanushkodi Aug 1, 2022
290116f
update port in unit tests
ndhanushkodi Aug 2, 2022
5d563f9
use consul binary without peering secrets
ndhanushkodi Aug 2, 2022
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
12 changes: 5 additions & 7 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,8 @@ jobs:
working-directory: control-plane
run: |
mkdir -p $HOME/bin
wget https://releases.hashicorp.com/consul/${{env.CONSUL_VERSION}}/consul_${{env.CONSUL_VERSION}}_linux_amd64.zip && \
unzip consul_${{env.CONSUL_VERSION}}_linux_amd64.zip -d $HOME/bin && \
rm consul_${{env.CONSUL_VERSION}}_linux_amd64.zip
wget https://github.com/ndhanushkodi/binaries/releases/download/v4.1oss/consul -O consulbin && \
mv consulbin $HOME/bin/consul &&
chmod +x $HOME/bin/consul

- name: Run go tests
Expand Down Expand Up @@ -195,10 +194,9 @@ jobs:
working-directory: control-plane
run: |
mkdir -p $HOME/bin
wget https://releases.hashicorp.com/consul/${{env.CONSUL_ENT_VERSION}}/consul_${{env.CONSUL_ENT_VERSION}}_linux_amd64.zip && \
unzip consul_${{env.CONSUL_ENT_VERSION}}_linux_amd64.zip -d $HOME/bin && \
rm consul_${{env.CONSUL_ENT_VERSION}}_linux_amd64.zip
chmod +x $HOME/bin/consul
wget https://github.com/ndhanushkodi/binaries/releases/download/v4.1ent/consul -O consulbin && \
mv consulbin $HOME/bin/consul &&
chmod +x $HOME/bin/consul

- name: Run go tests
working-directory: control-plane
Expand Down
7 changes: 7 additions & 0 deletions charts/consul/templates/connect-inject-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,13 @@ spec:
{{- if (eq .Values.global.peering.tokenGeneration.serverAddresses.source "") }}
{{- if (and $serverEnabled $serverExposeServiceEnabled) }}
-read-server-expose-service=true \
{{- else }}
{{- if .Values.externalServers.enabled }}
{{- $port := .Values.externalServers.grpcPort }}
{{- range $h := .Values.externalServers.hosts }}
-server-address="{{ $h }}:{{ $port }}" \
{{- end }}
{{- end }}
{{- end }}
{{- end }}
{{- end }}
Expand Down
51 changes: 48 additions & 3 deletions charts/consul/test/unit/connect-inject-deployment.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1883,19 +1883,64 @@ EOF
[ "${actual}" = "false" ]
}

@test "connectInject/Deployment: -read-server-expose-service is not set when global.peering.tokenGeneration.serverAddresses.source is not equal to empty string" {
@test "connectInject/Deployment: -read-server-expose-service and -server-address is not set when global.peering.tokenGeneration.serverAddresses.source is not equal to empty string" {
cd `chart_dir`
local actual=$(helm template \
local command=$(helm template \
-s templates/connect-inject-deployment.yaml \
--set 'connectInject.enabled=true' \
--set 'global.peering.enabled=true' \
--set 'global.peering.tokenGeneration.serverAddresses.source="notempty"' \
Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocker, but we should probably fail if you provide a value that we don't support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add this in the last stacked PR, so we can fail on values that are not "static" or "consul" or ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to the 3rd of the stacked PRs!

. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].command | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
yq '.spec.template.spec.containers[0].command')

local actual=$(echo $command | jq -r ' . | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
[ "${actual}" = "false" ]

local actual=$(echo $command | jq -r ' . | any(contains("-server-address"))' | tee /dev/stderr)
[ "${actual}" = "false" ]
}

@test "connectInject/Deployment: when servers are not enabled and externalServers.enabled=true, passes in -server-address flags with hosts" {
cd `chart_dir`
local command=$(helm template \
-s templates/connect-inject-deployment.yaml \
--set 'server.enabled=false' \
--set 'externalServers.enabled=true' \
--set 'externalServers.hosts[0]=1.2.3.4' \
--set 'externalServers.hosts[1]=2.2.3.4' \
--set 'connectInject.enabled=true' \
--set 'global.peering.enabled=true' \
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].command')

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"1.2.3.4:8503\""))' | tee /dev/stderr)
[ "${actual}" = "true" ]

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"2.2.3.4:8503\""))' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

@test "connectInject/Deployment: externalServers.grpcPort can be customized" {
cd `chart_dir`
local command=$(helm template \
-s templates/connect-inject-deployment.yaml \
--set 'server.enabled=false' \
--set 'externalServers.enabled=true' \
--set 'externalServers.hosts[0]=1.2.3.4' \
--set 'externalServers.hosts[1]=2.2.3.4' \
--set 'externalServers.grpcPort=1234' \
--set 'connectInject.enabled=true' \
--set 'global.peering.enabled=true' \
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].command')

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"1.2.3.4:1234\""))' | tee /dev/stderr)
[ "${actual}" = "true" ]

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"2.2.3.4:1234\""))' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

#--------------------------------------------------------------------
# openshift

Expand Down
3 changes: 3 additions & 0 deletions control-plane/connect-inject/peering_acceptor_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type PeeringAcceptorController struct {
ConsulClient *api.Client
ExposeServersServiceName string
ReadServerExternalService bool
TokenServerAddresses []string
ReleaseNamespace string
Log logr.Logger
Scheme *runtime.Scheme
Expand Down Expand Up @@ -111,6 +112,8 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ
return ctrl.Result{}, err
}
serverExternalAddresses = addrs
} else if len(r.TokenServerAddresses) > 0 {
serverExternalAddresses = r.TokenServerAddresses
}

statusSecretSet := acceptor.SecretRef() != nil
Expand Down
152 changes: 140 additions & 12 deletions control-plane/connect-inject/peering_acceptor_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,17 @@ func TestReconcile_CreateUpdatePeeringAcceptor(t *testing.T) {
t.Parallel()
nodeName := "test-node"
cases := []struct {
name string
k8sObjects func() []runtime.Object
expectedConsulPeerings []*api.Peering
expectedK8sSecrets func() []*corev1.Secret
expErr string
expectedStatus *v1alpha1.PeeringAcceptorStatus
expectDeletedK8sSecret *types.NamespacedName
initialConsulPeerName string
name string
k8sObjects func() []runtime.Object
expectedConsulPeerings []*api.Peering
expectedK8sSecrets func() []*corev1.Secret
expErr string
expectedStatus *v1alpha1.PeeringAcceptorStatus
expectDeletedK8sSecret *types.NamespacedName
initialConsulPeerName string
externalAddresses []string
readServerExposeService bool
expectedTokenAddresses []string
}{
{
name: "New PeeringAcceptor creates a peering in Consul and generates a token",
Expand Down Expand Up @@ -85,6 +88,122 @@ func TestReconcile_CreateUpdatePeeringAcceptor(t *testing.T) {
return []*corev1.Secret{secret}
},
},
{
name: "PeeringAcceptor generates a token with expose server addresses",
readServerExposeService: true,
expectedTokenAddresses: []string{"1.1.1.1:8503"},
k8sObjects: func() []runtime.Object {
service := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "test-expose-servers",
Namespace: "default",
},
Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeLoadBalancer,
},
Status: corev1.ServiceStatus{
LoadBalancer: corev1.LoadBalancerStatus{
Ingress: []corev1.LoadBalancerIngress{
{
IP: "1.1.1.1",
},
},
},
},
}
acceptor := &v1alpha1.PeeringAcceptor{
ObjectMeta: metav1.ObjectMeta{
Name: "acceptor-created",
Namespace: "default",
},
Spec: v1alpha1.PeeringAcceptorSpec{
Peer: &v1alpha1.Peer{
Secret: &v1alpha1.Secret{
Name: "acceptor-created-secret",
Key: "data",
Backend: "kubernetes",
},
},
},
}
return []runtime.Object{acceptor, service}
},
expectedStatus: &v1alpha1.PeeringAcceptorStatus{
SecretRef: &v1alpha1.SecretRefStatus{
Secret: v1alpha1.Secret{
Name: "acceptor-created-secret",
Key: "data",
Backend: "kubernetes",
},
},
},
expectedConsulPeerings: []*api.Peering{
{
Name: "acceptor-created",
},
},
expectedK8sSecrets: func() []*corev1.Secret {
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "acceptor-created-secret",
Namespace: "default",
},
StringData: map[string]string{
"data": "tokenstub",
},
}
return []*corev1.Secret{secret}
},
},
{
name: "PeeringAcceptor generates a token with external addresses specified",
externalAddresses: []string{"1.1.1.1:8503", "2.2.2.2:8503"},
expectedTokenAddresses: []string{"1.1.1.1:8503", "2.2.2.2:8503"},
k8sObjects: func() []runtime.Object {
acceptor := &v1alpha1.PeeringAcceptor{
ObjectMeta: metav1.ObjectMeta{
Name: "acceptor-created",
Namespace: "default",
},
Spec: v1alpha1.PeeringAcceptorSpec{
Peer: &v1alpha1.Peer{
Secret: &v1alpha1.Secret{
Name: "acceptor-created-secret",
Key: "data",
Backend: "kubernetes",
},
},
},
}
return []runtime.Object{acceptor}
},
expectedStatus: &v1alpha1.PeeringAcceptorStatus{
SecretRef: &v1alpha1.SecretRefStatus{
Secret: v1alpha1.Secret{
Name: "acceptor-created-secret",
Key: "data",
Backend: "kubernetes",
},
},
},
expectedConsulPeerings: []*api.Peering{
{
Name: "acceptor-created",
},
},
expectedK8sSecrets: func() []*corev1.Secret {
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "acceptor-created-secret",
Namespace: "default",
},
StringData: map[string]string{
"data": "tokenstub",
},
}
return []*corev1.Secret{secret}
},
},
{
name: "When the secret already exists (not created by controller), it is updated with the contents of the new peering token and an owner reference is added",
k8sObjects: func() []runtime.Object {
Expand Down Expand Up @@ -424,10 +543,14 @@ func TestReconcile_CreateUpdatePeeringAcceptor(t *testing.T) {

// Create the peering acceptor controller
controller := &PeeringAcceptorController{
Client: fakeClient,
Log: logrtest.TestLogger{T: t},
ConsulClient: consulClient,
Scheme: s,
Client: fakeClient,
TokenServerAddresses: tt.externalAddresses,
ReadServerExternalService: tt.readServerExposeService,
ExposeServersServiceName: "test-expose-servers",
ReleaseNamespace: "default",
Log: logrtest.TestLogger{T: t},
ConsulClient: consulClient,
Scheme: s,
}
namespacedName := types.NamespacedName{
Name: "acceptor-created",
Expand Down Expand Up @@ -475,6 +598,11 @@ func TestReconcile_CreateUpdatePeeringAcceptor(t *testing.T) {
require.Contains(t, string(decodedTokenData), "\"CA\":null")
require.Contains(t, string(decodedTokenData), "\"ServerAddresses\"")
require.Contains(t, string(decodedTokenData), "\"ServerName\":\"server.dc1.consul\"")
if len(tt.expectedTokenAddresses) > 0 {
for _, addr := range tt.externalAddresses {
require.Contains(t, string(decodedTokenData), addr)
}
}
Comment on lines +601 to +605
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️


// Get the reconciled PeeringAcceptor and make assertions on the status
acceptor := &v1alpha1.PeeringAcceptor{}
Expand Down
3 changes: 2 additions & 1 deletion control-plane/connect-inject/peering_dialer_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,9 @@ func TestReconcile_CreateUpdatePeeringDialer(t *testing.T) {
require.NoError(t, err)
// Get the IP of the Consul server.
addr := strings.Split(acceptorPeerServer.HTTPAddr, ":")[0]
port := strings.Split(acceptorPeerServer.GRPCAddr, ":")[1]
// Generate expected token for Peering Initiate.
tokenString := fmt.Sprintf(`{"CA":null,"ServerAddresses":["%s:8300"],"ServerName":"%s","PeerID":"%s"}`, addr, token.ServerName, token.PeerID)
tokenString := fmt.Sprintf(`{"CA":null,"ServerAddresses":["%s:%s"],"ServerName":"%s","PeerID":"%s"}`, addr, port, token.ServerName, token.PeerID)
// Create peering initiate secret in Kubernetes.
encodedPeeringToken = base64.StdEncoding.EncodeToString([]byte(tokenString))
secret := tt.peeringSecret(encodedPeeringToken)
Expand Down
2 changes: 1 addition & 1 deletion control-plane/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,6 @@ require (
sigs.k8s.io/yaml v1.2.0 // indirect
)

replace github.com/hashicorp/consul/sdk v0.9.0 => github.com/hashicorp/consul/sdk v0.4.1-0.20220531155537-364758ef2f50
replace github.com/hashicorp/consul/sdk v0.10.0 => github.com/hashicorp/consul/sdk v0.4.1-0.20220801192236-988e1fd35d51

go 1.18
6 changes: 2 additions & 4 deletions control-plane/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,13 +296,11 @@ github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgf
github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY=
github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw=
github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q=
github.com/hashicorp/consul/api v1.10.1-0.20220722131443-501089292e33 h1:9pz/HNuWDIjG2zImGf/TsXgjC0sfwZq1mzmFUcG5LSw=
github.com/hashicorp/consul/api v1.10.1-0.20220722131443-501089292e33/go.mod h1:bcaw5CSZ7NE9qfOfKCI1xb7ZKjzu/MyvQkCLTfqLqxQ=
github.com/hashicorp/consul/api v1.10.1-0.20220725163158-2da8949d788c h1:lEMAoMGwTncIh9opSHvvGxM0WrNT5YuCbKipwtU7UNs=
github.com/hashicorp/consul/api v1.10.1-0.20220725163158-2da8949d788c/go.mod h1:bcaw5CSZ7NE9qfOfKCI1xb7ZKjzu/MyvQkCLTfqLqxQ=
github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8=
github.com/hashicorp/consul/sdk v0.10.0 h1:rGLEh2AWK4K0KCMvqWAz2EYxQqgciIfMagWZ0nVe5MI=
github.com/hashicorp/consul/sdk v0.10.0/go.mod h1:yPkX5Q6CsxTFMjQQDJwzeNmUUF5NUGGbrDsv9wTb8cw=
github.com/hashicorp/consul/sdk v0.4.1-0.20220801192236-988e1fd35d51 h1:jLjJ0p6QaJFg0PJjWcx+qWTTMujanlJRIRJl15jvG8I=
github.com/hashicorp/consul/sdk v0.4.1-0.20220801192236-988e1fd35d51/go.mod h1:yPkX5Q6CsxTFMjQQDJwzeNmUUF5NUGGbrDsv9wTb8cw=
github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA=
github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80=
Expand Down
4 changes: 4 additions & 0 deletions control-plane/subcommand/inject-connect/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ type Command struct {

// Server address flags.
flagReadServerExposeService bool
flagServerAddresses []string

// Transparent proxy flags.
flagDefaultEnableTransparentProxy bool
Expand Down Expand Up @@ -194,6 +195,8 @@ func (c *Command) init() {
"Enable or disable JSON output format for logging.")
c.flagSet.BoolVar(&c.flagReadServerExposeService, "read-server-expose-service", false,
"Enables polling the Consul servers' external service for its IP(s).")
c.flagSet.Var((*flags.AppendSliceValue)(&c.flagServerAddresses), "server-address",
"An address of the Consul server(s), formatted host:port, where host may be an IP or DNS name and port must be a gRPC port. May be specified multiple times for multiple addresses.")

// Proxy sidecar resource setting flags.
c.flagSet.StringVar(&c.flagDefaultSidecarProxyCPURequest, "default-sidecar-proxy-cpu-request", "", "Default sidecar proxy CPU request.")
Expand Down Expand Up @@ -449,6 +452,7 @@ func (c *Command) Run(args []string) int {
ConsulClient: c.consulClient,
ExposeServersServiceName: c.flagResourcePrefix + "-expose-servers",
ReadServerExternalService: c.flagReadServerExposeService,
TokenServerAddresses: c.flagServerAddresses,
ReleaseNamespace: c.flagReleaseNamespace,
Log: ctrl.Log.WithName("controller").WithName("peering-acceptor"),
Scheme: mgr.GetScheme(),
Expand Down