Skip to content

Commit 22a4f65

Browse files
atburkeatburke
committed
Fix GCP metadata errors crashing Teleport
1 parent 9c87991 commit 22a4f65

File tree

5 files changed

+182
-22
lines changed

5 files changed

+182
-22
lines changed

‎lib/cloud/gcp/vm_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,8 @@ func TestConvertAPIError(t *testing.T) {
7373
}
7474
for _, tc := range tests {
7575
t.Run(tc.name, func(t *testing.T) {
76-
require.True(t, trace.IsAccessDenied(convertAPIError(tc.err)))
76+
err := convertAPIError(tc.err)
77+
require.True(t, trace.IsAccessDenied(err), "unexpected error of type %T: %v", tc.err, err)
7778
require.Contains(t, tc.err.Error(), "abcd1234")
7879
})
7980
}

‎lib/cloud/imds/gcp/imds.go

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,23 @@ func NewInstanceMetadataClient(ctx context.Context) (*InstanceMetadataClient, er
7676

7777
// IsAvailable checks if instance metadata is available.
7878
func (client *InstanceMetadataClient) IsAvailable(ctx context.Context) bool {
79-
instanceData, err := client.getMetadata(ctx, "instance")
80-
return err == nil && instanceData != ""
79+
_, err := client.getNumericID(ctx)
80+
return err == nil
8181
}
8282

83-
// GetTags gets all of the GCP instance's labels (note: these are separate from
84-
// its tags, which we do not use).
83+
func (client *InstanceMetadataClient) getNumericID(ctx context.Context) (uint64, error) {
84+
idStr, err := client.GetID(ctx)
85+
if err != nil {
86+
return 0, trace.Wrap(err)
87+
}
88+
id, err := strconv.ParseUint(idStr, 10, 64)
89+
if err != nil || id == 0 {
90+
return 0, trace.BadParameter("Invalid instance ID %q", idStr)
91+
}
92+
return id, nil
93+
}
94+
95+
// GetTags gets all of the GCP instance's labels and tags.
8596
func (client *InstanceMetadataClient) GetTags(ctx context.Context) (map[string]string, error) {
8697
// Get a bunch of info from instance metadata.
8798
projectID, err := client.GetProjectID(ctx)
@@ -96,11 +107,7 @@ func (client *InstanceMetadataClient) GetTags(ctx context.Context) (map[string]s
96107
if err != nil {
97108
return nil, trace.Wrap(err)
98109
}
99-
idStr, err := client.GetID(ctx)
100-
if err != nil {
101-
return nil, trace.Wrap(err)
102-
}
103-
id, err := strconv.ParseUint(idStr, 10, 64)
110+
id, err := client.getNumericID(ctx)
104111
if err != nil {
105112
return nil, trace.Wrap(err)
106113
}

‎lib/cloud/imds/gcp/imds_test.go

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,16 +58,51 @@ func (m *mockInstanceGetter) GetInstanceTags(ctx context.Context, req *gcp.Insta
5858
func TestIsInstanceMetadataAvailable(t *testing.T) {
5959
t.Parallel()
6060

61-
t.Run("not available", func(t *testing.T) {
62-
client := &InstanceMetadataClient{
61+
tests := []struct {
62+
name string
63+
getMetadata metadataGetter
64+
assert require.BoolAssertionFunc
65+
}{
66+
{
67+
name: "not available",
6368
getMetadata: func(ctx context.Context, path string) (string, error) {
6469
return "", trace.NotFound("")
6570
},
66-
}
67-
require.False(t, client.IsAvailable(context.Background()))
68-
})
71+
assert: require.False,
72+
},
73+
{
74+
name: "not on gcp",
75+
getMetadata: func(ctx context.Context, path string) (string, error) {
76+
return "non-numeric id", nil
77+
},
78+
assert: require.False,
79+
},
80+
{
81+
name: "zero ID",
82+
getMetadata: func(ctx context.Context, path string) (string, error) {
83+
return "0", nil
84+
},
85+
assert: require.False,
86+
},
87+
{
88+
name: "on mocked gcp",
89+
getMetadata: func(ctx context.Context, path string) (string, error) {
90+
return "12345678", nil
91+
},
92+
assert: require.True,
93+
},
94+
}
95+
96+
for _, tc := range tests {
97+
t.Run(tc.name, func(t *testing.T) {
98+
client := &InstanceMetadataClient{
99+
getMetadata: tc.getMetadata,
100+
}
101+
tc.assert(t, client.IsAvailable(context.Background()))
102+
})
103+
}
69104

70-
t.Run("on gcp", func(t *testing.T) {
105+
t.Run("on real gcp", func(t *testing.T) {
71106
if os.Getenv("TELEPORT_TEST_GCP") == "" {
72107
t.Skip("not on gcp")
73108
}

‎lib/service/service.go

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -955,8 +955,14 @@ func NewTeleport(cfg *servicecfg.Config) (*TeleportProcess, error) {
955955
imClient := cfg.InstanceMetadataClient
956956
if imClient == nil {
957957
imClient, err = cloud.DiscoverInstanceMetadata(supervisor.ExitContext())
958-
if err != nil && !trace.IsNotFound(err) {
959-
return nil, trace.Wrap(err)
958+
if err == nil {
959+
cfg.Logger.InfoContext(supervisor.ExitContext(),
960+
"Found an instance metadata service. Teleport will import labels from this cloud instance.",
961+
"type", imClient.GetType())
962+
} else if !trace.IsNotFound(err) {
963+
cfg.Logger.ErrorContext(supervisor.ExitContext(), "Error looking for cloud instance metadata", "error", err)
964+
// Keep going. Not being able to fetch labels isn't necessarily an error (e.g. the user doesn't need imported
965+
// labels and hasn't configured their cloud instance for it).
960966
}
961967
}
962968

@@ -965,23 +971,25 @@ func NewTeleport(cfg *servicecfg.Config) (*TeleportProcess, error) {
965971
if err == nil {
966972
cloudHostname = strings.ReplaceAll(cloudHostname, " ", "_")
967973
if utils.IsValidHostname(cloudHostname) {
968-
cfg.Logger.InfoContext(supervisor.ExitContext(), "Overriding hostname with value from cloud tag TeleportHostname.", "hostname", cloudHostname)
974+
cfg.Logger.InfoContext(supervisor.ExitContext(), "Overriding hostname with value from cloud tag TeleportHostname", "hostname", cloudHostname)
969975
cfg.Hostname = cloudHostname
970976

971977
// cloudHostname exists but is not a valid hostname.
972978
} else if cloudHostname != "" {
973-
cfg.Logger.InfoContext(supervisor.ExitContext(), "Found invalid hostname in cloud tag TeleportHostname.", "hostname", cloudHostname)
979+
cfg.Logger.InfoContext(supervisor.ExitContext(), "Found invalid hostname in cloud tag TeleportHostname", "hostname", cloudHostname)
974980
}
975981
} else if !trace.IsNotFound(err) {
976-
return nil, trace.Wrap(err)
982+
cfg.Logger.ErrorContext(supervisor.ExitContext(), "Error looking for hostname tag", "error", err)
983+
// Keep going.
977984
}
978985

979986
cloudLabels, err = labels.NewCloudImporter(supervisor.ExitContext(), &labels.CloudConfig{
980987
Client: imClient,
981988
Clock: cfg.Clock,
982989
})
983990
if err != nil {
984-
return nil, trace.Wrap(err)
991+
cfg.Logger.ErrorContext(supervisor.ExitContext(), "Cloud labels will not be imported", "error", err)
992+
// Keep going.
985993
}
986994
}
987995

‎lib/service/service_test.go

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1620,3 +1620,112 @@ func TestDebugServiceStartSocket(t *testing.T) {
16201620
defer req.Body.Close()
16211621
require.Equal(t, http.StatusNotFound, req.StatusCode)
16221622
}
1623+
1624+
type mockInstanceMetadata struct {
1625+
hostname string
1626+
hostnameErr error
1627+
}
1628+
1629+
func (m *mockInstanceMetadata) IsAvailable(ctx context.Context) bool {
1630+
return true
1631+
}
1632+
1633+
func (m *mockInstanceMetadata) GetTags(ctx context.Context) (map[string]string, error) {
1634+
return nil, nil
1635+
}
1636+
1637+
func (m *mockInstanceMetadata) GetHostname(ctx context.Context) (string, error) {
1638+
return m.hostname, m.hostnameErr
1639+
}
1640+
1641+
func (m *mockInstanceMetadata) GetType() types.InstanceMetadataType {
1642+
return "mock"
1643+
}
1644+
1645+
func (m *mockInstanceMetadata) GetID(ctx context.Context) (string, error) {
1646+
return "", nil
1647+
}
1648+
1649+
func TestInstanceMetadata(t *testing.T) {
1650+
t.Parallel()
1651+
1652+
newCfg := func() *servicecfg.Config {
1653+
cfg := servicecfg.MakeDefaultConfig()
1654+
cfg.Hostname = "default.example.com"
1655+
cfg.SetAuthServerAddress(utils.NetAddr{AddrNetwork: "tcp", Addr: "127.0.0.1:0"})
1656+
cfg.Auth.ListenAddr = utils.NetAddr{AddrNetwork: "tcp", Addr: "127.0.0.1:0"}
1657+
cfg.Proxy.Enabled = false
1658+
cfg.SSH.Enabled = false
1659+
cfg.CircuitBreakerConfig = breaker.NoopBreakerConfig()
1660+
return cfg
1661+
}
1662+
1663+
tests := []struct {
1664+
name string
1665+
imClient imds.Client
1666+
expectCloudLabels bool
1667+
expectedHostname string
1668+
}{
1669+
{
1670+
name: "no instance metadata",
1671+
imClient: imds.NewDisabledIMDSClient(),
1672+
expectCloudLabels: false,
1673+
expectedHostname: "default.example.com",
1674+
},
1675+
{
1676+
name: "instance metadata with valid hostname",
1677+
imClient: &mockInstanceMetadata{
1678+
hostname: "new.example.com",
1679+
},
1680+
expectCloudLabels: true,
1681+
expectedHostname: "new.example.com",
1682+
},
1683+
{
1684+
name: "instance metadata with no hostname",
1685+
imClient: &mockInstanceMetadata{
1686+
hostnameErr: trace.NotFound(""),
1687+
},
1688+
expectCloudLabels: true,
1689+
expectedHostname: "default.example.com",
1690+
},
1691+
{
1692+
name: "instance metadata with invalid hostname",
1693+
imClient: &mockInstanceMetadata{
1694+
hostname: ")7%#(*&@())",
1695+
},
1696+
expectCloudLabels: true,
1697+
expectedHostname: "default.example.com",
1698+
},
1699+
{
1700+
name: "instance metadata with hostname error",
1701+
imClient: &mockInstanceMetadata{
1702+
hostnameErr: trace.Errorf(""),
1703+
},
1704+
expectCloudLabels: true,
1705+
expectedHostname: "default.example.com",
1706+
},
1707+
}
1708+
1709+
for _, tc := range tests {
1710+
t.Run(tc.name, func(t *testing.T) {
1711+
cfg := newCfg()
1712+
cfg.DataDir = t.TempDir()
1713+
cfg.Auth.StorageConfig.Params["path"] = t.TempDir()
1714+
cfg.InstanceMetadataClient = tc.imClient
1715+
1716+
process, err := NewTeleport(cfg)
1717+
require.NoError(t, err)
1718+
t.Cleanup(func() {
1719+
require.NoError(t, process.Close())
1720+
})
1721+
1722+
if tc.expectCloudLabels {
1723+
require.NotNil(t, process.cloudLabels)
1724+
} else {
1725+
require.Nil(t, process.cloudLabels)
1726+
}
1727+
1728+
require.Equal(t, tc.expectedHostname, cfg.Hostname)
1729+
})
1730+
}
1731+
}

0 commit comments

Comments
 (0)