search

kube-object-storage / lib-bucket-provisioner

Library for the dynamic provisioning of object store buckets to be used by object store providers.
Apache License 2.0
20 stars 22 forks source link

api: add api support for Update events #213

Closed thotz closed 3 years ago

thotz commented 3 years ago

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 commented 3 years ago

@BlaineEXE @jeffvance @copejon @leseb PTAL

jeffvance commented 3 years ago

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 commented 3 years ago

Are these changes even needed any more given this comment? https://github.com/kube-object-storage/lib-bucket-provisioner/issues/195#issuecomment-742282150

thotz commented 3 years ago

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 https://github.com/rook/rook/issues/7146

thotz commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

Good catch @BlaineEXE !

jeffvance commented 3 years ago

Typo L133: "waith" -> "wait"

thotz commented 3 years ago

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 commented 3 years ago

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

leseb commented 3 years ago

@thotz is Rook going to consume that?

thotz commented 3 years ago

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