Skip to content

Commit

Permalink
Auto Discover Server must not overwrite manual changes
Browse files Browse the repository at this point in the history 
This PR changes the flow so that only discover managed installations
might have their teleport.yaml configuration replaced.
  • Loading branch information
@marcoandredinis
marcoandredinis committed Jul 26, 2024
1 parent d4e629d commit 28efbad
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, either remove the teleport configuration or create the discover notice file ($ touch /etc/teleport.yaml.discover).",
"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 28efbad

Please sign in to comment.