Skip to content

Support load balancer listeners with http protocol via annotation #239

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.

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

Merged
merged 4 commits into from
Aug 29, 2018

Conversation

harveylowndes
Copy link
Contributor

@harveylowndes harveylowndes commented Aug 24, 2018

Addresses #208

Currently only TCP listeners are supported when creating a service type load balancer. The oci console allows for either TCP or HTTP.

annotations:
    service.beta.kubernetes.io/oci-load-balancer-backend-protocol: "HTTP"

When the service is create with the service.beta.kubernetes.io/oci-load-balancer-backend-protocol annotation is set to "HTTP" the listener protocol will be overridden to use HTTP. The value "TCP" may also be used for TCP listeners, however, this is the default when the value is empty or when the annotation is not present.

Copy link
Contributor

@prydie prydie left a comment

Choose a reason for hiding this comment

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

Rather than having a boolean annotation I'd like us to follow AWS's lead:

https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L163-L170

@@ -70,6 +70,10 @@ const (
// ServiceAnnotaionLoadBalancerSecurityListManagementMode is a Service annotation for
// specifying the security list managment mode ("All", "Frontend", "None") that configures how security lists are managed by the CCM
ServiceAnnotaionLoadBalancerSecurityListManagementMode = "service.beta.kubernetes.io/oci-load-balancer-security-list-management-mode"

// ServiceAnnotationLoadBalancerEnableHTTPProtocol is a Service annotation for
// enabling/disabling the load balancer listener with the http protocol.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if the comment stated what the accepted values are, and also what is the effect of setting this, i.e. why would a user want to do it/why are we supporting it?

@harveylowndes harveylowndes changed the title WIP: Enable/disable load balancer listeners with http protocol via annotation WIP: Support load balancer listeners with http protocol via annotation Aug 28, 2018
@harveylowndes harveylowndes force-pushed the support-http-load-balancer-listeners branch from ff5dd26 to 23f5a14 Compare August 28, 2018 14:28
@harveylowndes harveylowndes force-pushed the support-http-load-balancer-listeners branch from 70a17aa to 0e39e7e Compare August 28, 2018 15:44
@harveylowndes harveylowndes changed the title WIP: Support load balancer listeners with http protocol via annotation Support load balancer listeners with http protocol via annotation Aug 28, 2018
@@ -28,6 +28,7 @@ spec:
| `oci-load-balancer-subnet2` | The OCID of the second [subnet][2] of the two required subnets to attach the load balancer to. Must be in separate Availability Domains. | Value provided in config file |
| `oci-load-balancer-connection-idle-timeout` | The maximum idle time, in seconds, allowed between two successive receive or two successive send operations between the client and backend servers. | `300` for TCP listeners, `60` for HTTP listeners |
| `oci-load-balancer-security-list-management-mode` | Specifies the [security list mode](##security-list-management-modes) (`"All"`, `"Frontend"`,`"None"`) to configure how security lists are managed by the CCM. | `"All"`
| `oci-load-balancer-backend-protocol` | Specifies the [load balancer listener][5] backend protocol ("TCP", "HTTP"). | `"TCP"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The linked page is too general maybe just copy from the API documentation:

Specify protocol on which the listener accepts connection requests. To get a list of valid protocols, use the ListProtocols operation.

Copy link
Member

@owainlewis owainlewis left a comment

Choose a reason for hiding this comment

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

LGTM. Good work. 👍

@harveylowndes harveylowndes force-pushed the support-http-load-balancer-listeners branch from 0e39e7e to 0de33ae Compare August 29, 2018 08:30
@harveylowndes harveylowndes force-pushed the support-http-load-balancer-listeners branch from 0de33ae to 1d5807d Compare August 29, 2018 08:41
@prydie prydie merged commit 45ff9c8 into master Aug 29, 2018
@prydie prydie deleted the support-http-load-balancer-listeners branch August 29, 2018 09:40
ayushverma14 pushed a commit to ayushverma14/oci-cloud-controller-manager that referenced this pull request Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants