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

Use internal address for admin API when appropriate #125

Merged
merged 2 commits into from
Apr 28, 2020

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Apr 24, 2020

What this PR does / why we need it:

If the admin service is disabled, but the admin listens are enabled (i.e. the configuration we expect for controller-managed instances), use 127.0.0.1 for the admin listen address to prevent access from outside the pod.

Additionally:

  • If a service's service is disabled, but its listens are enabled, do not render port information for that service in the Kong container template.
  • Listens now support specifying the address in general in the template, but currently only the admin API makes use of this, as I'm not aware of any situation where it's useful for the other services--although they support the same service disabled/listens enabled configuration, we shouldn't encounter that in the wild, and they don't have the same security concerns as the admin API.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the next branch and targets next, not master
  • Title of the PR and commit headers start with chart name (e.g. [kong])

If SVC.enabled=false, do not render ports for that service in the
deployment template.
Use 127.0.0.1 as the admin listen address if the admin service is
disabled. This is primarily for controller-managed deployments, where
the admin API should not be exposed outside the pod (removing it from
the deployment ports alone does not prevent this).

This changes adds generic support for specifying the listen address, but
does not use local listens for other services: generally, the admin API
is the only service we expect to encounter a disabled service with
enabled listens.
Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

I'm a little concerned about disabling exposing ports when there are no k8s service for a component (manager, portal etc). Could there be a case where someone doesn't want a k8s service but wants to expose the ports? While technically possible, the likelihood is extremely low given how these components interact.

Nice work with the patch and also the PR description. The description very clear and helped so much in understanding the changes.

@rainest
Copy link
Contributor Author

rainest commented Apr 28, 2020

Hard to say. Per https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#container-v1-core this is informational, and by that token they probably should be declared if the listen exists. The ports are always exposed if the listen exists, and removing this just kinda pretends they aren't.

That said, I have limited practical concerns about it:

  • At worst, it results in inaccurate display information. The port still functions normally.
  • For those services, there shouldn't be any actual reason to disable the service while keeping the listens, other than possibly in some embedded context where Kong lives within another app's pod.

@rainest rainest merged commit bfe2454 into next Apr 28, 2020
@hbagdi hbagdi deleted the feat/internal-admin branch April 28, 2020 18:04
rainest added a commit that referenced this pull request May 27, 2020
Use 127.0.0.1 as the admin listen address if the admin service is
disabled. This is primarily for controller-managed deployments, where
the admin API should not be exposed outside the pod (removing it from
the deployment ports alone does not prevent this).

This changes adds generic support for specifying the listen address, but
does not use local listens for other services: generally, the admin API
is the only service we expect to encounter a disabled service with
enabled listens.

If SVC.enabled=false, do not render ports for that service in the
deployment template.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants