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

feat: add ApisixPluginConfig translator and controller loop #791

Closed
wants to merge 20 commits into from

Conversation

neverCase
Copy link
Contributor

Please answer these questions before submitting a pull request

  • Why submit this pull request?

  • Bugfix

  • New feature provided

  • Improve performance

  • Backport patches

  • Related issues


Bugfix

  • Description

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.

Backport patches

@neverCase
Copy link
Contributor Author

@tao12345666333
I update it temporarily.
Please review it

@tao12345666333
Copy link
Member

Did you merge code from master?

@neverCase
Copy link
Contributor Author

Did you merge code from master?

Yes.
But the changes about the v2beta3 make me confused, cause my previous pr about the ApisixPluginConfig was in
the v2beta2 directory

@tao12345666333
Copy link
Member

But the changes about the v2beta3 make me confused, cause my previous pr about the ApisixPluginConfig was in
the v2beta2 directory

Yes, apiversion has been upgraded to v2beta3 from #746

You can open a new PR to upgrade ApisixPluginConfig to v2beta3.

@neverCase neverCase closed this Dec 10, 2021
@neverCase neverCase reopened this Dec 10, 2021
@neverCase
Copy link
Contributor Author

But the changes about the v2beta3 make me confused, cause my previous pr about the ApisixPluginConfig was in
the v2beta2 directory

Yes, apiversion has been upgraded to v2beta3 from #746

You can open a new PR to upgrade ApisixPluginConfig to v2beta3.

ok, got it

@neverCase neverCase closed this Dec 10, 2021
@tao12345666333
Copy link
Member

I think the current PR can be retained.

You can create a new PR and only upgrade ApisixPluginConfig to v2beta3

You can update this PR after the merger.

Of course, if you want to completely recreate the PR, you can

@neverCase
Copy link
Contributor Author

I think the current PR can be retained.

You can create a new PR and only upgrade ApisixPluginConfig to v2beta3

You can update this PR after the merger.

Of course, if you want to completely recreate the PR, you can

OK, I reopen it

@tao12345666333
Copy link
Member

#792 has been merged.

@neverCase
Copy link
Contributor Author

#792 has been merged.

ok, I will merge it from the master first

@neverCase
Copy link
Contributor Author

neverCase commented Dec 14, 2021

@tao12345666333
I had push codes temporarily.
But I can't find the relationship between the TranslatorContext, ApisixRoute and ApisixPluginConfig correctly.
Also in the configv2beta3.ApisixPluginConfig crd the Plugins was []ApisixRouteHTTPPluginConfig, but in the internal apisixv1.PluginConfig the Plugins was not a list structure.
The problem above make me confused.

@neverCase
Copy link
Contributor Author

ping @tao12345666333

@tao12345666333
Copy link
Member

Sorry for delay. Let me take a look.

@tao12345666333
Copy link
Member

You can just implement Translator interface, add TranslateApisixPluginConfig function.
You can refer to the implementation of ApisixConsumer.

@tao12345666333
Copy link
Member

APISIX route should add a plugin_config_id field, refer to ApisixPluginConfig.

https://apisix.apache.org/docs/apisix/architecture-design/plugin-config/

@tao12345666333
Copy link
Member

You can do it in two steps

  1. First complete the conversion from ApisixPluginConfig to APISIX
  2. Binding ApisixPluginConfig on ApisixRoute

Feel free to ping me, if you have any questions.

@neverCase
Copy link
Contributor Author

You can do it in two steps

  1. First complete the conversion from ApisixPluginConfig to APISIX
  2. Binding ApisixPluginConfig on ApisixRoute

Feel free to ping me, if you have any questions.

ok, got it.

@neverCase

This comment has been minimized.

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2021

Codecov Report

Merging #791 (e5fa404) into master (970df2b) will decrease coverage by 0.28%.
The diff coverage is 25.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
- Coverage   32.57%   32.29%   -0.29%     
==========================================
  Files          68       71       +3     
  Lines        7273     7674     +401     
==========================================
+ Hits         2369     2478     +109     
- Misses       4634     4919     +285     
- Partials      270      277       +7     
Impacted Files Coverage Δ
pkg/apisix/apisix.go 61.36% <ø> (-6.14%) ⬇️
pkg/apisix/cluster.go 32.57% <0.00%> (-0.92%) ⬇️
pkg/ingress/apisix_pluginconfig.go 0.00% <0.00%> (ø)
pkg/ingress/apisix_route.go 0.00% <0.00%> (ø)
pkg/ingress/compare.go 0.00% <0.00%> (ø)
pkg/ingress/controller.go 0.95% <0.00%> (-0.04%) ⬇️
pkg/ingress/ingress.go 6.81% <0.00%> (-0.08%) ⬇️
pkg/ingress/status.go 0.00% <0.00%> (ø)
pkg/kube/translation/translator.go 46.07% <ø> (ø)
pkg/kube/translation/apisix_route.go 21.93% <38.46%> (+0.90%) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 970df2b...e5fa404. Read the comment docs.

@neverCase neverCase marked this pull request as ready for review December 18, 2021 12:07
@neverCase
Copy link
Contributor Author

neverCase commented Dec 23, 2021

Move L282-L286 to the front of L272.

Also, please check other locations in this file at the same time to ensure that the order of execution is as expected.

Thanks!

ok, got it.
By the way, I found that there were some other errors from the ci.

....
W1222 17:45:44.757323       1 reflector.go:436] pkg/mod/k8s.io/client-go@v0.21.1/tools/cache/reflector.go:167: watch of *v2beta3.ApisixUpstream ended with: an error on the server ("unable to decode an event from the watch stream: unable to decode watch event: no kind \"ApisixUpstream\" is registered for version \"apisix.apache.org/v2beta3\" in scheme \"pkg/runtime/scheme.go:100\"") has prevented the request from succeeding
.....
W1222 17:45:58.030483       1 reflector.go:436] pkg/mod/k8s.io/client-go@v0.21.1/tools/cache/reflector.go:167: watch of *v2beta3.ApisixUpstream ended with: an error on the server ("unable to decode an event from the watch stream: unable to decode watch event: no kind \"ApisixUpstream\" is registered for version \"apisix.apache.org/v2beta3\" in scheme \"pkg/runtime/scheme.go:100\"") has prevented the request from succeeding

I could fix it at the same time.

@tao12345666333
Copy link
Member

I could fix it at the same time.

Great!

It looks like this is a bug, can you submit a separate PR?
Or use a separate commit for the fix, so that I can cherry-pick it to a PR

@neverCase
Copy link
Contributor Author

neverCase commented Dec 23, 2021

I could fix it at the same time.

Great!

It looks like this is a bug, can you submit a separate PR? Or use a separate commit for the fix, so that I can cherry-pick it to a PR

ok

@tao12345666333 tao12345666333 added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Dec 23, 2021
@neverCase
Copy link
Contributor Author

@tao12345666333
There were still some errors in ci which I should fix it before.
This branch are not accepted.

@tao12345666333
Copy link
Member

Don't worry, triage/accepted means that this PR has been accepted and is under active development. It does not mean that this PR is awaiting merger.

@tao12345666333
Copy link
Member

The reason why the test case failed was because APISIX did not check whether it was referenced when deleting PluginConfig.

apache/apisix#5917

We can modify the current implementation logic. When the user configures and needs PluginConfig, create it again.

@tao12345666333 tao12345666333 added this to In progress in v1.4 Planning via automation Dec 24, 2021
Copy link
Contributor

@gxthrj gxthrj left a comment

Choose a reason for hiding this comment

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

LGTM ,but CI need to be re-run, it is failed in resourcepushing.go:426.

	Error Trace:	resourcepushing.go:426
  	            				runner.go:113
  	            				runner.go:64
  	            				it_node.go:26
  	            				spec.go:215
  	            				spec.go:138
  	            				spec_runner.go:200
  	            				spec_runner.go:170
  	            				spec_runner.go:66
  	            				suite.go:79
  	            				ginkgo_dsl.go:238
  	            				ginkgo_dsl.go:213
  	            				e2e_test.go:26
  	Error:      	Expected nil, but got: &errors.errorString{s:"timed out waiting for the condition"}
  	Test:       	ApisixRoute Testing route priority
  

  /home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/ingress/resourcepushing.go:426

v1.4 Planning automation moved this from In progress to Reviewer approved Dec 24, 2021
@neverCase
Copy link
Contributor Author

ci show there were errors in e2e case check the ingress lb status is updated
@tao12345666333

@tao12345666333
Copy link
Member

#807

I will re-run CI jobs

@tao12345666333
Copy link
Member

It's green! Thanks @neverCase

@tao12345666333
Copy link
Member

I will do the final round of review

@tao12345666333 tao12345666333 changed the title feat: add ApisixPluginConfig translator (#638) feat: add ApisixPluginConfig translator and controller loop Dec 26, 2021
@tao12345666333
Copy link
Member

@neverCase Thanks for you contribution!!!

The code in this PR is LGTM.

In this PR, you not only implement the logic of the translator, but also increase the logic of the controller loop, so I hope you can add a little more content to ensure that its behavior meets our expectations.

  • modify the ApisixRoute CRD (only change the v2beta3 version) and add a reference to plugin Config

- name: v2beta3
served: true
storage: true
subresources:
status: { }
additionalPrinterColumns:
- jsonPath: .spec.http[].match.hosts
name: Hosts
type: string
priority: 0
- jsonPath: .spec.http[].match.paths
name: URIs
type: string
priority: 0
- jsonPath: .spec.http[].backends[].serviceName
name: Target Service(HTTP)
type: string
priority: 1
- jsonPath: .spec.tcp[].match.ingressPort
name: Ingress Server Port(TCP)
type: integer
priority: 1
- jsonPath: .spec.tcp[].match.backend.serviceName
name: Target Service(TCP)
type: string
priority: 1
- jsonPath: .metadata.creationTimestamp
name: Age
type: date
priority: 0
schema:
openAPIV3Schema:
type: object
properties:
spec:
type: object
anyOf:
- required: [ "http" ]
- required: [ "stream" ]
properties:
http:
type: array
minItems: 1
items:
type: object
required: [ "name", "match", "backends" ]
properties:
name:
type: string
minLength: 1
priority:
type: integer
timeout:
type: object
properties:
connect:
type: string
send:
type: string
read:
type: string
match:
type: object
required:
- paths
properties:
paths:
type: array
minItems: 1
items:
type: string
pattern: "^/[a-zA-Z0-9\\-._~%!$&'()+,;=:@/]*\\*?$"
hosts:
type: array
minItems: 1
items:
type: string
pattern: "^\\*?[0-9a-zA-Z-._]+$"
methods:
type: array
minItems: 1
items:
type: string
enum: [ "CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", "TRACE" ]
remoteAddrs:
type: array
minItems: 1
items:
type: string
exprs:
type: array
minItems: 1
items:
type: object
properties:
subject:
type: object
properties:
scope:
type: string
enum: [ "Cookie", "Header", "Path", "Query" ]
name:
type: string
minLength: 1
required:
- scope
op:
type: string
enum:
- Equal
- NotEqual
- GreaterThan
- LessThan
- In
- NotIn
- RegexMatch
- RegexNotMatch
- RegexMatchCaseInsensitive
- RegexNotMatchCaseInsensitive
value:
type: string
set:
type: array
items:
type: string
oneOf:
- required: [ "subject", "op", "value" ]
- required: [ "subject", "op", "set" ]
websocket:
type: boolean
backends:
type: array
minItems: 1
items:
type: object
properties:
serviceName:
type: string
minLength: 1
servicePort:
type: integer
minimum: 1
maximum: 65535
resolveGranularity:
type: string
enum: [ "endpoint", "service" ]
weight:
type: integer
minimum: 0
subset:
type: string
required:
- serviceName
- servicePort
plugins:
type: array
items:
type: object
properties:
name:
type: string
minLength: 1
enable:
type: boolean
config:
type: object
x-kubernetes-preserve-unknown-fields: true # we have to enable it since plugin config
required:
- name
- enable
authentication:
type: object
properties:
enable:
type: boolean
type:
type: string
enum: [ "basicAuth", "keyAuth" ]
keyAuth:
type: object
properties:
header:
type: string
required:
- enable
stream:
type: array
minItems: 1
items:
type: object
required: [ "name", "match", "backend", "protocol" ]
properties:
"protocol":
type: string
enum: [ "TCP", "UDP" ]
name:
type: string
minLength: 1
match:
type: object
properties:
ingressPort:
type: integer
minimum: 1
maximum: 65535
required:
- ingressPort
backend:
type: object
properties:
serviceName:
type: string
minLength: 1
servicePort:
type: integer
minimum: 1
maximum: 65535
resolveGranularity:
type: string
enum: [ "endpoint", "service" ]
subset:
type: string
required:
- serviceName
- servicePort
status:
type: object
properties:
conditions:
type: array
items:
type: object
properties:
"type":
type: string
reason:
type: string
status:
type: string
message:
type: string
observedGeneration:
type: integer

@neverCase
Copy link
Contributor Author

OK, I will update latter.

neverCase added a commit to neverCase/apisix-ingress-controller that referenced this pull request Dec 28, 2021
neverCase added a commit to neverCase/apisix-ingress-controller that referenced this pull request Dec 28, 2021
…dify crd yaml (apache#791)"

This reverts commit 03e93bf.

# Conflicts:
#	pkg/kube/translation/apisix_pluginconfig.go
#	samples/deploy/crd/v1/ApisixPluginConfig.yaml
#	test/e2e/plugins/plugin_config.go
@tao12345666333
Copy link
Member

I will close this one, using #815 instead.

v1.4 Planning automation moved this from Reviewer approved to Done Dec 29, 2021
@neverCase neverCase deleted the feat-638 branch December 29, 2021 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Issues with this label should be added to changelog when public a new release enhancement New feature or request triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants