-
Notifications
You must be signed in to change notification settings - Fork 497
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
Gateway api #1042
Gateway api #1042
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #1042 +/- ##
==========================================
+ Coverage 81.02% 81.09% +0.06%
==========================================
Files 136 136
Lines 15405 15407 +2
==========================================
+ Hits 12482 12494 +12
+ Misses 2336 2327 -9
+ Partials 587 586 -1 ☔ View full report in Codecov by Sentry. |
|
||
// DefaultSpec returns the default spec of GatewayController. | ||
func (gc *GatewayController) DefaultSpec() interface{} { | ||
return &Spec{} |
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.
IMO, ~/.kube/config
could be the default value. And I think the GatewayController could support ClientSetInCluster mode[1].
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.
the default is in cluster mode(when both KubeConfig
and MasterURL
are empty) as it will be deployed in a k8s cluster at most of the time. And ClientSetInCluster mode is used, please check L161 in k8s.go.
func (b *pipelineSpecBuilder) addBackend(addr string, weight int) { | ||
if b.proxy == nil { | ||
b.proxy = &httpproxy.Spec{} | ||
b.proxy.Pools = append(b.proxy.Pools, &httpproxy.ServerPoolSpec{ServerMaxBodySize: -1}) |
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 side question, why not make the default value 0 for unlimited body?
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.
To support unlimited body, we have to enable the stream mode, but in this mode, some of the new features of v2.0 becomes unavailable. so the meaning of "set ServerMaxBodySize
to zero" is a balance of different things, and we think using -1
for unlimited body is better for more cases.
pkg/object/gatewaycontroller/k8s.go
Outdated
return true | ||
} | ||
|
||
if oldObj.(metav1.Object).GetResourceVersion() == newObj.(metav1.Object).GetResourceVersion() { |
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.
Just wonder why *apicorev1.Endpoints
is not Object.
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.
apicorev1.Endpoints
is Object, but this line is only for other objects, for endpoints, we will do additional checks below.
however, just found some of the conditions and return values of this function is wrong, will fix them.
return false | ||
} | ||
|
||
func isSubsetsChanged(a, b apicorev1.EndpointSubset) bool { |
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.
Why reflect.DeepEqual
can not cover this function.
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.
DeepEqual
can be used here.
But for performance consideration, explicit comparsion is used.
pkg/object/gatewaycontroller/k8s.go
Outdated
|
||
if minor < 19 { | ||
// Ingress version v1 has been added after kubernetes 1.19 | ||
panic(fmt.Errorf("kubernetes version [%v] is too low, GatewayController requires kubernetes v1.19+", info.GitVersion)) |
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.
In doc, the require of k8s is v1.18+, but here is v1.19...
No description provided.