Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support typed parameter values #1376

Merged
merged 26 commits into from
Mar 6, 2020
Merged

Support typed parameter values #1376

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
749ae25
Support typed parameter values
Feb 28, 2020
effeed1
Update golden files
Mar 2, 2020
61c93dd
Fix a typo and document conversion functions
Mar 2, 2020
3e7da70
Fix 'parse-param-operator-in-operator'
Mar 2, 2020
bb35f03
Address review comments
Mar 3, 2020
13999e7
Update generated files
Mar 3, 2020
ac5703c
Golden files have to be updated as well
Mar 3, 2020
b51fb1a
Fix a compilation error
Mar 3, 2020
46072b8
object => map
Mar 3, 2020
5d00d42
Add 'ParamValue' to 'convert' package
Mar 4, 2020
77a6eaa
Improve readability of 'paramsMap'
Mar 4, 2020
bd94729
Reformat comment
Mar 4, 2020
73f222a
Update golden files
Mar 4, 2020
48ba154
Rename 'type' to 'value-type' to avoid bugs in JSON Schema validation
Mar 4, 2020
dbe000f
Allow parameter default values to be defined as YAML in 'params.yaml'
Mar 4, 2020
2b67286
Fix handling if empty default values
Mar 4, 2020
3505640
Rename conversion functions
Mar 4, 2020
9b0bbbf
Integration test for typed parameter values in operator package
Mar 4, 2020
ea8b0c7
Require an older version of Kubernetes
Mar 4, 2020
a10760a
Rename convert functions and parameters
Mar 5, 2020
f7c7cbf
Rename variable
Mar 5, 2020
ee4a316
Fix a compilation error
Mar 5, 2020
327e06a
Update pkg/util/convert/convert.go
Mar 5, 2020
82a47c2
Change function docs
Mar 5, 2020
cb920eb
Refactor package type converters into separate 'convert' package
Mar 6, 2020
91e69dc
Improve error messages
Mar 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/crds/kudo.dev_operatorversions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ spec:
this parameter changes in the Instance object. Default is `update`
if a plan with that name exists, otherwise it's `deploy`.
type: string
value-type:
description: Type specifies the value type. Defaults to `string`.
type: string
type: object
type: array
plans:
Expand Down
8 changes: 3 additions & 5 deletions pkg/apis/kudo/v1beta1/instance_types_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import (

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/uuid"

"github.com/kudobuilder/kudo/pkg/util/kudo"
)

