Implementation of KEP-30: Immutable Parameters #1575

ANeumann82 · 2020-07-02T14:12:30Z

What this PR does / why we need it:
Implementation of KEP-30: Immutable Parameters.

https://github.com/kudobuilder/kudo/blob/main/keps/0030-immutable-parameters.md

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

alenkacz

I did a first pass, looks good overall :) I'll get back to this after lunch.

I would definitely like to see the following (probably e2e?) test though:

install with immutable
try to update -> see it fails

Tests for the other scenarios in upgrade would be also nice but I won't block on that :)

pkg/kudoctl/cmd/package_list_params.go

alenkacz · 2020-07-13T10:41:00Z

pkg/apis/kudo/v1beta1/instance_types_helpers.go

@@ -170,6 +170,18 @@ func (i *Instance) TryRemoveFinalizer() bool {
 	return false
 }

+func (p *Parameter) IsImmutable() bool {


nice, these methods made the code much nicer 👍

alenkacz · 2020-07-13T10:42:27Z

pkg/kudoctl/packages/install/package_test.go

-		fakeDiscovery.FakedServerVersion = &version.Info{
-			GitVersion: "v1.16.0",
-		}
+		t.Run(tt.name, func(t *testing.T) {


hmm why this change?

Mostly because the test was failing and using the t.Run(tt.name, func.... shows the separate cases nicely in IntelliJ when the tests are ran through the IDE

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

alenkacz

Nice work 👏

kudo-e2e-test.yaml.tmpl

test/e2e/immutable-parameter/03-uninstall.yaml

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog

Nice work, I left a bunch of nits/suggestions/comments so PTAL

pkg/apis/kudo/v1beta1/instance_types_helpers.go

zen-dog · 2020-07-16T13:38:29Z

pkg/apis/kudo/v1beta1/instance_types_helpers.go

+}
+
+func (p *Parameter) HasDefault() bool {
+	return p.Default != nil


what about a nill-check here too?

The nil check is here, right? Did you mean a check against empty string?

I thought about it but decided against it, because an empty string might be a default? I'm not fully sure how we handle that though.

zen-dog · 2020-07-16T13:56:54Z

pkg/apis/kudo/v1beta1/instance_types_helpers.go

@@ -222,6 +234,44 @@ func GetParamDefinitions(params map[string]string, ov *OperatorVersion) ([]Param
 	return defs, nil
 }

+// GetChangedParameters returns a list of parameters from ov2 that changed based on the given compare function between ov1 and ov2
+func GetChangedParameters(ov1, ov2 *OperatorVersion, isEqual func(p1, p2 Parameter) bool) []Parameter {


Actually, the RichParameterDiff method below already gives you the changed and removed params. Wdyt about reusing it and modifying it to return updated, added, and removed params and remove these methods?

Well, the RichParameterDiff below works with the actual parameter values, so map[string]string - the new methods work on the ParameterDefinitions.

Basically the old ones are: Has the parameter values been changed
The new ones are: Have the parameter definitions been changed

They also have a parameter to decide how exactly the parameter was changed, so you can find out if the immutable flag has changed, or if the required flag has changed.

I don't think it really makes sense to join these funcs

pkg/kudoctl/packages/verifier/template/verify_parameters.go

pkg/webhook/instance_admission.go

zen-dog · 2020-07-16T15:24:53Z

pkg/webhook/instance_admission.go

+	if reflect.DeepEqual(old.Spec, new.Spec) && reflect.DeepEqual(old.Status, new.Status) {
+		return admission.Allowed("")
+	}
+
 	// fetch new OperatorVersion: we always fetch the new one, since if it's an update it's the same as the old one


This comment is outdated since we fetch both versions now.

Well - it's still valid, isn't it?

zen-dog · 2020-07-16T15:38:41Z

pkg/webhook/instance_admission.go

+}
+
+// setImmutableParameterDefaults sets the default values for immutable parameters into the instances parameter map
+func setImmutableParameterDefaults(ov *kudov1beta1.OperatorVersion, instance *kudov1beta1.Instance) {


I wonder if this is not better suited in the kudoctl 🤔 I was trying to minimize the mutations the IAW is doing to the instance (basically limiting it to PlanExecution). If we start to set the immutable defaults here, why not the normal defaults too?

I don't know, why don't we set the defaults here?

I think we should do as little as possible in kudoctl - I expect people to more and more use the CRDs directly, either with KUDO shim, or directly modifying the instance CRDs. From my perspective the kudoctl should only provide convenience functionality but nothing that directly affects the KUDO core funktionality.

zen-dog · 2020-07-16T15:46:03Z

pkg/apis/kudo/v1beta1/operatorversion_types.go

@@ -109,6 +109,9 @@ type Parameter struct {

 	// Type specifies the value type. Defaults to `string`.
 	Type ParameterType `json:"value-type,omitempty"`
+
+	// Specifies if the parameter can be changed after the initial installation of the operator
+	Immutable *bool `json:"immutable,omitempty"`


Why is Immutable a bool ptr? The immutable -> mutable transition is allowed and doesn't require a validation. So why can't we keep it a bool and treat all parameters as immutable = false by default?

Tbh, I'm not sure. I copied it from the required definition. I was wondering why "required" is a pointer though.

I guess it renders a little cleaner yaml if the bools are not set? But I don't really mind either way.

zen-dog · 2020-07-16T15:48:30Z

pkg/webhook/instance_admission_test.go

@@ -376,12 +394,169 @@ func TestValidateUpdate(t *testing.T) {
 			ov:      ov,
 			wantErr: true,
 		},
+		{
+			name: "parameter update to an immutable param IS NOT allowed",


Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 added 5 commits June 30, 2020 11:31

Started immutable params implementation

2b90105

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'main' into an/immutable-params-impl

24a5d0b

Implemented upgrade behavior and started adding tests

c96df4e

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Added more unit tests

8af6b50

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed bugs, updated failing tests

4ca29f4

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 requested review from alenkacz, gerred, kensipe, nfnt and zen-dog as code owners July 2, 2020 14:12

ANeumann82 added 3 commits July 2, 2020 16:21

Small cleanup

78fe7a9

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Updated generated files

306d9ce

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Updated generated files

b60b5b2

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 added the needs review label Jul 13, 2020

alenkacz reviewed Jul 13, 2020

View reviewed changes

ANeumann82 added 3 commits July 13, 2020 14:56

Review requests, added e2e test

0eaba61

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Removed weird additional character

e29dd33

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'main' into an/immutable-params-impl

50df168

alenkacz approved these changes Jul 14, 2020

View reviewed changes

kudo-e2e-test.yaml.tmpl Outdated Show resolved Hide resolved

test/e2e/immutable-parameter/03-uninstall.yaml Outdated Show resolved Hide resolved

Removed unnecessary uninstall step

5178eea

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog reviewed Jul 16, 2020

View reviewed changes

Cleanup and refactoring based on review

4188f58

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 added release/highlight This PR is a highlight for the next release and removed needs review labels Jul 20, 2020

ANeumann82 merged commit 88d6f1b into main Jul 20, 2020

ANeumann82 deleted the an/immutable-params-impl branch July 20, 2020 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of KEP-30: Immutable Parameters #1575

Implementation of KEP-30: Immutable Parameters #1575

ANeumann82 commented Jul 2, 2020

alenkacz left a comment

alenkacz Jul 13, 2020

alenkacz Jul 13, 2020

ANeumann82 Jul 13, 2020

alenkacz left a comment

zen-dog left a comment

zen-dog Jul 16, 2020

ANeumann82 Jul 17, 2020

zen-dog Jul 16, 2020

ANeumann82 Jul 17, 2020

zen-dog Jul 16, 2020

ANeumann82 Jul 17, 2020

zen-dog Jul 16, 2020

ANeumann82 Jul 17, 2020

zen-dog Jul 16, 2020

ANeumann82 Jul 17, 2020

zen-dog Jul 16, 2020

Implementation of KEP-30: Immutable Parameters #1575

Implementation of KEP-30: Immutable Parameters #1575

Conversation

ANeumann82 commented Jul 2, 2020

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

zen-dog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment