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

ISPN-14710 Allow mutation of cache authorization roles #10923

Merged

Conversation

tristantarrant
Copy link
Member

@ryanemerson
Copy link
Contributor

@tristantarrant We should really create a second Jira for the connection-interval as it's not related to authorization at all 🙂

@tristantarrant
Copy link
Member Author

@tristantarrant We should really create a second Jira for the connection-interval as it's not related to authorization at all slightly_smiling_face

https://issues.redhat.com/browse/ISPN-14861

@ryanemerson
Copy link
Contributor

Rebuilding as CI failed to complete again 😢

@tristantarrant tristantarrant force-pushed the ISPN-14710/mutable_cache_roles branch from 005cab9 to 29bf4db Compare May 19, 2023 13:11
@tristantarrant
Copy link
Member Author

Rebased on top of d0574a2

@tristantarrant tristantarrant force-pushed the ISPN-14710/mutable_cache_roles branch from 29bf4db to d80d210 Compare May 23, 2023 11:36
Copy link
Member

@jabolina jabolina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Check style is complaining on CI, though.

}

@Override
public int maxBatchSize() {
return maxBatchSize.get();
return attributes.attribute(MAX_BATCH_SIZE).get();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a stretch and does not need to apply. Now we're doing a map lookup, and this method is invoked within a loop on RemoteStore:388, so we could change there to hold the value on a variable instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we may want to allow mutation of this, I'm thinking of leaving it as-is here and extracting it from the loop in RemoteStore and ComposedSegmentedLoadStore

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the above change, I just noticed that SchemaTableOperations holds on to maxBatchSize, writeQueryTimeout and readQueryTimeout which means it won't be aware of any changes to these. I've addressed this too.

import org.infinispan.client.rest.RestEntity;
import org.infinispan.client.rest.RestRawClient;
import org.infinispan.client.rest.RestResponse;
import org.infinispan.client.rest.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import style seems different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@tristantarrant tristantarrant force-pushed the ISPN-14710/mutable_cache_roles branch 2 times, most recently from bfa5d4c to 993a460 Compare June 7, 2023 06:48
@ryanemerson
Copy link
Contributor

I'm not sure if the IT changes are related to the changes, but the native failure definitely is:

2023-06-07 03:57:54,135 ERROR [io.qua.run.Application] (main) Failed to start application (with profile [prod]): java.lang.NoSuchMethodException: java.lang.Object.attributes()
	at java.base@17.0.6/java.lang.Class.getDeclaredMethod(DynamicHub.java:2675)
	at org.infinispan.commons.util.ReflectionUtil.findMethod(ReflectionUtil.java:58)
	at org.infinispan.commons.util.ReflectionUtil.findMethod(ReflectionUtil.java:63)
	at org.infinispan.commons.util.ReflectionUtil.findMethod(ReflectionUtil.java:63)
	at org.infinispan.commons.util.ReflectionUtil.findMethod(ReflectionUtil.java:63)
	at org.infinispan.configuration.cache.AbstractStoreConfigurationBuilder.read(AbstractStoreConfigurationBuilder.java:233)
	at org.infinispan.persistence.sifs.configuration.SoftIndexFileStoreConfigurationBuilder.read(SoftIndexFileStoreConfigurationBuilder.java:150)
	at org.infinispan.persistence.sifs.configuration.SoftIndexFileStoreConfigurationBuilder.read(SoftIndexFileStoreConfigurationBuilder.java:23)
	at org.infinispan.configuration.cache.PersistenceConfigurationBuilder.read(PersistenceConfigurationBuilder.java:223)
	at org.infinispan.configuration.cache.ConfigurationBuilder.read(ConfigurationBuilder.java:269)
	at org.infinispan.configuration.parsing.CacheParser.getConfigurationBuilder(CacheParser.java:1140)
	at org.infinispan.configuration.parsing.CacheParser.parseLocalCache(CacheParser.java:138)
	at org.infinispan.configuration.parsing.Parser.parseContainer(Parser.java:764)
	at org.infinispan.configuration.parsing.Parser.readElement(Parser.java:91)
	at org.infinispan.configuration.parsing.ParserRegistry.parseElement(ParserRegistry.java:209)
	at org.infinispan.configuration.parsing.ParserRegistry.parse(ParserRegistry.java:187)
	at org.infinispan.configuration.parsing.ParserRegistry.parse(ParserRegistry.java:175)
	at org.infinispan.configuration.parsing.ParserRegistry.parse(ParserRegistry.java:157)
	at org.infinispan.quarkus.embedded.runtime.InfinispanEmbeddedProducer.manager(InfinispanEmbeddedProducer.java:44)
	at org.infinispan.quarkus.embedded.runtime.InfinispanEmbeddedProducer_ProducerMethod_manager_ed18b755467e1c5fdc8a90c62127a21080a24841_Bean.doCreate(Unknown Source)
	at org.infinispan.quarkus.embedded.runtime.InfinispanEmbeddedProducer_ProducerMethod_manager_ed18b755467e1c5fdc8a90c62127a21080a24841_Bean.create(Unknown Source)
	at org.infinispan.quarkus.embedded.runtime.InfinispanEmbeddedProducer_ProducerMethod_manager_ed18b755467e1c5fdc8a90c62127a21080a24841_Bean.create(Unknown Source)
	at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:113)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:37)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:34)
	at io.quarkus.arc.impl.LazyValue.get(LazyValue.java:26)
	at io.quarkus.arc.impl.ComputingCache.computeIfAbsent(ComputingCache.java:69)
	at io.quarkus.arc.impl.AbstractSharedContext.get(AbstractSharedContext.java:34)
	at org.infinispan.quarkus.embedded.runtime.InfinispanEmbeddedProducer_ProducerMethod_manager_ed18b755467e1c5fdc8a90c62127a21080a24841_Bean.get(Unknown Source)
	at org.infinispan.quarkus.embedded.runtime.InfinispanEmbeddedProducer_ProducerMethod_manager_ed18b755467e1c5fdc8a90c62127a21080a24841_Bean.get(Unknown Source)
	at org.infinispan.quarkus.embedded.EventListener_Bean.doCreate(Unknown Source)
	at org.infinispan.quarkus.embedded.EventListener_Bean.create(Unknown Source)
	at org.infinispan.quarkus.embedded.EventListener_Bean.create(Unknown Source)
	at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:113)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:37)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:34)
	at io.quarkus.arc.impl.LazyValue.get(LazyValue.java:26)
	at io.quarkus.arc.impl.ComputingCache.computeIfAbsent(ComputingCache.java:69)
	at io.quarkus.arc.impl.AbstractSharedContext.get(AbstractSharedContext.java:34)
	at org.infinispan.quarkus.embedded.EventListener_Observer_onStartup_906cc3b46e9e3074a19236551551be7839d6c11e.notify(Unknown Source)
	at io.quarkus.arc.impl.EventImpl$Notifier.notifyObservers(EventImpl.java:346)
	at io.quarkus.arc.impl.EventImpl$Notifier.notify(EventImpl.java:328)
	at io.quarkus.arc.impl.EventImpl.fire(EventImpl.java:82)
	at io.quarkus.arc.runtime.ArcRecorder.fireLifecycleEvent(ArcRecorder.java:155)
	at io.quarkus.arc.runtime.ArcRecorder.handleLifecycleEvents(ArcRecorder.java:106)
	at io.quarkus.deployment.steps.LifecycleEventsBuildStep$startupEvent1144526294.deploy_0(Unknown Source)
	at io.quarkus.deployment.steps.LifecycleEventsBuildStep$startupEvent1144526294.deploy(Unknown Source)
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at io.quarkus.runtime.ApplicationLifecycleManager.run(ApplicationLifecycleManager.java:111)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:71)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:44)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:124)
	at io.quarkus.runner.GeneratedMain.main(Unknown Source)

@tristantarrant
Copy link
Member Author

Well, that specific (horrid) code has been around since 2015, but I guess it's time to remove the use of reflection

@tristantarrant tristantarrant force-pushed the ISPN-14710/mutable_cache_roles branch from 993a460 to 858e99b Compare June 7, 2023 09:12
@tristantarrant
Copy link
Member Author

tristantarrant commented Jun 7, 2023

Let's see what CI says (try reading that aloud 5 times in a row :)

* Exposes mutable attributes
* Deprecate numerous attributes which are no longer in use
@ryanemerson
Copy link
Contributor

org.infinispan.server.security.authorization.RESTAuthorizationTest Seems related.

@ryanemerson ryanemerson merged commit 2a47a53 into infinispan:main Jun 15, 2023
3 of 4 checks passed
@ryanemerson
Copy link
Contributor

Thanks @tristantarrant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants