-
Notifications
You must be signed in to change notification settings - Fork 366
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
Do not export Services of ExternalName type #4814
Conversation
/test-multicluster-e2e |
@@ -199,6 +200,16 @@ func (r *ServiceExportReconciler) Reconcile(ctx context.Context, req ctrl.Reques | |||
return ctrl.Result{}, err | |||
} | |||
|
|||
// The ExternalName type of Service is not supported since it has no ClusterIP | |||
// assgined to the Service. | |||
if svc.Spec.Type == corev1.ServiceTypeExternalName { |
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 remember we check ClusterIP is valid or not somewhere. No? Should we check that instead?
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 had some codes to check available Endpoints, no check for ClusterIP.
I think we need both considering other types of Service's ClusterIP might be <none>
for some rare cases, I added a few lines to check Service with empty IP.
d354db4
to
781e921
Compare
return ctrl.Result{}, nil | ||
} else { | ||
eps.Subsets = []corev1.EndpointSubset{svcIPAsSubset} | ||
} | ||
} else { | ||
newSubsets := common.FilterEndpointSubsets(eps.Subsets) |
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.
Do we need to handle no Endpoint IP case?
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 already validate Endpoint IPs and will update ServiceExport status if it's empty or no available address before this, which is from line 246-255.
@@ -402,6 +423,12 @@ func (r *ServiceExportReconciler) updateSvcExportStatus(ctx context.Context, req | |||
case serviceNotFound: | |||
newCondition.Reason = getStringPointer("service_not_found") |
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 just checked K8s conditions. Seems the conventions for Reason
is something like "ServiceNotFound", "ServiceTypeNotSupported".
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.
Refined.
multicluster/controllers/multicluster/member/serviceexport_controller.go
Outdated
Show resolved
Hide resolved
multicluster/controllers/multicluster/member/serviceexport_controller.go
Outdated
Show resolved
Hide resolved
multicluster/controllers/multicluster/member/serviceexport_controller.go
Outdated
Show resolved
Hide resolved
newCondition.Message = getStringPointer("the ExternalName type of Service is not supported") | ||
case serviceClusterIPEmpty: | ||
newCondition.Reason = getStringPointer("service_clusterip_empty") | ||
newCondition.Message = getStringPointer("the ClusterIP of Service is empty") | ||
case serviceWithoutEndpoints: | ||
newCondition.Reason = getStringPointer("service_without_endpoints") | ||
newCondition.Message = getStringPointer("the Service has no Endpoints, failed to export") |
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 say: "Service has no Endpoints"
And for the following two:
"The Service is an imported Service and cannot be re-exported"
"Service is successfully exported". But for True
, do we still need a reason and message? I believe no.
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.
Done, removed reason and message if succeed.
781e921
to
f0bbeb7
Compare
newCondition.Reason = getStringPointer("ServiceTypeNotSupported") | ||
newCondition.Message = getStringPointer("Service of ExternalName type is not supported") | ||
case serviceClusterIPEmpty: | ||
newCondition.Reason = getStringPointer("ServiceClusterIPEmpty") |
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.
Probably "ServiceNoClusterIP"?
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.
multicluster/controllers/multicluster/member/serviceexport_controller.go
Outdated
Show resolved
Hide resolved
@@ -400,18 +421,22 @@ func (r *ServiceExportReconciler) updateSvcExportStatus(ctx context.Context, req | |||
|
|||
switch cause { | |||
case serviceNotFound: | |||
newCondition.Reason = getStringPointer("service_not_found") | |||
newCondition.Message = getStringPointer("the Service does not exist") | |||
newCondition.Reason = getStringPointer("ServiceNotFound") |
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.
Could you also check if we add conditions for other resources, and use consistent Reason and Message format for all?
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 one in ResourceExport, others are expected format.
84853e4
to
6f8d361
Compare
/test-multicluster-e2e |
@jianjuns can you help to move forward for this PR? thanks. |
/test-all |
@luolanzone : please make sure all required tests are passed. |
ExternalName type of Service shouldn't be exported since it has no ClusterIP assinged, and it's not supported by KEP1645 either. So add a few validation codes to disable ExternalName type of Service exporting. Signed-off-by: Lan Luo <luola@vmware.com>
6f8d361
to
aedd4db
Compare
/test-multicluster-e2e |
/test-networkpolicy |
Services of ExternalName type shouldn't be exported since it has no ClusterIP assigned, and it's not supported by KEP1645 either. So add validation code to disable ExternalName Service exporting. Signed-off-by: Lan Luo <luola@vmware.com>
Services of ExternalName type shouldn't be exported since it has no ClusterIP assigned, and it's not supported by KEP1645 either. So add validation code to disable ExternalName Service exporting. Signed-off-by: Lan Luo <luola@vmware.com>
Services of ExternalName type shouldn't be exported since it has no ClusterIP assigned, and it's not supported by KEP1645 either. So add validation code to disable ExternalName Service exporting. Signed-off-by: Lan Luo <luola@vmware.com>
ExternalName type of Service shouldn't be exported since it has no ClusterIP assinged, and it's not supported by KEP1645 either. So add a few validation codes to disable ExternalName type of Service exporting.