-
Notifications
You must be signed in to change notification settings - Fork 336
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: ingress annotations supports the specified upstream schema #1451
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1451 +/- ##
==========================================
+ Coverage 41.11% 41.38% +0.26%
==========================================
Files 82 84 +2
Lines 7340 7382 +42
==========================================
+ Hits 3018 3055 +37
- Misses 3969 3974 +5
Partials 353 353
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
sorry for delay, could you please merge master branch? thanks! |
func (w *upstreamscheme) Parse(e annotations.Extractor) (interface{}, error) { | ||
return e.GetStringAnnotation(annotations.AnnotationsUpstreamScheme), nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check that its value is within these available ranges
apisix-ingress-controller/pkg/types/apisix/v1/types.go
Lines 52 to 59 in c8d3bd5
// SchemeHTTP represents the HTTP protocol. | |
SchemeHTTP = "http" | |
// SchemeGRPC represents the GRPC protocol. | |
SchemeGRPC = "grpc" | |
// SchemeHTTPS represents the HTTPS protocol. | |
SchemeHTTPS = "https" | |
// SchemeGRPCS represents the GRPCS protocol. | |
SchemeGRPCS = "grpcs" |
Please take a look at this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add e2e test cases to cover this. Thanks
pkg/providers/ingress/translation/annotations/upstreamscheme/upstreamscheme.go
Outdated
Show resolved
Hide resolved
} | ||
|
||
func (w *upstreamscheme) Parse(e annotations.Extractor) (interface{}, error) { | ||
scheme := e.GetStringAnnotation(annotations.AnnotationsUpstreamScheme) | ||
|
||
if inSchemes(scheme) { | ||
_, ok := schemeMap[scheme] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we could ignore case?
both HTTP
and http
can be used.
You can convert the obtained configuration items to lowercase
Please help me re-run ci. |
could you please merge master branch first? |
pkg/providers/ingress/translation/annotations/upstreamscheme/upstreamscheme.go
Outdated
Show resolved
Hide resolved
} | ||
|
||
var schemeMap map[string]struct{} = map[string]struct{}{ | ||
"http": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small suggestion. Can we make this variable global? Together with the schemes in pkg/types/apisix/v1/types.go
.
Otherwise, we may need to deal with cross-file constant dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, we have
apisix-ingress-controller/pkg/types/apisix/v1/types.go
Lines 52 to 59 in c8d3bd5
// SchemeHTTP represents the HTTP protocol. | |
SchemeHTTP = "http" | |
// SchemeGRPC represents the GRPC protocol. | |
SchemeGRPC = "grpc" | |
// SchemeHTTPS represents the HTTPS protocol. | |
SchemeHTTPS = "https" | |
// SchemeGRPCS represents the GRPCS protocol. | |
SchemeGRPCS = "grpcs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We define SchemeMap to pkg/types/apisix/v1/types.go
Or just use constants like SchemeHTTP
, SchemeGRPC
pkg/types/apisix/v1/types.go
var SchemeMap map[string]struct{} = map[string]struct{}{
SchemeHTTP: {},
SchemeHTTPS: {},
SchemeGRPC: {},
SchemeGRPCS: {},
}
or
pkg/providers/ingress/translation/annotations/upstreamscheme/upstreamscheme.go
var schemeMap map[string]struct{} = map[string]struct{}{
types.SchemeHTTP: {},
types.SchemeHTTPS: {},
types.SchemeGRPC: {},
types.SchemeGRPCS: {},
}
Type of change:
#1420
What this PR does / why we need it:
Pre-submission checklist: