Skip to content

Commit

Permalink
[vcenterreceiver] TLS settings not honored for initial GetServiceCont…
Browse files Browse the repository at this point in the history
…ent call (open-telemetry#36482)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
The `govmomi` client used in the receiver attempts to validate the
connection to vcenter before the existing code sets the TLS options
(other than insecure) in the client. This is a limitation of the
`govmomi` wrapper, as discussed on this issue:
vmware/govmomi#1200 .

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Related issue in Grafana Alloy:
grafana/alloy#193

<!--Describe what testing was performed and which tests were added.-->
#### Testing
~~This has not been tested, I would appreciate the assistance of any
codeowner that could test.~~ See comments on the PR for test.

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
  • Loading branch information
2 people authored and ZenoCC-Peng committed Dec 6, 2024
1 parent c444a93 commit 59d25d7
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 36 deletions.
27 changes: 27 additions & 0 deletions .chloggen/vcenter-tls.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: vcenterreceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: The existing code did not honor TLS settings beyond 'insecure'. All TLS client config should now be honored.

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [36482]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user]
48 changes: 27 additions & 21 deletions receiver/vcenterreceiver/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import (
"strings"
"time"

"github.com/vmware/govmomi"
"github.com/vmware/govmomi/find"
"github.com/vmware/govmomi/object"
"github.com/vmware/govmomi/performance"
"github.com/vmware/govmomi/session"
"github.com/vmware/govmomi/view"
"github.com/vmware/govmomi/vim25"
"github.com/vmware/govmomi/vim25/mo"
Expand All @@ -30,14 +30,14 @@ import (

// vcenterClient is a client that collects data from a vCenter endpoint.
type vcenterClient struct {
logger *zap.Logger
moClient *govmomi.Client
vimDriver *vim25.Client
vsanDriver *vsan.Client
finder *find.Finder
pm *performance.Manager
vm *view.Manager
cfg *Config
logger *zap.Logger
sessionManager *session.Manager
vimDriver *vim25.Client
vsanDriver *vsan.Client
finder *find.Finder
pm *performance.Manager
vm *view.Manager
cfg *Config
}

var newVcenterClient = defaultNewVcenterClient
Expand All @@ -51,8 +51,8 @@ func defaultNewVcenterClient(l *zap.Logger, c *Config) *vcenterClient {

// EnsureConnection will establish a connection to the vSphere SDK if not already established
func (vc *vcenterClient) EnsureConnection(ctx context.Context) error {
if vc.moClient != nil {
sessionActive, _ := vc.moClient.SessionManager.SessionIsActive(ctx)
if vc.sessionManager != nil {
sessionActive, _ := vc.sessionManager.SessionIsActive(ctx)
if sessionActive {
return nil
}
Expand All @@ -62,24 +62,30 @@ func (vc *vcenterClient) EnsureConnection(ctx context.Context) error {
if err != nil {
return err
}
client, err := govmomi.NewClient(ctx, sdkURL, vc.cfg.Insecure)
if err != nil {
return fmt.Errorf("unable to connect to vSphere SDK on listed endpoint: %w", err)
}

soapClient := soap.NewClient(sdkURL, vc.cfg.Insecure)
tlsCfg, err := vc.cfg.LoadTLSConfig(ctx)
if err != nil {
return err
}
if tlsCfg != nil {
client.DefaultTransport().TLSClientConfig = tlsCfg
soapClient.DefaultTransport().TLSClientConfig = tlsCfg
}

client, err := vim25.NewClient(ctx, soapClient)
if err != nil {
return fmt.Errorf("unable to connect to vSphere SDK on listed endpoint: %w", err)
}

sessionManager := session.NewManager(client)

user := url.UserPassword(vc.cfg.Username, string(vc.cfg.Password))
err = client.Login(ctx, user)
err = sessionManager.Login(ctx, user)
if err != nil {
return fmt.Errorf("unable to login to vcenter sdk: %w", err)
}
vc.moClient = client
vc.vimDriver = client.Client
vc.sessionManager = sessionManager
vc.vimDriver = client
vc.finder = find.NewFinder(vc.vimDriver)
vc.pm = performance.NewManager(vc.vimDriver)
vc.vm = view.NewManager(vc.vimDriver)
Expand All @@ -94,8 +100,8 @@ func (vc *vcenterClient) EnsureConnection(ctx context.Context) error {

// Disconnect will logout of the autenticated session
func (vc *vcenterClient) Disconnect(ctx context.Context) error {
if vc.moClient != nil {
return vc.moClient.Logout(ctx)
if vc.sessionManager != nil {
return vc.sessionManager.Logout(ctx)
}
return nil
}
Expand Down
11 changes: 3 additions & 8 deletions receiver/vcenterreceiver/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"testing"

"github.com/stretchr/testify/require"
"github.com/vmware/govmomi"
"github.com/vmware/govmomi/find"
"github.com/vmware/govmomi/performance"
"github.com/vmware/govmomi/session"
Expand Down Expand Up @@ -292,10 +291,6 @@ func TestEmptyVAppInventoryListObjects(t *testing.T) {
func TestSessionReestablish(t *testing.T) {
simulator.Test(func(ctx context.Context, c *vim25.Client) {
sm := session.NewManager(c)
moClient := &govmomi.Client{
Client: c,
SessionManager: sm,
}
pw, _ := simulator.DefaultLogin.Password()
client := vcenterClient{
vimDriver: c,
Expand All @@ -307,19 +302,19 @@ func TestSessionReestablish(t *testing.T) {
Insecure: true,
},
},
moClient: moClient,
sessionManager: sm,
}
err := sm.Logout(ctx)
require.NoError(t, err)

connected, err := client.moClient.SessionManager.SessionIsActive(ctx)
connected, err := client.sessionManager.SessionIsActive(ctx)
require.NoError(t, err)
require.False(t, connected)

err = client.EnsureConnection(ctx)
require.NoError(t, err)

connected, err = client.moClient.SessionManager.SessionIsActive(ctx)
connected, err = client.sessionManager.SessionIsActive(ctx)
require.NoError(t, err)
require.True(t, connected)
})
Expand Down
11 changes: 4 additions & 7 deletions receiver/vcenterreceiver/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"time"

"github.com/stretchr/testify/require"
"github.com/vmware/govmomi"
"github.com/vmware/govmomi/find"
"github.com/vmware/govmomi/session"
"github.com/vmware/govmomi/simulator"
Expand Down Expand Up @@ -80,12 +79,10 @@ func TestIntegration(t *testing.T) {
s := session.NewManager(c)
newVcenterClient = func(l *zap.Logger, cfg *Config) *vcenterClient {
client := &vcenterClient{
logger: l,
cfg: cfg,
moClient: &govmomi.Client{
Client: c,
SessionManager: s,
},
logger: l,
cfg: cfg,
sessionManager: s,
vimDriver: c,
}
require.NoError(t, client.EnsureConnection(context.Background()))
client.vimDriver = c
Expand Down

0 comments on commit 59d25d7

Please sign in to comment.