Make checkCanWriteSystemInformation to deny by default

[kokosing]

Make checkCanWriteSystemInformation to deny by default

This is in order to prevent accidental access to modify the worker state
in case there was no configured access control on worker.

[kokosing]

I am not sure if this requires tests

[electrum]

I like this idea, but it might break graceful shutdown for k8s or other deployments. At a minimum, I think we need to document how someone would setup access control on workers to allow this.

[electrum]

cc @mosabua

[mosabua]

Uff.. yeah. I am pretty sure this would be a pretty big breaking change for many scenarios. At minimum we would need some docs on how to change/allow the old behavior in terms of access control.. or a property that allows to set a default to allow vs deny.

[ksobolew]

Do we even have tests for these defaults? This looks like a very short and very specific list of prohibited behaviors.

[kokosing]

this would be a pretty big breaking change for many scenarios

Not really that big. In order to restore previous behavior one can need to follow access-control.name=allow-all (https://trino.io/docs/current/security/built-in-system-access-control.html#read-only-system-access-control). However the best would be to configure https://trino.io/docs/current/security/file-system-access-control.html#system-information-rules to define who can have the "write" access to change the state of the work and so trigger the shutdown.

Do we even have tests for these defaults?

I do not want to reverse the implementation in test as such tests do not introduce much value. So I think I will skip those.

[kokosing]

@mosabua can you please follow with the release notes for this? The content is here #17105 (comment)

[mosabua]

This should not have been merged without documentation update imho. I am working with @colebow to get min info into release notes.

cc @martint

Can you supply exact details on how to configure what so that the old behavior continues to work. I think this might have to be the default for many users so the impact on docs and usage advice is potentially large.

[mosabua]

Also .. doesnt this mean that what we call "default" access control is now changed?

[kokosing]

This should not have been merged without documentation update imho.

I am sorry. I was not aware that we documented the default access control. From the comments I understood that we need to handle backward compatibility only.

[kokosing]

Please see #17142