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: avoid config.yaml modification in standalone mode due to read only file system. #558

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

TheShubhendra
Copy link
Contributor

  • Removed modification of config.yaml in standalone mode.
  • Added checks to make sure it uses data plane with yaml config provider.
  • Added separate config file for standalone mode example.

…in standalone mode.

- Removed direct file modifications in read-only environments
- Added checks for required fields in config.yaml for standalone mode
- Ensure the script exits with an error if the required fields are missing
@TheShubhendra
Copy link
Contributor Author

This PR will close #541 , #547 and #557 .

@TheShubhendra
Copy link
Contributor Author

Can you please review @kayx23

@shreemaan-abhishek
Copy link
Contributor

the CI is not running, please push an empty commit

@TheShubhendra
Copy link
Contributor Author

the CI is not running, please push an empty commit

@shreemaan-abhishek I have pushed an empty commit but CI is still not running.

@TheShubhendra
Copy link
Contributor Author

TheShubhendra commented Jul 24, 2024

I had tested workflow apisix dashboard all in one docker on my local system, it works fine after 2ef539f .
So I had modified the workflow file in my fork that shows it is not able to run the container to due to segmentation fault as mentioned in this job run https://github.com/TheShubhendra/apisix-docker/actions/runs/10062135660/job/27813891259 .

@TheShubhendra
Copy link
Contributor Author

Can someone please review?

@shreemaan-abhishek
Copy link
Contributor

@TheShubhendra, please check why is the dashboard CI failing. Thank you for your patience.

@TheShubhendra
Copy link
Contributor Author

TheShubhendra commented Aug 4, 2024

@TheShubhendra, please check why is the dashboard CI failing. Thank you for your patience.

As I mentioned before, the same workflow works on my PC, but it gives a segmentation fault on GitHub Actions. https://github.com/TheShubhendra/apisix-docker/actions/runs/10062135660/job/27813891259#:~:text=Segmentation%20fault%20(core%20dumped) This could be due to GitHub's resource limits or possibly another issue not related to apisix, I suppose?

@kayx23
Copy link
Member

kayx23 commented Aug 4, 2024

Re-running failed jobs.
image

@TheShubhendra
Copy link
Contributor Author

Why all actions are now failing with the same reason docker-compose: command not found

@TheShubhendra
Copy link
Contributor Author

TheShubhendra commented Aug 5, 2024

image

This issue seems to be from the main APISIX source code, not this repository.

Edit: My bad it uses local Dockerfile to build

@shreemaan-abhishek
Copy link
Contributor

@TheShubhendra, thanks for the fix. Please make this fix in another PR and then pull changes from master when the fix PR is merged. One PR for one thing. Thanks for your patience.

@TheShubhendra TheShubhendra marked this pull request as draft August 5, 2024 17:03
@TheShubhendra TheShubhendra force-pushed the master branch 2 times, most recently from 2ef539f to b6efaac Compare August 5, 2024 18:15
@shreemaan-abhishek
Copy link
Contributor

@TheShubhendra, please merge master branch to pull the fix.

echo "$(sed 's/role: traditional/role: data_plane/g' ${PREFIX}/conf/config.yaml)" > ${PREFIX}/conf/config.yaml
echo "$(sed 's/role_traditional:/role_data_plane:/g' ${PREFIX}/conf/config.yaml)" > ${PREFIX}/conf/config.yaml
echo "$(sed 's/config_provider: etcd/config_provider: yaml/g' ${PREFIX}/conf/config.yaml)" > ${PREFIX}/conf/config.yaml
# Check if the deployment role is set to data_plane and config provider is set to yaml for standalone mode
Copy link
Contributor

Choose a reason for hiding this comment

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

this part of code is being repeated across entrypoint scripts. Please DRY.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion. What we can do is extract the logic into a single file and place it in the root directory. Then, we can modify the Makefile to copy this file into the appropriate build context directories (e.g., /debian, /redhat, and /debian-dev) before the Docker build process, and remove it afterward.

Please share your thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#!/bin/bash

check_config() {
    local key="$1"
    local expected_value="$2"
    local error_message="$3"

    if ! grep -q "${key}: ${expected_value}" "${PREFIX}/conf/config.yaml"; then
        echo "Error: ${PREFIX}/conf/config.yaml does not contain '${key}: ${expected_value}'. ${error_message}"
        echo "Please refer to the APISIX documentation for deployment modes: https://apisix.apache.org/docs/apisix/deployment-modes/"
        exit 1
    fi
}

check_config "role" "data_plane" "Deployment role must be set to 'data_plane' for standalone mode."
check_config "role_data_plane" "" "Role must be set to 'role_data_plane:' for standalone mode."
check_config "config_provider" "yaml" "Config provider must be set to 'yaml' for standalone mode."

This file can be placed into /utils and then sourced into all entrypoint files.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@TheShubhendra TheShubhendra marked this pull request as ready for review August 14, 2024 10:44
@TheShubhendra
Copy link
Contributor Author

can someone please review this PR

Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time ordiscuss it on the [email protected] list. Thank you for your contributions.

@TheShubhendra
Copy link
Contributor Author

Any updates on this?

@hiddenmarten
Copy link

I would like to vote on this PR, it's really helpful and solves a lot of confusing issues in the current repository state.

@moonming moonming merged commit 3ec5547 into apache:master Dec 11, 2024
9 checks passed
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.

5 participants