From 6c77f7883e6470150a17a9edeed54bed5e3d1ea2 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Thu, 7 Jun 2018 10:17:44 +0100 Subject: [PATCH] Misc comment cleanups --- agent/config/config.go | 2 +- agent/config/runtime.go | 3 +-- api/health.go | 2 +- api/health_test.go | 12 +++--------- connect/proxy/config_test.go | 3 --- connect/service.go | 4 +--- connect/service_test.go | 14 +++++--------- 7 files changed, 12 insertions(+), 28 deletions(-) diff --git a/agent/config/config.go b/agent/config/config.go index 6cc640a81..5b8819525 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -368,7 +368,7 @@ type ServiceConnectProxy struct { // Connect is the agent-global connect configuration. type Connect struct { // Enabled opts the agent into connect. It should be set on all clients and - // servers in a cluster for correct connect operation. TODO(banks) review that. + // servers in a cluster for correct connect operation. Enabled *bool `json:"enabled,omitempty" hcl:"enabled" mapstructure:"enabled"` ProxyDefaults *ConnectProxyDefaults `json:"proxy_defaults,omitempty" hcl:"proxy_defaults" mapstructure:"proxy_defaults"` CAProvider *string `json:"ca_provider,omitempty" hcl:"ca_provider" mapstructure:"ca_provider"` diff --git a/agent/config/runtime.go b/agent/config/runtime.go index 8baf02ab2..1399c5744 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -617,8 +617,7 @@ type RuntimeConfig struct { ClientAddrs []*net.IPAddr // ConnectEnabled opts the agent into connect. It should be set on all clients - // and servers in a cluster for correct connect operation. TODO(banks) review - // that. + // and servers in a cluster for correct connect operation. ConnectEnabled bool // ConnectProxyBindMinPort is the inclusive start of the range of ports diff --git a/api/health.go b/api/health.go index 5fcb39b5c..1835da559 100644 --- a/api/health.go +++ b/api/health.go @@ -164,7 +164,7 @@ func (h *Health) Service(service, tag string, passingOnly bool, q *QueryOptions) // Connect is equivalent to Service except that it will only return services // which are Connect-enabled and will returns the connection address for Connect -// client's to use which may be a proxy in front of the named service. TODO: If +// client's to use which may be a proxy in front of the named service. If // passingOnly is true only instances where both the service and any proxy are // healthy will be returned. func (h *Health) Connect(service, tag string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) { diff --git a/api/health_test.go b/api/health_test.go index 78867909f..f9356262d 100644 --- a/api/health_test.go +++ b/api/health_test.go @@ -7,7 +7,7 @@ import ( "github.com/hashicorp/consul/testutil" "github.com/hashicorp/consul/testutil/retry" "github.com/pascaldekloe/goe/verify" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPI_HealthNode(t *testing.T) { @@ -297,10 +297,7 @@ func TestAPI_HealthConnect(t *testing.T) { Port: 8000, } err := agent.ServiceRegister(reg) - // TODO replace with require.Nil when we have it vendored in OSS and rebased - if !assert.Nil(t, err) { - return - } + require.NoError(t, err) defer agent.ServiceDeregister("foo") // Register the proxy @@ -311,10 +308,7 @@ func TestAPI_HealthConnect(t *testing.T) { ProxyDestination: "foo", } err = agent.ServiceRegister(proxyReg) - // TODO replace with require.Nil when we have it vendored in OSS and rebased - if !assert.Nil(t, err) { - return - } + require.NoError(t, err) defer agent.ServiceDeregister("foo-proxy") retry.Run(t, func(r *retry.R) { diff --git a/connect/proxy/config_test.go b/connect/proxy/config_test.go index d395c9767..34080fb5a 100644 --- a/connect/proxy/config_test.go +++ b/connect/proxy/config_test.go @@ -179,9 +179,6 @@ func TestAgentConfigWatcher(t *testing.T) { assert.Equal(t, expectCfg, cfg) - // TODO(banks): Sanity check the service is viable and gets TLS certs eventually from - // the agent. - // Now keep watching and update the config. go func() { // Wait for watcher to be watching diff --git a/connect/service.go b/connect/service.go index b00775c1c..4db4ce8ca 100644 --- a/connect/service.go +++ b/connect/service.go @@ -47,8 +47,6 @@ type Service struct { // httpResolverFromAddr is a function that returns a Resolver from a string // address for HTTP clients. It's privately pluggable to make testing easier // but will default to a simple method to parse the host as a Consul DNS host. - // - // TODO(banks): write the proper implementation httpResolverFromAddr func(addr string) (Resolver, error) rootsWatch *watch.Plan @@ -217,7 +215,7 @@ func (s *Service) HTTPDialTLS(network, func (s *Service) HTTPClient() *http.Client { t := &http.Transport{ // Sadly we can't use DialContext hook since that is expected to return a - // plain TCP connection an http.Client tries to start a TLS handshake over + // plain TCP connection and http.Client tries to start a TLS handshake over // it. We need to control the handshake to be able to do our validation. // So we have to use the older DialTLS which means no context/timeout // support. diff --git a/connect/service_test.go b/connect/service_test.go index 64ca28fc7..ba4a9b0a0 100644 --- a/connect/service_test.go +++ b/connect/service_test.go @@ -171,7 +171,7 @@ func TestService_ServerTLSConfig(t *testing.T) { // After some time, both root and leaves should be different but both should // still be correct. oldRootSubjects := bytes.Join(tlsCfg.RootCAs.Subjects(), []byte(", ")) - //oldLeafSerial := connect.HexString(cert.SerialNumber.Bytes()) + oldLeafSerial := connect.HexString(cert.SerialNumber.Bytes()) oldLeafKeyID := connect.HexString(cert.SubjectKeyId) retry.Run(t, func(r *retry.R) { updatedCfg := service.ServerTLSConfig() @@ -188,14 +188,10 @@ func TestService_ServerTLSConfig(t *testing.T) { cert, err := x509.ParseCertificate(leaf.Certificate[0]) r.Check(err) - // TODO(banks): Current CA implementation resets the serial index when CA - // config changes which means same serial is issued by new CA config failing - // this test. Re-enable once the CA is changed to fix that. - - // if oldLeafSerial == connect.HexString(cert.SerialNumber.Bytes()) { - // r.Fatalf("leaf certificate should have changed, got serial %s", - // oldLeafSerial) - // } + if oldLeafSerial == connect.HexString(cert.SerialNumber.Bytes()) { + r.Fatalf("leaf certificate should have changed, got serial %s", + oldLeafSerial) + } if oldLeafKeyID == connect.HexString(cert.SubjectKeyId) { r.Fatalf("leaf should have a different key, got matching SubjectKeyID = %s", oldLeafKeyID)