From f06e66b94bc7aaba241ca04e5ecbc67cd18e2d68 Mon Sep 17 00:00:00 2001 From: Tomas Hozza Date: Thu, 1 Jul 2021 12:00:22 +0200 Subject: [PATCH] osbuild2: support for `servers` option in `org.osbuild.chrony` Add support for the newly added `servers` option in the `org.osbuild.chrony` osbuild stage [1]. The option allows one to specify timeservers to be used by chrony, including a subset of lower-level configuration options per each server. Implement a custom JSON marshaller method for `ChronyStageOptions` to ensure that exactly one of 'timeservers' or 'servers' is specified, as mandated by the stage schema. Optional values in `ChronyConfigServer` are declared as pointers to distinguish the case when the value was explicitly set by the user from the default value when the structure instance is created. All of these options should be omitted from the JSON, but only when not explicitly set, not when their value us "0" for int or "false" for bool. Downside of this approach is that one can not easily use pointer to a basic type literal in the struct literal. Passing the basic type literal has to be workarounded using an intermediate variable, which address is used in the struct literal. Add unit test cases for the added functionality. [1] https://github.com/osbuild/osbuild/pull/692 Signed-off-by: Tomas Hozza --- internal/osbuild2/chrony_stage.go | 32 +++++++++++++++++++++- internal/osbuild2/chrony_stage_test.go | 26 ++++++++++++++++++ internal/osbuild2/stage_test.go | 37 +++++++++++++++++++++++--- 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/internal/osbuild2/chrony_stage.go b/internal/osbuild2/chrony_stage.go index 4233d02e5..1e3e4835c 100644 --- a/internal/osbuild2/chrony_stage.go +++ b/internal/osbuild2/chrony_stage.go @@ -1,11 +1,41 @@ package osbuild2 +import ( + "encoding/json" + "fmt" +) + +// Exactly one of 'Timeservers' or 'Servers' must be specified type ChronyStageOptions struct { - Timeservers []string `json:"timeservers"` + Timeservers []string `json:"timeservers,omitempty"` + Servers []ChronyConfigServer `json:"servers,omitempty"` } func (ChronyStageOptions) isStageOptions() {} +// Use '*ToPtr()' functions from the internal/common package to set the pointer values in literals +type ChronyConfigServer struct { + Hostname string `json:"hostname"` + Minpoll *int `json:"minpoll,omitempty"` + Maxpoll *int `json:"maxpoll,omitempty"` + Iburst *bool `json:"iburst,omitempty"` + Prefer *bool `json:"prefer,omitempty"` +} + +// Unexported struct for use in ChronyStageOptions's MarshalJSON() to prevent recursion +type chronyStageOptions struct { + Timeservers []string `json:"timeservers,omitempty"` + Servers []ChronyConfigServer `json:"servers,omitempty"` +} + +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/osbuild2/chrony_stage_test.go b/internal/osbuild2/chrony_stage_test.go index afb8ef8ce..2d8e390c0 100644 --- a/internal/osbuild2/chrony_stage_test.go +++ b/internal/osbuild2/chrony_stage_test.go @@ -1,6 +1,7 @@ package osbuild2 import ( + "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -14,3 +15,28 @@ 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) + }) + } +} diff --git a/internal/osbuild2/stage_test.go b/internal/osbuild2/stage_test.go index 86c4cb57b..6f5b073fa 100644 --- a/internal/osbuild2/stage_test.go +++ b/internal/osbuild2/stage_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/google/uuid" + "github.com/osbuild/osbuild-composer/internal/common" ) func TestStage_UnmarshalJSON(t *testing.T) { @@ -83,13 +84,41 @@ func TestStage_UnmarshalJSON(t *testing.T) { }, }, { - name: "chrony", + name: "chrony-timeservers", fields: fields{ - Type: "org.osbuild.chrony", - Options: &ChronyStageOptions{}, + Type: "org.osbuild.chrony", + Options: &ChronyStageOptions{ + Timeservers: []string{ + "ntp1.example.com", + "ntp2.example.com", + }, + }, }, args: args{ - data: []byte(`{"type":"org.osbuild.chrony","options":{"timeservers":null}}`), + data: []byte(`{"type":"org.osbuild.chrony","options":{"timeservers":["ntp1.example.com","ntp2.example.com"]}}`), + }, + }, + { + name: "chrony-servers", + fields: fields{ + Type: "org.osbuild.chrony", + Options: &ChronyStageOptions{ + Servers: []ChronyConfigServer{ + { + Hostname: "127.0.0.1", + Minpoll: common.IntToPtr(0), + Maxpoll: common.IntToPtr(4), + Iburst: common.BoolToPtr(true), + Prefer: common.BoolToPtr(false), + }, + { + Hostname: "ntp.example.com", + }, + }, + }, + }, + args: args{ + data: []byte(`{"type":"org.osbuild.chrony","options":{"servers":[{"hostname":"127.0.0.1","minpoll":0,"maxpoll":4,"iburst":true,"prefer":false},{"hostname":"ntp.example.com"}]}}`), }, }, {