Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Add fallback to no gRPC #454

Merged
merged 4 commits into from
Nov 17, 2022
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
2 changes: 1 addition & 1 deletion .github/workflows/conformance_with_build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ jobs:
api-gateway-image: "hashicorppreview/consul-api-gateway:0.4-dev"
consul-image: "hashicorppreview/consul:1.14-dev"
envoy-image: "envoyproxy/envoy:v1.22-latest"
consul-k8s-version: "v0.49.0"
consul-k8s-version: "main"
fail-fast: true
name: "${{ matrix.config.name }}"

Expand Down
7 changes: 3 additions & 4 deletions internal/commands/exec/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,11 @@ func (c *Command) Run(args []string) (ret int) {
bearerToken = strings.TrimSpace(string(data))
}

if c.flagGatewayNamespace != "" {
cfg.Namespace = c.flagGatewayNamespace
}

consulClientConfig := consul.ClientConfig{
Name: c.flagGatewayName,
Namespace: c.flagGatewayNamespace,
ApiClientConfig: cfg,
UseDynamic: os.Getenv("CONSUL_DYNAMIC_SERVER_DISCOVERY") == "true",
Addresses: c.flagConsulHTTPAddress,
HTTPPort: c.flagConsulHTTPPort,
GRPCPort: c.flagConsulXDSPort,
Expand Down
2 changes: 2 additions & 0 deletions internal/commands/exec/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package exec

import (
"context"
"os"
"testing"

"github.com/mitchellh/cli"
Expand Down Expand Up @@ -124,6 +125,7 @@ func TestExec(t *testing.T) {
output: "did not get state within time limit",
}} {
t.Run(test.name, func(t *testing.T) {
os.Setenv("CONSUL_DYNAMIC_SERVER_DISCOVERY", "true")
ctx := context.Background()
ui := cli.NewMockUi()
var buffer gwTesting.Buffer
Expand Down
1 change: 1 addition & 0 deletions internal/commands/exec/exec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ func runMockConsulServer(t *testing.T, opts mockConsulOptions) *mockConsulServer
})

server.clientConfig = consul.ClientConfig{
UseDynamic: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add another test where this is false?

Addresses: serverURL.Hostname(),
ApiClientConfig: clientConfig,
GRPCPort: grpcPort,
Expand Down
8 changes: 7 additions & 1 deletion internal/commands/server/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,15 @@ func (c *Command) Run(args []string) int {
MirrorKubernetesNamespacePrefix: c.flagMirrorK8SNamespacePrefix,
}

consulHTTPAddressOrCommand, port, err := parseConsulHTTPAddress()
consulScheme, consulHTTPAddressOrCommand, port, err := parseConsulHTTPAddress()
if err != nil {
logger.Error("error reading "+consulHTTPAddressEnvName, "error", err)
return 1
}
if consulCfg.Scheme != "https" {
// override only if it needs to be explicitly marked
consulCfg.Scheme = consulScheme
}
Comment on lines +180 to +183
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, this is to say, if we already have the scheme set to https (via things like ENV variables, which DefaultConfig will source), then don't override it -- otherwise set it based off of what we got from parsing it out of the CONSUL_ADDR (in 1.0.0 we don't set the scheme and use CONSUL_USE_SSL, in <= 0.49 we use the scheme as part of the address, this handles both cases)


tlsCfg, err := api.SetupTLSConfig(&consulCfg.TLSConfig)
if err != nil {
Expand All @@ -201,8 +205,10 @@ func (c *Command) Run(args []string) int {
}

consulClientConfig := consul.ClientConfig{
Name: "api-gateway-controller",
ApiClientConfig: consulCfg,
Addresses: consulHTTPAddressOrCommand,
UseDynamic: os.Getenv("CONSUL_DYNAMIC_SERVER_DISCOVERY") == "true",
HTTPPort: port,
GRPCPort: grpcPort(),
PlainText: consulCfg.Scheme == "http",
Expand Down
13 changes: 10 additions & 3 deletions internal/commands/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,21 @@ func registerSecretClients(config ServerConfig) (*envoy.MultiSecretClient, error
return secretClient, nil
}

func parseConsulHTTPAddress() (cmd string, port int, err error) {
func parseConsulHTTPAddress() (scheme string, cmd string, port int, err error) {
consulhttpAddress := os.Getenv(consulHTTPAddressEnvName)
scheme = "http"
if os.Getenv("CONSUL_HTTP_SSL") == "true" || strings.HasPrefix(consulhttpAddress, "https://") {
scheme = "https"
}

// (?:http|https|ftp)://)
consulhttpAddress = strings.TrimPrefix(consulhttpAddress, "http://")
consulhttpAddress = strings.TrimPrefix(consulhttpAddress, "https://")
index := strings.LastIndex(consulhttpAddress, ":")
cmd, portString := consulhttpAddress[:index], consulhttpAddress[index+1:]
port, err = strconv.Atoi(portString)
if err != nil {
return "", 0, err
return "", "", 0, err
}
return cmd, port, nil
return scheme, cmd, port, nil
}
84 changes: 84 additions & 0 deletions internal/consul/connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ type Client interface {
}

type ClientConfig struct {
Name string
Namespace string
ApiClientConfig *api.Config
UseDynamic bool
PlainText bool
Addresses string
HTTPPort int
Expand Down Expand Up @@ -81,6 +84,53 @@ func (c *client) Wait(until time.Duration) error {
}

func (c *client) WatchServers(ctx context.Context) error {
if !c.config.UseDynamic {
cfg := c.config.ApiClientConfig
cfg.Address = fmt.Sprintf("%s:%d", c.config.Addresses, c.config.HTTPPort)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely specific to this line, but do we have unit test coverage for checking that both http:// prefixed and plain IPs are handled for agentful and agentless paths?


var err error
var client *api.Client
var token string
if c.config.Credentials.Type == discovery.CredentialsTypeLogin {
baseClient, err := api.NewClient(cfg)
if err != nil {
c.initialized <- err
return err
}
if c.config.Namespace != "" {
cfg.Namespace = c.config.Namespace
}
client, token, err = login(ctx, baseClient, cfg, c.config)
if err != nil {
c.initialized <- err
return err
}
defer logout(baseClient, token, c.config)

} else {
// this might be empty
cfg.Token = c.config.Credentials.Static.Token
if c.config.Namespace != "" {
cfg.Namespace = c.config.Namespace
}
client, err = api.NewClient(cfg)
if err != nil {
c.initialized <- err
return err
}
}

c.mutex.Lock()
c.client = client
c.token = cfg.Token
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 this should get set to token (and set token = c.config.Credentials.Static.Token; cfg.Token = token in the non-login path) unless login is mutating cfg.Token?

Edit: reading the login impl it looks like it does mutate cfg.Token so this should be okay, but it is a bit unintuitive.

c.mutex.Unlock()

close(c.initialized)

<-ctx.Done()
return nil
}

ctx, cancel := context.WithCancel(ctx)
c.stop = cancel

Expand Down Expand Up @@ -124,6 +174,9 @@ func (c *client) WatchServers(ctx context.Context) error {
}
updateClient := func(s discovery.State) error {
cfg := c.config.ApiClientConfig
if c.config.Namespace != "" {
cfg.Namespace = c.config.Namespace
}
cfg.Address = fmt.Sprintf("%s:%d", s.Address.IP.String(), c.config.HTTPPort)
if static {
// This is to fix the fact that s.Address always resolves to an IP, if
Expand Down Expand Up @@ -220,3 +273,34 @@ func (c *client) Internal() *api.Client {

return c.client
}

func login(ctx context.Context, client *api.Client, cfg *api.Config, config ClientConfig) (*api.Client, string, error) {
authenticator := NewAuthenticator(
config.Logger.Named("authenticator"),
client,
config.Credentials.Login.AuthMethod,
config.Credentials.Login.Namespace,
)

token, err := authenticator.Authenticate(ctx, config.Name, config.Credentials.Login.BearerToken)
if err != nil {
return nil, "", fmt.Errorf("error logging in to consul: %w", err)
}

// Now update the client so that it will read the ACL token we just fetched.
cfg.Token = token
newClient, err := api.NewClient(cfg)
if err != nil {
return nil, "", fmt.Errorf("error updating client connection with token: %w", err)
}
return newClient, token, nil
}

func logout(client *api.Client, token string, config ClientConfig) error {
config.Logger.Info("deleting acl token")
_, err := client.ACL().Logout(&api.WriteOptions{Token: token})
if err != nil {
return fmt.Errorf("error deleting acl token: %w", err)
}
return nil
}
4 changes: 4 additions & 0 deletions internal/k8s/builder/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,10 @@ func (b *GatewayDeploymentBuilder) envVars() []corev1.EnvVar {
Name: "CONSUL_LOGIN_DATACENTER",
Value: os.Getenv("CONSUL_LOGIN_DATACENTER"),
},
{
Name: "CONSUL_DYNAMIC_SERVER_DISCOVERY",
Value: os.Getenv("CONSUL_DYNAMIC_SERVER_DISCOVERY"),
},
{
Name: "CONSUL_PARTITION",
Value: orDefault(b.gwConfig.Spec.ConsulSpec.Partition, defaultPartition),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ spec:
fieldPath: status.hostIP
- name: CONSUL_LOGIN_PARTITION
- name: CONSUL_LOGIN_DATACENTER
- name: CONSUL_DYNAMIC_SERVER_DISCOVERY
- name: CONSUL_PARTITION
- name: CONSUL_TLS_SERVER_NAME
- name: PATH
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ spec:
fieldPath: status.hostIP
- name: CONSUL_LOGIN_PARTITION
- name: CONSUL_LOGIN_DATACENTER
- name: CONSUL_DYNAMIC_SERVER_DISCOVERY
- name: CONSUL_PARTITION
- name: CONSUL_TLS_SERVER_NAME
- name: PATH
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ spec:
fieldPath: status.hostIP
- name: CONSUL_LOGIN_PARTITION
- name: CONSUL_LOGIN_DATACENTER
- name: CONSUL_DYNAMIC_SERVER_DISCOVERY
- name: CONSUL_PARTITION
- name: CONSUL_TLS_SERVER_NAME
- name: PATH
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ spec:
fieldPath: status.hostIP
- name: CONSUL_LOGIN_PARTITION
- name: CONSUL_LOGIN_DATACENTER
- name: CONSUL_DYNAMIC_SERVER_DISCOVERY
- name: CONSUL_PARTITION
- name: CONSUL_TLS_SERVER_NAME
- name: PATH
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ spec:
fieldPath: status.hostIP
- name: CONSUL_LOGIN_PARTITION
- name: CONSUL_LOGIN_DATACENTER
- name: CONSUL_DYNAMIC_SERVER_DISCOVERY
- name: CONSUL_PARTITION
- name: CONSUL_TLS_SERVER_NAME
- name: PATH
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ spec:
fieldPath: status.hostIP
- name: CONSUL_LOGIN_PARTITION
- name: CONSUL_LOGIN_DATACENTER
- name: CONSUL_DYNAMIC_SERVER_DISCOVERY
- name: CONSUL_PARTITION
- name: CONSUL_TLS_SERVER_NAME
- name: PATH
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ spec:
fieldPath: status.hostIP
- name: CONSUL_LOGIN_PARTITION
- name: CONSUL_LOGIN_DATACENTER
- name: CONSUL_DYNAMIC_SERVER_DISCOVERY
- name: CONSUL_PARTITION
- name: CONSUL_TLS_SERVER_NAME
- name: PATH
Expand Down