Misc comment cleanups

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"`

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
+	// and servers in a cluster for correct connect operation.
-	// that.
 	ConnectEnabled bool
 
 	// ConnectProxyBindMinPort is the inclusive start of the range of ports

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) {

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
+	require.NoError(t, err)
-	if !assert.Nil(t, err) {
-		return
-	}
 	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
+	require.NoError(t, err)
-	if !assert.Nil(t, err) {
-		return
-	}
 	defer agent.ServiceDeregister("foo-proxy")
 
 	retry.Run(t, func(r *retry.R) {

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

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.

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
+		if oldLeafSerial == connect.HexString(cert.SerialNumber.Bytes()) {
-		// config changes which means same serial is issued by new CA config failing
+			r.Fatalf("leaf certificate should have changed, got serial %s",
-		// this test. Re-enable once the CA is changed to fix that.
+				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)