NE-705: IngressController subnet selection in AWS

[gcs278]

Adds lb-subnet-selection-aws.md enhancement for specifying IngressController's load balancer type service subnets. This enhancement introduces spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.subnets which allows cluster admins to specify the subnets for the load balancer.

Epic: https://issues.redhat.com/browse/NE-705
RFE: https://issues.redhat.com/browse/RFE-1717

[openshift-ci-robot]

@gcs278: This pull request references NE-705 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.16." or "openshift-4.16.", but it targets "openshift-4.13" instead.

In response to this:

Adds lb-subnet-selection-aws.md enhancement for specifying IngressController's load balancer type service subnets.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@gcs278: This pull request references NE-705 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.16." or "openshift-4.16.", but it targets "openshift-4.13" instead.

In response to this:

Adds lb-subnet-selection-aws.md enhancement for specifying IngressController's load balancer type service subnets.

Epic: https://issues.redhat.com/browse/NE-705
RFE: https://issues.redhat.com/browse/RFE-1717

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 openshift-eng/jira-lifecycle-plugin repository.

[gcs278]

Ready for initial review, but will keep hold on until I feel it has consensus.

Currently missing install-time design, but I have stubbed out the sections that need updating for that.

/hold

[openshift-ci-robot]

@gcs278: This pull request references NE-705 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.16." or "openshift-4.16.", but it targets "openshift-4.13" instead.

In response to this:

Adds lb-subnet-selection-aws.md enhancement for specifying IngressController's load balancer type service subnets. This enhancement introduces spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.subnets which allows cluster admins to specify the subnets for the load balancer.

Epic: https://issues.redhat.com/browse/NE-705
RFE: https://issues.redhat.com/browse/RFE-1717

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 openshift-eng/jira-lifecycle-plugin repository.

[gcs278]

Updating the Enhancement for the API to be immutable. Temporarily WIP
/wip

[gcs278]

/wip cancel

[Miciah]

/assign

[candita]

nit: this doesn't really explain why it is unsupported. Does this mean that Ingress Operator will revert the changes, or something else?

[gcs278]

Does this mean that Ingress Operator will revert the changes, or something else?

No it won't revert, but that's an unfortunate oversight. The Ingress Operator should have been designed to revert unmanaged annotations (that would have made our lives easier). Users are not supposed to modify the objects created and managed by the Ingress Operator.

So it's an awkward situation. But just because a cluster admin can do something, doesn't mean that it's supported. But just because something is unsupported, doesn't mean a cluster admin is doing it.

I updated to be a little more specific about that it's 1. untested and 2. could interfere with the operations of the Ingress Operator.

[candita]

Is it perpetuating the pattern of the previous sentence? You lost me.

[gcs278]

This proposal for Subnets perpetuates the pattern of ensuring that status.endpointPublishingStrategy reflects the actual value (in our case, the annotation value).

It perpetuates the pattern of ensuring that status.endpointPublishingStrategy reflects the _actual_ value (in our case, the annotation value).

Does that make sense? I'm not sure how else to word it.

[candita]

I would give the pattern a good short name, like "triggered consistency", or something like that.

[gcs278]

This pattern I'm describing here doesn't really involve "triggered consistency".

This pattern is that: status == the effectuated value (i.e. the service.beta.kubernetes.io/aws-load-balancer-subnets annotation on the service in our case).

This is a generic concept of how to keep our IngressController's status consistent, regardless if the effectuated value is triggered or delayed by having the cluster admin do something.

I think we have to spell this concept verbosely out here.

[gcs278]

Thanks for taking another look @candita

Changes are here: https://github.com/openshift/enhancements/compare/a6cb330b3138b5a2203eadfe5c1ce4fe9fd87f1a..03777c644c401bcfc786115180aacf121918a537

[gcs278]

Does this mean that Ingress Operator will revert the changes, or something else?

No it won't revert, but that's an unfortunate oversight. The Ingress Operator should have been designed to revert unmanaged annotations (that would have made our lives easier). Users are not supposed to modify the objects created and managed by the Ingress Operator.

So it's an awkward situation. But just because a cluster admin can do something, doesn't mean that it's supported. But just because something is unsupported, doesn't mean a cluster admin is doing it.

I updated to be a little more specific about that it's 1. untested and 2. could interfere with the operations of the Ingress Operator.

[gcs278]

@candita updated the EP reflect that we will do the install-time API as another EP as suggested by Miciah. PTAL when you get a chance. Thanks.

[candita]

Just a few remaining nits. Otherwise:
/lgtm

[openshift-ci]

New changes are detected. LGTM label has been removed.

[gcs278]

Thanks for the LGTM @candita. Addressed your latest comments.

Could I get another look and re-LGTM if good?

[gcs278]

This pattern I'm describing here doesn't really involve "triggered consistency".

This pattern is that: status == the effectuated value (i.e. the service.beta.kubernetes.io/aws-load-balancer-subnets annotation on the service in our case).

This is a generic concept of how to keep our IngressController's status consistent, regardless if the effectuated value is triggered or delayed by having the cluster admin do something.

I think we have to spell this concept verbosely out here.

[openshift-ci]

@gcs278: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[gcs278]

Removing hold because we've decided to address install time (day 1) design in a separate EP.
/hold cancel