Projects tigase _server server-core Issues #927
Improve DomainFilter configuration in VHostItem (#927)
wojciech.kapcia@tigase.net opened 7 years ago

Instead of parsing string make it configurable properly - separately DomainFilterPolicy and separately additional parameters.

Andrzej Wójcik (Tigase) commented 7 years ago

%wojtek To be honest, this may be a difficult thing to do, because we have some predefined fields in VHostItem like domainFilter, messageForward, presenceForward, etc and we have optional values kept in data field of type Map<String,Object>. Due to that use of default kernel mechanism for injecting configuration is not a good idea, as it works only for beans and we do not want items of any user repository or config repository to be beans. (It would be messy and complicated to deal with that.

Right now we have:

'virtual-hosts' = [
    'domain1:-anon:register:-tls-required:s2s-secret=s2spasswd:domain-filter=LOCAL:max-users=1000', 
    'domain2', 
    'domain3:c2s-ports-allowed=5222;5223;5280;5290', 
    'domain4:domain-filter=LIST=whitedomain1;whitedomain2;whitedomain3' 
]

As you can see we set virtual-hosts (and items for every config repository) as a list of object (strings to be exact, where first part of a string is a key in map (ie. domain name)). I suppose that we should change items from String[] which is then converted to Map<String,Item> to something like Map<String, Map<String, Object>>. This way we could configure items in following form:

'virtual-hosts' = [ 
    'domain1' {
        anon = false
        register = true
        'tls-required' = false
        's2s-secret' = "s2spasswd"
        'domain-filter'=LOCAL
        'max-users' = 1000
    },
    'domain2' {},
    'domain3' {
        'c2s-ports-allowed' = [5222, 5223, 5280, 5290]
    } 
    'domain4' {
        'domain-filter' = ['whitedomain1', 'whitedomain2'; 'whitedomain3']
    }
]

Using this format would allow us to easily set any field of VHostItem (or any other implementation of an item, but we would need to create a new method for RepositoryItem such as initFromMap instead of initFromPropertyString and this new method would be required to fill fields within VHostItem with proper values. We should be able to do a generic method for that using @ConfigField annotations similar to how is done in case of beans by a kernel, but we would need a separate mechanism as we cannot make items beans as it would be very problematic.

What do you think?

Version is set to next minor, so it is for 8.1.0, so it should wait? or can it be done for 8.0.0?

Note: We will change how to set things in config file so we should consider if it should be done in 8.0.0 or 8.1.0

wojciech.kapcia@tigase.net commented 7 years ago

The proposed format (with map) is good enough (improves greatly readability). Given that 8.0.0 already introduces complete change to the configuration I would push that in the 8.0.0 ( %kobit ?), but the question remains about remaining CompRepos -- should we apply same treatment to them?

Andrzej Wójcik (Tigase) commented 7 years ago

%wojtek My suggestion for the new format was for all repositories based on CompRepo. We cannot have it only for single implementation, we need to make it for all of them to keep implementations unified.

Artur Hefczyc commented 7 years ago

My first reaction is to stop making any more changes to 8.0.0 ans finally release the version to the public. However, if Wojciech, who is against making any more changes to 8.0 suggests to add this to the version, then I am OK with adding this change to the 8.0.0.

Andrzej Wójcik (Tigase) commented 7 years ago

I've modified the implementation of configuration handling for classes related to CompRepo and CompRepoItem and added generic support for handling configuration using DSL-like configuration. Code for converting old configuration format (string) to new DSL configuration is already in place and in my test cases, all worked well.

Wojtek, please check if it works ok for you.

Later on, we will need to update the documentation to reflect changes which I've made in this task.

wojciech.kapcia@tigase.net commented 7 years ago

It seems to fail on TTS-NG results:

  =============================================================================
  ERROR! Terminating the server process.
  Problem initializing the server: java.lang.ClassCastException: tigase.conf.ConfigReader$CompositeVariable cannot be cast to java.util.List
  Please fix the problem and start the server again.
  =============================================================================

We have there:

'virtual-hosts' = 'localhost,a.localhost:clientCertCA=' + env('CONFIG_BASE_DIR') + '/certs/root_ca.pem:clientCertRequired=true'

Could you please check/adjust TTS-NG configuration (i.e. tigase-tts-ng/src/test/resources/server/etc/config.tdsl)?

Andrzej Wójcik (Tigase) commented 7 years ago

%wojtek I think that if env is used it fails to do the conversion to the new format. I can adjust config for TTS-NG or try work on the converter to accept env and deal with it. What would be better?

wojciech.kapcia@tigase.net commented 7 years ago

Andrzej Wójcik wrote:

%wojtek I think that if env is used it fails to do the conversion to the new format. I can adjust config for TTS-NG or try work on the converter to accept env and deal with it. What would be better?

I think that we should only adjust the configuration in TTS as:

  • environment variables are a new thing to DSL configuration (so majority of configurations won't fall under it)
  • it's a bit niché feature so I reckon it's use is not that wide.
Andrzej Wójcik (Tigase) commented 7 years ago

There were a few issues here:

  • old config repo items format in the config file
  • issue with dumping config in which property is assigned a composite variable
  • values in the component repository where not set properly
issue 1 of 1
Type
New Feature
Priority
Normal
Assignee
RedmineID
6834
Version
tigase-server-8.0.0
Spent time
52h 30m
Issue Votes (0)
Watchers (0)
Reference
tigase/_server/server-core#927
Please wait...
Page is in error, reload to recover