api: add api support for Update events

[thotz]

Adding support to handle Update Events of OBC, currently AdditionalConfig needs this change.
Other fields are immutable
Fixes #195
Signed-off-by: Jiffin Tony Thottan thottanjiffin@gmail.com

[thotz]

@BlaineEXE @jeffvance @copejon @leseb PTAL

[jeffvance]

There is no error reported if the user tries to update OBC fields other than additionalCongifData.
Also, have you consider that the user can mutate additionalConfigData other than quota (which the user is allowed to change). For example, the storage class's parameters field can contain bucket config data and is passed to the provisioner. What happens if the user changes this config data by updating their obc's additionalConfigData. It seems now a user is allowed to change any bucket config data.

[BlaineEXE]

Are these changes even needed any more given this comment? #195 (comment)

[thotz]

Are these changes even needed any more given this comment? #195 (comment)

Atleast at that time focus was needed for bucket notifications(decided to go with label notations instead of additionalconfig), now users have similar request for quota as well rook/rook#7146

[thotz]

There is no error reported if the user tries to update OBC fields other than additionalCongifData.
Also, have you consider that the user can mutate additionalConfigData other than quota (which the user is allowed to change). For example, the storage class's parameters field can contain bucket config data and is passed to the provisioner. What happens if the user changes this config data by updating their obc's additionalConfigData. It seems now a user is allowed to change any bucket config data.

@jeffvance : I agree that other fields apart from additionalConfigData should not be modified, this field is opaque to the library so it won't able to know changes made to additionalConfigData. Sorry I didn't fully understand how storageclass.Parameter related to additionalConfigData

[jeffvance]

@jeffvance : I agree that other fields apart from additionalConfigData should not be modified, this field is opaque to the library so it won't able to know changes made to additionalConfigData. Sorry I didn't fully understand how storageclass.Parameter related to additionalConfigData

1. what happens if the user modifies OBC fields other than additionalConfigData? It looks like there will be no error reported.
2. Re. storage class and parameters- nevermind. I didn't remember that your are passing an options struct to the Update method, which defines the field the Update method sees. Be careful if change to pass a map to Update because then there's a possibility of collisions with the StorageClass.parameters config map.

[jeffvance]

at that time focus was needed for bucket notifications (decided to go with label notations instead of additionalconfig)

So do we want to also support notification via additionalConfigData?

[jeffvance]

From above:

what happens if the user modifies OBC fields other than additionalConfigData? It looks like there will be no error reported.

This has not been addressed.

@BlaineEXE what are your thoughts?

[thotz]

at that time focus was needed for bucket notifications (decided to go with label notations instead of additionalconfig)

So do we want to also support notification via additionalConfigData?

Currently not planned to do so

[jeffvance]

But what if a user modifies both the spec and the additionalConfig?

Good catch @BlaineEXE !

[jeffvance]

why do you need to reference oldSpec... as a pointer?

[thotz]

without pointer reference the check was always returning false

[jeffvance]

Is it always true now? You want to make sure you are not comparing an address with a spec.

[thotz]

IMO oldSpecWithNewAdditionalConfig is pointer than a Spec. I have added following function to print Spec values

diff --git a/pkg/provisioner/controller.go b/pkg/provisioner/controller.go
index 68f47e9..ddc4cc7 100644
--- a/pkg/provisioner/controller.go
+++ b/pkg/provisioner/controller.go
@@ -105,6 +105,7 @@ func NewController(provisionerName string, provisioner api.Provisioner, clientse
                        oldSpecWithNewAdditionalConfig := oldObc.Spec.DeepCopy()
                        overwriteAdditionalConfig(newObc.Spec.AdditionalConfig, oldSpecWithNewAdditionalConfig.AdditionalConfig)
                        if !reflect.DeepEqual(newObc.Spec, oldObc.Spec) {
+                               printObcSpec(oldSpecWithNewAdditionalConfig)
                                // new OBC is changed
                                if !reflect.DeepEqual(*oldSpecWithNewAdditionalConfig, newObc.Spec) {
                                        // new OBC has changed something other than additionalConfig
@@ -695,3 +696,13 @@ func overwriteAdditionalConfig(srcAdditionalConfig map[string]string, destAdditi
                destAdditionalConfig[key] = value
        }
 }
+
+func printObcSpec(spec v1alpha1.ObjectBucketClaimSpec) {
+       logD.Info("storageclassName ", spec.StorageClassName)
+       logD.Info("bucketname ", spec.BucketName)
+       logD.Info("generatebucknam", spec.GenerateBucketName)
+       logD.Info("ObjectBucketName", spec.ObjectBucketName)
+       for key, value := range spec.AdditionalConfig {
+               logD.Info("key ", key, "value ", value)
+       }
+}

And give me following error :

go build'ing package ./pkg/...
# github.com/kube-object-storage/lib-bucket-provisioner/pkg/provisioner
pkg/provisioner/controller.go:108:17: cannot use oldSpecWithNewAdditionalConfig (type *"github.com/kube-object-storage/lib-bucket-provisioner/pkg/apis/objectbucket.io/v1alpha1".ObjectBucketClaimSpec) as type "github.com/kube-object-storage/lib-bucket-provisioner/pkg/apis/objectbucket.io/v1alpha1".ObjectBucketClaimSpec in argument to printObcSpec

[jeffvance]

Typo L133: "waith" -> "wait"

[BlaineEXE]

In Go, maps need to be deep copied. This will only change the pointer, and tmp will therefore be overwritten by overwriteAdditionalConfig. If updating the object bucket fails, this will not properly reset the value.

See my test here: https://play.golang.org/p/z0BurgSZEM8

Given this potential bug, it would be best to have a unit test that verifies the provisioner is properly reset when updating the OB fails.

[jeffvance]

Yes you are right! Sorry @thotz

[thotz]

@BlaineEXE : I agree it is good to have a unit test. Sorry but I don't understand which function I need write the unit test.
Is it for overwriteAdditionalConfig() with set of string maps like in ur example. Or for the handleUpdateClaim()?
If it is handleUpdateClaim()
First I need to create OBC and then call handleUpdateClaim() with that OBC for different test cases?

[BlaineEXE]

I think for handleUpdateClaim and make sure the unit test coverage report activates the lines below

if err != nil {
		log.Error(err, "updating OB failed, reverting provisioner to original value")
    		overwriteAdditionalConfig(tmp, ob.Spec.Endpoint.AdditionalConfigData)
    		err = c.provisioner.Update(ob)
}

And also verify that the additionalConfig in the activated code is properly reset to the original value and doesn't still contain the updated additionalConfig

[copejon]

Is it necessary to compare the whole spec if the only supported field is additionalConfig? Why not instead check if additionalConfig has changed?

[jeffvance]

One of the goals is to log an error or warning if the user attempts to change anything in the obc other than .spec.additionalConfig. I guess DeepEqual handles map compares in cases where they keys may be in different order, right?

[copejon]

Nevermind, I believe it's just to short circuit the call if a non-spec field was changed, which can be ignored. Makes sense to me.

[jeffvance]

lgtm

(for future reference, I prefer to see pr changes in separate commits so that I can re-review only the code that has changed since my previous review)

[thotz]

lgtm

(for future reference, I prefer to see pr changes in separate commits so that I can re-review only the code that has changed since my previous review)

Sure I keep this in mind while sending PR. Sorry for the trouble and thanks for your patience while reviewing the PR

[jeffvance]

No worries @thotz - single commit PRs are common, but, IMO, there are advantages to multi-commit PRs with sensible squashing just before merging.

[BlaineEXE]

I didn't realize I was the hold-up. Sorry. I think this looks good now. I will merge this, and then I think the next step is for @thotz to update the lib-bucket-provisioner lib in Rook.

[Update: @thotz be sure to add a unit or integration test in Rook that tests OBC updating]

[leseb]

@thotz is Rook going to consume that?

[thotz]

Yes I can update the library for Rook by adding necessary changes