Skip to content
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

[OCCM] Set Hostname in load balancer status when using PROXY protocol #1449

Merged
merged 1 commit into from
Mar 23, 2021
Merged

[OCCM] Set Hostname in load balancer status when using PROXY protocol #1449

merged 1 commit into from
Mar 23, 2021

Conversation

lingxiankong
Copy link
Contributor

What this PR does / why we need it:
Related issues in other repos:

In PROXY mode, set Hostname field using IP address and a suffix .nip.io to expose the Service of LoadBalancer type.

A new config option enable-ingress-hostname is introduced but may be removed in the future, please see the document change.

Which issue this PR fixes(if applicable):
fixes #1287

Special notes for reviewers:

Release note:

[openstack-cloud-controller-manager] Fixed the broken header issue when accessing the load balancer service with PROXY protocol enabled from within the cluster.

@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 Mar 18, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 18, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build failed.

@lingxiankong
Copy link
Contributor Author

/test cloud-provider-openstack-acceptance-test-lb-octavia

@k8s-ci-robot
Copy link
Contributor

@lingxiankong: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-lb-octavia

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.

@lingxiankong
Copy link
Contributor Author

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

@k8s-ci-robot
Copy link
Contributor

@lingxiankong: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

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.

@lingxiankong
Copy link
Contributor Author

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.18

@k8s-ci-robot
Copy link
Contributor

@lingxiankong: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.18

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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 19, 2021

Build failed.

@lingxiankong
Copy link
Contributor Author

/test cloud-provider-openstack-acceptance-test-lb-octavia

@k8s-ci-robot
Copy link
Contributor

@lingxiankong: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-lb-octavia

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
Copy link
Contributor

@lingxiankong: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.18

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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 21, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 21, 2021

Build failed.

@lingxiankong
Copy link
Contributor Author

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

@k8s-ci-robot
Copy link
Contributor

@lingxiankong: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 21, 2021

Build failed.

@lingxiankong
Copy link
Contributor Author

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

@k8s-ci-robot
Copy link
Contributor

@lingxiankong: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cloud-provider-openstack-check
  • /test pull-cloud-provider-openstack-test

Use /test all to run all jobs.

In response to this:

/test cloud-provider-openstack-acceptance-test-e2e-conformance-stable-branch-v1.19

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.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 21, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 21, 2021

Build succeeded.

@lingxiankong
Copy link
Contributor Author

@jichenjc @ramineni please help review this PR

@ramineni
Copy link
Contributor

Thanks. changes looks ok to me .
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2021
@jichenjc
Copy link
Contributor

/approve
thanks~

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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:

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021
@k8s-ci-robot k8s-ci-robot merged commit 00b99a0 into kubernetes:master Mar 23, 2021
@lingxiankong lingxiankong deleted the fix-proxy-internal-access branch March 31, 2021 00:02
@multi-io
Copy link
Contributor

Looks like this breaks external-dns, which sees that status.loadBalancer.ingress[].hostname (of the service or of an ingress resource if you use an ingress controller with the service) now contains <IP>.nip.io instead of just <IP> and as a consequence deletes the A record (removing the service from DNS) and tries to create a CNAME record, which DNS APIs (only tested Openstack Designate for now, but probably true for others as well) reject because apparently RFC 1033 forbids CNAME records with the same name as other records (and external-dns already creates a TXT record with the same name).

time="2021-04-12T12:40:47Z" level=info msg="Creating records: ingresstest.scratch.io-multi.de./CNAME: 185.56.132.175.nip.io."
time="2021-04-12T12:40:47Z" level=info msg="Creating records: ingresstest.scratch.io-multi.de./TXT: \"heritage=external-dns,external-dns/owner=default,external-dns/resource=ingress/ingress-test/ingress-test\""
time="2021-04-12T12:40:48Z" level=info msg="Deleting records for ingresstest.scratch.io-multi.de./A"
time="2021-04-12T12:40:48Z" level=error msg="Bad request with: [POST https://designate.cloud.syseleven.net:9001/v2/zones/7b2f1706-e0d3-4bb3-8400-7ecc78eb463c/recordsets], error message: {\"message\": \"CNAME recordsets may not share a name with any other records\", \"code\": 400, \"type\": \"invalid_recordset_location\", \"request_id\": \"req-949d736b-1e8a-4e53-a423-3048c8da55ea\"}"

@lingxiankong
Copy link
Contributor Author

Hi @multi-io, I'm not familiar with external-dns, but if using external-dns, I would assume the pod is using dns (rather than external IP) to access the service, so without enabling the config enable-ingress-hostname, that would work, right?

@multi-io
Copy link
Contributor

multi-io commented Apr 14, 2021

Hi @lingxiankong, external-dns just connects to some external DNS service like AWS Route53 or Openstack Designate and sets up DNS records for your ingress hostnames, pointing to the LB IP. The pod looks up those names and they resolve to the LB IP, which the pod then tries to connect to, which will fail without the new enable-ingress-hostname config enabled. But with this config enabled, the status.loadBalancer.ingress[].hostname of a service or ingress resource now contains <IP>.nip.io instead of just <IP>, which causes external-DNS to try to create a CNAME record for <IP>.nip.io instead of just an A record for <IP>, and this breaks because the CNAME record clashes with a TXT record of the same name that external-dns also creates for internal purposes (storing some metadata). I've created a PR to external-DNS that circumvents this, see kubernetes-sigs/external-dns#2049 for more information.

@lingxiankong
Copy link
Contributor Author

Thanks for the detailed clarification, I understand now.

Yes, this workaround doesn't work properly with external-dns, to fully fix this issue, we have to wait for the k8s upstream solution. I will propose a PR to add something in the doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[occm] PROXY protocol breaks connections from inside the cluster
6 participants