From 5d760c48adda0580eccbe55a0e9ac9b6ea9ef903 Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Wed, 2 Nov 2022 15:30:35 +0100 Subject: [PATCH] osbuild: remove Timeservers slice from chrony stage The org.osbuild.chrony stage was extended to allow additional directives alongside time servers [1]. The old Timeservers string slice was kept for backwards compatibility. Removing support for it in osbuild-composer makes working with the stage's options simpler. Using the new struct slice Servers field and only specifying a Hostname for each element is equivalent to the old behaviour, so no functionality is lost. This simplifies the chrony stage since no validation is required anymore. It also simplifies the propagation of configuration options through the pipeline generation code which doesn't need to check for both types of stage options. [1] https://github.com/osbuild/osbuild/pull/692 --- internal/distro/fedora/images.go | 6 ++++-- internal/distro/image_config_test.go | 14 +++++++------- internal/distro/rhel7/pipelines.go | 6 +++++- internal/distro/rhel8/distro.go | 2 +- internal/distro/rhel8/pipelines.go | 6 +++++- internal/distro/rhel9/gce.go | 2 +- internal/distro/rhel9/images.go | 7 +++++-- internal/distro/rhel9/pipelines.go | 6 +++++- internal/manifest/os.go | 9 +++++++-- internal/osbuild/chrony_stage.go | 22 ++-------------------- internal/osbuild/chrony_stage_test.go | 26 -------------------------- 11 files changed, 42 insertions(+), 64 deletions(-) diff --git a/internal/distro/fedora/images.go b/internal/distro/fedora/images.go index 06da64755..be4741e74 100644 --- a/internal/distro/fedora/images.go +++ b/internal/distro/fedora/images.go @@ -91,9 +91,11 @@ func osCustomizations( } if len(ntpServers) > 0 { - osc.NTPServers = ntpServers + for _, server := range ntpServers { + osc.NTPServers = append(osc.NTPServers, osbuild.ChronyConfigServer{Hostname: server}) + } } else if imageConfig.TimeSynchronization != nil { - osc.NTPServers = imageConfig.TimeSynchronization.Timeservers + osc.NTPServers = imageConfig.TimeSynchronization.Servers } // Relabel the tree, unless the `NoSElinux` flag is explicitly set to `true` diff --git a/internal/distro/image_config_test.go b/internal/distro/image_config_test.go index 470f0f3c8..ddf0779c4 100644 --- a/internal/distro/image_config_test.go +++ b/internal/distro/image_config_test.go @@ -20,7 +20,7 @@ func TestImageConfigInheritFrom(t *testing.T) { distroConfig: &ImageConfig{ Timezone: common.StringToPtr("America/New_York"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"127.0.0.1"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "127.0.0.1"}}, }, Locale: common.StringToPtr("en_US.UTF-8"), Keyboard: &osbuild.KeymapStageOptions{ @@ -123,7 +123,7 @@ func TestImageConfigInheritFrom(t *testing.T) { distroConfig: &ImageConfig{ Timezone: common.StringToPtr("America/New_York"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"127.0.0.1"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "127.0.0.1"}}, }, Locale: common.StringToPtr("en_US.UTF-8"), Keyboard: &osbuild.KeymapStageOptions{ @@ -137,7 +137,7 @@ func TestImageConfigInheritFrom(t *testing.T) { expectedConfig: &ImageConfig{ Timezone: common.StringToPtr("America/New_York"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"127.0.0.1"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "127.0.0.1"}}, }, Locale: common.StringToPtr("en_US.UTF-8"), Keyboard: &osbuild.KeymapStageOptions{ @@ -154,7 +154,7 @@ func TestImageConfigInheritFrom(t *testing.T) { imageConfig: &ImageConfig{ Timezone: common.StringToPtr("America/New_York"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"127.0.0.1"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "127.0.0.1"}}, }, Locale: common.StringToPtr("en_US.UTF-8"), Keyboard: &osbuild.KeymapStageOptions{ @@ -167,7 +167,7 @@ func TestImageConfigInheritFrom(t *testing.T) { expectedConfig: &ImageConfig{ Timezone: common.StringToPtr("America/New_York"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"127.0.0.1"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "127.0.0.1"}}, }, Locale: common.StringToPtr("en_US.UTF-8"), Keyboard: &osbuild.KeymapStageOptions{ @@ -184,7 +184,7 @@ func TestImageConfigInheritFrom(t *testing.T) { imageConfig: &ImageConfig{ Timezone: common.StringToPtr("America/New_York"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"127.0.0.1"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "127.0.0.1"}}, }, Locale: common.StringToPtr("en_US.UTF-8"), Keyboard: &osbuild.KeymapStageOptions{ @@ -197,7 +197,7 @@ func TestImageConfigInheritFrom(t *testing.T) { expectedConfig: &ImageConfig{ Timezone: common.StringToPtr("America/New_York"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"127.0.0.1"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "127.0.0.1"}}, }, Locale: common.StringToPtr("en_US.UTF-8"), Keyboard: &osbuild.KeymapStageOptions{ diff --git a/internal/distro/rhel7/pipelines.go b/internal/distro/rhel7/pipelines.go index b283c8a1a..07de17ae4 100644 --- a/internal/distro/rhel7/pipelines.go +++ b/internal/distro/rhel7/pipelines.go @@ -82,7 +82,11 @@ func osPipeline(t *imageType, } if len(ntpServers) > 0 { - p.AddStage(osbuild.NewChronyStage(&osbuild.ChronyStageOptions{Timeservers: ntpServers})) + servers := make([]osbuild.ChronyConfigServer, len(ntpServers)) + for idx, server := range ntpServers { + servers[idx] = osbuild.ChronyConfigServer{Hostname: server} + } + p.AddStage(osbuild.NewChronyStage(&osbuild.ChronyStageOptions{Servers: servers})) } else if imageConfig.TimeSynchronization != nil { p.AddStage(osbuild.NewChronyStage(imageConfig.TimeSynchronization)) } diff --git a/internal/distro/rhel8/distro.go b/internal/distro/rhel8/distro.go index f7b8a4bb5..dd0a5bc96 100644 --- a/internal/distro/rhel8/distro.go +++ b/internal/distro/rhel8/distro.go @@ -1348,7 +1348,7 @@ func newDistro(distroName string) distro.Distro { defaultGceByosImageConfig := &distro.ImageConfig{ Timezone: common.StringToPtr("UTC"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"metadata.google.internal"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "metadata.google.internal"}}, }, Firewall: &osbuild.FirewallStageOptions{ DefaultZone: "trusted", diff --git a/internal/distro/rhel8/pipelines.go b/internal/distro/rhel8/pipelines.go index 124d5684c..59180e30e 100644 --- a/internal/distro/rhel8/pipelines.go +++ b/internal/distro/rhel8/pipelines.go @@ -440,7 +440,11 @@ func osPipeline(t *imageType, } if len(ntpServers) > 0 { - p.AddStage(osbuild.NewChronyStage(&osbuild.ChronyStageOptions{Timeservers: ntpServers})) + servers := make([]osbuild.ChronyConfigServer, len(ntpServers)) + for idx, server := range ntpServers { + servers[idx] = osbuild.ChronyConfigServer{Hostname: server} + } + p.AddStage(osbuild.NewChronyStage(&osbuild.ChronyStageOptions{Servers: servers})) } else if imageConfig.TimeSynchronization != nil { p.AddStage(osbuild.NewChronyStage(imageConfig.TimeSynchronization)) } diff --git a/internal/distro/rhel9/gce.go b/internal/distro/rhel9/gce.go index 54336711c..d84b0d56d 100644 --- a/internal/distro/rhel9/gce.go +++ b/internal/distro/rhel9/gce.go @@ -57,7 +57,7 @@ var ( defaultGceImageConfig = &distro.ImageConfig{ Timezone: common.StringToPtr("UTC"), TimeSynchronization: &osbuild.ChronyStageOptions{ - Timeservers: []string{"metadata.google.internal"}, + Servers: []osbuild.ChronyConfigServer{{Hostname: "metadata.google.internal"}}, }, Firewall: &osbuild.FirewallStageOptions{ DefaultZone: "trusted", diff --git a/internal/distro/rhel9/images.go b/internal/distro/rhel9/images.go index c8028cb42..1f1694339 100644 --- a/internal/distro/rhel9/images.go +++ b/internal/distro/rhel9/images.go @@ -87,9 +87,12 @@ func osCustomizations( } if len(ntpServers) > 0 { - osc.NTPServers = ntpServers + for _, server := range ntpServers { + osc.NTPServers = append(osc.NTPServers, osbuild.ChronyConfigServer{Hostname: server}) + } } else if imageConfig.TimeSynchronization != nil { - osc.NTPServers = imageConfig.TimeSynchronization.Timeservers + osc.NTPServers = imageConfig.TimeSynchronization.Servers + osc.LeapSecTZ = imageConfig.TimeSynchronization.LeapsecTz } // Relabel the tree, unless the `NoSElinux` flag is explicitly set to `true` diff --git a/internal/distro/rhel9/pipelines.go b/internal/distro/rhel9/pipelines.go index 870c922d6..c66773b3e 100644 --- a/internal/distro/rhel9/pipelines.go +++ b/internal/distro/rhel9/pipelines.go @@ -307,7 +307,11 @@ func osPipeline(t *imageType, } if len(ntpServers) > 0 { - p.AddStage(osbuild.NewChronyStage(&osbuild.ChronyStageOptions{Timeservers: ntpServers})) + servers := make([]osbuild.ChronyConfigServer, len(ntpServers)) + for idx, server := range ntpServers { + servers[idx] = osbuild.ChronyConfigServer{Hostname: server} + } + p.AddStage(osbuild.NewChronyStage(&osbuild.ChronyStageOptions{Servers: servers})) } else if imageConfig.TimeSynchronization != nil { p.AddStage(osbuild.NewChronyStage(imageConfig.TimeSynchronization)) } diff --git a/internal/manifest/os.go b/internal/manifest/os.go index 97be19b69..8016d4a67 100644 --- a/internal/manifest/os.go +++ b/internal/manifest/os.go @@ -47,7 +47,6 @@ type OSCustomizations struct { Keyboard *string Hostname string Timezone string - NTPServers []string EnabledServices []string DisabledServices []string DefaultTarget string @@ -82,6 +81,8 @@ type OSCustomizations struct { AuthConfig *osbuild.AuthconfigStageOptions PwQuality *osbuild.PwqualityConfStageOptions OpenSCAPConfig *osbuild.OscapRemediationStageOptions + NTPServers []osbuild.ChronyConfigServer + LeapSecTZ *string Subscription *distro.SubscriptionImageOptions RHSMConfig map[distro.RHSMSubscriptionStatus]*osbuild.RHSMStageOptions @@ -264,7 +265,11 @@ func (p *OS) serialize() osbuild.Pipeline { pipeline.AddStage(osbuild.NewTimezoneStage(&osbuild.TimezoneStageOptions{Zone: p.Timezone})) if len(p.NTPServers) > 0 { - pipeline.AddStage(osbuild.NewChronyStage(&osbuild.ChronyStageOptions{Timeservers: p.NTPServers})) + chronyOptions := &osbuild.ChronyStageOptions{Servers: p.NTPServers} + if p.LeapSecTZ != nil { + chronyOptions.LeapsecTz = p.LeapSecTZ + } + pipeline.AddStage(osbuild.NewChronyStage(chronyOptions)) } if len(p.Groups) > 0 { diff --git a/internal/osbuild/chrony_stage.go b/internal/osbuild/chrony_stage.go index 507bfb7a0..400cb8c9a 100644 --- a/internal/osbuild/chrony_stage.go +++ b/internal/osbuild/chrony_stage.go @@ -1,15 +1,8 @@ package osbuild -import ( - "encoding/json" - "fmt" -) - -// Exactly one of 'Timeservers' or 'Servers' must be specified type ChronyStageOptions struct { - Timeservers []string `json:"timeservers,omitempty"` - Servers []ChronyConfigServer `json:"servers,omitempty"` - LeapsecTz *string `json:"leapsectz,omitempty"` + Servers []ChronyConfigServer `json:"servers,omitempty"` + LeapsecTz *string `json:"leapsectz,omitempty"` } func (ChronyStageOptions) isStageOptions() {} @@ -23,17 +16,6 @@ type ChronyConfigServer struct { Prefer *bool `json:"prefer,omitempty"` } -// Unexported alias for use in ChronyStageOptions's MarshalJSON() to prevent recursion -type chronyStageOptions ChronyStageOptions - -func (o ChronyStageOptions) MarshalJSON() ([]byte, error) { - if (len(o.Timeservers) != 0 && len(o.Servers) != 0) || (len(o.Timeservers) == 0 && len(o.Servers) == 0) { - return nil, fmt.Errorf("exactly one of 'Timeservers' or 'Servers' must be specified") - } - stageOptions := chronyStageOptions(o) - return json.Marshal(stageOptions) -} - func NewChronyStage(options *ChronyStageOptions) *Stage { return &Stage{ Type: "org.osbuild.chrony", diff --git a/internal/osbuild/chrony_stage_test.go b/internal/osbuild/chrony_stage_test.go index e0e30755a..0fd3aa082 100644 --- a/internal/osbuild/chrony_stage_test.go +++ b/internal/osbuild/chrony_stage_test.go @@ -1,7 +1,6 @@ package osbuild import ( - "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -15,28 +14,3 @@ func TestNewChronyStage(t *testing.T) { actualStage := NewChronyStage(&ChronyStageOptions{}) assert.Equal(t, expectedStage, actualStage) } - -func TestChronyStage_MarshalJSON_Invalid(t *testing.T) { - tests := []struct { - name string - options ChronyStageOptions - }{ - { - name: "not-timeservers-nor-servers", - options: ChronyStageOptions{}, - }, - { - name: "timeservers-and-servers", - options: ChronyStageOptions{ - Timeservers: []string{"ntp.example.com"}, - Servers: []ChronyConfigServer{{Hostname: "ntp2.example.com"}}, - }, - }, - } - for idx, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := json.Marshal(tt.options) - assert.NotNilf(t, err, "json.Marshal() didn't return an error [idx: %d]", idx) - }) - } -}