-
Couldn't load subscription status.
- Fork 41.6k
kubeadm: support specifying HTTP endpoints for external etcd, allowing users to separate gRPC and HTTP traffic for etcd #134890
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
base: master
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SataQiu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // HTTPEndpoints are the HTTP endpoints used to communicate with the etcd cluster. | ||
| // If not provided, the same values as in Endpoints are used. | ||
| // +optional | ||
| HTTPEndpoints []string `json:"httpEndpoints,omitempty"` |
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 existing endpoints list is validated to have http protocol in the URLs.
i think this is missing some validation in:
- endpoints should be validated to be non-empty and to have https?
- if httpenpoints is not empty we should also validate it for https?
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.
new types should be documented in doc.go following the similar pattern from before explaining in what release it was added.
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.
Updated. httpenpoints can be http instead of https.
|
/hold |
|
/priority important-soon |
c080adc to
6e7fe31
Compare
6e7fe31 to
9578a41
Compare
… to configure the HTTP endpoints for etcd communication.
9578a41 to
a279f64
Compare
|
/retest |
|
i will review again later today or tomorrow morning. |
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'd like to know if we have test jobs based on this PR, or if tests have been run locally. The code looks great, but we still need to verify that it functions correctly. Running tests would also help us validate whether HTTPEndpoints and Endpoints provide full compatibility. If they aren't fully compatible, it could potentially cause issues.
| out.Endpoints = *(*[]string)(unsafe.Pointer(&in.Endpoints)) | ||
| // set the HTTPEndpoints to the same as the Endpoints | ||
| // this is to maintain backwards compatibility with the v1beta3 API | ||
| out.HTTPEndpoints = *(*[]string)(unsafe.Pointer(&in.Endpoints)) |
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.
| out.Endpoints = *(*[]string)(unsafe.Pointer(&in.Endpoints)) | |
| // set the HTTPEndpoints to the same as the Endpoints | |
| // this is to maintain backwards compatibility with the v1beta3 API | |
| out.HTTPEndpoints = *(*[]string)(unsafe.Pointer(&in.Endpoints)) | |
| out.Endpoints = *(&in.Endpoints) | |
| // set the HTTPEndpoints to the same as the Endpoints | |
| // this is to maintain backwards compatibility with the v1beta3 API | |
| out.HTTPEndpoints = *(&in.Endpoints) |
will be simper?
| } | ||
|
|
||
| allErrs = append(allErrs, ValidateURLs(e.External.Endpoints, requireHTTPS, externalPath.Child("endpoints"))...) | ||
| if len(e.External.Endpoints) == 0 { |
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.
This appears to be behavior that is incompatible with previous versions (previously we did not validate whether the length of Endpoints was 0). However, given that Endpoints should not be empty, this should not pose an issue.
What type of PR is this?
/kind feature
What this PR does / why we need it:
kubeadm: support specifying HTTP endpoints for external etcd.
Which issue(s) this PR is related to:
Fixes kubernetes/kubeadm#3234
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: