ingress: Add mutable-publishing-scope enhancement

[Miciah]

This enhancement defines an approach for allowing users to modify the scope of a service load-balancer for an IngressController that uses the LoadBalancerService endpoint publishing strategy type.

[Miciah]

Last push abbreviates the status condition message in the example ingresscontroller to satisfy the lint check.

[candita]

Suggested change

	and the operation of changing it scope is complete.
	and the operation of changing its scope is complete.

[candita]

The message is really long and probably too much information. Could we get away with something like: "Scope changed from ___ to ___, requires delete of service ___"

[Miciah]

The detailed wording is the result of some discussion here: openshift/cluster-ingress-operator#582 (comment)

The explicit and detailed message is intended to prevent mistakes and reduce support cases. I can try to make this more concise, but I think it is important to communicate the options and their consequences because otherwise users will break their clusters' ingress.

[candita]

If you change it to not use the "have" and "want" I think that's much better. The rest is ok.

Suggested change

	- message: Have load balancer with scope "External", want load balancer with scope "Internal". You can delete the openshift-ingress/router-default service to proceed [...]. Alternatively, you can change the IngressController's spec.endpointPublishingStrategy.loadBalancer.scope field value back to its previous value [...].
	- message: You changed load balancer scope from "External", to "Internal" and need to adjust the service. You can delete the openshift-ingress/router-default service to proceed [...]. Alternatively, you can change the IngressController's spec.endpointPublishingStrategy.loadBalancer.scope field value back to its previous value [...].

[Miciah]

I've reworded the message to avoid the "Have ___, want ___" phrasing.

[candita]

I think of #2 as a side note, not an option we need to emphasize here or in user messages.

[Miciah]

When we introduced an earlier version of this feature (which we since reverted) that automatically deleted and recreated the service, we had at least one user who changed the scope and then realized it would require deprovisioning and recreating the LB and wanted to revert the change. The option to back out the change is closely tied to the motivation for this enhancement; I'll revise the motivation section to call this out.

[candita]

Ok

[candita]

Is this different than the annotation mentioned earlier in Line 59? If not, can you add the name of the annotation the first place it appears? If so, can you name the one on Line 59?

[Miciah]

It is a different annotation. I'll try to clarify that on line 59.

[candita]

Suggested change

	In addition to deleting the IngressController explicitly, it is possible to
	In addition to deleting the Service explicitly, it is possible to

[candita]

The annotation is sort of awkward. Seems like it would be better specifiied as a flag on the oc patch operation, something like oc patch ... --auto-update or --propagate or similar.

[Miciah]

Would the oc patch command have custom logic to add the ingress.operator.openshift.io/auto-delete-load-balancer annotation when the user provided the --auto-update command and an IngressController? I'd rather avoid adding IngressController-specific or (OpenShift-specific) logic to oc patch, and anyway, the annotation is intended for automation such as the cloud-ingress-operator's publishingstrategy controller.

[candita]

ok

[candita]

If it's not intended for end-users, then maybe it should not be an annotation?

[Miciah]

Do you have an alternative in mind? The annotation was requested by Service Delivery; we need something that cloud-ingress-operator could use.

[candita]

I think this use case will be of little value, and though it is easy to implement and describe, it somewhat muddies an otherwise straightforward documentation.

[Miciah]

It's important because we've had support cases where users changed the scope on the IngressController and then realized that completing the change would cause the service load-balancer's IP address to change and wanted to back out.

[candita]

Suggested change

	oc -n openshift-ingress-operator annotate ingresscontrollers/default ingress.operator.openshift.io/auto-delete-load-balancer=
	oc -n openshift-ingress-operator annotate ingresscontrollers/default ingress.operator.openshift.io/auto-delete-load-balancer=true

Or some value

[candita]

Would you need to add the annotation first, before the change is made?

[Miciah]

The annotation value doesn't actually matter.

Would you need to add the annotation first, before the change is made?

No, so automation could either add the annotation when creating the IngressController or add it when updating the scope. I'll try to clarify this point.

[candita]

The dangling = still looks odd but it's not a show-stopper.

[candita]

Which annotations, just some pre-existing ones?

[Miciah]

Right; I'll add clarify that point.

[candita]

Suggested change

	Crucially, by default, the operator *never* deletes the Service as long as the
	Crucially, by default, the operator ***never*** deletes the Service as long as the

[candita]

Suggested change

	the IngressController, in which case the operator *does* delete the Service.
	the IngressController, in which case the operator **does** delete the Service.

[candita]

It might be cleaner if it were guaranteed to properly update the Progessing status in this case (reason and message).

[Miciah]

I added some language to clarify that the downgraded operator will continue updating status conditions. Does that make things clearer?

[Miciah]

Updated per @candita's comments.

[brandisher]

I had one question around the wording of a user story but aside from that, this proposal looks reasonable and will likely be the most customer amenable option.

[brandisher]

I'm not sure if its me or the wording but this story seems a little confusing. I readt his has have: internal IngressController, want external IngressController. If that's accurate, shouldn't the statement be: ...change the scope of an IngressController to be external (public) ... along with an update to External in the patch command?

[Miciah]

Thanks! You're right. I've fixed this in the latest push.

[candita]

/lgtm

[candita]

/lgtm

[frobware]

LGTM, just a few comments/observations to clear up or clarify.

[frobware]

Can we also emit oc commands that you can simply cut & paste? I'm assuming we have enough context (e.g., namespace and names, et al). Might be helpful should you want to restore "its previous value" too.

[Miciah]

We can emit oc commands, but the patch command alone makes the message too long to pass the markdownlint CI job. I'll try using double quotes with a backslash-escaped newline to see whether the linter allows that. I believe that it'll still be valid yaml, although it won't be exactly what oc get ... -o yaml would actually output.

[frobware]

This is just stating that if you went from External=>Internal and then issued the delete the operator tracks the the scope you wanted and it will come back as Internal - correct?

[Miciah]

More or less. The main point that I'm trying to convey is that the user must explicitly perform the delete operation if one is needed, but to your question, yes, the operator will recreate the service with the scope specified in the ingresscontroller. Is the current phrasing clear, or does it need some wordsmithing?

[frobware]

If this operation fails where would it be reported? Although the Kubernetes service controller makes the changes I'd like to know where we diagnose failures should that fail in any way.

[Miciah]

In case of failure, the service controller should emit an event with reason "SyncLoadBalancerFailed", which the ingress operator would observe and reflect in the ingresscontroller's status conditions as LoadBalancerReady=False and ultimately Available=False.

[Miciah]

I added this information under the new "Support Procedures" section.

[frobware]

Is it also feasible to have test setups that make progress but ultimately fail so that Progressing=False does not occur? I'm asking for both the happy path and the unhappy path.

[Miciah]

Progressing=True indicates that the operator is in an intermediate state: the user has specified one scope, and the service currently has a different scope. Once this discrepancy is resolved (by deleting the service or by updating the ingresscontroller to have the same scope as the service), then the operator returns to its baseline of reporting Progressing=False. For reference, here is the proof-of-concept implementation of the status reporting: openshift/cluster-ingress-operator@7daeb6f#diff-56b131774a926e7a0e30a9be7dac7bf5c5cec11ff709aa6604cecc9ef117ede2R547-R584. If the service controller raises an error while updating or recreating the service load-balancer, then it will report an event that the operator will observe and report as described in my previous comment. Is that sufficient?

[Miciah]

The new "Operational Aspects of API Extensions" section should clarify this.

[frobware]

How/Why will it not delete the service in this case?

[Miciah]

The how and why are the same in the upgrade case as in the normal case: If the ingresscontroller and the service specify different scopes and we don't know that the platform supports scope changes in situ, then the operator does not delete the service because doing so could disrupt traffic. The behavior on upgrade or downgrade follows from and is consistent with the logic described in the proposal and implementation details. Should I add something to that effect to the enhancement text? I sought to adhere to the enhancement format without being too verbose or repetitive.

[openshift-bot]

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

[Miciah]

The CI job failed with no obvious reason:

 + markdownlint-cli2 '**/*.md'
markdownlint-cli2 v0.3.2 (markdownlint v0.24.0)
Finding: **/*.md
Linting: 343 file(s)
Summary: 0 error(s)
++ dirname hack/markdownlint.sh
+ hack/template-lint.sh
Checking enhancements/ingress/mutable-publishing-scope.md
enhancements/ingress/mutable-publishing-scope.md missing "### API Extensions"
enhancements/ingress/mutable-publishing-scope.md missing "### Operational Aspects of API Extensions"
enhancements/ingress/mutable-publishing-scope.md missing "#### Failure Modes"
enhancements/ingress/mutable-publishing-scope.md missing "#### Support Procedures"
{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-11-17T03:22:45Z"} 

/test markdownlint

[Miciah]

Wait, those are obvious reasons.

[Miciah]

c260fc8 → 51e0f70 adds the newly required "API Extensions", "Operational Aspects of API Extensions", "Failure Modes", and "Support Procedures" sections.

[frobware]

/lgtm

[frobware]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware

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:

• OWNERS [frobware]

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