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

fix(*): expose Bazel INSTALL_DESTDIR to Lua code and fix admin_gui in dev [KM-117] #12774

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sumimakito
Copy link
Member

@sumimakito sumimakito commented Mar 22, 2024

Summary

Exposing INSTALL_DESTDIR in Bazel build via an auto-generated Lua module kong.kong_usr_path.

This fixes an issue where Admin GUI is not correctly prepared for the configured prefix while developing with Bazel's vdev environment.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

KM-117

@github-actions github-actions bot added core/cli cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Mar 22, 2024
@sumimakito sumimakito requested review from fffonion and nekolab March 22, 2024 11:08
@@ -828,7 +828,8 @@ local function prepare_prefix(kong_config, nginx_custom_template_path, skip_writ
end

if kong_config.admin_gui_listeners then
prepare_prefixed_interface_dir("/usr/local/kong", "gui", kong_config)
-- use LIBRARY_PREFIX as an alternative to /usr/local/kong in the Bazel dev environment
prepare_prefixed_interface_dir(os.getenv("LIBRARY_PREFIX") or "/usr/local/kong", "gui", kong_config)
Copy link
Contributor

@fffonion fffonion Mar 24, 2024

Choose a reason for hiding this comment

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

I'm slightly worried about the use of LIBRARY_PREFIX here as it's currently only used by dev scripts but now it could has effect in production as well. Some idea to improve it:

  • Prefix it with KONG_ to avoid surprises. We will then need to set the new env var in our dev script too.
  • Use a hardcoded path when a env var indicating we are in dev env, instead using directly from the value of LIBRARY_PREFIX. This could help us prevent path traversal even if malicious user injected this env var in production.

Copy link
Member Author

@sumimakito sumimakito Mar 24, 2024

Choose a reason for hiding this comment

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

@fffonion I agree with you. I'm converting this PR into a draft and will refactor it. Meanwhile, do we have best practices (e.g., env vars, helpers) to tell if we are in the dev env?

Update: It appears that we cannot hardcode the venv path as it is subject to user overrides on BUILD_NAME.

Copy link
Contributor

@fffonion fffonion Mar 25, 2024

Choose a reason for hiding this comment

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

We can use BUILD_NAME and use that to construct path. Or it just occurs to me that we can also produce a lua file to populate the prefix, and require that from the file that you change in this PR. So you don't need to rely on the
fact that BUILD_NAME or some other vars are populated, which is done by the venv scripts/may not work in other tooling.

It can be a simple genrule that produce a lua file, something like

genrule(
    name = "kong_path_config",
    outs = ["path/to/the/kong_path_config.lua"],
    cmd = "echo 'return \"%s/kong\"' >$@" %  KONG_VAR["INSTALL_DESTDIR"],
)

then in lua we can just

local the_path = require "path.to.the.kong_path_config"

Copy link
Contributor

Choose a reason for hiding this comment

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

Or to take a completely different path, that to start using of datafile to ship admin_gui assets, like the saml plugin did for xml assets would also be a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I've appended the echo command to the kong target which would be more straightforward.

@sumimakito sumimakito marked this pull request as draft March 24, 2024 15:08
@sumimakito sumimakito force-pushed the fix/dev-cmd-admin-gui-prepare branch 2 times, most recently from 29fb13a to adee7e0 Compare March 26, 2024 08:08
@sumimakito sumimakito marked this pull request as ready for review March 26, 2024 08:08
@sumimakito sumimakito changed the title fix(cmd): prepare admin_gui in Bazel dev env fix(*): expose Bazel INSTALL_DESTDIR to Lua code and fix admin_gui in dev Mar 26, 2024
@sumimakito sumimakito force-pushed the fix/dev-cmd-admin-gui-prepare branch from adee7e0 to 92dec88 Compare March 27, 2024 03:36
@sumimakito sumimakito force-pushed the fix/dev-cmd-admin-gui-prepare branch 2 times, most recently from f9fc2cf to beca877 Compare April 8, 2024 09:57
@sumimakito sumimakito changed the title fix(*): expose Bazel INSTALL_DESTDIR to Lua code and fix admin_gui in dev fix(*): expose Bazel INSTALL_DESTDIR to Lua code and fix admin_gui in dev [KM-117] Apr 18, 2024
@sumimakito sumimakito force-pushed the fix/dev-cmd-admin-gui-prepare branch from beca877 to fcb3888 Compare April 18, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/bazel cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/cli size/S skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants