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

Need to support per API Log Filtering based on API Resource #1564

Closed
tharikaGitHub opened this issue Mar 8, 2023 · 27 comments
Closed

Need to support per API Log Filtering based on API Resource #1564

tharikaGitHub opened this issue Mar 8, 2023 · 27 comments

Comments

@tharikaGitHub
Copy link
Member

tharikaGitHub commented Mar 8, 2023

Description

Currently we do not have the capability to enable per API Logging based on API Resource as well. We need to add an improvement to have this capability in the product.

As an additional requirement, the changes to this log configuration should be logged as audit logs. These audit logs should contain the timestamp, type of the event, subject identified (if applicable) and the outcome (Success / Failure).

Affected Component

APIM

Version

4.2.0

Related Issues

No response

Suggested Labels

No response

@shnrndk
Copy link

shnrndk commented Mar 13, 2023

Update 2023-03-13

Understood the codebase and identified what methods need to be changed
Identified the logAPI method, which is used to print the logs
Changed the LoggingApiInput schema to include the resourceMethod and resourcePath keys in the devops-api.yaml
Changed the tenantLogsTenantIdApisApiIdPut to include the resourceMethod and resourcePath parameters

@shnrndk
Copy link

shnrndk commented Mar 15, 2023

Update 2023-03-14

Identified that previously log info per API was added to the AM_API table.
But when we doing it for the resourceMethod and resourcePath level, it is not practical to add this the same AM_API table.
Therefore had a call to discuss this issue with Tharika and Arshadh.
We have decided to put the log info to the AM_API_URL_MAPPING table since it have HTTP_METHOD and URL_PATTERN columns.

@shnrndk
Copy link

shnrndk commented Mar 20, 2023

Update (2023-03-17)

Added a new LOG_LEVEL column to AM_API_URL_MAPPING table to store the log status of the each resource and removed the LOG_LEVEL column from AM_API table.
Updated the SQL queries to support with the AM_API_URL_MAPPING table.

The problem with this approach is that

  • All the revisions that have the same resource ex: GET /menu will get the same log level defined by the user.
  • We can provide an option for the user in the request body to selectively revert all the previously applied log levels on other resources and keep the current log level on the current resource.
  • By default we will keep the previously applied log levels for the same API when logging is enabled for a specific resource.
  • In that case how do we handle the log levels of the newly created resources?

On 17 th I had a call with Arshash, Tharika and Dushani, We were decided to schedule a design review to talk about this issue.
I have created a design doc for this.

@shnrndk
Copy link

shnrndk commented Mar 21, 2023

Update 2023-03-21

Had the design review today. At this meeting we finalyzed the DB design.

Attendees: Arshardh Ifthikar, Chamila Adhikarinayake, Isuru Udana, Nandika Jayawardana, Tharika Madurapperuma, Thilini Shanika

Meeting Notes

  • We should be able to use the Log_Level column in the AM_API table as well without removing it. Here we can check API level first and apply the highest out of the 2. Then no migration is required. This approach will be backward compatible. In the Gateway side we will have to keep 2 mappings in this case.
  • We need to check who will be doing the resolving if we move ahead with the above approach.
  • Url mappings also get applied to the products. Hence we need to check if anything breaks when products are used.

@shnrndk
Copy link

shnrndk commented Mar 24, 2023

Update 2023-03-23

Changed the methods related to

  1. Get log level details of APIs.
  2. Get log level of an API.
  3. Set log level of an API.

According to the suggestions given by the design review.

  • Now the first call will get results from both the tables which are AM_API and AM_API_URL_MAPPING and display a consolidated result for the user.
  • Second call will get results from both the tables display the most significant log level for the API.
  • Now by using the third call users can set the log level for the api level and for the api resource level as well as.

@shnrndk
Copy link

shnrndk commented Mar 24, 2023

Update 2023-03-24

  • Fixed some bugs of the new implementation of the get log level of API rest API call.
  • Added to display only the significant resource log level for the API, and discard the insignificant log level lesser than the log level of the Api level.
  • Added the resourceMethod and resourcePath to the result when user is requesting to get log level info for the API.
  • Made the PR for the current work.

@shnrndk
Copy link

shnrndk commented Mar 28, 2023

Update 2023-03-28

  • Fixed a unit test failure.
  • There is an integration test failure in the current work I've made, I have identified the reason for this and fixed it.
  • Identified that there needs to add the resourcePath and resourceMethod properties in the logAPI method in order to filter the logs according to resourcePath and resourceMethod.
  • Had a call Tharika how to add these properties.
  • We have identified that to get the log level per API it uses this rest api - "/api-logging-configs".
  • I have changed this rest api to get the resource Level and resource Path properties also.
  • Then we have Identified that getAPILogLevel() method, APILoggerManager class and getMatchingLogLevel() methods have to be changed in order to add these properties.
  • Today I have changed these methods.

@shnrndk
Copy link

shnrndk commented Mar 30, 2023

Update 2023-03-30

  • Fixed some bugs of the current implementation
  • Added a way to get the resourceMethod and resourcePath In the getMatchingLogLevel() method
  • Added a method to choose the most significant log level from api level and resource level.
  • Added these current changes to the PR.
  • Currently the main implementation is almost done right now.
  • Small changes need to be done from tomorrow onwards.

@shnrndk
Copy link

shnrndk commented Apr 5, 2023

Update 2023-04-04

  • Changed logResourceProperties inner class to properties hash map.
  • Added resource Method and resource Path to APIEvent class.
  • Changed RETRIEVE_ALL_PER_API_RESOURCE_LOGGING_SQL to return only the non revision APIs
  • Fixed some bugs in the implementation
  • Started out testing the implementation by giving out some scenarios.

@shnrndk
Copy link

shnrndk commented Apr 8, 2023

Update 2023-04-07

  • Tested out some scenarios
  • In the testing I have found that /order/10 like queries do not match /order/{orderId}. Therefore I have added a regex matching to support these types of queries. This was added with the commit wso2/carbon-apimgt@8496d75.
  • Also in the testing I have found that when the user sets the log level per resource, and set another log level for the the same resource, after a small amount of time, the previous log level do not get changed. This was a bug and I fixed it with wso2/carbon-apimgt@557a1d5.

@shnrndk
Copy link

shnrndk commented Apr 12, 2023

Update 2023-04-12

  • Tested out the whole scenario.
  • In the testing I have found that there are some null point exceptions happening in some of the flows. Today I've fixed those issues.
  • And in the regex matching I have found that it only matches /order/{orderId} if only orderId is in digits. If the orderId is like 24234-3423-4324-343, then it doesn't get matched and then it doesn't get filtered. This bug is fixed right now.
  • Also If the user sets the log level for an unavailable resource, there should be a validation to check this. I have added a validation method to set log level flow.

@shnrndk
Copy link

shnrndk commented Apr 24, 2023

Update 2023-04-20

Had the code review on 18th, At this meeting I've showed a small demo of the implementation, and explained the codebase.
Tharindu mentioned that, in the getMatchingLogLevel method,

if (apiCtx.split("/", 3).length > 2) {
    apiResourcePath = apiCtx.split("/", 3)[2];
} else {
    apiResourcePath = "";
}

Needs to be changed, Since if the context contains forward slashes, then whole splitting process will get wronged.
Therefore I have changed this to

String apiResourcePath = ("/" + apiCtx).replaceAll(key.get("context"), "");

Like this.

And also Tharindu mentioned that it is not good to match resources using regex again in this method. Since regex matching is a very high CPU intensive task. Tharindu suggested to discuss this again in another meeting to find a solution to this.

resourcePathRegexPattern = logResourcePath.replace("/", "\\/");
                // Replace the curly braces with the given regex pattern
                resourcePathRegexPattern = resourcePathRegexPattern.replaceAll("\\{.*?\\}",
                        "\\\\w{0,}([-,_]?\\\\w{1,})+");
} else if ((apiResourcePath).matches(resourcePathRegexPattern)
                        && apiHttpMethod.equals(key.get("resourceMethod"))) {
                    resourceLogLevel = entry.getValue();
                }

We had another discussion for this issue on 20th with Isuru Udana. Isuru mentioned that if we put a matching resource pattern property in a messageContext, then we can resolve this issue easily and it would be beneficial for future fixes as well.
For that he suggested to look in the synapse repo.
On 21st I am on leave, therefore in this week I am working in this solution.

@shnrndk shnrndk closed this as completed Apr 24, 2023
@shnrndk shnrndk reopened this Apr 24, 2023
@chamikasudusinghe chamikasudusinghe self-assigned this May 24, 2023
@chamikasudusinghe
Copy link

chamikasudusinghe commented May 24, 2023

Date: 23rd & 24th May 2023

Tasks:

Update

Currently working on building the branch from the public repository. It seems there is an issue with the dependencies in the local machines with has caused a compilations error.

  • Went back and forth between java versions (JAVA 8 and JAVA 11)
  • Tried to build using both mvn clean install -Dmaven.test.skip=true and mvn -s /Users/chamika/u2-nexus-maven-settings.xml clean install -Dmaven.test.skip=true -U for both have versions.
  • For mvn clean install -Dmaven.test.skip=true, only 4 components are built and the following error comes
[ERROR] Failed to execute goal on project org.wso2.carbon.apimgt.eventing.hub: Could not resolve dependencies for project org.wso2.carbon.apimgt:org.wso2.carbon.apimgt.eventing.hub:bundle:9.28.146-SNAPSHOT: 
Failed to collect dependencies at org.wso2.carbon.analytics-common:org.wso2.carbon.event.output.adapter.core:jar:5.3.5 -> org.wso2.carbon:org.wso2.carbon.core:jar:4.8.1 -> wsdl4j.wso2:wsdl4j:jar:1.6.2.wso2v4: 
Failed to read artifact descriptor for wsdl4j.wso2:wsdl4j:jar:1.6.2.wso2v4: Failure to find org.wso2.wsdl4j:wsdl4j-parent:pom:1.6.2-wso2v4 in https://maven.wso2.org/nexus/content/groups/wso2-public/ was cached in the local repository, resolution will not be reattempted until the update interval of wso2-nexus has elapsed or updates are forced -> [Help 1]
  • For mvn -s /Users/chamika/u2-nexus-maven-settings.xml clean install -Dmaven.test.skip=true -U, 10 components are built and the following error comes
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project org.wso2.carbon.apimgt.impl: Compilation failure
[ERROR] /Users/chamika/Documents/carbon-apimgt/components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/kmclient/KMClientErrorDecoder.java:[55,45] no suitable method found for toString(java.io.InputStream,java.nio.charset.Charset)
[ERROR]     method org.apache.commons.io.IOUtils.toString(java.io.InputStream,java.lang.String) is not applicable
[ERROR]       (argument mismatch; java.nio.charset.Charset cannot be converted to java.lang.String)
[ERROR]     method org.apache.commons.io.IOUtils.toString(byte[],java.lang.String) is not applicable
[ERROR]       (argument mismatch; java.io.InputStream cannot be converted to byte[])
  • Resolve this dependency issue in carbon-apimgt by purging the package using mvn dependency:purge-local-repository clean package.
  • Currently facing an error in building the product and trying to purge the dependencies as did for carbon-apimgt.
[ERROR] Failed to execute goal on project org.wso2.carbon.am.integration.benchmark.test: Could not resolve dependencies for project org.wso2.am:org.wso2.carbon.am.integration.benchmark.test:jar:4.2.0-SNAPSHOT: 
Failure to find org.wso2.am:org.wso2.carbon.am.integration.backend.test:jar:tests:4.2.0-SNAPSHOT in https://maven.wso2.org/nexus/content/groups/wso2-public/ was cached in the local repository, resolution will not be reattempted until the update interval of wso2-nexus has elapsed or updates are forced -> [Help 1]

@chamikasudusinghe
Copy link

Date: 25th of May 2023

Completed watching both the available recordings (code review & discussion on implementation with synapse) for this feature. The pack was built (along with the benchmark test failure) and the current fix was debugged for the current version of the fix using the branch https://github.com/shnrndk/carbon-apimgt/tree/ResourceLevelLogfilteringCopy.

@chamikasudusinghe
Copy link

Update

  • Tested the flow of the current fix and the suggested fix in the code review and other discussions.

In the current implementation, resource-level log details are identified in the logs handler which is executed at first before creating the API object from the API class. When logs are enabled this is called in 4 instances; handleRequestInFlow, handleRequestOutFlow, handleResponseInFlow and handleResponseOutFlow. Here, in the implementation, the identification of the resource is happening as follows.

  1. API context with the resource is identified using getTransportInURL()
  2. Currently executing REST method is identified by getHTTPMethod ()
  3. Then there is a loop to go through the entries for resources in log properties
  4. From the recourses in log properties the resource path is identified and it is changed into a regex pattern
  5. The above templated path will be compared with the resource path received from API context along with the comparison with the resource method.

In synapse, there is a comparison of resources happening using the findResource() method. Unlike to the approach above, this happens within process() method in the API class. By the time this happens, the message context is populated with the respective API object. Following is the flow here;

  1. An API object is created using the createAPI method
  2. Resources are added using addResource method from the OMElement
  3. From this added recourses, acceptable resources are chosen with the condition isBound(r, synCtx) && r.canProcess(synCtx)
  4. If there exists acceptable resources in the findResource method, matching is done. For this there exist three approaches; URITemplateBasedDispatcher, URLMappingBasedDispatcher, DefaultDispatcher

The difference between the two approaches are, considering where we are trying now to implement this solution, we do not have an API object when we execute the log handler. But in synapse, when it is doing the matching for resources, the API is already process and the object is there.

The class execution happens in the following order;

  1. LogsHandler
  2. API
  3. CORSRequestHandler

Hence, in order to get a similar implementation here, we need to figure out a way to create an api object in the logs handler and then using that try to create similar methods to do the comparison.

@chamikasudusinghe
Copy link

chamikasudusinghe commented Jun 5, 2023

Update

  • Implementation of a function to get the API object and do the resource matching in the LogsHandler

The API collection can be obtained from the message context using the synapse configuration. Using that, we can iterate through the api set to identify the API. In the below code, as for testing, the method identifyAPI() has been taken as it is from synapse.

Once an API is identified, the resources can be obtained from the selected api and then proceed with step 3 & 4 as in above.

messageContext.getEnvironment().getSynapseConfiguration().getAPIs();
selectedApi.getResources();

This approach cannot be directly implemented here since in identifyAPI(), the API is processed which is done in synapses not in the global handle. Hence for identification of the API a different approach is needed to be taken into consideration.

protected static void fetchLogLevel(MessageContext messageContext,
                                                Map<Map<String, String>, String> logProperties) {
        Collection<API> apiSet = messageContext.getEnvironment().getSynapseConfiguration().getAPIs();
        List<API> defaultStrategyApiSet = new ArrayList<API>(apiSet);
        List<API> duplicateApiSet = new ArrayList<API>(apiSet);
        API selectedApi = null;
        for (API api : duplicateApiSet) {
            if (identifyAPI(api, messageContext, defaultStrategyApiSet)) {
                selectedApi = api;
            }
        }
        String httpMethod = (String) ((Axis2MessageContext) messageContext).getAxis2MessageContext().
                getProperty(Constants.Configuration.HTTP_METHOD);
        org.apache.axis2.context.MessageContext axis2MC = ((Axis2MessageContext) messageContext)
                .getAxis2MessageContext();
        Map headers = (Map) axis2MC.getProperty(org.apache.axis2.context.MessageContext.TRANSPORT_HEADERS);
        String corsRequestMethod = (String) headers.get(APIConstants.CORSHeaders.ACCESS_CONTROL_REQUEST_METHOD);
        Resource selectedResource = null;

        if (selectedApi != null) {
            Resource[] allAPIResources = selectedApi.getResources();
            Set<Resource> acceptableResources = new LinkedHashSet<>();

            for (Resource resource : allAPIResources) {
                //If the requesting method is OPTIONS or if the Resource contains the requesting method
                if ((RESTConstants.METHOD_OPTIONS.equals(httpMethod) && resource.getMethods() != null &&
                        Arrays.asList(resource.getMethods()).contains(corsRequestMethod)) ||
                        (resource.getMethods() != null && Arrays.asList(resource.getMethods()).contains(httpMethod))) {
                    acceptableResources.add(resource);
                }
            }

            if (!acceptableResources.isEmpty()) {
                for (RESTDispatcher dispatcher : RESTUtils.getDispatchers()) {
                    Resource resource = dispatcher.findResource(messageContext, acceptableResources);
                    if (resource != null) {
                        selectedResource = resource;
                        break;
                    }
                }
            }
        }

    }
  • Current approach in identifying the resource from the message context and way forward

Currently, API resource path is identified by splitting the api context received from getTransportInURL().

apiResourcePath = apiCtx.split("/", 3)[2];

Here, we make an assumption that the api resource path is as follows; ex: api/version/resource/identifier. This is directly compared with the resource path in log properties. However, we need to have a better approach to find the resource first from an API and that resource should be compared with the resource path in log properties.

  • Identify the API

When it comes to the resources in log properties, these log-enabled resources may belong to multiple APIs. Since now the API object is involved in this scenario, we need to identify the API as well in this mechanism without processing the API.

@chamikasudusinghe
Copy link

chamikasudusinghe commented Jun 5, 2023

Update

Next Steps

Resource

  • Identify resources belonging to an API in the logshandler
  • From the resources in a selected API, match with loggable resources

APIs

  • Identify the API invoked in the message context from the set of APIs

@chamikasudusinghe
Copy link

Update

  • Identification of resources was done using the same implementation as in synapse using the message context and the available resources. Here, to get the available resources, the API object is used to find all resources.

dispatcher.findResource(messageContext, acceptableResources);

  • Matching with the resource(s) in log properties is done by comparing the resource paths in the templatehelper and the resource path in the particular object within log properties. Before matching the paths, the method is matched using the API context to reduce computations.

apiHttpMethod.equals(key.get("resourceMethod")
templateHelper.getString().equals(key.get("resourcePath"));

  • Identification of the invoked API is done considering the API context received both from the TransportInURL and the API object. Since the TransportInURL includes the resource as well, it is checked whether that contains the API context in the object.
apiContext = ((Axis2MessageContext)messageContext).getAxis2MessageContext().getProperty("TransportInURL").toString();
apiContext.contains(api.getContext());

Using this approach allows to replace the IdentifyAPI() method which processes the API. However, dropping this method causes a java.lang.NullPointerException when the resources are being searched for REST methods that were not invoked.

ex: OPTIONS method run first when GET is called. And when resources are searched for OPTIONS, the following error happens.

[2023-06-06 16:59:03,543] ERROR - ServerWorker Error processing OPTIONS request for : /pizzashack/1.0.0/order/12345. 
java.lang.NullPointerException: null
	at org.apache.synapse.commons.templates.uri.parser.Literal.match(Literal.java:46) ~[synapse-commons_4.0.0.wso2v20.jar:?]
	at org.apache.synapse.commons.templates.uri.parser.Node.matchAll(Node.java:48) ~[synapse-commons_4.0.0.wso2v20.jar:?]
	at org.apache.synapse.commons.templates.uri.URITemplate.matches(URITemplate.java:49) ~[synapse-commons_4.0.0.wso2v20.jar:?]
	at org.apache.synapse.api.dispatch.URITemplateBasedDispatcher.findResource(URITemplateBasedDispatcher.java:39) ~[synapse-core_4.0.0.wso2v20.jar:4.0.0-wso2v20]
	at org.wso2.carbon.apimgt.gateway.handlers.LogUtils.fetchLogLevel_aroundBody40(LogUtils.java:184) ~[org.wso2.carbon.apimgt.gateway_9.28.126.SNAPSHOT.jar:?]
	at org.wso2.carbon.apimgt.gateway.handlers.LogUtils.fetchLogLevel(LogUtils.java:1) ~[org.wso2.carbon.apimgt.gateway_9.28.126.SNAPSHOT.jar:?]
	at org.wso2.carbon.apimgt.gateway.handlers.LogsHandler.getAPILogLevel_aroundBody24(LogsHandler.java:307) ~[org.wso2.carbon.apimgt.gateway_9.28.126.SNAPSHOT.jar:?]
	at org.wso2.carbon.apimgt.gateway.handlers.LogsHandler.getAPILogLevel(LogsHandler.java:1) ~[org.wso2.carbon.apimgt.gateway_9.28.126.SNAPSHOT.jar:?]
	at org.wso2.carbon.apimgt.gateway.handlers.LogsHandler.handleRequestInFlow_aroundBody2(LogsHandler.java:88) ~[org.wso2.carbon.apimgt.gateway_9.28.126.SNAPSHOT.jar:?]
	at org.wso2.carbon.apimgt.gateway.handlers.LogsHandler.handleRequestInFlow(LogsHandler.java:1) ~[org.wso2.carbon.apimgt.gateway_9.28.126.SNAPSHOT.jar:?]
	at org.apache.synapse.core.axis2.Axis2SynapseEnvironment.invokeHandlers(Axis2SynapseEnvironment.java:1178) ~[synapse-core_4.0.0.wso2v20.jar:4.0.0-wso2v20]
	at org.apache.synapse.core.axis2.Axis2SynapseEnvironment.injectMessage(Axis2SynapseEnvironment.java:284) ~[synapse-core_4.0.0.wso2v20.jar:4.0.0-wso2v20]
	at org.apache.synapse.core.axis2.SynapseMessageReceiver.receive(SynapseMessageReceiver.java:101) ~[synapse-core_4.0.0.wso2v20.jar:4.0.0-wso2v20]
	at org.apache.axis2.engine.AxisEngine.receive(AxisEngine.java:180) ~[axis2_1.6.1.wso2v83.jar:?]
	at org.apache.synapse.transport.passthru.ServerWorker.processNonEntityEnclosingRESTHandler(ServerWorker.java:379) ~[synapse-nhttp-transport_4.0.0.wso2v20.jar:?]
	at org.apache.synapse.transport.passthru.ServerWorker.run(ServerWorker.java:193) ~[synapse-nhttp-transport_4.0.0.wso2v20.jar:?]
	at org.apache.axis2.transport.base.threads.NativeWorkerPool$1.run(NativeWorkerPool.java:172) ~[axis2_1.6.1.wso2v83.jar:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) ~[?:?]
	at java.lang.Thread.run(Thread.java:829) [?:?]

@chamikasudusinghe
Copy link

Update

Current implementation to identify the resource

protected static void fetchLogLevel(MessageContext messageContext,
                                            Map<Map<String, String>, String> logProperties) {
    Collection<API> apiSet = messageContext.getEnvironment().getSynapseConfiguration().getAPIs();
    List<API> duplicateApiSet = new ArrayList<API>(apiSet);
    API selectedApi = null;
    String apiContext = ((Axis2MessageContext)messageContext).getAxis2MessageContext().
            getProperty("TransportInURL").toString();
    for (API api : duplicateApiSet) {
        if (apiContext.contains(api.getContext())) {
            selectedApi = api;
            //break;
        }
    }
    String httpMethod = (String) ((Axis2MessageContext) messageContext).getAxis2MessageContext().
            getProperty(Constants.Configuration.HTTP_METHOD);
    String apiHttpMethod = LogUtils.getHTTPMethod(messageContext);
    org.apache.axis2.context.MessageContext axis2MC = ((Axis2MessageContext) messageContext)
            .getAxis2MessageContext();
    Map headers = (Map) axis2MC.getProperty(org.apache.axis2.context.MessageContext.TRANSPORT_HEADERS);
    String corsRequestMethod = (String) headers.get(APIConstants.CORSHeaders.ACCESS_CONTROL_REQUEST_METHOD);
    if (selectedApi != null) {
        String path = ApiUtils.getFullRequestPath(messageContext);
        String subPath;
        VersionStrategy versionStrategy = new DefaultStrategy(selectedApi);
        if (versionStrategy.getVersionType().equals(VersionStrategyFactory.TYPE_URL)) {
            //for URL based
            //request --> http://{host:port}/context/version/path/to/resource
            subPath = path.substring(selectedApi.getContext().length() + versionStrategy.getVersion().length() + 1);
        } else {
            subPath = path.substring(selectedApi.getContext().length());
        }
        if ("".equals(subPath)) {
            subPath = "/";
        }
        messageContext.setProperty(RESTConstants.REST_SUB_REQUEST_PATH, subPath);
        Resource[] allAPIResources = selectedApi.getResources();
        Set<Resource> acceptableResources = new LinkedHashSet<>();
        for (Resource resource : allAPIResources) {
            //If the requesting method is OPTIONS or if the Resource contains the requesting method
            if ((RESTConstants.METHOD_OPTIONS.equals(httpMethod) && resource.getMethods() != null &&
                    Arrays.asList(resource.getMethods()).contains(corsRequestMethod)) ||
                    (resource.getMethods() != null && Arrays.asList(resource.getMethods()).contains(httpMethod))) {
                acceptableResources.add(resource);
            }
        }

        if (!acceptableResources.isEmpty()) {
            for (RESTDispatcher dispatcher : ApiUtils.getDispatchers()) {
                if (dispatcher instanceof URITemplateBasedDispatcher){
                    Resource selectedResource = dispatcher.findResource(messageContext, acceptableResources);
                    DispatcherHelper helper = selectedResource.getDispatcherHelper();
                    if (helper instanceof URITemplateHelper) {
                        URITemplateHelper templateHelper = (URITemplateHelper) helper;
                        for (Map.Entry<Map<String, String>, String> entry : logProperties.entrySet()) {
                            Map<String, String> key = entry.getKey();
                            if (apiHttpMethod.equals(key.get("resourceMethod"))){
                                boolean flag = templateHelper.getString().equals(key.get("resourcePath"));
                                System.out.println(flag);
                                String resourceLogLevel = entry.getValue();
                                String resourcePath = key.get("resourcePath");
                                String resourceMethod = key.get("resourceMethod");
                            }
                        }
                    }
                }
            }
        }
    }

}

chamikasudusinghe added a commit to chamikasudusinghe/carbon-apimgt that referenced this issue Jun 12, 2023
used the implementation of URITemplateBasedDispatcher for resource matching

issue: wso2/api-manager#1564
@chamikasudusinghe
Copy link

Update

The following fixes were done

  • Provided a fix to remove the regex matching and use the URITemplateHelper to find the resource path.
  • Remove redundant methods used in LogUtils
  • Provided an approach to find the invoked API from the collection of APIs using the API context and the message context
  • Got rid of the invocation of findresouce method in CORSRequestHandler if the resource has been already identified in logshandler
  • Formatted the code and added comments

@chamikasudusinghe
Copy link

Update

  • Added the following Util methods in Utils.java (carbon-apimgt)

  • getAPIByContext -> selecting the invoked API from the collection of APIs using the message context and synapse configuration.

  • getAcceptableResources -> this method is specific to how the acceptable resources were handled in CORSRequestHandler. this will match the request methods and resource methods to select the set of resources for the specific API.

  • getSelectedResource -> this method is specific to the CORSRequestHandler when a resource is already identified as a selected resource. this method checks whether the selected resource can be considered as part of the list of resources in the invoked API and the executing rest method.

  • Added the following Util methods in APIUtils.java & API. java (synapse)

  • getAcceptableResources -> synapse implementation to get the acceptable resources from the resource map based on the request and resource methods.

  • getResourcesMap -> getting a map of the resources of the invoked API to selected the acceptable resource when getAcceptableResources from synapse is used.

  • identifySelectedAPI -> this method is extracted from identifyAPI method without processing the API to find the invoked API at the global handler.

  • getSelectedAPI -> synapse implementation of finding the executing API (this can identify defaultStrategyApiSet too)

  • Developed and tested three different ways to find the invoked API at the global handler

  • using getAPIByContext util in Utils.java with apiContext.contains(api.getContext())

  • using getAPIByContext util in Utils.java with ApiUtils.matchApiPath(requestPath, api.getContext())

  • using getSelectedAPI & identifySelectedAPI in APIUtils.java from synapse side

@chamikasudusinghe
Copy link

Note:

API Resource Log Filtering happens at the global handler through the getAPILogLevel() method in the LogsHandler class. When this method is called, we are yet to process both the API and the Resouces, which is done by the process() method in the API class in the synapse. Hence, many parameters in the message context are not set/configured at this moment.

In order to do log filtering for resources, first we need to identify the resource based on the information available in the message context. In order to get that specific information needed to identify the resource, we need to find the specific API that has been invoked from the collection of the APIs. To do that, we can use the identifyAPI() method from AbstractApiHandler class in wso2-synapse.

Once both the API and the resource has been identified, it will be matched with the map of resources in logProperties to log the resource based on both resource name and resource method. This will be done for handleRequestInFlow, handleRequestOutFlow, handleResponseInFlow and handleResponseOutFlow.

To find the resource, we use findResouce methods from wso2-synapse. When running this method, it was modified to set the selected object to the message context to reduce repetitive executions of the methods in instances such as CORSRequestHandler, etc. The resource path is matched using the dispatcher helper logic. -> helper.getString().equals(key.get("resourcePath")

To find the API, we can use multiple options. One option is the match the API context path by matching the URL that can be obtained from axis2MsgContext.getProperty("TransportInURL") and the inbuild getContext() method of the API. Another approach is to match the API context using the inbuild getConext() method of the API and the matchApiPath() method from ApiUtils in synaspe.

Yet, we have a specific method in synapse to find the API, called identifyAPI() method. The current approach was developed by getting the necessary parts from that method and dispatchToAPI () method to come up with new methods called getSelectedAPI() and identifySelectedAPI (). However, all these happen before the API is being processed at the global handler.

Hence, when the API is processed the same code segment is called in the identifyAPI() method. Yet, since process() method is written inside this method, we cannot call this method in the global handler. Hence we have to study dispatchToAPI(), identifyAPI(), canProcess() and process() methods and refactor the methods in a way where we can isolate identifying an API and processing an API.

@shnrndk
Copy link

shnrndk commented Jul 6, 2023

Update 05/07/2023

I have started to work on this feature on 29/07/2023. Had a call with Chamika to get an idea on how to refactor the identifyAPI by isolating the logic on identifying API.
I have refactored the identifyAPI method by introducing getIdentifiedApi() method. Since getIdentifiedApi() required two parameters, which is api Object, and the synctx, To call in the canProcess method, I had to pass the api Object to canProcess as an additional parameter. By doing so, I had to create another canProcess(MessageContext synCtx, API api) method.
Had a call with arshadh, and he suggested to use only the original canProcess method and remove the newly introduced canProcess method.
There was another problem occured, after the refactoring, the
requestPath = fullRequestPath.substring(apiContext.length(), fullRequestPath.length()); line in APIKeyValidator Class occures null pointer exception. This is because after introducing the RESTConstants.REST_FULL_REQUEST_PATH property in the getIdentified() method. The property gets deleted in the DefaultAPIHandler class by the line
messageContext.getPropertyKeySet().remove(RESTConstants.REST_FULL_REQUEST_PATH);
This issue was only occured when the logging enabled flow. And in the normal flow without enabling the logging, there was a method I have added in the

       if (synCtx.getProperty(RESTConstants.IDENTIFIED_API) == null) {
            ApiUtils.getIdentifiedApi(this.getContext(),this.getVersionStrategy(), synCtx);
       }

Setting up the RESTConstants.REST_FULL_REQUEST_PATH property is happening inside the ApiUtils.getFullRequestPath(synCtx) which is located inside the getIdentifiedApi method.;.
Since in the logging enabled flow, synCtx.getProperty(RESTConstants.IDENTIFIED_API) is not null since it identifies the api at the first, therefore ApiUtils.getFullRequestPath(synCtx) is not called again. Therefore arshadh suggested to introduce an else part to the previous if statement.

        if (synCtx.getProperty(RESTConstants.IDENTIFIED_API) == null) {
            ApiUtils.getIdentifiedApi(this.getContext(),this.getVersionStrategy(), synCtx);
        } else {
            ApiUtils.getFullRequestPath(synCtx);
        }

As per the suggestions I have modified the if condition and the I have reduced the canProcess to the original method only.

shnrndk pushed a commit to shnrndk/wso2-synapse that referenced this issue Jul 17, 2023
shnrndk pushed a commit to shnrndk/carbon-apimgt that referenced this issue Jul 17, 2023
used the implementation of URITemplateBasedDispatcher for resource matching

issue: wso2/api-manager#1564
(cherry picked from commit 5b3818c)
@shnrndk
Copy link

shnrndk commented Jul 18, 2023

Update 18/07/2023

  • Done the refactoring part of the synapse.
  • In 10/07/2023 Had a code review with isuru udana, arshadh and chamika to review the refactoring part of the api identification in synapse.
  • At this code review, Isuru mentioned to refactor the identifyApi method by adding the identify_api flag checking to method.
  • Last week I had completed this task.
  • There are two unit test failures because of this fix in the CORSRequestHandlerTestCase.testHandleRequest() method. This was happened due to getAcceptableResources(Resource[] allAPIResources, String httpMethod, String corsRequestMethod) hasn't mocked yet.
  • For that I have mocked this method.
  • Now the task is almost finished and PRs have been made.
  • There is a test failure in the integration tests - APILoggingTest.testAPIPerAPILoggingTestcase
  • Currenlty I am looking to change the APILoggingTest.testAPIPerAPILoggingTestcase according to the implemented feature.

@shnrndk
Copy link

shnrndk commented Jul 25, 2023

Update 24/07/2023

  • I have identified there was api invoking failure when logs enabled, in the integration tests. These the bugs I have identified which was occured this issue.
  • Fixed a bug in the synapse side. After the new implementation once the api is identified, in the dipsatchToApi() method do not try to identify the api again. This is done by setting up new system property called IDENTIFIED_API. By checking this property is not null, we can identify the api is identified or not. But in the normal behaviour after processing the api, there is another checking for the apis from the defaultStrategyApiSet. For that checking, previously added system property affecting and it will cause to disrupt the normal behaviour, therefore after normal identification, I have added this condition to remove the property for further processing.
        if(synCtx.getProperty(RESTConstants.IDENTIFIED_API) != null) {
            synCtx.getPropertyKeySet().remove(RESTConstants.IDENTIFIED_API);
        } 
  • Also there was another bug in the carbon side. When there are multiple APIs available, if the user enables api logging for only a particular api, it does enables the logging for every other variable. This was fixed by adding the below if condition to compare the contexts.
       if(selectedApi.getContext().equals(key.get("context"))) {}
  • After fixing those bugs, the integration tests related to api logging are passing without issues. Currently I am writing a new integration test related to resource level api logging.

@shnrndk
Copy link

shnrndk commented Jul 31, 2023

Update 31/07/2023

@YasasRangika YasasRangika added this to the 4.3.0-M1 milestone Jan 16, 2024
@npamudika npamudika added the 4.3.0-M1 4.3.0 M1 Milestone label Jan 17, 2024
@npamudika npamudika modified the milestones: 4.3.0-M1, 4.3.0-Alpha Jan 19, 2024
@npamudika npamudika added 4.3.0-alpha and removed 4.3.0-M1 4.3.0 M1 Milestone labels Jan 19, 2024
tharikaGitHub pushed a commit to tharikaGitHub/wso2-synapse that referenced this issue Feb 21, 2024
@tharikaGitHub
Copy link
Member Author

Closing as the task is completed.

ljuillerat pushed a commit to AvintisSolutions/wso2-synapse that referenced this issue Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment