Cherry-pick: #6375, #6376 (#6383)

Correct remove interaction exit path
Fixing failure cases and timeouts for Admiral config update

lib/apiservers/engine/backends/backends.go

@@ -29,6 +29,7 @@ import (
 	"github.com/go-openapi/runtime"
 	rc "github.com/go-openapi/runtime/client"
 	"github.com/go-openapi/swag"
+	"golang.org/x/sync/singleflight"
 
 	"github.com/vmware/vic/lib/apiservers/engine/backends/cache"
 	"github.com/vmware/vic/lib/apiservers/engine/backends/container"
@@ -84,6 +85,9 @@ type dynConfig struct {
 
 	Whitelist, Blacklist, Insecure registry.Set
 	remoteWl                       bool
+
+	group   singleflight.Group
+	lastCfg *dynConfig
 }
 
 func Init(portLayerAddr, product string, port uint, config *config.VirtualContainerHostConfigSpec) error {
@@ -360,6 +364,7 @@ func EventService() *events.Events {
 // registries lists. It returns true for each list where u matches that list.
 func (d *dynConfig) RegistryCheck(ctx context.Context, u *url.URL) (wl bool, bl bool, insecure bool) {
 	m := d.update(ctx)
+
 	us := u.String()
 	wl = len(m.Whitelist) == 0 || m.Whitelist.Match(us)
 	bl = len(m.Blacklist) == 0 || !m.Blacklist.Match(us)
@@ -368,40 +373,63 @@ func (d *dynConfig) RegistryCheck(ctx context.Context, u *url.URL) (wl bool, bl
 }
 
 func (d *dynConfig) update(ctx context.Context) *dynConfig {
-	d.Lock()
-	src := d.src
-	d.Unlock()
+	const key = "RegistryCheck"
+	resCh := d.group.DoChan(key, func() (interface{}, error) {
+		d.Lock()
+		src := d.src
+		d.Unlock()
 
-	c, err := src.Get(ctx)
-	if err != nil {
-		log.Warnf("error getting config from source: %s", err)
-	}
+		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+		defer cancel()
 
-	d.Lock()
-	defer d.Unlock()
+		c, err := src.Get(ctx)
+		if err != nil {
+			log.Warnf("error getting config from source: %s", err)
+		}
 
-	m := d
-	if c != nil {
-		// update config
-		if m, err = d.merged(c); err != nil {
-			log.Errorf("error updating config: %s", err)
-			m = d
-		} else {
-			if len(c.RegistryWhitelist) > 0 {
-				m.remoteWl = true
+		d.Lock()
+		defer d.Unlock()
+
+		m := d
+		if c != nil {
+			// update config
+			if m, err = d.merged(c); err != nil {
+				log.Errorf("error updating config: %s", err)
+				m = d
+			} else {
+				if len(c.RegistryWhitelist) > 0 {
+					m.remoteWl = true
+				}
+			}
+		} else if err == nil && src == d.src {
+			// err == nil and c == nil, which
+			// indicates no remote sources
+			// were found, try resetting the
+			// source for next time
+			if err := d.resetSrc(); err != nil {
+				log.Warnf("could not reset config source: %s", err)
 			}
 		}
-	} else if err == nil && src == d.src {
-		// err == nil and c == nil, which
-		// indicates no remote sources
-		// were found, try resetting the
-		// source for next time
-		if err := d.resetSrc(); err != nil {
-			log.Warnf("could not reset config source: %s", err)
-		}
-	}
 
-	return m
+		d.lastCfg = m
+		return m, nil
+	})
+
+	select {
+	case res := <-resCh:
+		return res.Val.(*dynConfig)
+	case <-ctx.Done():
+		return func() *dynConfig {
+			d.Lock()
+			defer d.Unlock()
+
+			if d.lastCfg == nil {
+				return d
+			}
+
+			return d.lastCfg
+		}()
+	}
 }
 
 func (d *dynConfig) resetSrc() error {
@@ -415,7 +443,9 @@ func (d *dynConfig) resetSrc() error {
 }
 
 func newDynConfig(ctx context.Context, c *config.VirtualContainerHostConfigSpec) (*dynConfig, error) {
-	d := &dynConfig{Cfg: c}
+	d := &dynConfig{
+		Cfg: c,
+	}
 	var err error
 	if d.Insecure, err = dynamic.ParseRegistries(c.InsecureRegistries); err != nil {
 		return nil, err

lib/apiservers/engine/backends/image.go

@@ -370,7 +370,9 @@ func (i *Image) PullImage(ctx context.Context, image, tag string, metaHeaders ma
 	}
 
 	// Check if url is contained within set of whitelisted or insecure registries
-	whitelistOk, _, insecureOk := vchConfig.RegistryCheck(ctx, hostnameURL)
+	regctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+	defer cancel()
+	whitelistOk, _, insecureOk := vchConfig.RegistryCheck(regctx, hostnameURL)
 	if !whitelistOk {
 		err = fmt.Errorf("Access denied to unauthorized registry (%s) while VCH is in whitelist mode", hostnameURL.Host)
 		log.Errorf(err.Error())

lib/apiservers/engine/backends/system.go

@@ -68,7 +68,7 @@ const (
 	systemProductName        = " VMware Product"
 	volumeStoresID           = "VolumeStores"
 	loginTimeout             = 20 * time.Second
-	infoTimeout              = 20 * time.Second
+	infoTimeout              = 5 * time.Second
 	vchWhitelistMode         = " Registry Whitelist Mode"
 	whitelistRegistriesLabel = " Whitelisted Registries"
 	insecureRegistriesLabel  = " Insecure Registries"
@@ -341,7 +341,9 @@ func (s *System) AuthenticateToRegistry(ctx context.Context, authConfig *types.A
 	}
 
 	// Check if registry is contained within whitelisted or insecure registries
-	whitelistOk, _, insecureOk := vchConfig.RegistryCheck(ctx, loginURL)
+	regctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+	defer cancel()
+	whitelistOk, _, insecureOk := vchConfig.RegistryCheck(regctx, loginURL)
 	if !whitelistOk {
 		msg := fmt.Sprintf("Access denied to unauthorized registry (%s) while VCH is in whitelist mode", loginURL.Host)
 		return msg, "", fmt.Errorf(msg)

lib/config/dynamic/admiral/source.go

@@ -118,7 +118,7 @@ func (a *source) Get(ctx context.Context) (*vchcfg.VirtualContainerHostConfigSpe
 
 	c, projs, err := a.discover(ctx)
 	if err != nil {
-		log.Warnf("error locating Admiral, returning last known config: %s", err)
+		log.Warnf("could not locate management portal, returning last known config: %s", err)
 		return lastCfg, nil
 	}
 
@@ -225,7 +225,7 @@ func (a *source) discover(ctx context.Context) (*client.Admiral, []string, error
 	for _, v := range vms {
 		u, token, err := admiralEndpoint(ctx, v)
 		if err != nil {
-			log.Warnf("ignoring potential product VM: %s", err)
+			log.Warnf("ignoring potential product VM %s: %s", v, err)
 			continue
 		}
 
@@ -387,25 +387,14 @@ func (o *productDiscovery) Discover(ctx context.Context, sess *session.Session)
 	}
 
 	service.User = sess.User
-	t, err := func() (*tags.RestClient, error) {
-		o.mu.Lock()
-		defer o.mu.Unlock()
-
-		t := o.clients[service.String()]
-		if t == nil {
-			t = tags.NewClient(service, sess.Insecure, sess.Thumbprint)
-			if err = t.Login(ctx); err != nil {
-				return nil, err
-			}
-			o.clients[service.String()] = t
-		}
 
-		return t, nil
-	}()
-
-	if err != nil {
-		return nil, err
+	o.mu.Lock()
+	t := o.clients[service.String()]
+	if t == nil {
+		t = tags.NewClient(service, sess.Insecure, sess.Thumbprint)
+		o.clients[service.String()] = t
 	}
+	o.mu.Unlock()
 
 	var tag string
 	tag, err = findOVATag(ctx, t)

lib/portlayer/attach/communication/connector.go

@@ -203,6 +203,11 @@ func (c *Connector) RemoveInteraction(id string) error {
 	}
 	c.mutex.Unlock()
 
+	// the !ok case, but let's check the actual condition that impacts us
+	if v == nil {
+		return nil
+	}
+
 	conn, err := v.Cleanup()
 	if conn != nil {
 		err = conn.Close()

pkg/vsphere/tags/rest_client.go

@@ -18,12 +18,12 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"net/url"
 	"strings"
+	"sync"
 
 	"github.com/pkg/errors"
 
@@ -32,29 +32,38 @@ import (
 
 const (
 	RestPrefix = "/rest"
+	loginURL   = "/com/vmware/cis/session"
 )
 
 type RestClient struct {
+	mu       sync.Mutex
 	host     string
 	scheme   string
 	endpoint *url.URL
+	user     *url.Userinfo
 	HTTP     *http.Client
 	cookies  []*http.Cookie
 }
 
 func NewClient(u *url.URL, insecure bool, thumbprint string) *RestClient {
+	endpoint := &url.URL{}
+	*endpoint = *u
 	Logger.Debugf("Create rest client")
-	u.Path = RestPrefix
+	endpoint.Path = RestPrefix
 
-	sc := soap.NewClient(u, insecure)
+	sc := soap.NewClient(endpoint, insecure)
 	if thumbprint != "" {
-		sc.SetThumbprint(u.Host, thumbprint)
+		sc.SetThumbprint(endpoint.Host, thumbprint)
 	}
 
+	user := endpoint.User
+	endpoint.User = nil
+
 	return &RestClient{
-		endpoint: u,
-		host:     u.Host,
-		scheme:   u.Scheme,
+		endpoint: endpoint,
+		user:     user,
+		host:     endpoint.Host,
+		scheme:   endpoint.Scheme,
 		HTTP:     &sc.Client,
 	}
 }
@@ -99,17 +108,17 @@ func (c *RestClient) clientRequest(ctx context.Context, method, path string, in
 		in = bytes.NewReader([]byte{})
 	}
 
-	req, err := http.NewRequest(method, fmt.Sprintf("%s%s", RestPrefix, path), in)
+	req, err := c.newRequest(method, path, in)
 	if err != nil {
 		return nil, nil, -1, errors.Wrap(err, "failed to create request")
 	}
 
 	req = req.WithContext(ctx)
-	req.URL.Host = c.host
-	req.URL.Scheme = c.scheme
+	c.mu.Lock()
 	if c.cookies != nil {
 		req.AddCookie(c.cookies[0])
 	}
+	c.mu.Unlock()
 
 	if headers != nil {
 		for k, v := range headers {
@@ -155,17 +164,15 @@ func (c *RestClient) handleResponse(resp *http.Response, err error) (io.ReadClos
 }
 
 func (c *RestClient) Login(ctx context.Context) error {
-	Logger.Debugf("Login to %s through rest API.", c.host)
+	c.mu.Lock()
+	defer c.mu.Unlock()
 
-	targetURL := c.endpoint.String() + "/com/vmware/cis/session"
+	Logger.Debugf("Login to %s through rest API.", c.host)
 
-	request, err := http.NewRequest("POST", targetURL, nil)
+	request, err := c.newRequest("POST", loginURL, nil)
 	if err != nil {
 		return errors.Wrap(err, "login failed")
 	}
-	request = request.WithContext(ctx)
-	password, _ := c.endpoint.User.Password()
-	request.SetBasicAuth(c.endpoint.User.Username(), password)
 	resp, err := c.HTTP.Do(request)
 	if err != nil {
 		return errors.Wrap(err, "login failed")
@@ -185,3 +192,17 @@ func (c *RestClient) Login(ctx context.Context) error {
 	Logger.Debugf("Login succeeded")
 	return nil
 }
+
+func (c *RestClient) newRequest(method, urlStr string, body io.Reader) (*http.Request, error) {
+	req, err := http.NewRequest(method, c.endpoint.String()+urlStr, body)
+	if err != nil {
+		return nil, err
+	}
+
+	if c.user != nil {
+		password, _ := c.user.Password()
+		req.SetBasicAuth(c.user.Username(), password)
+	}
+
+	return req, nil
+}