From e9077a6fb9db5bcadea342200f057c1dc6ffb9af Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Mon, 21 Aug 2023 21:53:43 +0400 Subject: [PATCH] feat: filter the hostname to produce nodename Fixes #7615 This extends the previous handling when Talos did `ToLower()` on the hostname to do the full filtering as expected. Signed-off-by: Andrey Smirnov --- .../k8s/internal/nodename/nodename.go | 56 ++++++++ .../k8s/internal/nodename/nodename_test.go | 73 +++++++++++ .../machined/pkg/controllers/k8s/nodename.go | 27 ++-- .../pkg/controllers/k8s/nodename_test.go | 123 ++++-------------- 4 files changed, 167 insertions(+), 112 deletions(-) create mode 100644 internal/app/machined/pkg/controllers/k8s/internal/nodename/nodename.go create mode 100644 internal/app/machined/pkg/controllers/k8s/internal/nodename/nodename_test.go diff --git a/internal/app/machined/pkg/controllers/k8s/internal/nodename/nodename.go b/internal/app/machined/pkg/controllers/k8s/internal/nodename/nodename.go new file mode 100644 index 000000000..bd1d416e3 --- /dev/null +++ b/internal/app/machined/pkg/controllers/k8s/internal/nodename/nodename.go @@ -0,0 +1,56 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +// Package nodename provides utility functions to generate nodenames. +package nodename + +import ( + "fmt" + "strings" +) + +// FromHostname converts a hostname to Kubernetes Node name. +// +// UNIX hostname has almost no restrictions, but Kubernetes Node name has +// to be RFC 1123 compliant. This function converts a hostname to a valid +// Kubernetes Node name (if possible). +// +// The allowed format is: +// +// [a-z0-9]([-a-z0-9]*[a-z0-9])? +// +//nolint:gocyclo +func FromHostname(hostname string) (string, error) { + nodename := strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z': + // allow lowercase + return r + case r >= 'A' && r <= 'Z': + // lowercase uppercase letters + return r - 'A' + 'a' + case r >= '0' && r <= '9': + // allow digits + return r + case r == '-' || r == '_': + // allow dash, convert underscore to dash + return '-' + case r == '.': + // allow dot + return '.' + default: + // drop anything else + return -1 + } + }, hostname) + + // now drop any dashes/dots at the beginning or end + nodename = strings.Trim(nodename, "-.") + + if len(nodename) == 0 { + return "", fmt.Errorf("could not convert hostname %q to a valid Kubernetes Node name", hostname) + } + + return nodename, nil +} diff --git a/internal/app/machined/pkg/controllers/k8s/internal/nodename/nodename_test.go b/internal/app/machined/pkg/controllers/k8s/internal/nodename/nodename_test.go new file mode 100644 index 000000000..a2f8e397e --- /dev/null +++ b/internal/app/machined/pkg/controllers/k8s/internal/nodename/nodename_test.go @@ -0,0 +1,73 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +package nodename_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/siderolabs/talos/internal/app/machined/pkg/controllers/k8s/internal/nodename" +) + +func TestFromHostname(t *testing.T) { + for _, test := range []struct { + hostname string + + expectedNodeName string + expectedError string + }{ + { + hostname: "foo", + + expectedNodeName: "foo", + }, + { + hostname: "foo_ია", + + expectedNodeName: "foo", + }, + { + hostname: "Node1", + + expectedNodeName: "node1", + }, + { + hostname: "MY_test_server_", + + expectedNodeName: "my-test-server", + }, + { + hostname: "123", + + expectedNodeName: "123", + }, + { + hostname: "-my-server-", + + expectedNodeName: "my-server", + }, + { + hostname: "კომპიუტერი", + + expectedError: "could not convert hostname \"კომპიუტერი\" to a valid Kubernetes Node name", + }, + { + hostname: "foo.bar.tld.", + + expectedNodeName: "foo.bar.tld", + }, + } { + t.Run(test.hostname, func(t *testing.T) { + nodename, err := nodename.FromHostname(test.hostname) + if test.expectedError != "" { + require.EqualError(t, err, test.expectedError) + } else { + require.NoError(t, err) + require.Equal(t, test.expectedNodeName, nodename) + } + }) + } +} diff --git a/internal/app/machined/pkg/controllers/k8s/nodename.go b/internal/app/machined/pkg/controllers/k8s/nodename.go index b3106edee..35b4b1bf9 100644 --- a/internal/app/machined/pkg/controllers/k8s/nodename.go +++ b/internal/app/machined/pkg/controllers/k8s/nodename.go @@ -7,15 +7,14 @@ package k8s import ( "context" "fmt" - "strings" "github.com/cosi-project/runtime/pkg/controller" - "github.com/cosi-project/runtime/pkg/resource" "github.com/cosi-project/runtime/pkg/safe" "github.com/cosi-project/runtime/pkg/state" "github.com/siderolabs/go-pointer" "go.uber.org/zap" + "github.com/siderolabs/talos/internal/app/machined/pkg/controllers/k8s/internal/nodename" "github.com/siderolabs/talos/pkg/machinery/resources/config" "github.com/siderolabs/talos/pkg/machinery/resources/k8s" "github.com/siderolabs/talos/pkg/machinery/resources/network" @@ -83,7 +82,7 @@ func (ctrl *NodenameController) Run(ctx context.Context, r controller.Runtime, l continue } - hostnameResource, err := r.Get(ctx, resource.NewMetadata(network.NamespaceName, network.HostnameStatusType, network.HostnameID, resource.VersionUndefined)) + hostnameStatus, err := safe.ReaderGetByID[*network.HostnameStatus](ctx, r, network.HostnameID) if err != nil { if state.IsNotFoundError(err) { continue @@ -92,22 +91,26 @@ func (ctrl *NodenameController) Run(ctx context.Context, r controller.Runtime, l return err } - hostnameStatus := hostnameResource.(*network.HostnameStatus).TypedSpec() - - if err = r.Modify( + if err = safe.WriterModify( ctx, + r, k8s.NewNodename(k8s.NamespaceName, k8s.NodenameID), - func(r resource.Resource) error { - nodename := r.(*k8s.Nodename) //nolint:errcheck,forcetypeassert + func(res *k8s.Nodename) error { + var hostname string if cfgProvider.Machine().Kubelet().RegisterWithFQDN() { - nodename.TypedSpec().Nodename = strings.ToLower(hostnameStatus.FQDN()) + hostname = hostnameStatus.TypedSpec().FQDN() } else { - nodename.TypedSpec().Nodename = strings.ToLower(hostnameStatus.Hostname) + hostname = hostnameStatus.TypedSpec().Hostname } - nodename.TypedSpec().HostnameVersion = hostnameResource.Metadata().Version().String() - nodename.TypedSpec().SkipNodeRegistration = cfgProvider.Machine().Kubelet().SkipNodeRegistration() + res.TypedSpec().Nodename, err = nodename.FromHostname(hostname) + if err != nil { + return err + } + + res.TypedSpec().HostnameVersion = hostnameStatus.Metadata().Version().String() + res.TypedSpec().SkipNodeRegistration = cfgProvider.Machine().Kubelet().SkipNodeRegistration() return nil }, diff --git a/internal/app/machined/pkg/controllers/k8s/nodename_test.go b/internal/app/machined/pkg/controllers/k8s/nodename_test.go index a4e4e78a0..d63c10e97 100644 --- a/internal/app/machined/pkg/controllers/k8s/nodename_test.go +++ b/internal/app/machined/pkg/controllers/k8s/nodename_test.go @@ -2,29 +2,21 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -//nolint:dupl package k8s_test import ( - "context" - "fmt" - "log" "net/url" - "sync" "testing" "time" - "github.com/cosi-project/runtime/pkg/controller/runtime" "github.com/cosi-project/runtime/pkg/resource" - "github.com/cosi-project/runtime/pkg/state" - "github.com/cosi-project/runtime/pkg/state/impl/inmem" - "github.com/cosi-project/runtime/pkg/state/impl/namespaced" + "github.com/cosi-project/runtime/pkg/resource/rtestutils" "github.com/siderolabs/go-pointer" - "github.com/siderolabs/go-retry/retry" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" + "github.com/siderolabs/talos/internal/app/machined/pkg/controllers/ctest" k8sctrl "github.com/siderolabs/talos/internal/app/machined/pkg/controllers/k8s" - "github.com/siderolabs/talos/pkg/logging" "github.com/siderolabs/talos/pkg/machinery/config/container" "github.com/siderolabs/talos/pkg/machinery/config/types/v1alpha1" "github.com/siderolabs/talos/pkg/machinery/resources/config" @@ -33,69 +25,13 @@ import ( ) type NodenameSuite struct { - suite.Suite - - state state.State - - runtime *runtime.Runtime - wg sync.WaitGroup - - ctx context.Context //nolint:containedctx - ctxCancel context.CancelFunc + ctest.DefaultSuite } -func (suite *NodenameSuite) SetupTest() { - suite.ctx, suite.ctxCancel = context.WithTimeout(context.Background(), 3*time.Minute) - - suite.state = state.WrapCore(namespaced.NewState(inmem.Build)) - - var err error - - suite.runtime, err = runtime.NewRuntime(suite.state, logging.Wrap(log.Writer())) - suite.Require().NoError(err) - - suite.Require().NoError(suite.runtime.RegisterController(&k8sctrl.NodenameController{})) - - suite.startRuntime() -} - -func (suite *NodenameSuite) startRuntime() { - suite.wg.Add(1) - - go func() { - defer suite.wg.Done() - - suite.Assert().NoError(suite.runtime.Run(suite.ctx)) - }() -} - -//nolint:dupl -func (suite *NodenameSuite) assertNodename(expected string) error { - resources, err := suite.state.List( - suite.ctx, - resource.NewMetadata(k8s.NamespaceName, k8s.NodenameType, "", resource.VersionUndefined), - ) - if err != nil { - return err - } - - if len(resources.Items) != 1 { - return retry.ExpectedErrorf("expected 1 item, got %d", len(resources.Items)) - } - - if resources.Items[0].Metadata().ID() != k8s.NodenameID { - return fmt.Errorf("unexpected ID") - } - - if resources.Items[0].(*k8s.Nodename).TypedSpec().Nodename != expected { - return retry.ExpectedErrorf( - "expected %q, got %q", - expected, - resources.Items[0].(*k8s.Nodename).TypedSpec().Nodename, - ) - } - - return nil +func (suite *NodenameSuite) assertNodename(expected string) { + rtestutils.AssertResources(suite.Ctx(), suite.T(), suite.State(), []resource.ID{k8s.NodenameID}, func(nodename *k8s.Nodename, asrt *assert.Assertions) { + asrt.Equal(expected, nodename.TypedSpec().Nodename) + }) } func (suite *NodenameSuite) TestDefault() { @@ -118,21 +54,15 @@ func (suite *NodenameSuite) TestDefault() { ), ) - suite.Require().NoError(suite.state.Create(suite.ctx, cfg)) + suite.Require().NoError(suite.State().Create(suite.Ctx(), cfg)) hostnameStatus := network.NewHostnameStatus(network.NamespaceName, network.HostnameID) - hostnameStatus.TypedSpec().Hostname = "Foo" + hostnameStatus.TypedSpec().Hostname = "Foo-" hostnameStatus.TypedSpec().Domainname = "bar.ltd" - suite.Require().NoError(suite.state.Create(suite.ctx, hostnameStatus)) + suite.Require().NoError(suite.State().Create(suite.Ctx(), hostnameStatus)) - suite.Assert().NoError( - retry.Constant(3*time.Second, retry.WithUnits(100*time.Millisecond)).Retry( - func() error { - return suite.assertNodename("foo") - }, - ), - ) + suite.assertNodename("foo") } func (suite *NodenameSuite) TestFQDN() { @@ -159,33 +89,26 @@ func (suite *NodenameSuite) TestFQDN() { ), ) - suite.Require().NoError(suite.state.Create(suite.ctx, cfg)) + suite.Require().NoError(suite.State().Create(suite.Ctx(), cfg)) hostnameStatus := network.NewHostnameStatus(network.NamespaceName, network.HostnameID) hostnameStatus.TypedSpec().Hostname = "foo" hostnameStatus.TypedSpec().Domainname = "bar.ltd" - suite.Require().NoError(suite.state.Create(suite.ctx, hostnameStatus)) + suite.Require().NoError(suite.State().Create(suite.Ctx(), hostnameStatus)) - suite.Assert().NoError( - retry.Constant(3*time.Second, retry.WithUnits(100*time.Millisecond)).Retry( - func() error { - return suite.assertNodename("foo.bar.ltd") - }, - ), - ) -} - -func (suite *NodenameSuite) TearDownTest() { - suite.T().Log("tear down") - - suite.ctxCancel() - - suite.wg.Wait() + suite.assertNodename("foo.bar.ltd") } func TestNodenameSuite(t *testing.T) { t.Parallel() - suite.Run(t, new(NodenameSuite)) + suite.Run(t, &NodenameSuite{ + DefaultSuite: ctest.DefaultSuite{ + Timeout: 3 * time.Second, + AfterSetup: func(s *ctest.DefaultSuite) { + s.Require().NoError(s.Runtime().RegisterController(&k8sctrl.NodenameController{})) + }, + }, + }) }