Skip to content

Commit

Permalink
[v16] Auto Discover Server must not overwrite manual changes (#44848)
Browse files Browse the repository at this point in the history 
* Auto Discover Server must not overwrite manual changes

This PR changes the flow so that only discover managed installations
might have their teleport.yaml configuration replaced.

* Update lib/srv/server/installer/autodiscover.go

Co-authored-by: Ryan Clark <[email protected]>

* Update lib/srv/server/installer/autodiscover.go

Co-authored-by: Roman Tkachenko <[email protected]>

* Update lib/srv/server/installer/autodiscover.go

---------

Co-authored-by: Marco Dinis <[email protected]>
Co-authored-by: Ryan Clark <[email protected]>
Co-authored-by: Gavin Frazar <[email protected]>
  • Loading branch information
@r0mant @marcoandredinis
@ryanclark @GavinFrazar
4 people authored Jul 31, 2024
1 parent 81aa12a commit b29f53c
Show file tree
Hide file tree
Showing 2 changed files with 187 additions and 1 deletion.
32 changes: 32 additions & 0 deletions lib/srv/server/installer/autodiscover.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ import (
"github.com/gravitational/teleport/lib/utils/packagemanager"
)

const (
discoverNotice = "" +
"Teleport Discover has successfully configured /etc/teleport.yaml for this server to join the cluster.\n" +
"Discover might replace the configuration if the server fails to join the cluster.\n" +
"Please remove this file if you are managing this instance using another tool or doing so manually.\n"
)

// AutoDiscoverNodeInstallerConfig installs and configures a Teleport Server into the current system.
type AutoDiscoverNodeInstallerConfig struct {
Logger *slog.Logger
Expand Down Expand Up @@ -312,6 +319,7 @@ func (ani *AutoDiscoverNodeInstaller) configureTeleportNode(ctx context.Context,
_ = os.Remove(teleportYamlConfigurationPathNew)
}()

discoverNoticeFile := teleportYamlConfigurationPath + ".discover"
// Check if file already exists and has the same content that we are about to write
if _, err := os.Stat(teleportYamlConfigurationPath); err == nil {
hashExistingFile, err := checksum(teleportYamlConfigurationPath)
Expand All @@ -325,14 +333,38 @@ func (ani *AutoDiscoverNodeInstaller) configureTeleportNode(ctx context.Context,
}

if hashExistingFile == hashNewFile {
if err := os.WriteFile(discoverNoticeFile, []byte(discoverNotice), 0o644); err != nil {
return trace.Wrap(err)
}

return trace.AlreadyExists("teleport.yaml is up to date")
}

// If a previous /etc/teleport.yaml configuration file exists and is different from the target one, it might be because one of the following reasons:
// - discover installation params (eg token name) were changed
// - `$ teleport node configure` command produces a different output
// - teleport was manually installed / configured
//
// For the first two scenarios, it's fine, and even desired in most cases, to restart teleport with the new configuration.
//
// However, for the last scenario (teleport was manually installed), this flow must not replace the currently running teleport service configuration.
// To prevent this, this flow checks for the existence of the discover notice file, and only allows replacement if it does exist.
if _, err := os.Stat(discoverNoticeFile); err != nil {
ani.Logger.InfoContext(ctx, "Refusing to replace the existing teleport configuration. For the script to replace the existing configuration remove the Teleport configuration.",
"teleport_configuration", teleportYamlConfigurationPath,
"discover_notice_file", discoverNoticeFile)
return trace.BadParameter("missing discover notice file")
}
}

if err := os.Rename(teleportYamlConfigurationPathNew, teleportYamlConfigurationPath); err != nil {
return trace.Wrap(err)
}

if err := os.WriteFile(discoverNoticeFile, []byte(discoverNotice), 0o644); err != nil {
return trace.Wrap(err)
}

return nil
}

Expand Down
156 changes: 155 additions & 1 deletion lib/srv/server/installer/autodiscover_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,8 @@ func TestAutoDiscoverNode(t *testing.T) {
for binName, mockBin := range mockBins {
require.True(t, mockBin.Check(t), "mismatch between expected invocations and actual calls for %q", binName)
}
require.FileExists(t, testTempDir+"/etc/teleport.yaml")
require.FileExists(t, testTempDir+"/etc/teleport.yaml.discover")
})
}
}
Expand Down Expand Up @@ -277,6 +279,8 @@ func TestAutoDiscoverNode(t *testing.T) {
for binName, mockBin := range mockBins {
require.True(t, mockBin.Check(t), "mismatch between expected invocations and actual calls for %q", binName)
}
require.FileExists(t, testTempDir+"/etc/teleport.yaml")
require.FileExists(t, testTempDir+"/etc/teleport.yaml.discover")
})

t.Run("fails when imds server is not available", func(t *testing.T) {
Expand Down Expand Up @@ -314,9 +318,11 @@ func TestAutoDiscoverNode(t *testing.T) {
for binName, mockBin := range mockBins {
require.True(t, mockBin.Check(t), "mismatch between expected invocations and actual calls for %q", binName)
}
require.NoFileExists(t, testTempDir+"/etc/teleport.yaml")
require.NoFileExists(t, testTempDir+"/etc/teleport.yaml.discover")
})

t.Run("reconfigures and restarts if target teleport.yaml is different", func(t *testing.T) {
t.Run("reconfigures and restarts if target teleport.yaml is different and discover file exists", func(t *testing.T) {
distroConfig := wellKnownOS["ubuntu"]["24.04"]

testTempDir := t.TempDir()
Expand Down Expand Up @@ -345,6 +351,8 @@ func TestAutoDiscoverNode(t *testing.T) {

// create an existing teleport.yaml configuration file
require.NoError(t, os.WriteFile(testTempDir+"/etc/teleport.yaml", []byte("has wrong config"), 0o644))
// create a teleport.yaml.discover to indicate that this host is controlled by the discover flow
require.NoError(t, os.WriteFile(testTempDir+"/etc/teleport.yaml.discover", []byte(""), 0o644))

// package manager is not called in this scenario because teleport binary already exists in the system
require.FileExists(t, mockBins["teleport"].Path)
Expand Down Expand Up @@ -374,11 +382,72 @@ func TestAutoDiscoverNode(t *testing.T) {

require.NoFileExists(t, testTempDir+"/etc/teleport.yaml.new")
require.FileExists(t, testTempDir+"/etc/teleport.yaml")
require.FileExists(t, testTempDir+"/etc/teleport.yaml.discover")
bs, err := os.ReadFile(testTempDir + "/etc/teleport.yaml")
require.NoError(t, err)
require.Equal(t, "teleport.yaml configuration bytes", string(bs))
})

t.Run("does not reconfigure if teleport.yaml exists but discover file does not exists", func(t *testing.T) {
distroConfig := wellKnownOS["ubuntu"]["24.04"]

testTempDir := t.TempDir()

setupDirsForTest(t, testTempDir, distroConfig)

installerConfig := &AutoDiscoverNodeInstallerConfig{
RepositoryChannel: "stable/rolling",
AutoUpgrades: false,
ProxyPublicAddr: "proxy.example.com",
TeleportPackage: "teleport",
TokenName: "my-token",
AzureClientID: "azure-client-id",

fsRootPrefix: testTempDir,
imdsProviders: mockIMDSProviders,
binariesLocation: binariesLocation,
aptPublicKeyEndpoint: mockRepoKeys.URL,
}

teleportInstaller, err := NewAutoDiscoverNodeInstaller(installerConfig)
require.NoError(t, err)

// package manager is not called in this scenario because teleport binary already exists in the system
require.FileExists(t, mockBins["teleport"].Path)

// create an existing teleport.yaml configuration file
require.NoError(t, os.WriteFile(testTempDir+"/etc/teleport.yaml", []byte("has wrong config"), 0o644))

// package manager is not called in this scenario because teleport binary already exists in the system
require.FileExists(t, mockBins["teleport"].Path)

mockBins["teleport"].Expect("node",
"configure",
"--output=file://"+testTempDir+"/etc/teleport.yaml.new",
"--proxy=proxy.example.com",
"--join-method=azure",
"--token=my-token",
"--labels=teleport.internal/region=eastus,teleport.internal/resource-group=TestGroup,teleport.internal/subscription-id=5187AF11-3581-4AB6-A654-59405CD40C44,teleport.internal/vm-id=ED7DAC09-6E73-447F-BD18-AF4D1196C1E4",
"--azure-client-id=azure-client-id",
).AndCallFunc(func(c *bintest.Call) {
// create a teleport.yaml configuration file
require.NoError(t, os.WriteFile(testTempDir+"/etc/teleport.yaml.new", []byte("teleport.yaml configuration bytes"), 0o644))
c.Exit(0)
})

require.ErrorContains(t, teleportInstaller.Install(ctx), "missing discover notice file")

for binName, mockBin := range mockBins {
require.True(t, mockBin.Check(t), "mismatch between expected invocations and actual calls for %q", binName)
}

require.NoFileExists(t, testTempDir+"/etc/teleport.yaml.new")
require.FileExists(t, testTempDir+"/etc/teleport.yaml")
bs, err := os.ReadFile(testTempDir + "/etc/teleport.yaml")
require.NoError(t, err)
require.Equal(t, "has wrong config", string(bs))
})

t.Run("does nothing if teleport is already installed and target teleport.yaml configuration already exists", func(t *testing.T) {
distroName := "ubuntu"
distroVersion := "24.04"
Expand Down Expand Up @@ -440,6 +509,91 @@ func TestAutoDiscoverNode(t *testing.T) {
require.NoError(t, err)
require.Equal(t, "teleport.yaml configuration bytes", string(bs))
})

t.Run("does nothing if target teleport.yaml configuration exists but was manually created/edited", func(t *testing.T) {
distroName := "ubuntu"
distroVersion := "24.04"
distroConfig := wellKnownOS[distroName][distroVersion]

testTempDir := t.TempDir()

setupDirsForTest(t, testTempDir, distroConfig)

installerConfig := &AutoDiscoverNodeInstallerConfig{
RepositoryChannel: "stable/rolling",
AutoUpgrades: false,
ProxyPublicAddr: "proxy.example.com",
TeleportPackage: "teleport",
TokenName: "my-token",
AzureClientID: "azure-client-id",

fsRootPrefix: testTempDir,
imdsProviders: mockIMDSProviders,
binariesLocation: binariesLocation,
aptPublicKeyEndpoint: mockRepoKeys.URL,
}

teleportInstaller, err := NewAutoDiscoverNodeInstaller(installerConfig)
require.NoError(t, err)

// package manager is not called in this scenario because teleport binary already exists in the system
require.FileExists(t, mockBins["teleport"].Path)

// create an existing teleport.yaml configuration file
require.NoError(t, os.WriteFile(testTempDir+"/etc/teleport.yaml", []byte("teleport.yaml bytes"), 0o644))

// package manager is not called in this scenario because teleport binary already exists in the system
require.FileExists(t, mockBins["teleport"].Path)

mockBins["teleport"].Expect("node",
"configure",
"--output=file://"+testTempDir+"/etc/teleport.yaml.new",
"--proxy=proxy.example.com",
"--join-method=azure",
"--token=my-token",
"--labels=teleport.internal/region=eastus,teleport.internal/resource-group=TestGroup,teleport.internal/subscription-id=5187AF11-3581-4AB6-A654-59405CD40C44,teleport.internal/vm-id=ED7DAC09-6E73-447F-BD18-AF4D1196C1E4",
"--azure-client-id=azure-client-id",
).AndCallFunc(func(c *bintest.Call) {
// create a teleport.yaml configuration file
require.NoError(t, os.WriteFile(testTempDir+"/etc/teleport.yaml.new", []byte("teleport.yaml configuration bytes"), 0o644))
c.Exit(0)
})

require.ErrorContains(t, teleportInstaller.Install(ctx), "missing discover notice file")

for binName, mockBin := range mockBins {
require.True(t, mockBin.Check(t), "mismatch between expected invocations and actual calls for %q", binName)
}

t.Run("even if it runs multiple times", func(t *testing.T) {

// create an existing teleport.yaml configuration file
require.NoError(t, os.WriteFile(testTempDir+"/etc/teleport.yaml", []byte("manual configuration already exists"), 0o644))

// package manager is not called in this scenario because teleport binary already exists in the system
require.FileExists(t, mockBins["teleport"].Path)

mockBins["teleport"].Expect("node",
"configure",
"--output=file://"+testTempDir+"/etc/teleport.yaml.new",
"--proxy=proxy.example.com",
"--join-method=azure",
"--token=my-token",
"--labels=teleport.internal/region=eastus,teleport.internal/resource-group=TestGroup,teleport.internal/subscription-id=5187AF11-3581-4AB6-A654-59405CD40C44,teleport.internal/vm-id=ED7DAC09-6E73-447F-BD18-AF4D1196C1E4",
"--azure-client-id=azure-client-id",
).AndCallFunc(func(c *bintest.Call) {
// create a teleport.yaml configuration file
require.NoError(t, os.WriteFile(testTempDir+"/etc/teleport.yaml.new", []byte("teleport.yaml configuration bytes"), 0o644))
c.Exit(0)
})

require.ErrorContains(t, teleportInstaller.Install(ctx), "missing discover notice file")

for binName, mockBin := range mockBins {
require.True(t, mockBin.Check(t), "mismatch between expected invocations and actual calls for %q", binName)
}
})
})
}

// wellKnownOS lists the officially supported repositories for Linux Distros
Expand Down

0 comments on commit b29f53c

Please sign in to comment.