Skip to content

Commit

Permalink
Use simple DataListener
Browse files Browse the repository at this point in the history 
Reuse the utility provided from mdsal-binding-api rather than rolling
our own -- making things a lot simpler.

Change-Id: I04a7deb174a362d89957211c63bd660af551ee40
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
  • Loading branch information
@rovarga
rovarga committed Dec 29, 2023
1 parent c71ebda commit 0c63846
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 116 deletions.
28 changes: 9 additions & 19 deletions ...pl/src/main/java/org/opendaylight/aaa/encrypt/impl/OSGiEncryptionServiceConfigurator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,21 @@
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Iterables;
import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.MoreExecutors;
import java.security.SecureRandom;
import java.util.Base64;
import java.util.Collection;
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import org.apache.commons.lang3.RandomStringUtils;
import org.checkerframework.checker.lock.qual.GuardedBy;
import org.checkerframework.checker.lock.qual.Holding;
import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.Nullable;
import org.opendaylight.mdsal.binding.api.ClusteredDataTreeChangeListener;
import org.opendaylight.mdsal.binding.api.DataBroker;
import org.opendaylight.mdsal.binding.api.DataListener;
import org.opendaylight.mdsal.binding.api.DataTreeIdentifier;
import org.opendaylight.mdsal.binding.api.DataTreeModification;
import org.opendaylight.mdsal.common.api.CommitInfo;
import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
import org.opendaylight.odlparent.logging.markers.Markers;
Expand All @@ -52,12 +49,11 @@
* <p>
* We primarily listen to the configuration being present. Whenever the salt is missing or the password does not match
* the required length, we generate them and persist them. This mode of operation means we potentially have a loop, i.e.
* our touching the datastore will trigger again {@link #onDataTreeChanged(Collection)}, which will re-evaluate the
* conditions and we try again.
* our touching the datastore will trigger again {@link #dataChangedTo(AaaEncryptServiceConfig)}, which will re-evaluate
* the conditions and we try again.
*/
@Component(service = { })
public final class OSGiEncryptionServiceConfigurator
implements ClusteredDataTreeChangeListener<AaaEncryptServiceConfig> {
public final class OSGiEncryptionServiceConfigurator implements DataListener<AaaEncryptServiceConfig> {
private static final Logger LOG = LoggerFactory.getLogger(OSGiEncryptionServiceConfigurator.class);
private static final SecureRandom RANDOM = new SecureRandom();
private static final @NonNull AaaEncryptServiceConfig DEFAULT_CONFIG = new AaaEncryptServiceConfigBuilder()
Expand Down Expand Up @@ -86,7 +82,7 @@ public OSGiEncryptionServiceConfigurator(@Reference final DataBroker dataBroker,
final ComponentFactory<AAAEncryptionServiceImpl> factory) {
this.dataBroker = requireNonNull(dataBroker);
this.factory = requireNonNull(factory);
reg = dataBroker.registerDataTreeChangeListener(
reg = dataBroker.registerDataListener(
DataTreeIdentifier.create(LogicalDatastoreType.CONFIGURATION,
InstanceIdentifier.create(AaaEncryptServiceConfig.class)),
this);
Expand All @@ -102,23 +98,17 @@ public synchronized void deactivate() {
}

@Override
public void onDataTreeChanged(final Collection<DataTreeModification<AaaEncryptServiceConfig>> changes) {
public void dataChangedTo(final AaaEncryptServiceConfig data) {
// Acquire the last reported configuration and check if it needs to have salt/password generated.
final var dsConfig = Iterables.getLast(changes).getRootNode().getDataAfter();
if (dsConfig == null || needKey(dsConfig) || needSalt(dsConfig)) {
if (data == null || needKey(data) || needSalt(data)) {
// Generate salt/key as needed and persist it -- causing us to be re-invoked later.
updateDatastore(dsConfig);
updateDatastore(data);
} else {
// Configuration is self-consistent, proceed to activate an instance based on it
updateInstance(dsConfig);
updateInstance(data);
}
}

@Override
public void onInitialData() {
updateDatastore(null);
}

@VisibleForTesting
static @NonNull AaaEncryptServiceConfig generateConfig(final @Nullable AaaEncryptServiceConfig datastoreConfig) {
// Select template and decide which parts need to be updated
Expand Down
24 changes: 7 additions & 17 deletions ...rc/test/java/org/opendaylight/aaa/encrypt/impl/OSGiEncryptionServiceConfiguratorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import java.util.Base64;
import java.util.Dictionary;
import java.util.List;
import java.util.Optional;
import org.eclipse.jdt.annotation.NonNull;
import org.junit.Before;
Expand All @@ -30,16 +29,14 @@
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import org.opendaylight.mdsal.binding.api.DataBroker;
import org.opendaylight.mdsal.binding.api.DataObjectModification;
import org.opendaylight.mdsal.binding.api.DataTreeChangeListener;
import org.opendaylight.mdsal.binding.api.DataListener;
import org.opendaylight.mdsal.binding.api.DataTreeIdentifier;
import org.opendaylight.mdsal.binding.api.DataTreeModification;
import org.opendaylight.mdsal.binding.api.ReadWriteTransaction;
import org.opendaylight.mdsal.common.api.CommitInfo;
import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
import org.opendaylight.yang.gen.v1.config.aaa.authn.encrypt.service.config.rev160915.AaaEncryptServiceConfig;
import org.opendaylight.yang.gen.v1.config.aaa.authn.encrypt.service.config.rev160915.EncryptServiceConfig;
import org.opendaylight.yangtools.concepts.ListenerRegistration;
import org.opendaylight.yangtools.concepts.Registration;
import org.opendaylight.yangtools.util.concurrent.FluentFutures;
import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
import org.osgi.service.component.ComponentFactory;
Expand All @@ -57,17 +54,13 @@ public class OSGiEncryptionServiceConfiguratorTest {
@Mock
private ComponentInstance<AAAEncryptionServiceImpl> instance;
@Mock
private ListenerRegistration<?> registration;
private Registration registration;
@Mock
private ReadWriteTransaction transaction;
@Mock
private DataTreeModification<AaaEncryptServiceConfig> treeModification;
@Mock
private DataObjectModification<AaaEncryptServiceConfig> objectModification;
@Captor
private ArgumentCaptor<DataTreeIdentifier<AaaEncryptServiceConfig>> treeIdCaptor;
@Captor
private ArgumentCaptor<DataTreeChangeListener<AaaEncryptServiceConfig>> listenerCaptor;
private ArgumentCaptor<DataListener<AaaEncryptServiceConfig>> listenerCaptor;
@Captor
private ArgumentCaptor<AaaEncryptServiceConfig> configCaptor;
@Captor
Expand All @@ -77,8 +70,7 @@ public class OSGiEncryptionServiceConfiguratorTest {

@Before
public void before() {
doReturn(registration).when(dataBroker).registerDataTreeChangeListener(treeIdCaptor.capture(),
listenerCaptor.capture());
doReturn(registration).when(dataBroker).registerDataListener(treeIdCaptor.capture(), listenerCaptor.capture());

configurator = new OSGiEncryptionServiceConfigurator(dataBroker, factory);

Expand All @@ -101,7 +93,7 @@ public void testEmptyDatastore() {
doNothing().when(transaction).put(eq(LogicalDatastoreType.CONFIGURATION), eq(IID), configCaptor.capture());
doReturn(CommitInfo.emptyFluentFuture()).when(transaction).commit();

configurator.onInitialData();
configurator.dataChangedTo(null);

final var config = configCaptor.getValue();
assertEquals("AES/CBC/PKCS5Padding", config.getCipherTransforms());
Expand All @@ -119,11 +111,9 @@ public void testEmptyDatastore() {
assertEquals(12, key.length());

// Now we circle around are report that config. We expect the factory to be called
doReturn(config).when(objectModification).getDataAfter();
doReturn(objectModification).when(treeModification).getRootNode();
doReturn(instance).when(factory).newInstance(propertiesCaptor.capture());

configurator.onDataTreeChanged(List.of(treeModification));
configurator.dataChangedTo(config);

final var props = propertiesCaptor.getValue();
assertNotNull(props);
Expand Down
29 changes: 7 additions & 22 deletions ...n/java/org/opendaylight/aaa/impl/password/service/OSGiPasswordServiceConfigBootstrap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,13 @@
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.Beta;
import com.google.common.collect.Iterables;
import java.util.Collection;
import org.checkerframework.checker.lock.qual.Holding;
import org.opendaylight.mdsal.binding.api.ClusteredDataTreeChangeListener;
import org.opendaylight.mdsal.binding.api.DataBroker;
import org.opendaylight.mdsal.binding.api.DataListener;
import org.opendaylight.mdsal.binding.api.DataTreeIdentifier;
import org.opendaylight.mdsal.binding.api.DataTreeModification;
import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
import org.opendaylight.yang.gen.v1.urn.opendaylight.aaa.password.service.config.rev170619.PasswordServiceConfig;
import org.opendaylight.yang.gen.v1.urn.opendaylight.aaa.password.service.config.rev170619.PasswordServiceConfigBuilder;
import org.opendaylight.yangtools.concepts.ListenerRegistration;
import org.opendaylight.yangtools.concepts.Registration;
import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
import org.osgi.service.component.ComponentFactory;
import org.osgi.service.component.ComponentInstance;
Expand All @@ -33,20 +29,19 @@

@Beta
@Component(service = { })
public final class OSGiPasswordServiceConfigBootstrap
implements ClusteredDataTreeChangeListener<PasswordServiceConfig> {
public final class OSGiPasswordServiceConfigBootstrap implements DataListener<PasswordServiceConfig> {
private static final Logger LOG = LoggerFactory.getLogger(OSGiPasswordServiceConfigBootstrap.class);

private final ComponentFactory<OSGiPasswordServiceConfig> configFactory;
private ListenerRegistration<?> registration;
private Registration registration;
private ComponentInstance<?> instance;

@Activate
public OSGiPasswordServiceConfigBootstrap(@Reference final DataBroker dataBroker,
@Reference(target = "(component.factory=" + OSGiPasswordServiceConfig.FACTORY_NAME + ")")
final ComponentFactory<OSGiPasswordServiceConfig> configFactory) {
this.configFactory = requireNonNull(configFactory);
registration = dataBroker.registerDataTreeChangeListener(
registration = dataBroker.registerDataListener(
DataTreeIdentifier.create(LogicalDatastoreType.CONFIGURATION,
InstanceIdentifier.create(PasswordServiceConfig.class)), this);
LOG.info("Listening for password service configuration");
Expand All @@ -64,21 +59,11 @@ synchronized void deactivate() {
}

@Override
public synchronized void onInitialData() {
updateInstance(null);
}

@Override
public synchronized void onDataTreeChanged(final Collection<DataTreeModification<PasswordServiceConfig>> changes) {
public synchronized void dataChangedTo(final PasswordServiceConfig data) {
// FIXME: at this point we need to populate default values -- from the XML file
updateInstance(Iterables.getLast(changes).getRootNode().getDataAfter());
}

@Holding("this")
private void updateInstance(final PasswordServiceConfig config) {
if (registration != null) {
final var newInstance = configFactory.newInstance(
OSGiPasswordServiceConfig.props(config != null ? config : new PasswordServiceConfigBuilder().build()));
OSGiPasswordServiceConfig.props(data != null ? data : new PasswordServiceConfigBuilder().build()));
if (instance != null) {
instance.dispose();
}
Expand Down
23 changes: 8 additions & 15 deletions .../impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@
import static com.google.common.base.Verify.verifyNotNull;
import static java.util.Objects.requireNonNull;

import com.google.common.collect.Iterables;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Comparator;
import java.util.List;
import java.util.Optional;
Expand All @@ -25,16 +23,14 @@
import javax.servlet.http.HttpServletRequest;
import org.apache.shiro.subject.Subject;
import org.apache.shiro.web.filter.authz.AuthorizationFilter;
import org.opendaylight.mdsal.binding.api.ClusteredDataTreeChangeListener;
import org.opendaylight.mdsal.binding.api.DataBroker;
import org.opendaylight.mdsal.binding.api.DataListener;
import org.opendaylight.mdsal.binding.api.DataTreeIdentifier;
import org.opendaylight.mdsal.binding.api.DataTreeModification;
import org.opendaylight.mdsal.binding.api.ReadTransaction;
import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.HttpAuthorization;
import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.http.authorization.policies.Policies;
import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.http.permission.Permissions;
import org.opendaylight.yangtools.concepts.ListenerRegistration;
import org.opendaylight.yangtools.concepts.Registration;
import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
import org.slf4j.Logger;
Expand All @@ -49,9 +45,7 @@
* <p>This mechanism will only work when put behind <code>authcBasic</code>.
*/
@SuppressWarnings("checkstyle:AbbreviationAsWordInName")
public class MDSALDynamicAuthorizationFilter extends AuthorizationFilter
implements ClusteredDataTreeChangeListener<HttpAuthorization> {

public class MDSALDynamicAuthorizationFilter extends AuthorizationFilter implements DataListener<HttpAuthorization> {
private static final Logger LOG = LoggerFactory.getLogger(MDSALDynamicAuthorizationFilter.class);

private static final DataTreeIdentifier<HttpAuthorization> AUTHZ_CONTAINER = DataTreeIdentifier.create(
Expand All @@ -61,7 +55,7 @@ public class MDSALDynamicAuthorizationFilter extends AuthorizationFilter

private final DataBroker dataBroker;

private ListenerRegistration<?> reg;
private Registration reg;
private volatile ListenableFuture<Optional<HttpAuthorization>> authContainer;

public MDSALDynamicAuthorizationFilter() {
Expand All @@ -82,7 +76,7 @@ public Filter processPathConfig(final String path, final String config) {
try (ReadTransaction tx = dataBroker.newReadOnlyTransaction()) {
authContainer = tx.read(AUTHZ_CONTAINER.getDatastoreType(), AUTHZ_CONTAINER.getRootIdentifier());
}
reg = dataBroker.registerDataTreeChangeListener(AUTHZ_CONTAINER, this);
reg = dataBroker.registerDataListener(AUTHZ_CONTAINER, this);
return super.processPathConfig(path, config);
}

Expand All @@ -96,10 +90,9 @@ public void destroy() {
}

@Override
public void onDataTreeChanged(final Collection<DataTreeModification<HttpAuthorization>> changes) {
final HttpAuthorization newVal = Iterables.getLast(changes).getRootNode().getDataAfter();
LOG.debug("Updating authorization information to {}", newVal);
authContainer = Futures.immediateFuture(Optional.ofNullable(newVal));
public void dataChangedTo(final HttpAuthorization data) {
LOG.debug("Updating authorization information to {}", data);
authContainer = Futures.immediateFuture(Optional.ofNullable(data));
}

@Override
Expand Down Expand Up @@ -149,7 +142,7 @@ public boolean isAccessAllowed(final ServletRequest request, final ServletRespon
LOG.debug("paths match for pattern={} and requestURI={}", resource, requestURI);
final String method = httpServletRequest.getMethod();
LOG.trace("method={}", method);
for (Permissions permission : policy.getPermissions()) {
for (Permissions permission : policy.nonnullPermissions()) {
final String role = permission.getRole();
LOG.trace("role={}", role);
for (Permissions.Actions action : permission.getActions()) {
Expand Down
Loading

0 comments on commit 0c63846

Please sign in to comment.