Convert error messages to use event recorder

[wccsama]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

todo: Convert error messages to use event recorder.

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

[k8s-ci-robot]

Hi @wccsama. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[MrHohn]

/ok-to-test

[MrHohn]

This is a strange place to do validation like this (this function is only triggered when comparing an updated service object). Also, on the above it says the cloud provider itself is responsible for the protocol validation. Why don't we remove the validation here and delegate EnsureLoadBalancer() for doing the check of mixed protocol?

[wccsama]

Yeah~, mixed protocol may be checked in the function EnsureLoadBalancer() in a future version.

[wccsama]

/retest

[wccsama]

/assign @MrHohn Should I remove the validation, or just print err message like this pr.

[MrHohn]

Should I remove the validation, or just print err message like this pr.

@wccsama I would suggest removing the check completely so that getPortsForLB() is sorely used for getting the ports from service. The mixed protocol config is already gated by the validation on apiserver anyway:

kubernetes/pkg/apis/core/validation/validation.go

Lines 3998 to 4012 in 3698100

	if service.Spec.Type == core.ServiceTypeLoadBalancer {
	portsPath := specPath.Child("ports")
	includeProtocols := sets.NewString()
	for i := range service.Spec.Ports {
	portPath := portsPath.Index(i)
	if !supportedPortProtocols.Has(string(service.Spec.Ports[i].Protocol)) {
	allErrs = append(allErrs, field.Invalid(portPath.Child("protocol"), service.Spec.Ports[i].Protocol, "cannot create an external load balancer with non-TCP/UDP/SCTP ports"))
	} else {
	includeProtocols.Insert(string(service.Spec.Ports[i].Protocol))
	}
	}
	if includeProtocols.Len() > 1 {
	allErrs = append(allErrs, field.Invalid(portsPath, service.Spec.Ports, "cannot create an external load balancer with mix protocols"))
	}
	}

[wccsama]

/retest

[wccsama]

Should I remove the validation, or just print err message like this pr.

@wccsama I would suggest removing the check completely so that getPortsForLB() is sorely used for getting the ports from service. The mixed protocol config is already gated by the validation on apiserver anyway:

kubernetes/pkg/apis/core/validation/validation.go

Lines 3998 to 4012 in 3698100

	if service.Spec.Type == core.ServiceTypeLoadBalancer {
	portsPath := specPath.Child("ports")
	includeProtocols := sets.NewString()
	for i := range service.Spec.Ports {
	portPath := portsPath.Index(i)
	if !supportedPortProtocols.Has(string(service.Spec.Ports[i].Protocol)) {
	allErrs = append(allErrs, field.Invalid(portPath.Child("protocol"), service.Spec.Ports[i].Protocol, "cannot create an external load balancer with non-TCP/UDP/SCTP ports"))
	} else {
	includeProtocols.Insert(string(service.Spec.Ports[i].Protocol))
	}
	}
	if includeProtocols.Len() > 1 {
	allErrs = append(allErrs, field.Invalid(portsPath, service.Spec.Ports, "cannot create an external load balancer with mix protocols"))
	}
	}

@MrHohn Done, PTAL thanks~

[MrHohn]

/approve

[MrHohn]

Seems like we don't need to check nil?

	xPorts := getPortsForLB(x)
	yPorts := getPortsForLB(y)
	return portSlicesEqualForLB(xPorts, yPorts)

[wccsama]

yeah~

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrHohn, wccsama

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:

• pkg/controller/service/OWNERS [MrHohn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[MrHohn]

This LGTM, can you squash the fix-up commits?

[wccsama]

@MrHohn already fixed~

[MrHohn]

Thanks!
/lgtm