Proposal: Add API Server Service Information to KarmadaStatus
#5599
Conversation
Signed-off-by: Joe Nathan Abellard <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Joe Nathan Abellard <[email protected]>
karmada-bot
added
the
size/M
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5599 +/- ##
==========================================
- Coverage 34.84% 34.82% -0.03%
==========================================
Files 645 645
Lines 44861 44861
==========================================
- Hits 15633 15624 -9
- Misses 28021 28028 +7
- Partials 1207 1209 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // KarmadaStatus defines the most recently observed status of the Karmada. | ||
| type KarmadaStatus struct { | ||
| // Other existing fields... | ||
|
|
||
| // APIServerServiceStatus contains the current status of the Karmada API server service. | ||
| // +optional | ||
| APIServerServiceStatus APIServerServiceStatus `json:"apiServerServiceStatus,omitempty"` | ||
| } | ||
|
|
||
| // APIServerServiceStatus contains the current status of the API server service. | ||
| type APIServerServiceStatus struct { | ||
| // Name represents the name of the service. | ||
| Name string `json:"name"` | ||
|
|
||
| // Port represents the client port exposed by the service. | ||
| Port int32 `json:"port"` | ||
| } |
There was a problem hiding this comment.
Is this structure suitable for all kinds of Services?
As you know, karmada-operator allows users to specify Karmada APIServer ServiceType which including ClusterIP, NodePort, LoadBalancer, and ExternalName.
I'm not sure if all of these service approaches have been implemented or work as expected.
But I think it's a great chance to revisit the design about how to expose Karmada APIServer Service.
Any thoughts?
Also, cc @chaunceyjiang who might interested in this.
There was a problem hiding this comment.
@RainbowMango , you bring up a great point. As far as I can tell by looking at the code, there's only support for ClusterIP, NodePort, and LoadBalancer service types. I don't see support for service type ExternalName, which I expected as I don't think that would make sense for the API server. In addition to the service name and its type, let me break down what I think will be required for each type.
ClusterIP: the portNodePort: the port and node portLoadBalancer: the port, node port, and load balancer status
As such, the status struct will look something like this:
type APIServerServiceStatus struct {
// Name represents the name of the service.
Name string `json:"name"`
// ServiceType represents the service type
ServiceType corev1.ServiceType `json:"serviceType"`
// Port represents the port exposed by the service.
Port int32 `json:"port"`
// NodePort represents the node port on which the service is exposed
// when service is of type NodePort or LoadBalancer
// +optional
NodePort int32 `json:"nodePort"`
// LoadBalancerStatus represents the status of a load-balancer when
// the service is of type LoadBalancer
// +optional
LoadBalancerStatus corev1.LoadBalancerStatus `json:"loadBalancerStatus"`
}Anything I may have missed? Would love to hear your thoughts?
What type of PR is this?
/kind design
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: