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

[BUILD] fix docker configuration for the Quickstart application #90

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

Conversation

anthony-murphy-lrn
Copy link

Summary:

This PR addresses issues with the PHP SDK running within a Docker container for the Quickstart application, specifically related to domain whitelisting and API access. The following changes were made to resolve these issues:

Issues Addressed:

  1. Whitelisting Failure:
  • The PHP development server was previously set to bind to 0.0.0.0 to allow external connections. Since 0.0.0.0 is not a valid IP address, the Learnosity API calls failed as it was not whitelisted.
  1. Mismatch Between Server Address and Requesting IP:
  • After switching to 127.0.0.1 for local binding, the server configuration still led to a mismatch between the server’s address and the requesting IP address when running the command php -S $(LOCALHOST):8000. This mismatch caused signature security checks to fail.

Proposed Solution:

To resolve the issues:

  • A new Docker container configuration has been implemented with Nginx and PHP, where the server name is explicitly set to localhost. This ensures:
    • The Quickstart application’s API requests pass through the domain whitelist check.
    • The server and requesting IP addresses match, allowing the security signature checks to pass.

Additionally, customers can still run the Quickstart application locally without Docker. This can be tested by setting LRN_SDK_NO_DOCKER=false, allowing the PHP server to run independently of Docker.

Acceptance Criteria:

  • The Docker container with both Nginx and PHP containers correctly binds to localhost, the Quickstart application passes both whitelisting and security signature checks.
  • Customers should be able to run make quickstart locally, without Docker, and successfully start the PHP server serving the Quickstart application.

QA Testing:

  1. Run make quickstart with Docker installed.

    • Verify Docker containers start correctly and serve the Quickstart app without errors.
    • Ensure whitelisting and security checks pass.
  2. Run make quickstart without Docker (using a local PHP server).

    • Ensure PHP and Composer are installed locally.
    • Run the command make LRN_SDK_NO_DOCKER=false quickstart and verify that the PHP server starts and serves the Quickstart app without errors.

Checklist

  • Feature

  • Bug

  • ChangeLog.md updated

  • Tests added

  • All testsuite passes

  • make dist completed successfully

@anthony-murphy-lrn anthony-murphy-lrn changed the title Lrn 45518/bug/php sdk docker setup quickstart api access [Build] fix docker configuration for the Quickstart application Nov 15, 2024
@anthony-murphy-lrn anthony-murphy-lrn changed the title [Build] fix docker configuration for the Quickstart application [BUILD] fix docker configuration for the Quickstart application Nov 15, 2024
Copy link
Contributor

@walsh-conor walsh-conor left a comment

Choose a reason for hiding this comment

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

LGTM

Makefile Outdated
Comment on lines 35 to 37
docker-targets = quickstart
$(docker-targets): docker-build
$(DOCKER_COMPOSE) run --rm php make -e MAKEFLAGS="$(MAKEFLAGS)" $@
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule conflicts with a later one, so it gets overridden, except for the dependency on docker-build. Why not just make the later quickstart target depend on docker-build, if that is required? Either way, might as well remove this conditional section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the intention was to still run the old way, so you don't need to have PHP installed? But the dependency docker-build runs composer locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

It still works for me, even though I don't have PHP installed, because I have a composer that is a script that runs composer inside a container.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed these targets as they were based on the old docker logic and trying to run a php container to do the build. This lead to two containers being created.

Makefile Outdated
Comment on lines 59 to 64
quickstart:
ifneq (,$(DOCKER))
$(MAKE) docker-quickstart
else
$(MAKE) local-quickstart
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to run these in a vanilla sub-make? The only effect seems to be to make you have to export your environment variable settings. What about

Suggested change
quickstart:
ifneq (,$(DOCKER))
$(MAKE) docker-quickstart
else
$(MAKE) local-quickstart
endif
quickstart: $(if $(DOCKER),docker-build) $(if $(DOCKER),docker,local)-quickstart

Or create more variables if the if syntax seems too much in situ.

Copy link
Author

Choose a reason for hiding this comment

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

Makefile updated

Makefile Outdated
Comment on lines 20 to 22
export PHP_VERSION
export DEBIAN_VERSION
export COMPOSER_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure why you need to export these explicitly.

Copy link
Author

Choose a reason for hiding this comment

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

Not needed, orphaned from some attempt to make the original docker container work with binding to 0.0.0.0 and running php natively within the conttainer

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.

3 participants