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.

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

Tenant Quota #65

Open
wehowski opened this issue Aug 4, 2024 · 29 comments
Open

Tenant Quota #65

wehowski opened this issue Aug 4, 2024 · 29 comments
Labels
idea Ideas

Comments

@wehowski
Copy link
Collaborator

wehowski commented Aug 4, 2024

If tenant has no own DB (using prefix) we MUST track/limit tenants quotas.
(Or by number of objects?)

Added to IO4 Plugin but maybe introduced to core:

	public static function getQuotaUsedDB(){
		$sum = 0;
		$q="SELECT table_name AS `table`,
ROUND(((data_length + index_length) / 1024 / 1024), 2) AS `megabyte`
FROM information_schema.TABLES
WHERE table_schema = ? AND table_name LIKE '".str_replace('_', '\_', OIDplus::baseConfig()->getValue('TABLENAME_PREFIX'))."%'
ORDER BY (data_length + index_length) DESC";
		
		//  die($q);
	  $resQ = OIDplus::db()->query($q, [
	OIDplus::baseConfig()->getValue('MYSQL_DATABASE'), 
]);
		$t = [];
		while ($row = $resQ->fetch_array()) { 
			$sum+=$row['megabyte'];
			$t[$row['table']] = $row['megabyte'];
		}
		
		return [
			'used'=>$sum,			
			'tables'=>$t,			
		];
	}

Then it would make sense to introduce a MAX_QUOTA for Tenant Config Value !?
(Later... abstract to "service plans"?)

If quota reached, adding data/upload files should be blocked.

IF we use attachment we should add the files quota to the tenants used resources.
Idea: USE league/flysystem TO ABSTRACT ATTACHMENTS TO REMOTE STORAGES/abstract adapters!?!

@wehowski wehowski added the idea Ideas label Aug 4, 2024
@danielmarschall
Copy link
Owner

danielmarschall commented Aug 5, 2024

Thank you for the suggestion.

I have an idea.

For objects, we have the interface method "beforeObject". We can make a "tenantQuota" plugin, which implements this interface and throws an Exception if the creating of the object is denied.

For attachments, we should also create a similar plugin with methods like "beforeUpload" and "afterUpload". The "tenantQuota" plugin then also can deny/revert the upload if the file is too large or if there are too many files.

Furthermore, the "tenantQuota" plugin can be configured with the ###config table.

What do you think?

@wehowski
Copy link
Collaborator Author

wehowski commented Aug 5, 2024

before*
Yes, this sounds reasonable.

Hab mal eine zip generation für die frdl_plugins hinzugefügt (um nicht jeden dev-schritt nach github laden zu müssen)

...

@danielmarschall
Copy link
Owner

Das frdl_plugins Repo war eigentlich nur eine Übergangslösung, damit ich deine Plugins updaten konnte. Wenn du es behalten möchtest, ist das natürlich kein Problem. Idealerweise sollte der Quellcode mit den "richtigen" Repos vereint werden, weil ich Sorge habe, dass die Versionen auseinanderlaufen. Und richtig super wäre natürlich, wenn die Änderungen der GitHub Repos automatisch von GitHub nach registry.frdl.de geladen werden, sodass man nur an einer Stelle ändern muss (und ich im Notfall einen Patch einfügen kann). Kann ich dir beim Zusammenführen helfen?

@wehowski
Copy link
Collaborator Author

wehowski commented Aug 5, 2024 via email

@danielmarschall
Copy link
Owner

Hallo Till,
es ist jetzt etwas offtopic zu diesem GitHub, aber noch eine Frage:
Wo genau liegt die "Mastercopy"? Wenn die Mastercopy auf deinem Server liegt, dann sollte sie idealerweise auf Github synchronisiert werden. Die Änderungen solltest du zusammenführen, denn im Moment ist es echt ein Chaos und ich blicke mit den vielen verschiedenen Versionen nicht mehr durch :-(

@wehowski
Copy link
Collaborator Author

wehowski commented Aug 5, 2024

Die "master copy" wird generiert (siehe den code den ich verlinkt habe),
und ist dann hier downloadbar: https://registry.frdl.de/frdl-plugins.zip

dann sollte sie idealerweise auf Github
Auf github lade ich FERTIGES, auf github kann ich nicht entwickeln.

Bisher wird doch nur das RDAP Plugin vom core verwendet!?
Die IO4 Plugins sind "spezifisch", bzw. meine "Heim-Version" die geht eben aus oben genannten Gründen NICHT auf github.

Das für Dich relevante repository, das für den Core sollte sein: https://github.com/frdl/oidplus-frdlweb-rdap
Lass Dich bitte nicht durch meine Versionen irritieren!
das danielmarschall/frdl_plugins ist optional/obsolet/zusätzlicher Test, da (siehe oben) der download dafür ist https://registry.frdl.de/frdl-plugins.zip

...

Viele Grüße
Till

@wehowski
Copy link
Collaborator Author

The quota the user can use may depand on "plans" the user can register/buy/order, the plans could add available quota (within a time period), this may out of scope of OIDplus.
But maybe it would make sense to add a plugins (interface) method

getQuotaAvailable()

???

Auch out of scope, aber Vorschlag, oder per adapter/apis https://startforum.de/content/perma?id=2150 ?

@danielmarschall
Copy link
Owner

danielmarschall commented Oct 26, 2024

The next version of OIDplus (to be released in a few days) will introduce the following feature:

namespace ViaThinkSoft\OIDplus\Plugins\PublicPages\Attachments;
interface INTF_OID_1_3_6_1_4_1_37476_2_5_2_3_11 {
        public function beforeAttachmentUpload(string $id, string $filename_relative, array $file_data): void;
        public function afterAttachmentUpload(string $id, string $filename_relative, array $file_data): void;
        public function beforeAttachmentDelete(string $id, string $filename_relative): void;
        public function afterAttachmentDelete(string $id, string $filename_relative): void;
        public function beforeAttachmentDownload(string $id, string $filename_relative): void;
}

$id is the id of the object, e.g. "oid:"

$filename_relative is the filename "test.docx"

To find out the size of the file to be uploaded, you can query filesize($file_data['tmp_name']);

You can create a quota plugin which implements afterAttachmentUpload() and throws an Exception if the quota has been reached.

danielmarschall added a commit that referenced this issue Oct 27, 2024
@wehowski
Copy link
Collaborator Author

@danielmarschall Thank you! I stay tuned for the next version.
I added a global function to the wtf plugin related to quota:
https://github.com/frdl/frdl-oidplus-wtf-plugin/blob/main/oidplus-functions.php

I will next think how to implement provisioning of quota.

P.S.: Ich mache eventuell heut Nachtschicht.

@wehowski
Copy link
Collaborator Author

... für die before* after* Object Hooks wurden do_action hooks getriggert:
https://github.com/frdl/frdl-oidplus-wtf-plugin/blob/main/OIDplusPageWTFunctions.class.php#L57
So das Plugins über add_action in die Functions "einhooken" können.

@danielmarschall
Copy link
Owner

Version ist veröffentlicht

@danielmarschall
Copy link
Owner

@danielmarschall Thank you! I stay tuned for the next version. I added a global function to the wtf plugin related to quota: https://github.com/frdl/frdl-oidplus-wtf-plugin/blob/main/oidplus-functions.php

Are you sure this will work? I am not sure if data size and index size will bloat up if the database tables are not defragmented regularly. At least for SQL Server and I think also for InnoDB, the files will grow, even if no data is added.

Also, if the database is filled up with OIDplus Log entries, then it will also grow in size. We should not punish the customer if we went crazy with logging too much stuff :-)

So, I would rather count:

  1. the amount of objects, iri and asn1id
  2. the amount of RA
  3. the file sizes of attachments

This should be a good quota which is a measurement how much the customer is using.

@wehowski
Copy link
Collaborator Author

I am not sure to account the logs or not.
Maybe it would be a solution to let the tenant prune the logs?

@danielmarschall
Copy link
Owner

danielmarschall commented Oct 29, 2024

In any case, you should do a small experiment by adding and removing data and then check if data + index sizes increase and don't lower again. As far as I know, a lot of DBMS have this behavior, so even pruning log files won't shrink the database!

So either there would be a prune + defrag command...

... or you check the data (row count) instead of the database size. Monitoring the row count also has the advantage that your quota plugin is independent of DBMS (which would be a requirement if the plugin would be bundled to the core, since OIDplus must support SQL Server, SQlite and many more)

@wehowski
Copy link
Collaborator Author

...it would also be convenient if we would have access to seperate DB Connections for the central domain/system and the current tenant system.

E.g.

 //for the CURRENT system (central or tenant):
OIDplus::db()-> ...

 //for the EXAMPLE tenant
OIDplus::db('example.com')-> ...

 //for the CENTRAL system
OIDplus::db(true)-> ...

 //for the TENANT system
OIDplus::db(false)-> ...

The same syntax way we could select a specific config to act on... and other OIDplus::service() Object getters

 //for the CURRENT system (central or tenant):
OIDplus::config()-> ...

 //for the EXAMPLE tenant
OIDplus::config('example.com')-> ...

 //for the CENTRAL config
OIDplus::config(true)-> ...

 //for the TENANT config
OIDplus::config(false)-> ...

@wehowski
Copy link
Collaborator Author

namespace ViaThinkSoft\OIDplus\Plugins\PublicPages\Attachments;
interface INTF_OID_1_3_6_1_4_1_37476_2_5_2_3_11 {
        public function beforeAttachmentUpload(string $id, string $filename_relative, array $file_data): void;
        public function afterAttachmentUpload(string $id, string $filename_relative, array $file_data): void;
        public function beforeAttachmentDelete(string $id, string $filename_relative): void;
        public function afterAttachmentDelete(string $id, string $filename_relative): void;
        public function beforeAttachmentDownload(string $id, string $filename_relative): void;
}

I implemented it in the wtf plugin: https://github.com/frdl/frdl-oidplus-wtf-plugin/blob/main/OIDplusPageWTFunctions.class.php#L55

I think I will re-introduce the/a seperate tenant plugin to use the above wtf hooks.

I also want to make a wtf OAuth Plugin, now I am not sure to make it tenant, central or system wide...!??

OIDplus::config('example.com')->
The tenant plugin e.g. could make an config-external.inc.php from the config to return an pure php array or similar config to be required by the system (from another OIDplus::method from current instance.
So e.g. the central domain can manage tenants better without to load or proxy a whole other instance!?!

I think specially different Database connections are need (if in central/management mode) like
OIDplus::db('example.com')->
as the DB for example com can be either the same as central with different prefix, or another DB from a different webspace from another customer of the same server or even external (not recommended).
This also would allow imports/exports between tenants?

If the OIDplus::db('example.com')-> connection is on a different customer space on the server, the (DB-) quota could be managed completly by its Plesk plan.

For files, attachments and there quota I would a solution like https://flysystem.thephpleague.com/docs/ to handle filesystem connections for tenants. Flysystem abstracts the filesystem so the tenant could use
Local
FTP
SFTP
Memory
AWS S3
AsyncAws S3
Google Cloud Storage
Azure Blob Storage
MongoDB GridFS
WebDAV
Third party Adapters
Gitlab
Google Drive (using regular paths)
bunny.net / BunnyCDN
Sharepoint 365 / One Drive (Using MS Graph)
OneDrive
Dropbox
ReplicateAdapter
and more adapters, drives ...

...and or connections for RAs and there objects, like tenants of the tenant but without domain/system?

@danielmarschall
Copy link
Owner

@wehowski What use-cases would having OIDplus::config('example.com')-> or OIDplus::db('example.com')-> be good for?

In my opinion, tenants should not be able to "see" or "access" data of other tenants.

@wehowski
Copy link
Collaborator Author

wehowski commented Dec 1, 2024

In my opinion, tenants should not be able to "see" or "access" data of other tenants.
The tenant not BUT the system, e.g. the central system should be able to read/config/import/export/... tenants.
The tenant does not have access to "the system".
The system must use this while prevent unwanted operations by the tenant(s user interaction).

@danielmarschall
Copy link
Owner

danielmarschall commented Dec 1, 2024

What is the actual use-case where the central system needs to update configuration/data of a tenant system?

@wehowski
Copy link
Collaborator Author

wehowski commented Dec 1, 2024

In my use-cases the tenant is a "managed" tenant.
The central system may like to add,update,config... the tenant system, copy import export OID trees and so on.
"The tenant" is NOT a system admin.
There must be an additonal layer of API for tenant management.

I had one use case, I did not remember now fast, I had to perform a (proxy) http request to the tenant domain(s). This could be inconvenient and affect performance. I think it was about to read client DB config/use additional DB connections independent from the central.

The (central) system should manage the tenants, there configs and DB connections in another layer.
A tenant is a special user (could be connected with an RA-.Account), not an admin and not a webhosting customer per default.

And: Everything ABSTRACTING things (like calling routes, DB Connections, File-Adapters etc.. ) is good in my believe,
e.g. I may like to "request" an OIDplus instance by console instead of HTTP.

...

I could manage different DB connections e.g. by own plugins etc. but OIDplus::db('example.com')-> would be nicer and more consistent?

However, it is just an idea, maybe for later, the todo stack is full!?!

EDIT : One use case would be moving / alias domains... e.g ...

@danielmarschall
Copy link
Owner

danielmarschall commented Dec 1, 2024

I think it would be pretty complicated to achieve: OIDplus::config('example.com')->

This is because the OIDplus class would need to remember not only the own $db, $config, $... , but also the ones from tenants.

And the baseconfig.inc.php of the tenant system contains lines such as OIDplus::config()->setValue(), so if this code is executed, then config() will be interpreted as the config of the base system.

So, very complicated!

I'd rather think that it would be good if the class OIDplus would become a non-singleton, then you could have the following code:

$oidplus_tenant = new OIDplus();
$oidplus_tenant->forceTenantSubDirName('example.com');
$oidplus_tenant->init(true); // do this after forceTenant(), so that the tenant DB will be loaded
$oidplus_tenant->config()->....
$oidplus_tenant->db()->....
$oidplus_tenant->exit();

But note that this might be a bit slow because the constructon of such an OIDplus object would be very slow, since DB connection, setting buffer and object cache would be queried as if the page was loaded completely new.

However, this is also a lot of work, because in the complete code, OIDplus:: would need to be replaced with the global symbol:

Search:

OIDplus::xyz

Replace:

global $OIDplus:
$OIDplus->xyz

Also, no security here, because the new OIDplus object cannot know if it was called inside the base system or inside the tenant system. (... or maybe, it does, if it would call the $OIDplus global variable ...?)

@wehowski
Copy link
Collaborator Author

wehowski commented Dec 1, 2024

Yes, so the tenant could write a raw php config file that orther systems can perform connections withou to load the whole system!?!

However, your idea is good!

Due to static properties and calls to OIDplus inside the class, an private properties, maybe I have to rewrite the complete class!?

    class Tenant extends OIDplus 
	{
	  public function __construct(?string $tenant = null) {
		  
	  }		
	  public function exit() {
		 self::db()->disconnect();
	  }		
		
	}

...

@danielmarschall
Copy link
Owner

I made a few experiments to day in order to rename OIDplus to OIDplusSession, and then create a OIDplus class which contains a __callStatic method that forwards all static methods calls from OIDplus to OIDplusSession. With this, everything worked, but the problem is that PHPStan won't accept that, because it cannot see the methods of OIDplus anymore. So I gave up on that idea.

Your sounds like a good idea!

I think it could work like this, but not 100% sure:

    class Tenant extends OIDplus 
	{
	  public function __construct(?string $tenant = null) {
	  	  parent::__construct();
	  	  if (!is_null($tenant)) self::forceTenantSubDirName($tenant);
	  }		
	  public function exit() {
		 self::db()->disconnect();
	  }		
		
	}

I think in the core OIDplus.class.php , I would need to replace all OIDplus:: with self::. I actually planned this some time ago, so I will do that now.

@danielmarschall
Copy link
Owner

danielmarschall commented Dec 1, 2024

Oh, but actually...

what happens if Tenant('example.com') calls OIDplusDatabasePlugin, which itself calls OIDplus::baseConfig() ?
I would need to write a small Dependency Injection then, so I can tell all core classes which OIDplus object they shall use?

@danielmarschall
Copy link
Owner

danielmarschall commented Dec 1, 2024

Just tried it. I can't use Dependency Injection, because OIDplus has static methods and is not an instance

Inside OIDplus::mailUtils(), the code new OIDplusMailUtils($this) does not work because $this is not possible in the static method OIDplus::mailUtils()

new OIDplusMailUtils(self) is not a valid syntax, and would not help either, because we need to have Tenant as subclass of OIDplus being an instance.

So, all static methods would need to become non-static. But this means that ALL EXISTING code cannot use OIDplus:: anymore, but need to call something like $OIDplus-> . PHP 8.1 forbids writing to $GLOBALS, so we would also need to add global $OIDplus to EACH method that uses $OIDplus->.

I am out of ideas

@danielmarschall
Copy link
Owner

danielmarschall commented Dec 1, 2024

I think we have no good solution to change things. I wish I had designed the classes differently in 2019, and not use that stupid singleton design pattern. But I didn't knew better.

I have another idea which is often used in C programming.

We could put all (static) class variables in an object that is called OIDplusContext. So, the class OIDplus only contains methods and no data anymore!

Now you could do the following:

$bakContext = OIDplus::getCurrentContext(); // make a copy of main system's state
try {
    OIDplus::setCurrentContext(new OIDplusContext()); // create a new state
    OIDplus::forceTenantSubDirName('example.com'); // switch to tenant
    OIDplus::init();
    OIDplus::db()->doSomething;
    OIDplus::invoke_shutdown();
} finally {
    OIDplus::setCurrentContext($bakContext); // switch back to main system
}

Disadvantage: You CANNOT work in multiple contexts at the same time.
For example, you CANNOT do:

TenantA::config()->setValue('XYZ', TenantB::config()->getValue('XYZ'));

This is due to the fact that the whole codebase calls things like OIDplus::db(), which is static, so we can only have ONE context active at the same time.

What do you think about this solution?


Note to myself: If we do this, then I need to check if there are other classes with static variables in the core, which need to go into the context

danielmarschall added a commit that referenced this issue Dec 1, 2024
@danielmarschall
Copy link
Owner

Done.

I think this is a good solution to make the best of the situation. It would also allow us to do unit tests someday...

With commit 0d6a51c should be able to do this (not tested):

$bakContext = OIDplus::getCurrentContext(); // make a copy of main system's state
try {
    OIDplus::setCurrentContext(new OIDplusContext()); // create a new state
    OIDplus::forceTenantSubDirName('example.com'); // switch to tenant
    OIDplus::init();
    OIDplus::db()->doSomething;
    OIDplus::invoke_shutdown();
} finally {
    OIDplus::setCurrentContext($bakContext); // switch back to main system
}

@wehowski
Copy link
Collaborator Author

wehowski commented Dec 1, 2024

👍 I will try this out...

@danielmarschall
Copy link
Owner

danielmarschall commented Dec 1, 2024

(The GitHub issue has become kind off-topic, I feel like this would need to go to a different issue?)

The OIDplusContext solution is actually pretty useful, since the OIDplus::init() method now completely resets the context, which means that ALL data (such as object cache) gets destroyed now. This is very important because crob.sh iterates through every tenant, so it was horribly wrong that the object cache was not reset!

In the latest Git commit, I also give plugins the possibility to write their own stuff to the context.

If one of your plugins uses static variables like this (example):

static $cache = null;

Then you should replace it with this, to make sure that the data gets reset if the context switches, and restored if the context is restored:

$cache = &OIDplus::getCurrentContext()->pluginData('1.3.6......pluginoid......', 'MyCache', null);
$cache = 'something'; // writes directly into OIDplusContext, because this is a reference!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea Ideas
Projects
None yet
Development

No branches or pull requests

2 participants