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

TT-4354 Go plugins to support multiple Tyk versions #3873

Merged

Conversation

sredxny
Copy link
Contributor

@sredxny sredxny commented Feb 7, 2022

Description

In order to support plugins with multiple tyk versions then was added a logic on how to load them using information such as: tyk version, operative system and architecture in which its running. The load Go Plugin logic will find first if there's an exact match with the provided path, if that file doesn't exist then it will build a path+pluginName like this:

1- Gather information like: os, tyk version and architecture
2- Obtain the clean name of the plugin
3- Obtain the directory where the plugins are stored
4- Build a new plugin name in the format: {plugin-dir}/{plugin-name}_{GW-version}_{OS}_{arch}.so
5- Load the plugin using the new name

Related Issue

https://tyktech.atlassian.net/browse/TT-4354

Motivation and Context

Allow users to upload multiple version of a go plugin, and load it based on where its running

How This Has Been Tested

 "custom_middleware": {
      "pre": [],
      "post": [
        {
          "name": "AddFooBarHeader",
          "path": "./mygoplugin/mygoplugin.so",
          "require_session": false,
          "raw_body_only": false
        }
      ],
      "post_key_auth": [],
      "auth_check": {
        "name": "",
        "path": "",
        "require_session": false,
        "raw_body_only": false
      },
      "response": [],
      "driver": "goplugin",
      "id_extractor": {
        "extract_from": "",
        "extract_with": "",
        "extractor_config": {}
      }
    },
  • Name the plugin: mygoplugin_v4.1.0_darwin_amd64.so
  • Curl the api: curl -i http://tyk-gateway:8081/goplugin/get
  • In the result I can see the header Foo: Bar

Testing the Plugin compiler
Now the plugin compiler have the ability to generate the plugins with the name format set as explained above, in order to test it we need to create the plugin and check that the name is set as expected. The plugin compiler have the ability to receive by params the OS and Arch as 3rd and 4th param. To test, lets do:

  • Create the plugin directory
  • go mod init tyk_plugin
  • go mod edit -replace github.com/jensneuse/graphql-go-tools=github.com/TykTechnologies/graphql-go-tools@f818861b88dc
  • go get github.com/TykTechnologies/tyk@af8883a
  • Write the plugin following the instructions of https://tyk.io/docs/3-lts/plugins/supported-languages/golang/#write-the-plugin
  • Then, to build the plugin lets use the docker image: tykio/tyk-plugin-compiler:v4.1.0-rc3 and at this point we need to test it in 2 ways: sending the arch and os, and not sending thos params:

1- Sending OS/Arch (Skip this section as cross compilation is not working):

  • Type docker run --rm -v pwd:/plugin-source tykio/tyk-plugin-compiler:v4.1.0-rc5 plugin.so pluginid testOS testArch and as output it will generate a file named: plugin_v4.1.0_testOS_testArch.so
    The bad side of this approach is that is always required to send a pluginID.

If we attempt to send an invalid OS or arch, we will receive a message like this:
Captura de Pantalla 2022-02-13 a la(s) 8 50 44 p  m

2- Without sending the OS/Arch:

  • Type docker run --rm -v pwd:/plugin-source tykio/tyk-plugin-compiler:v4.1.0-rc5 plugin.so as the docker container is running linux it will detect that OS and architecture and will create a file with a name like: plugin_v4.1.0_linux_amd64.so

Captura de Pantalla 2022-02-13 a la(s) 8 50 17 p  m

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). If pulling from your own
    fork, don't request your master!
  • Make sure you are making a pull request against the master branch (left side). Also, you should start
    your branch off our latest master.
  • My change requires a change to the documentation.
    • If you've changed APIs, describe what needs to be updated in the documentation.
    • If new config option added, ensure that it can be set via ENV variable
  • I have updated the documentation accordingly.
  • Modules and vendor dependencies have been updated; run go mod tidy && go mod vendor
  • When updating library version must provide reason/explanation for this update.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Check your code additions will not fail linting checks:
    • go fmt -s
    • go vet

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 7, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit:
Triggered by: pull_request (@sredxny)
Execution page

@sredxny sredxny changed the title started to build new plugin path based on gwVersion+os+architecture TT-4354 Go plugins to support multiple Tyk versions Feb 7, 2022
@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 7, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: 589b263
Triggered by: pull_request (@sredxny)
Execution page

@sredxny sredxny marked this pull request as ready for review February 8, 2022 01:38
Copy link
Contributor

@tbuchaillot tbuchaillot left a comment

Choose a reason for hiding this comment

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

@sredxny requested some small validation but it looks solid 👌.
Have you tested this with bundles?

gateway/mw_go_plugin.go Show resolved Hide resolved
@tbuchaillot tbuchaillot self-requested a review February 8, 2022 21:43
@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 8, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: 72950e6
Triggered by: pull_request (@tbuchaillot)
Execution page

Copy link
Contributor

@matiasinsaurralde matiasinsaurralde left a comment

Choose a reason for hiding this comment

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

Looks good so far

@sredxny sredxny requested a review from buger February 9, 2022 17:11
@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 9, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: 1463b9f
Triggered by: pull_request (@tbuchaillot)
Execution page

@buger
Copy link
Member

buger commented Feb 9, 2022

Do I have to always put them in the same order? Can I skip some of it?
For example will {plugin-name}{OS}{arch}.so also can work?
Or maybe the other way always force the same structure?

Also it would be great if we modify our plugin compiler to always generate plugins with the given prefixes.
E.g. you run it without single argument for plugin name, and as output you get: {plugin-name}_{GW-version}_{OS}_{arch}.so?

And OS/arch can be read from GOOS and GOARCH arguments, if they passed to the image, and read from the current linux env if not specified.

…of github.com:TykTechnologies/tyk into TT-4354-feature/plugins-support-multiple-tyk-versions
@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 10, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: b75f212
Triggered by: pull_request (@sredxny)
Execution page

@@ -1,6 +1,7 @@
#!/bin/bash
set -xe

CURRENTVERS=$(perl -n -e'/v(\d+).(\d+).(\d+)/'' && print "v$1\.$2\.$3"' ./../../../gateway/version.go)
Copy link
Contributor

Choose a reason for hiding this comment

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

The entry point for plugin compiler is this build.sh at path /build.sh. So ./../../../gateway/version.go might not resolve correctly. gw code is copied to TYK_GW_PATH here. ./../../../gateway/version.go can be changed to $TYK_GW_PATH/gateway/version.go

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 11, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: a6bcf76
Triggered by: pull_request (@sredxny)
Execution page

@github-actions
Copy link

💥 CI tests failed 🙈

CI test log

all ok

gofmt

all ok

goimports

all ok

If the above are ok, please look at the run or in the Checks tab.

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 11, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: 5ae87c8
Triggered by: pull_request (@sredxny)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 11, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: 0fd50e6
Triggered by: pull_request (@sredxny)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 13, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: 0168b00
Triggered by: pull_request (@sredxny)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 13, 2022

API tests result: success
Branch used: refs/pull/3873/merge
Commit: da7ecd4
Triggered by: pull_request (@sredxny)
Execution page

@matiasinsaurralde
Copy link
Contributor

Now am having issues compiling the plugin to any other platform different than linux/amd64 (tested with freebsd, linux and darwin). So, it seems like we need to install a cross compiler in the container? if so, how would we do it?

I think that cross compilation is complex in this case (it's easier for Go builds that don't rely on cgo).

@sredxny
Copy link
Contributor Author

sredxny commented Feb 15, 2022

@matiasinsaurralde @buger @tbuchaillot I created this ticket https://tyktech.atlassian.net/browse/TT-4637 to support cross compilation in the plugin compiler and limit the scope of the current ticket to support different versions of Tyk. Can you please review again? Now the plugin compiler generate files following the naming convention stablished in this PR

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 15, 2022

API tests result: skipped 🚫
Branch used: refs/pull/3873/merge
Commit: 910bf43
Triggered by: pull_request (@tbuchaillot)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 16, 2022

API tests result: skipped 🚫
Branch used: refs/pull/3873/merge
Commit: 0df1c0b
Triggered by: pull_request (@tbuchaillot)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 16, 2022

API tests result: skipped 🚫
Branch used: refs/pull/3873/merge
Commit: 0df1c0b
Triggered by: pull_request (@sredxny)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 16, 2022

API tests result: skipped 🚫
Branch used: refs/pull/3873/merge
Commit: 0df1c0b
Triggered by: pull_request (@tbuchaillot)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 25, 2022

API tests result: skipped 🚫
Branch used: refs/pull/3873/merge
Commit: 08656af
Triggered by: pull_request (@sredxny)
Execution page

@sonarcloud
Copy link

sonarcloud bot commented Feb 25, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

62.5% 62.5% Coverage
0.0% 0.0% Duplication

@tbuchaillot tbuchaillot merged commit e60c6f2 into master Feb 25, 2022
@tbuchaillot tbuchaillot deleted the TT-4354-feature/plugins-support-multiple-tyk-versions branch February 25, 2022 12:12
@tbuchaillot
Copy link
Contributor

/release to release-4

@tykbot
Copy link

tykbot bot commented Feb 25, 2022

Working on it! Note that it can take a few minutes.

@tykbot
Copy link

tykbot bot commented Feb 25, 2022

@tbuchaillot Seems like there is conflict and it require manual merge.

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Feb 25, 2022

API tests result: skipped 🚫
Branch used: refs/heads/master
Commit: e60c6f2 TT-4354 Go plugins to support multiple Tyk versions (#3873)

  • started to build new plugin path based on gwVersion+os+architecture

  • added test coverage.Small refactor to load go plugin

  • if goarch and goos are present then build the plugin name using those values

  • receive params via terminal

  • rename the plugin name and not the path

  • fix naming of the plugin format

  • build the plugin considering os and arch params

  • enable cross compiling

  • set CGO_ENABLE=1

  • enable cgo only when its not linux

Co-authored-by: Tomas Buchaillot tombuchaillot89@gmail.com
Triggered by: push (@tbuchaillot)
Execution page

buraksezer added a commit that referenced this pull request Feb 28, 2022
…ce-tests' of github.com:TykTechnologies/tyk into fix/TT-4655/federation-crashes-gateway-during-performance-tests

* 'fix/TT-4655/federation-crashes-gateway-during-performance-tests' of github.com:TykTechnologies/tyk:
  TT-4354 Go plugins to support multiple Tyk versions (#3873)
  update sprig to v3 (#3897)
@sredxny
Copy link
Contributor Author

sredxny commented Feb 28, 2022

cherry picked into release-4

asutosh added a commit that referenced this pull request Mar 11, 2022
Revert the changes inherited from PR #3873.
asutosh added a commit that referenced this pull request Mar 11, 2022
Revert the changes inherited from PR #3873.
@sredxny
Copy link
Contributor Author

sredxny commented May 30, 2022

/release to release-4.1

@tykbot
Copy link

tykbot bot commented May 30, 2022

@sredxny Release branch not found

@@ -53,5 +73,5 @@ rm -rf $TYK_GW_PATH/vendor

rm /go/src/modules.txt

GO111MODULE=off go build -buildmode=plugin -o $plugin_name \
GO111MODULE=off CGO_ENABLE=$CGO_ENABLE GOOS=$GOOS GOARCH=$GOARCH go build -buildmode=plugin -o $plugin_name \
Copy link
Contributor

Choose a reason for hiding this comment

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

We have $CGOENABLED as a variable, the env to use is CGO_ENABLED but we define CGO_ENABLE 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point... Created #4160

however unsure if we should support this var any longer

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

Successfully merging this pull request may close these issues.

None yet

7 participants