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

Modify LB IP with dns suffix when the proxy protocol is used #323

Merged
merged 2 commits into from
Feb 14, 2023

Conversation

@@ -7,3 +7,4 @@ application-credential-secret="${appcredsecret}"
[LoadBalancer]
manage-security-groups=true
use-octavia=true
enable-ingress-hostname=true
Copy link
Member

Choose a reason for hiding this comment

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

You have probably seen the logic in apply_nginx_ingress.sh to use kustomize to patch in the openstack annotations to enable the health monitor and the proxy protocol if NGINX_INGRESS_PROXY is set.
I would prefer to implement the hostname setting also via a conditional kustomization, so we only enable it when we need it. Looks like the annotation approach requires an explicit hostname though (and I don't know what it should be set to ...).
Are you sure this setting is harmless when the proxy-protocol is not enabled?

Copy link
Member

Choose a reason for hiding this comment

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

Does anyone know the answer to the "Are you sure this is harmless ... ?" question?

Copy link
Member

@garloff garloff Feb 8, 2023

Choose a reason for hiding this comment

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

Hmm, reading up kubernetes/ingress-nginx#3996, it would seem that an annotation loadbalancer.openstack.org/hostname: nip.io or the default setting you suggest would be needed until (KEP-1860)[https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1860-kube-proxy-IP-node-binding] gets implemented.
Any information on what enable-ingress-hostname=true would do when the proxy protocol is not enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

enable-ingress-hostname=true is used only when the annotation loadbalancer.openstack.org/proxy-protocol: "true". It is written here and I also found it in the code.

Copy link
Member

Choose a reason for hiding this comment

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

OK, looks like a safe choice then, thanks!

@garloff garloff self-assigned this Feb 2, 2023
@garloff garloff requested a review from jschoone February 2, 2023 12:12
@garloff garloff added bug Something isn't working Container Issues or pull requests relevant for Team 2: Container Infra and Tooling labels Feb 2, 2023
Copy link
Member

@garloff garloff left a comment

Choose a reason for hiding this comment

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

/lgtm

@Nils98Ar
Copy link
Member

It seems that the enhancement KEP-1860 will be included in K8s 1.29: kubernetes/enhancements#4114.

@chess-knight
Copy link
Member Author

chess-knight commented Oct 25, 2023

Yeah, it will be in the alpha stage. But still, probably OCCM will need to adapt to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Container Issues or pull requests relevant for Team 2: Container Infra and Tooling
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants