Reset attributes of endpoints correctly

[haoruan]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Reset ports of endpoints to correct values if it is changed to some nonsense that won't work.

Which issue(s) this PR fixes:

Fixes #104252

Special notes for your reviewer:

The master service ports and type are configurable since #15791, and will be corrected only during start, that's why the param is always false for UpdateKubernetesService.

// Service definition is not reconciled after first
// run, ports and type will be corrected only during
// start.
if err := c.UpdateKubernetesService(false); err != nil {

But for endpoints, ports should be reset to correct value.

Does this PR introduce a user-facing change?

NONE

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

[k8s-ci-robot]

Hi @haoruan. 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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haoruan
To complete the pull request process, please assign deads2k after the PR has been reviewed.
You can assign the PR to them by writing /assign @deads2k in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• pkg/controlplane/OWNERS

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

[aojea]

do we know why this was this way?
The comment in line 236 explains this is on purpose

  // Service definition is not reconciled after first
  // run, ports and type will be corrected only during
  // start.

[aojea]

anyway, if this has to change, I think that the right place is in Line239 or just removing the (reconcile bool) input variable.

[haoruan]

In that case, it will also impact master services in line 256:

	if err := c.CreateOrUpdateMasterServiceIfNeeded(kubernetesServiceName, c.ServiceIP, servicePorts, serviceType, reconcile); err != nil {

but if it's safe, I agree, removing the bool variable is better.

[aojea]

👀

[liggitt]

I think it was related to #15791 (comment)

[liggitt]

what does it look like to transition the extra ports setting on masters? do we want skewed masters with different settings dueling/churning setting/resetting these, or should the latest master started win?

[aojea]

my main concern, after reading the original PR, is that this was done this way in purpose

@liggitt may remember the reason, but the issue referred in this PR seems legit

If this moves forward, this will need a test verifying the issue referred so we don't regress

[fedebongio]

/triage accepted

[aojea]

This exhibits the same behavior as current endpoints generated by the endpoints controller, users with permission can modify endpoints (and they are not reconciled), if you have power permissions the system doesn't stop you to do "bad things" ( i.e. sudo rm -rf / ) 😄

See related discussion here #98066 (comment)

Moreover, Jordan comment is spot on #107773 (comment) , right now there is the convention that all apiserver have the same port, and this change will make impossible to move to extra ports in the future.

In my opinion we should not modify this behavior