// GetPlanInProgress returns plan status of currently active plan or nil if no plan is running
Expand Down Expand Up @@ -162,9 +160,9 @@ func ParameterDiff(old, new map[string]string) map[string]string {

// SelectPlan returns nil if none of the plan exists, otherwise the first one in list that exists
func SelectPlan(possiblePlans []string, ov *OperatorVersion) *string {
for _, n := range possiblePlans {
if _, ok := ov.Spec.Plans[n]; ok {
return kudo.String(n)
for _, plan := range possiblePlans {
if _, ok := ov.Spec.Plans[plan]; ok {
return &plan
}
}
return nil
Expand Down
17 changes: 17 additions & 0 deletions pkg/apis/kudo/v1beta1/operatorversion_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,20 @@ type Plan struct {
Phases []Phase `json:"phases"`
}

// ParameterType specifies the type of a parameter value.
type ParameterType string

const (
// StringValueType is used for parameter values that are provided as a string.
StringValueType ParameterType = "string"

// ArrayValueType is used for parameter values that described an array of values.
ArrayValueType ParameterType = "array"

// MapValueType is used for parameter values that describe a mapping type.
MapValueType ParameterType = "map"
)

// Parameter captures the variability of an OperatorVersion being instantiated in an instance.
type Parameter struct {
// DisplayName can be used by UIs.
Expand All @@ -95,6 +109,9 @@ type Parameter struct {
// Trigger identifies the plan that gets executed when this parameter changes in the Instance object.
// Default is `update` if a plan with that name exists, otherwise it's `deploy`.
Trigger string `json:"trigger,omitempty"`

// Type specifies the value type. Defaults to `string`.
Type ParameterType `json:"value-type,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use type here, because this will result in bugs with Kubernetes' schema validation. This bug didn't show when testing locally against a Kubernetes 1.17.2 cluster, so it might already be fixed. Anyways, the version of control-plane used in the integration tests is affected, other versions possibly as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's the combination of type: array triggering this, as this is also used in JSON Schema. The test was working with type: list. But we also use type: string which might trigger bugs as well.

}

// Phase specifies a list of steps that contain Kubernetes objects.
Expand Down
54 changes: 32 additions & 22 deletions pkg/controller/instance/instance_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ import (
"github.com/kudobuilder/kudo/pkg/engine/renderer"
"github.com/kudobuilder/kudo/pkg/engine/task"
"github.com/kudobuilder/kudo/pkg/engine/workflow"
"github.com/kudobuilder/kudo/pkg/util/kudo"
"github.com/kudobuilder/kudo/pkg/util/convert"
)

const (
Expand Down Expand Up @@ -207,12 +207,12 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
return reconcile.Result{}, err
}
if planToBeExecuted != nil {
log.Printf("InstanceController: Going to start execution of plan %s on instance %s/%s", kudo.StringValue(planToBeExecuted), instance.Namespace, instance.Name)
err = startPlanExecution(instance, kudo.StringValue(planToBeExecuted), ov)
log.Printf("InstanceController: Going to start execution of plan %s on instance %s/%s", convert.StringValue(planToBeExecuted), instance.Namespace, instance.Name)
err = startPlanExecution(instance, convert.StringValue(planToBeExecuted), ov)
if err != nil {
return reconcile.Result{}, r.handleError(err, instance, oldInstance)
}
r.Recorder.Event(instance, "Normal", "PlanStarted", fmt.Sprintf("Execution of plan %s started", kudo.StringValue(planToBeExecuted)))
r.Recorder.Event(instance, "Normal", "PlanStarted", fmt.Sprintf("Execution of plan %s started", convert.StringValue(planToBeExecuted)))
}

// ---------- 4. If there's currently active plan, continue with the execution ----------
Expand Down Expand Up @@ -303,7 +303,11 @@ func preparePlanExecution(instance *kudov1beta1.Instance, ov *kudov1beta1.Operat
return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not find required plan: %v", engine.ErrFatalExecution, activePlanStatus.Name), EventName: "InvalidPlan"}
}

params := paramsMap(instance, ov)
params, err := paramsMap(instance, ov)
if err != nil {
return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not parse parameters: %v", engine.ErrFatalExecution, err), EventName: "InvalidParams"}
}

pipes, err := pipesMap(activePlanStatus.Name, &planSpec, ov.Spec.Tasks, meta)
if err != nil {
return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not make task pipes: %v", engine.ErrFatalExecution, err), EventName: "InvalidPlan"}
Expand Down Expand Up @@ -347,7 +351,7 @@ func (r *Reconciler) handleError(err error, instance *kudov1beta1.Instance, oldI
var iError *kudov1beta1.InstanceError
if errors.As(err, &iError) {
if iError.EventName != nil {
r.Recorder.Event(instance, "Warning", kudo.StringValue(iError.EventName), err.Error())
r.Recorder.Event(instance, "Warning", convert.StringValue(iError.EventName), err.Error())
}
}
return err
Expand Down Expand Up @@ -383,21 +387,27 @@ func GetOperatorVersion(instance *kudov1beta1.Instance, c client.Client) (ov *ku
}

// paramsMap generates {{ Params.* }} map of keys and values which is later used during template rendering.
func paramsMap(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.OperatorVersion) map[string]string {
params := make(map[string]string)
func paramsMap(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.OperatorVersion) (map[string]interface{}, error) {
params := make(map[string]interface{}, len(operatorVersion.Spec.Parameters))

for k, v := range instance.Spec.Parameters {
params[k] = v
}

// Merge instance parameter overrides with operator version, if no override exist, use the default one
for _, param := range operatorVersion.Spec.Parameters {
if _, ok := params[param.Name]; !ok {
params[param.Name] = kudo.StringValue(param.Default)
var value *string

if v, ok := instance.Spec.Parameters[param.Name]; ok {
value = &v
} else {
value = param.Default
}

var err error

params[param.Name], err = convert.UnwrapParamValue(value, param.Type)
if err != nil {
return nil, err
}
}

return params
return params, nil
}

// pipesMap generates {{ Pipes.* }} map of keys and values which is later used during template rendering.
Expand Down Expand Up @@ -472,7 +482,7 @@ func startPlanExecution(i *v1beta1.Instance, planName string, ov *v1beta1.Operat
}
}
if notFound {
return &v1beta1.InstanceError{Err: fmt.Errorf("asked to execute a plan %s but no such plan found in instance %s/%s", planName, i.Namespace, i.Name), EventName: kudo.String("PlanNotFound")}
return &v1beta1.InstanceError{Err: fmt.Errorf("asked to execute a plan %s but no such plan found in instance %s/%s", planName, i.Namespace, i.Name), EventName: convert.StringPtr("PlanNotFound")}
}

err := saveSnapshot(i)
Expand Down Expand Up @@ -567,7 +577,7 @@ func getPlanToBeExecuted(i *v1beta1.Instance, ov *v1beta1.OperatorVersion) (*str

// new instance, need to run deploy plan
if i.NoPlanEverExecuted() {
return kudo.String(v1beta1.DeployPlanName), nil
return convert.StringPtr(v1beta1.DeployPlanName), nil
}

// did the instance change so that we need to run deploy/upgrade/update plan?
Expand All @@ -577,15 +587,15 @@ func getPlanToBeExecuted(i *v1beta1.Instance, ov *v1beta1.OperatorVersion) (*str
}
if instanceSnapshot == nil {
// we don't have snapshot -> we never run deploy, also we cannot run update/upgrade. This should never happen
return nil, &v1beta1.InstanceError{Err: fmt.Errorf("unexpected state: no plan is running, no snapshot present - this should never happen :) for instance %s/%s", i.Namespace, i.Name), EventName: kudo.String("UnexpectedState")}
return nil, &v1beta1.InstanceError{Err: fmt.Errorf("unexpected state: no plan is running, no snapshot present - this should never happen :) for instance %s/%s", i.Namespace, i.Name), EventName: convert.StringPtr("UnexpectedState")}
}
if instanceSnapshot.OperatorVersion.Name != i.Spec.OperatorVersion.Name {
// this instance was upgraded to newer version
log.Printf("Instance: instance %s/%s was upgraded from %s to %s operatorVersion", i.Namespace, i.Name, instanceSnapshot.OperatorVersion.Name, i.Spec.OperatorVersion.Name)
plan := kudov1beta1.SelectPlan([]string{v1beta1.UpgradePlanName, v1beta1.UpdatePlanName, v1beta1.DeployPlanName}, ov)
if plan == nil {
return nil, &v1beta1.InstanceError{Err: fmt.Errorf("supposed to execute plan because instance %s/%s was upgraded but none of the deploy, upgrade, update plans found in linked operatorVersion", i.Namespace, i.Name),
EventName: kudo.String("PlanNotFound")}
EventName: convert.StringPtr("PlanNotFound")}
}
return plan, nil
}
Expand All @@ -597,7 +607,7 @@ func getPlanToBeExecuted(i *v1beta1.Instance, ov *v1beta1.OperatorVersion) (*str
paramDefinitions := kudov1beta1.GetParamDefinitions(paramDiff, ov)
plan, err := planNameFromParameters(paramDefinitions, ov)
if err != nil {
return nil, &v1beta1.InstanceError{Err: fmt.Errorf("supposed to execute plan because instance %s/%s was updated but no valid plan found: %v", i.Namespace, i.Name, err), EventName: kudo.String("PlanNotFound")}
return nil, &v1beta1.InstanceError{Err: fmt.Errorf("supposed to execute plan because instance %s/%s was updated but no valid plan found: %v", i.Namespace, i.Name, err), EventName: convert.StringPtr("PlanNotFound")}
}
return plan, nil
}
Expand All @@ -610,7 +620,7 @@ func planNameFromParameters(params []v1beta1.Parameter, ov *v1beta1.OperatorVers
for _, p := range params {
if p.Trigger != "" {
if kudov1beta1.SelectPlan([]string{p.Trigger}, ov) != nil {
return kudo.String(p.Trigger), nil
return convert.StringPtr(p.Trigger), nil
}
return nil, fmt.Errorf("param %s defined trigger plan %s, but plan not defined in operatorversion", p.Name, p.Trigger)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/instance/instance_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/engine"
"github.com/kudobuilder/kudo/pkg/test/utils"
"github.com/kudobuilder/kudo/pkg/util/convert"
"github.com/kudobuilder/kudo/pkg/util/kudo"
)

Expand Down Expand Up @@ -82,7 +83,7 @@ func TestRestartController(t *testing.T) {
Parameters: []v1beta1.Parameter{
{
Name: "param",
Default: kudo.String("default"),
Default: convert.StringPtr("default"),
},
},
},
Expand Down
6 changes: 3 additions & 3 deletions pkg/engine/task/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ type Context struct {
Config *rest.Config
Enhancer renderer.Enhancer
Meta renderer.Metadata
Templates map[string]string // Raw templates
Parameters map[string]string // Instance and OperatorVersion parameters merged
Pipes map[string]string // Pipe artifacts
Templates map[string]string // Raw templates
Parameters map[string]interface{} // Instance and OperatorVersion parameters merged
Pipes map[string]string // Pipe artifacts
}

// Tasker is an interface that represents any runnable task for an operator. This method is treated
Expand Down
8 changes: 7 additions & 1 deletion pkg/engine/task/task_toggle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,13 @@ func (tt ToggleTask) delegateTask(ctx Context) (Tasker, error) {
if !exists {
return task, fmt.Errorf("no value for parameter %s found", tt.Parameter)
}
enabled, err := strconv.ParseBool(val)

stringVal, ok := val.(string)
if !ok {
return task, fmt.Errorf("value of parameter %s isn't a string", tt.Parameter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't there be an error on the use of a toggle param that is NOT a string... which is technically a bool ?
It seems like we are missing that..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this what this check does? We try to cast to a string, if it's !ok we error out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that done by the line below? If stringVal is neither "true" nor "false", strconv.ParseBool will return an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is somewhat confusing, we expect a string/boolean there. Maybe smth. like:

Suggested change
return task, fmt.Errorf("value of parameter %s isn't a string", tt.Parameter)
return task, fmt.Errorf("value of parameter %s must be either 'true' or 'false'", tt.Parameter)

wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, because this is checked in the line below when parsing the string to bool. This line is checking that typeof(interface) == string, not the content of the string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not a string it can't be true or false which is what we expect.

Copy link
Member Author

@nfnt nfnt Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Booleans can also be represented by a boolean or integer type. Also strconv.ParseBool accepts 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False.
At this point it's only clear whether the input is a string or not, the content of the input isn't known yet.

}

enabled, err := strconv.ParseBool(stringVal)
if err != nil {
return task, fmt.Errorf("could not parse value of parameter %s: %v", tt.Parameter, err)
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/engine/task/task_toggle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func TestToggleTask_Run(t *testing.T) {
done: true,
wantErr: false,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "true",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -86,7 +86,7 @@ func TestToggleTask_Run(t *testing.T) {
done: false,
wantErr: false,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "true",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand Down Expand Up @@ -142,7 +142,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: false,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "true",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -162,7 +162,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: false,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "false",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -182,7 +182,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: true,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "notABooleanValue",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -202,7 +202,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: true,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand All @@ -221,7 +221,7 @@ func TestToggleTask_intermediateTask(t *testing.T) {
},
wantErr: true,
ctx: Context{
Parameters: map[string]string{
Parameters: map[string]interface{}{
"feature-enabled": "someValue",
},
Client: fake.NewFakeClientWithScheme(scheme.Scheme),
Expand Down
2 changes: 1 addition & 1 deletion pkg/engine/workflow/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type ActivePlan struct {
Spec *v1beta1.Plan
Tasks []v1beta1.Task
Templates map[string]string
Params map[string]string
Params map[string]interface{}
Pipes map[string]string
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/kudoctl/cmd/generate/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func Operator(fs afero.Fs, dir string, op *packages.OperatorFile, overwrite bool
// if params doesn't exist create it
p := packages.ParamsFile{
APIVersion: reader.APIVersion,
Parameters: []v1beta1.Parameter{},
Parameters: []packages.Parameter{},
}
return writeParameters(fs, dir, p)
}
Expand Down
3 changes: 1 addition & 2 deletions pkg/kudoctl/cmd/generate/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"

"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
"github.com/kudobuilder/kudo/pkg/kudoctl/packages/reader"
)
Expand Down Expand Up @@ -70,7 +69,7 @@ func TestOperator_Write(t *testing.T) {
// updating params file and testing params are not overwritten
pf := packages.ParamsFile{
APIVersion: "FOO",
Parameters: []v1beta1.Parameter{},
Parameters: []packages.Parameter{},
}
// replace param file with a marker "FOO" to test that we do NOT overwrite it
err = writeParameters(fs, "operator", pf)
Expand Down
4 changes: 2 additions & 2 deletions pkg/kudoctl/cmd/generate/parameter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ package generate
import (
"github.com/spf13/afero"

"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
"github.com/kudobuilder/kudo/pkg/kudoctl/packages/reader"
)

// AddParameter writes a parameter to the params.yaml file
func AddParameter(fs afero.Fs, path string, p *v1beta1.Parameter) error {
func AddParameter(fs afero.Fs, path string, p *packages.Parameter) error {

pf, err := reader.ReadDir(fs, path)
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions pkg/kudoctl/cmd/generate/parameter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import (
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"

"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/kudoctl/files"
"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
)

func TestAddParameter(t *testing.T) {
Expand All @@ -20,7 +20,7 @@ func TestAddParameter(t *testing.T) {
files.CopyOperatorToFs(fs, "../../packages/testdata/zk", "/opt")

bar := "Bar"
p := v1beta1.Parameter{
p := packages.Parameter{
Name: "Foo",
Default: &bar,
}
Expand Down Expand Up @@ -52,7 +52,7 @@ func TestAddParameter_bad_path(t *testing.T) {
fs := afero.OsFs{}

bar := "Bar"
p := v1beta1.Parameter{
p := packages.Parameter{
Name: "Foo",
Default: &bar,
}
Expand Down
Loading