Skip to content

Fix backend protocol annotation incorrectly documented #1187

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

samuel-esp
Copy link

What type of PR is this?
/kind bug

What this PR does / why we need it:

Fix backend protocol and type annotations in documentation: the backend protocol annotation only applies to Classic ELB, therefore there are incorrect statements inside the description (tcp is the default protocol, not http). Therefore, in the type annotation, the user should be able to understand the behavior behind the protocol assignment for NLB since it is different from the one described inside the backend protocol annotation (in the past, the user might have been misled into thinking the process was the same since both ELB type were shown to be compatible with the backend protocol annotation)

Which issue(s) this PR fixes:

Fixes #1186

Does this PR introduce a user-facing change?:
None

Fix backend protocol annotation scope in documentation: which only applies to classic ELB. The description for the backend is rewritten to be more clear and to clean incorrect statements. The description for the type annotation is enhanced to explain the behavior behind the protocol assignment for NLB

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 2, 2025
@k8s-ci-robot k8s-ci-robot requested a review from hakman July 2, 2025 17:14
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kmala for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jul 2, 2025
@k8s-ci-robot k8s-ci-robot requested a review from nckturner July 2, 2025 17:14
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 2, 2025
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 2, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @samuel-esp. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 2, 2025
| service.beta.kubernetes.io/aws-load-balancer-access-log-s3-bucket-name | - | - | ELB | Access log S3 bucket name. |
| service.beta.kubernetes.io/aws-load-balancer-access-log-s3-bucket-prefix | - | - | ELB | Access log S3 bucket prefix. |
| service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags | Comma-separated list of key=value | - | ELB,NLB | A comma-separated list of key-value pairs which will be recorded as additional tags in the ELB. For example: "Key1=Val1,Key2=Val2,KeyNoVal1=,KeyNoVal2" |
| service.beta.kubernetes.io/aws-load-balancer-backend-protocol | [http\|https\|ssl\|tcp] | tcp | ELB | Specifies the protocol spoken by the backend (pod) behind a listener. If set to `http` and the `aws-load-balancer-ssl-cert` annotation is missing or its value is empty (`""`), a HTTP listener is created; if set to `http` or `https` and the `aws-load-balancer-ssl-cert`annotation is also present, an HTTPS listener is created to terminate SSL and parse headers. If set to `ssl` or `tcp`, a "raw" SSL/TCP listener is used. if the annotation is missing, its value is empty (`""`), or set to `https` without the `aws-load-balancer-ssl-cert`annotation also being present, the default protocol is assumed to be `tcp`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like NLB also supports it:

if isNLB(annotations) {
portMapping := nlbPortMapping{
FrontendPort: int32(port.Port),
FrontendProtocol: elbv2types.ProtocolEnum(port.Protocol),
TrafficPort: int32(port.NodePort),
TrafficProtocol: elbv2types.ProtocolEnum(port.Protocol),
}
var err error
if portMapping.HealthCheckConfig, err = c.buildNLBHealthCheckConfiguration(apiService); err != nil {
return nil, err
}
certificateARN := annotations[ServiceAnnotationLoadBalancerCertificate]
if port.Protocol != v1.ProtocolUDP && certificateARN != "" && (sslPorts == nil || sslPorts.numbers.Has(port.Port) || sslPorts.names.Has(port.Name)) {
portMapping.FrontendProtocol = elbv2types.ProtocolEnumTls
portMapping.SSLCertificateARN = certificateARN
portMapping.SSLPolicy = annotations[ServiceAnnotationLoadBalancerSSLNegotiationPolicy]
if backendProtocol := annotations[ServiceAnnotationLoadBalancerBEProtocol]; backendProtocol == "ssl" {
portMapping.TrafficProtocol = elbv2types.ProtocolEnumTls
}

Suggested change
| service.beta.kubernetes.io/aws-load-balancer-backend-protocol | [http\|https\|ssl\|tcp] | tcp | ELB | Specifies the protocol spoken by the backend (pod) behind a listener. If set to `http` and the `aws-load-balancer-ssl-cert` annotation is missing or its value is empty (`""`), a HTTP listener is created; if set to `http` or `https` and the `aws-load-balancer-ssl-cert`annotation is also present, an HTTPS listener is created to terminate SSL and parse headers. If set to `ssl` or `tcp`, a "raw" SSL/TCP listener is used. if the annotation is missing, its value is empty (`""`), or set to `https` without the `aws-load-balancer-ssl-cert`annotation also being present, the default protocol is assumed to be `tcp`. |
| service.beta.kubernetes.io/aws-load-balancer-backend-protocol | [http\|https\|ssl\|tcp] | tcp | ELB,NLB | Specifies the protocol spoken by the backend (pod) behind a listener. If set to `http` and the `aws-load-balancer-ssl-cert` annotation is missing or its value is empty (`""`), a HTTP listener is created; if set to `http` or `https` and the `aws-load-balancer-ssl-cert`annotation is also present, an HTTPS listener is created to terminate SSL and parse headers. If set to `ssl` or `tcp`, a "raw" SSL/TCP listener is used. if the annotation is missing, its value is empty (`""`), or set to `https` without the `aws-load-balancer-ssl-cert`annotation also being present, the default protocol is assumed to be `tcp`. |

Copy link
Author

Choose a reason for hiding this comment

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

Hey @mtulio, could you try to replicate that? Because when i wrote the docs I did several tests and the protocol showed in the console never matched the one I used in the annotation; looking at the code this made sense to me. A double check would be awesome

Copy link
Author

Choose a reason for hiding this comment

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

Hey @mtulio did you have time to check this?

Co-authored-by: Marco Braga <braga@mtulio.eng.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backend protocol annotation incorrectly documented as supported for NLB
3 participants