Skip to content

Commit

Permalink
Merging to release-5.3: [TT-13155] Explicitly copy BaseMiddleware for…
Browse files Browse the repository at this point in the history
… each middleware that takes it (#6744) (#6764)

### **User description**
[TT-13155] Explicitly copy BaseMiddleware for each middleware that takes
it (#6744)

<details open>
<summary><a href="https://tyktech.atlassian.net/browse/TT-13155"
title="TT-13155" target="_blank">TT-13155</a></summary>
  <br />
  <table>
    <tr>
      <th>Summary</th>
<td>[Regression] Gateway Debug logs starting v5.3 only logs
AccessRightsCheck for most of the middlewares</td>
    </tr>
    <tr>
      <th>Type</th>
      <td>
<img alt="Bug"

src="https://tyktech.atlassian.net/rest/api/2/universal_avatar/view/type/issuetype/avatar/10303?size=medium"
/>
        Bug
      </td>
    </tr>
    <tr>
      <th>Status</th>
      <td>In Dev</td>
    </tr>
    <tr>
      <th>Points</th>
      <td>N/A</td>
    </tr>
    <tr>
      <th>Labels</th>
<td><a

href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20'24Bugsmash%20ORDER%20BY%20created%20DESC"
title="'24Bugsmash">'24Bugsmash</a>, <a

href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%202025lts%20ORDER%20BY%20created%20DESC"
title="2025lts">2025lts</a>, <a

href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20SESAP%20ORDER%20BY%20created%20DESC"
title="SESAP">SESAP</a>, <a

href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20customer_bug%20ORDER%20BY%20created%20DESC"
title="customer_bug">customer_bug</a>, <a

href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20jira_escalated%20ORDER%20BY%20created%20DESC"
title="jira_escalated">jira_escalated</a></td>
    </tr>
  </table>
</details>
<!--
  do not remove this marker as it will break jira-lint's functionality.
  added_by_jira_lint
-->

---

### **PR Type**
Enhancement

PR:

- implements per-middleware basemiddleware copy behaviour
- reverts the logger+mutex on base middleware
- touches coprocess/ to better handle grpc server startup, shutdown,
conflicts on static port number - not to swallow errors when
net.Listener fails

___

### **Description**
- Refactored the `BaseMiddleware` initialization process by introducing
a `NewBaseMiddleware` function, encapsulating the creation logic.
- Added a `Copy` method to `BaseMiddleware` to create scoped copies with
a duplicated logger, ensuring middleware-specific logging.
- Updated all middleware initialization in `gateway/api_loader.go` to
use `baseMid.Copy()` for better isolation and logging scope.
- Enhanced code readability and maintainability by centralizing
`BaseMiddleware` creation logic and ensuring proper separation of
concerns.

---------

Co-authored-by: Tit Petric <[email protected]>

[TT-13155]:
https://tyktech.atlassian.net/browse/TT-13155?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ


___

### **PR Type**
Bug fix, Enhancement


___

### **Description**
- Refactored `BaseMiddleware` initialization by introducing
`NewBaseMiddleware` and `Copy` methods for better middleware isolation
and logging scope.
- Updated middleware initialization in `gateway/api_loader.go` to use
`baseMid.Copy()` for improved isolation and maintainability.
- Enhanced gRPC test setup by introducing `startTestServices` and
`stopTestServices` helpers, improving test isolation and reliability.
- Added a dedicated test file for dispatcher logic, improving test
coverage and organization.
- Skipped slow and unreliable tests in `dnscache/storage_test.go` and
`gateway_test.go`.
- Updated test tasks in `.taskfiles/test.yml` and
`coprocess/Taskfile.yml` to improve test reporting and execution.



___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Tests</strong></td><td><details><summary>5
files</summary><table>
<tr>
  <td>
    <details>
<summary><strong>coprocess_grpc_test.go</strong><dd><code>Refactor gRPC
test setup and improve test isolation</code>&nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; </dd></summary>
<hr>

coprocess/grpc/coprocess_grpc_test.go

<li>Refactored test setup to use <code>startTestServices</code> for
better test <br>isolation.<br> <li> Removed redundant dispatcher and
server setup code.<br> <li> Updated test cases to use new helper
functions for cleaner code.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-c5b0bcf682a084942dee611f5b6c7ff202dd49424b90d39658c4634f0422064a">+33/-198</a></td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>dispatcher_test.go</strong><dd><code>Add dedicated
dispatcher test file and improve coverage</code>&nbsp; &nbsp;
</dd></summary>
<hr>

coprocess/grpc/dispatcher_test.go

<li>Added a new test file for dispatcher logic.<br> <li> Moved
dispatcher-related test cases to a dedicated file.<br> <li> Improved
test coverage for dispatcher functionality.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-6bd52ba54179dd053502987e2c001e88f0bab67165e7c99f23c58e633f868c0d">+127/-0</a>&nbsp;
</td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>services_test.go</strong><dd><code>Add helpers for gRPC
service test setup and teardown</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
</dd></summary>
<hr>

coprocess/grpc/services_test.go

<li>Introduced <code>startTestServices</code> and
<code>stopTestServices</code> helpers for test <br>setup and
teardown.<br> <li> Improved test reliability by dynamically assigning
gRPC server ports.<br> <li> Enhanced test maintainability by
centralizing service setup logic.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-b7682389757a6f578eec7f5c345d3ea315276ccf877f82fc91ef6ce6f49c8a48">+69/-0</a>&nbsp;
&nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>storage_test.go</strong><dd><code>Skip slow test with
bad practices in storage tests</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; </dd></summary>
<hr>

dnscache/storage_test.go

<li>Skipped a slow test with bad practices using
<code>time.Sleep</code>.<br> <li> Marked the test for future improvement
or removal.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-4c91a89e72c2a9df7fcc58a4dbdfabe80794cfb227dc60f8a2d95ff439e0e78a">+2/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>testutil.go</strong><dd><code>Improve test utility
reliability and error handling</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; </dd></summary>
<hr>

gateway/testutil.go

<li>Added error handling for temporary directory creation in test
<br>utilities.<br> <li> Improved test reliability by ensuring proper
cleanup of temporary <br>resources.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-7aaf6ae49fb8f58a8c99d337fedd15b3e430dd928ed547e425ef429b10d28ce8">+5/-1</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

</table></details></td></tr><tr><td><strong>Enhancement</strong></td><td><details><summary>4
files</summary><table>
<tr>
  <td>
    <details>
<summary><strong>api_loader.go</strong><dd><code>Refactor middleware
initialization for better isolation</code>&nbsp; &nbsp; </dd></summary>
<hr>

gateway/api_loader.go

<li>Refactored <code>BaseMiddleware</code> initialization to use
<code>NewBaseMiddleware</code>.<br> <li> Updated middleware
initialization to use <code>baseMid.Copy()</code> for better
<br>isolation.<br> <li> Improved middleware chain creation by ensuring
each middleware gets a <br>unique <code>BaseMiddleware</code>
instance.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-cdf0b7f176c9d18e1a314b78ddefc2cb3a94b3de66f1f360174692c915734c68">+58/-66</a>&nbsp;
</td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>coprocess.go</strong><dd><code>Enhance CoProcess
middleware creation with better isolation</code></dd></summary>
<hr>

gateway/coprocess.go

<li>Updated <code>CreateCoProcessMiddleware</code> to use
<code>BaseMiddleware.Copy()</code> for <br>better isolation.<br> <li>
Improved error handling and logging in middleware creation.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-9cbfe628982b2afb94d1e9a5200fc9a4fdc00cb58fe65d1090a3725e4e4c5953">+3/-3</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>middleware.go</strong><dd><code>Add `NewBaseMiddleware`
and `Copy` for better middleware management</code></dd></summary>
<hr>

gateway/middleware.go

<li>Introduced <code>NewBaseMiddleware</code> to encapsulate
<code>BaseMiddleware</code> creation <br>logic.<br> <li> Added
<code>Copy</code> method to <code>BaseMiddleware</code> for creating
scoped copies with <br>unique loggers.<br> <li> Improved thread safety
for logger updates in <code>BaseMiddleware</code>.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1b">+81/-22</a>&nbsp;
</td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>mw_go_plugin.go</strong><dd><code>Enhance Go plugin
middleware with isolated BaseMiddleware instances</code></dd></summary>
<hr>

gateway/mw_go_plugin.go

<li>Updated Go plugin middleware to use
<code>BaseMiddleware.Copy()</code> for better <br>isolation.<br> <li>
Improved success handler initialization for plugin middleware.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-0f31abef73bf795e1a8d331202330c73011a74d10fd66e49b9359716a6d18fd9">+2/-2</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></details></td></tr><tr><td><strong>Configuration
changes</strong></td><td><details><summary>2 files</summary><table>
<tr>
  <td>
    <details>
<summary><strong>test.yml</strong><dd><code>Improve test task to
generate and consolidate JSON coverage reports</code></dd></summary>
<hr>

.taskfiles/test.yml

<li>Updated test task to generate JSON coverage files for each
package.<br> <li> Enhanced test reporting by consolidating JSON files
into a single <br>report.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-f1fbe7f7f14888019b8845634ed008e1c43f6e5a5c0b2707336fc7f8e15a36fb">+5/-4</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>Taskfile.yml</strong><dd><code>Enhance test task with
`gotestsum` and sequential execution</code></dd></summary>
<hr>

coprocess/Taskfile.yml

<li>Updated test task to use <code>gotestsum</code> for better test
output formatting.<br> <li> Improved test execution by running tests
sequentially with <code>-p 1</code>.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6764/files#diff-3570989f97390bd8441ef414dffdefb7dd0b29bfa6f01e413c68cc51a3f68485">+4/-1</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></details></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
request to receive relevant information

---------

Co-authored-by: Tit Petric <[email protected]>
Co-authored-by: Tit Petric <[email protected]>
  • Loading branch information
3 people authored Dec 12, 2024
1 parent f5450bf commit d35b673
Show file tree
Hide file tree
Showing 14 changed files with 393 additions and 306 deletions.
9 changes: 5 additions & 4 deletions .taskfiles/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ tasks:
- rm -rf coverage && mkdir -p coverage
- for: { var: packages, as: package }
cmd: |-
gotestsum --no-color=false --hide-summary=skipped --raw-command \
go test -p 1 -parallel 1 -json {{.testArgs}} {{.args}} -count=1 -v \
gotestsum --no-color=false --hide-summary=skipped \
--jsonfile coverage/{{.product}}-{{.package | replace "." "gateway" | replace "/" "-"}}.json \
--raw-command go test -p 1 -parallel 1 -json {{.testArgs}} {{.args}} -count=1 -v \
-coverprofile=coverage/{{.package | replace "." "gateway" | replace "/" "-"}}.cov {{.package}} | head -n -2
integration-combined:
Expand Down Expand Up @@ -86,8 +87,8 @@ tasks:
vars:
count: '{{ .count | default "10" }}'
cmds:
- gotestsum --hide-summary=skipped --junitfile=unit-tests.xml --raw-command cat coverage/{{.product}}-all.json
- echo "Slowest {{.count}} tests:" && cat coverage/{{.product}}-all.json | gotestsum tool slowest | head -n {{.count}} | sed -e 's|{{.package}}/||g'
- gotestsum --hide-summary=skipped --junitfile=unit-tests.xml --raw-command cat coverage/*.json
- echo "Slowest {{.count}} tests:" && cat coverage/*.json | gotestsum tool slowest | head -n {{.count}} | sed -e 's|{{.package}}/||g'

clean:
desc: "Clean test outputs"
Expand Down
5 changes: 4 additions & 1 deletion coprocess/Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@ tasks:
test:
desc: "Run tests"
deps: [ services:up ]
env:
GOTESTSUM_FORMAT: testname
cmds:
- defer: { task: services:down }
- go fmt ./...
- go test -count=1 ./...
- go test -p 1 -parallel 1 -count=1 -json ./... | gotestsum --format testname --hide-summary=all


# lint target is run from CI
lint:
Expand Down
Loading

0 comments on commit d35b673

Please sign in to comment.