Skip to content

Commit

Permalink
Set global.platform when not set by the user (#451)
Browse files Browse the repository at this point in the history 
This change ensures that the `openshift` platform is set correctly even when the `openshift` profile isn't selected via `spec.profile`.

Users can now select a profile (e.g. `remote`) and still have the correct platform selected automatically.

Signed-off-by: Marko Lukša <[email protected]>
  • Loading branch information
@luksa
luksa authored Oct 28, 2024
1 parent 16c3e03 commit 1ac8e14
Show file tree
Hide file tree
Showing 14 changed files with 136 additions and 30 deletions.
6 changes: 3 additions & 3 deletions cmd/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,14 @@ func main() {
defaultProfile = "default"
}

err = istio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, defaultProfile).
err = istio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, platform, defaultProfile).
SetupWithManager(mgr)
if err != nil {
setupLog.Error(err, "unable to create controller", "controller", "Istio")
os.Exit(1)
}

err = remoteistio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, defaultProfile).
err = remoteistio.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, platform, defaultProfile).
SetupWithManager(mgr)
if err != nil {
setupLog.Error(err, "unable to create controller", "controller", "RemoteIstio")
Expand All @@ -158,7 +158,7 @@ func main() {
os.Exit(1)
}

err = istiocni.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, chartManager, defaultProfile).
err = istiocni.NewReconciler(mgr.GetClient(), mgr.GetScheme(), resourceDirectory, chartManager, platform, defaultProfile).
SetupWithManager(mgr)
if err != nil {
setupLog.Error(err, "unable to create controller", "controller", "IstioCNI")
Expand Down
7 changes: 5 additions & 2 deletions controllers/istio/istio_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"github.com/go-logr/logr"
"github.com/istio-ecosystem/sail-operator/api/v1alpha1"
"github.com/istio-ecosystem/sail-operator/pkg/config"
"github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger"
"github.com/istio-ecosystem/sail-operator/pkg/errlist"
"github.com/istio-ecosystem/sail-operator/pkg/kube"
Expand All @@ -46,14 +47,16 @@ import (
// Reconciler reconciles an Istio object
type Reconciler struct {
ResourceDirectory string
Platform config.Platform
DefaultProfile string
client.Client
Scheme *runtime.Scheme
}

func NewReconciler(client client.Client, scheme *runtime.Scheme, resourceDir string, defaultProfile string) *Reconciler {
func NewReconciler(client client.Client, scheme *runtime.Scheme, resourceDir string, platform config.Platform, defaultProfile string) *Reconciler {
return &Reconciler{
ResourceDirectory: resourceDir,
Platform: platform,
DefaultProfile: defaultProfile,
Client: client,
Scheme: scheme,
Expand Down Expand Up @@ -108,7 +111,7 @@ func validate(istio *v1alpha1.Istio) error {
func (r *Reconciler) reconcileActiveRevision(ctx context.Context, istio *v1alpha1.Istio) error {
values, err := revision.ComputeValues(
istio.Spec.Values, istio.Spec.Namespace, istio.Spec.Version,
r.DefaultProfile, istio.Spec.Profile,
r.Platform, r.DefaultProfile, istio.Spec.Profile,
r.ResourceDirectory, getActiveRevisionName(istio))
if err != nil {
return err
Expand Down
11 changes: 6 additions & 5 deletions controllers/istio/istio_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/istio-ecosystem/sail-operator/api/v1alpha1"
"github.com/istio-ecosystem/sail-operator/pkg/config"
"github.com/istio-ecosystem/sail-operator/pkg/scheme"
"github.com/istio-ecosystem/sail-operator/pkg/test/testtime"
"github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion"
Expand Down Expand Up @@ -60,7 +61,7 @@ func TestReconcile(t *testing.T) {
cl := newFakeClientBuilder().
WithObjects(istio).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "")

_, err := reconciler.Reconcile(ctx, istio)
if err == nil {
Expand Down Expand Up @@ -96,7 +97,7 @@ func TestReconcile(t *testing.T) {
WithStatusSubresource(&v1alpha1.Istio{}).
WithObjects(istio).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "invalid-profile")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "invalid-profile")

_, err := reconciler.Reconcile(ctx, istio)
if err == nil {
Expand Down Expand Up @@ -136,7 +137,7 @@ func TestReconcile(t *testing.T) {
},
}).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "")

_, err := reconciler.Reconcile(ctx, istio)
if err == nil {
Expand Down Expand Up @@ -532,7 +533,7 @@ func TestDetermineStatus(t *testing.T) {
WithObjects(initObjs...).
WithInterceptorFuncs(interceptorFuncs).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "")

status, err := reconciler.determineStatus(ctx, istio, tc.reconciliationErr)
if (err != nil) != tc.wantErr {
Expand Down Expand Up @@ -733,7 +734,7 @@ func TestUpdateStatus(t *testing.T) {
WithObjects(initObjs...).
WithInterceptorFuncs(interceptorFuncs).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "")

err := reconciler.updateStatus(ctx, istio, tc.reconciliationErr)
if (err != nil) != tc.wantErr {
Expand Down
7 changes: 5 additions & 2 deletions controllers/istiocni/istiocni_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,19 @@ const (
// Reconciler reconciles an IstioCNI object
type Reconciler struct {
ResourceDirectory string
Platform config.Platform
DefaultProfile string
client.Client
Scheme *runtime.Scheme
ChartManager *helm.ChartManager
}

func NewReconciler(
client client.Client, scheme *runtime.Scheme, resourceDir string, chartManager *helm.ChartManager, defaultProfile string,
client client.Client, scheme *runtime.Scheme, resourceDir string, chartManager *helm.ChartManager, platform config.Platform, defaultProfile string,
) *Reconciler {
return &Reconciler{
ResourceDirectory: resourceDir,
Platform: platform,
DefaultProfile: defaultProfile,
Client: client,
Scheme: scheme,
Expand Down Expand Up @@ -150,7 +152,8 @@ func (r *Reconciler) installHelmChart(ctx context.Context, cni *v1alpha1.IstioCN
userValues = applyImageDigests(cni, userValues, config.Config)

// apply userValues on top of defaultValues from profiles
mergedHelmValues, err := istiovalues.ApplyProfiles(r.ResourceDirectory, cni.Spec.Version, r.DefaultProfile, cni.Spec.Profile, helm.FromValues(userValues))
mergedHelmValues, err := istiovalues.ApplyProfilesAndPlatform(
r.ResourceDirectory, cni.Spec.Version, r.Platform, r.DefaultProfile, cni.Spec.Profile, helm.FromValues(userValues))
if err != nil {
return fmt.Errorf("failed to apply profile: %w", err)
}
Expand Down
6 changes: 3 additions & 3 deletions controllers/istiocni/istiocni_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func TestValidate(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)
cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tc.objects...).Build()
r := NewReconciler(cl, scheme.Scheme, "", nil, "")
r := NewReconciler(cl, scheme.Scheme, "", nil, config.PlatformKubernetes, "")

err := r.validate(context.TODO(), tc.cni)
if tc.expectErr == "" {
Expand Down Expand Up @@ -282,7 +282,7 @@ func TestDetermineReadyCondition(t *testing.T) {

cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tt.clientObjects...).WithInterceptorFuncs(tt.interceptors).Build()

r := NewReconciler(cl, scheme.Scheme, resourceDir, nil, "")
r := NewReconciler(cl, scheme.Scheme, resourceDir, nil, config.PlatformKubernetes, "")

cni := &v1alpha1.IstioCNI{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -464,7 +464,7 @@ func TestDetermineStatus(t *testing.T) {
ctx := context.TODO()
resourceDir := t.TempDir()
cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()
r := NewReconciler(cl, scheme.Scheme, resourceDir, nil, "")
r := NewReconciler(cl, scheme.Scheme, resourceDir, nil, config.PlatformKubernetes, "")

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
7 changes: 5 additions & 2 deletions controllers/remoteistio/remoteistio_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"github.com/go-logr/logr"
"github.com/istio-ecosystem/sail-operator/api/v1alpha1"
"github.com/istio-ecosystem/sail-operator/pkg/config"
"github.com/istio-ecosystem/sail-operator/pkg/errlist"
"github.com/istio-ecosystem/sail-operator/pkg/kube"
"github.com/istio-ecosystem/sail-operator/pkg/reconciler"
Expand All @@ -44,14 +45,16 @@ import (
// Reconciler reconciles a RemoteIstio object
type Reconciler struct {
ResourceDirectory string
Platform config.Platform
DefaultProfile string
client.Client
Scheme *runtime.Scheme
}

func NewReconciler(client client.Client, scheme *runtime.Scheme, resourceDir string, defaultProfile string) *Reconciler {
func NewReconciler(client client.Client, scheme *runtime.Scheme, resourceDir string, platform config.Platform, defaultProfile string) *Reconciler {
return &Reconciler{
ResourceDirectory: resourceDir,
Platform: platform,
DefaultProfile: defaultProfile,
Client: client,
Scheme: scheme,
Expand Down Expand Up @@ -106,7 +109,7 @@ func validate(istio *v1alpha1.RemoteIstio) error {
func (r *Reconciler) reconcileActiveRevision(ctx context.Context, istio *v1alpha1.RemoteIstio) error {
values, err := revision.ComputeValues(
istio.Spec.Values, istio.Spec.Namespace, istio.Spec.Version,
r.DefaultProfile, istio.Spec.Profile,
r.Platform, r.DefaultProfile, istio.Spec.Profile,
r.ResourceDirectory, getActiveRevisionName(istio))
if err != nil {
return err
Expand Down
11 changes: 6 additions & 5 deletions controllers/remoteistio/remoteistio_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/istio-ecosystem/sail-operator/api/v1alpha1"
"github.com/istio-ecosystem/sail-operator/pkg/config"
"github.com/istio-ecosystem/sail-operator/pkg/scheme"
"github.com/istio-ecosystem/sail-operator/pkg/test/testtime"
"github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion"
Expand Down Expand Up @@ -60,7 +61,7 @@ func TestReconcile(t *testing.T) {
cl := newFakeClientBuilder().
WithObjects(istio).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "")

_, err := reconciler.Reconcile(ctx, istio)
if err == nil {
Expand Down Expand Up @@ -96,7 +97,7 @@ func TestReconcile(t *testing.T) {
WithStatusSubresource(&v1alpha1.RemoteIstio{}).
WithObjects(istio).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "invalid-profile")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "invalid-profile")

_, err := reconciler.Reconcile(ctx, istio)
if err == nil {
Expand Down Expand Up @@ -136,7 +137,7 @@ func TestReconcile(t *testing.T) {
},
}).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "")

_, err := reconciler.Reconcile(ctx, istio)
if err == nil {
Expand Down Expand Up @@ -532,7 +533,7 @@ func TestDetermineStatus(t *testing.T) {
WithObjects(initObjs...).
WithInterceptorFuncs(interceptorFuncs).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "")

status, err := reconciler.determineStatus(ctx, istio, tc.reconciliationErr)
if (err != nil) != tc.wantErr {
Expand Down Expand Up @@ -733,7 +734,7 @@ func TestUpdateStatus(t *testing.T) {
WithObjects(initObjs...).
WithInterceptorFuncs(interceptorFuncs).
Build()
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, "")
reconciler := NewReconciler(cl, scheme.Scheme, resourceDir, config.PlatformKubernetes, "")

err := reconciler.updateStatus(ctx, istio, tc.reconciliationErr)
if (err != nil) != tc.wantErr {
Expand Down
1 change: 1 addition & 0 deletions pkg/config/platform.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
type Platform string

const (
PlatformUndefined Platform = ""
PlatformOpenShift Platform = "openshift"
PlatformKubernetes Platform = "kubernetes"
)
Expand Down
13 changes: 13 additions & 0 deletions pkg/helm/values.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,19 @@ func (h *Values) Set(key string, val any) error {
return unstructured.SetNestedField(*h, val, toKeys(key)...)
}

// SetIfAbsent sets the value of a nested field to a deep copy of the value
// provided if the field does not exist.
func (h *Values) SetIfAbsent(key string, val any) error {
if _, found, err := unstructured.NestedFieldNoCopy(*h, toKeys(key)...); err != nil {
return fmt.Errorf("failed to get value %s: %w", key, err)
} else if !found {
if err := h.Set(key, val); err != nil {
return fmt.Errorf("failed to set value %s: %w", key, err)
}
}
return nil
}

func toKeys(key string) []string {
return strings.Split(key, ".")
}
Expand Down
68 changes: 68 additions & 0 deletions pkg/helm/values_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,71 @@ func TestSet(t *testing.T) {
})
}
}

func TestSetIfAbsent(t *testing.T) {
tests := []struct {
name string
input Values
key string
val string
expected Values
expectErr bool
}{
{
name: "Key Exists",
input: Values{
"foo": map[string]any{
"bar": "baz",
},
},
key: "foo.bar",
val: "newVal",
expected: Values{
"foo": map[string]any{
"bar": "baz",
},
},
},
{
name: "New Key",
input: Values{
"foo": map[string]any{
"bar": "baz",
},
},
key: "foo.baz",
val: "newVal",
expected: Values{
"foo": map[string]any{
"bar": "baz",
"baz": "newVal",
},
},
},
{
name: "Key Exists, but is not a map",
input: Values{"foo": "bar"},
key: "foo.baz",
val: "newVal",
expectErr: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
err := test.input.SetIfAbsent(test.key, test.val)
if test.expectErr {
if err == nil {
t.Errorf("Expected an error, but got nil")
}
} else {
if err != nil {
t.Errorf("Expected no error, but got an error: %v", err)
}
if !reflect.DeepEqual(test.input, test.expected) {
t.Errorf("Expected %v, but got %v", test.expected, test.input)
}
}
})
}
}
14 changes: 12 additions & 2 deletions pkg/istiovalues/profiles.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"os"
"path"

"github.com/istio-ecosystem/sail-operator/pkg/config"
"github.com/istio-ecosystem/sail-operator/pkg/helm"
"github.com/istio-ecosystem/sail-operator/pkg/reconciler"
"gopkg.in/yaml.v3"
Expand All @@ -27,13 +28,22 @@ import (
"istio.io/istio/pkg/util/sets"
)

func ApplyProfiles(resourceDir string, version string, defaultProfile, userProfile string, userValues helm.Values) (helm.Values, error) {
func ApplyProfilesAndPlatform(
resourceDir string, version string, platform config.Platform, defaultProfile, userProfile string, userValues helm.Values,
) (helm.Values, error) {
profile := resolve(defaultProfile, userProfile)
defaultValues, err := getValuesFromProfiles(path.Join(resourceDir, version, "profiles"), profile)
if err != nil {
return nil, fmt.Errorf("failed to get values from profile %q: %w", profile, err)
}
return mergeOverwrite(defaultValues, userValues), nil
values := helm.Values(mergeOverwrite(defaultValues, userValues))

if platform != config.PlatformKubernetes && platform != config.PlatformUndefined {
if err = values.SetIfAbsent("global.platform", string(platform)); err != nil {
return nil, fmt.Errorf("failed to set global.platform: %w", err)
}
}
return values, nil
}

func resolve(defaultProfile, userProfile string) []string {
Expand Down
Loading

0 comments on commit 1ac8e14

Please sign in to comment.