Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do not export Services of ExternalName type #4814

Merged
merged 1 commit into from
Apr 26, 2023
Merged

Do not export Services of ExternalName type #4814

merged 1 commit into from
Apr 26, 2023

Conversation

luolanzone
Copy link
Contributor

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.

@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Apr 6, 2023
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@luolanzone luolanzone requested a review from jianjuns April 6, 2023 09:49
jianjuns
jianjuns reviewed Apr 6, 2023
View reviewed changes
multicluster/controllers/multicluster/member/serviceexport_controller.go
@@ -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 {
Copy link
Contributor

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?

Copy link
Contributor Author

@luolanzone luolanzone Apr 12, 2023 •
edited
Loading

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.

jianjuns
jianjuns reviewed Apr 12, 2023
View reviewed changes
multicluster/controllers/multicluster/member/serviceexport_controller.go
return ctrl.Result{}, nil
} else {
eps.Subsets = []corev1.EndpointSubset{svcIPAsSubset}
}
} else {
newSubsets := common.FilterEndpointSubsets(eps.Subsets)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

multicluster/controllers/multicluster/member/serviceexport_controller.go Outdated
@@ -402,6 +423,12 @@ func (r *ServiceExportReconciler) updateSvcExportStatus(ctx context.Context, req
case serviceNotFound:
newCondition.Reason = getStringPointer("service_not_found")
Copy link
Contributor

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".

Copy link
Contributor Author

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
multicluster/controllers/multicluster/member/serviceexport_controller.go Outdated
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")
Copy link
Contributor

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.

Copy link
Contributor Author

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.

jianjuns
jianjuns reviewed Apr 13, 2023
View reviewed changes
multicluster/controllers/multicluster/member/serviceexport_controller.go Outdated
newCondition.Reason = getStringPointer("ServiceTypeNotSupported")
newCondition.Message = getStringPointer("Service of ExternalName type is not supported")
case serviceClusterIPEmpty:
newCondition.Reason = getStringPointer("ServiceClusterIPEmpty")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably "ServiceNoClusterIP"?

Copy link
Contributor Author

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
multicluster/controllers/multicluster/member/serviceexport_controller.go
@@ -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")
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@luolanzone luolanzone force-pushed the check-service-type branch 3 times, most recently from 84853e4 to 6f8d361 Compare April 14, 2023 01:48
jianjuns
jianjuns previously approved these changes Apr 14, 2023
View reviewed changes
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@luolanzone
Copy link
Contributor Author

@jianjuns can you help to move forward for this PR? thanks.

@jianjuns
Copy link
Contributor

/test-all

@jianjuns
Copy link
Contributor

@jianjuns can you help to move forward for this PR? thanks.

@luolanzone : please make sure all required tests are passed.

@luolanzone
Disable ExternalName type of Service exporting
aedd4db 
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>
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e
/test-all
Fixed the integration failure.

@luolanzone
Copy link
Contributor Author

/test-networkpolicy

@jianjuns jianjuns merged commit feb4122 into antrea-io:main Apr 26, 2023
@jianjuns jianjuns changed the title Disable ExternalName type of Service exporting Do not export Services of ExternalName type Apr 26, 2023
jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023
@luolanzone
Do not export Services of ExternalName type (antrea-io#4814)
c655a78 
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>
rajnkamr pushed a commit to rajnkamr/antrea that referenced this pull request May 4, 2023
@luolanzone @rajnkamr
Do not export Services of ExternalName type (antrea-io#4814)
8c91ddf 
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>
@luolanzone luolanzone mentioned this pull request May 19, 2023
Merged
ceclinux pushed a commit to ceclinux/antrea that referenced this pull request Jun 5, 2023
@luolanzone @ceclinux
Do not export Services of ExternalName type (antrea-io#4814)
9f89364 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianjuns jianjuns jianjuns approved these changes

Assignees
No one assigned
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luolanzone @jianjuns