Another attempt at #29550, which was reverted. Fallback is not happening
if cert provisioning fails _despite_ having the correct header. But with
the changes in this PR, since we'll listen on `:80`, fallback _will_
happen when cert provisioning fails due to incorrect domain
configuration.
We're also adding [Hurl](https://hurl.dev) based tests. They're not run
in any CI yet. That'll come in soon.
Defining custom domain as `https://example.com/` is invalid.
It should be just the domain, just `example.com`. But turns out a lot of
our users have the incorrect configuration, and our previous stack of
NGINX+Certbot was able to ignore this and serve without HTTPS. This PR
brings that behaviour back.
## Test performed
Have Appsmith running on an EC2 instance, and a domain `correct.com`
with an A-record pointed to this EC2 instance.
In the instance, we run Appsmith with `APPSMITH_CUSTOM_DOMAIN` set to
`wrong.com`. Caddy will obviously fail to provision the cert, and so we
expect it to accept connections on just HTTP.
So hitting `curl -i http://correct.com` produced a 200 with the HTML
response, and not a 308 with a redirect. Before the changes from this
PR, the same curl command produced a 308 with a redirect to
`https://correct.com`, which fails with a certificate error.
Next up, we run Appsmith with `APPSMITH_CUSTOM_DOMAIN` set to
`correct.com`. Caddy will succeed in provisioning a cert, and so we
expect HTTP URLs to be redirected to HTTPS.
So hitting `curl -i http://correct.com` produces a 308 redirect to
`http://correct.com` which then works fine, since Caddy now has the cert
for the domain.
We're setting the default value for `APPSMITH_ALLOWED_FRAME_ANCESTORS`
before we initialize env variables from `docker.env`. This make the
default value take a higher precedence over the value configured in
`docker.env`. And since the value in `docker.env` is the one configured
from Admin Settings, it feels like the value configured from the UI is
being ignored.
This fixes the problem by moving the check for this env variable to
_inside_ the reconfigure script, and so doesn't affect any env
variables.
Fixes#29114
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Performance Improvements**
- Enhanced logging capabilities to include memory footprint and context
details for better performance monitoring.
- **Configuration Updates**
- Increased the number of log file backups from 2 to 10, allowing for
more historical log retention.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Chores**
- Optimized auto-healing script by removing an unnecessary 60-second
delay.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- Introduced auto-healing functionality to automatically restart
unresponsive backend services.
- Added SSL configuration support for custom domains.
- **Chores**
- Implemented periodic backend service status checks.
- Enhanced startup scripts to support new auto-healing feature based on
environment configuration.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
I think the route precedence in Caddy is different when using `handle`
directive, vs when directly using the `error` directive.
This is causing the file `handle {` route, which is a catch-all route is
handling `/static/*` requests that don't have a corresponding file. This
handler however, doesn't respond with 404 status, it responds with 200
status for missing files, and render the `index.html` for our SPA
behaviour.
Now, the CDN we have on release.app.appsmith.com caches responses from
upstream when the status is 200. If it is 404, it won't cache and retry
next time. This is why it's essential that we respond with 404 for files
that don't exist, irrespective of the content of the response.
When the container is starting up, Caddy doesn't have all the
information yet, and may have responded with not-found for one of the
assets. But since this went out with 200 status, our CDN cached it, and
once the file _was_ available with Caddy, the CDN wouldn't retry ever.
This fix will ensure we get 404 status code for requests to `/static/*`
that point to files that don't exist.
Fixes#29114
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- Enhanced logging capabilities for better performance insights.
- **Improvements**
- Increased the number of log file backups to ensure more historical
data is preserved.
- **Documentation**
- Updated internal documentation to reflect new logging and performance
monitoring features.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR replaces NGINX and Certbot with Caddy.
1. Auto-HTTPS when custom domain is set, is handled by Caddy.
2. If past certs exist, that were provisioned by Certbot in older
Appsmith versions, we configure Caddy to make use of them. But this only
applies if the certs aren't already expired. If they're expired, point 1
applies.
3. If custom certs are provided in `ssl` folder, Caddy will be
configured to use them.
4. Incoming `Forwarded` header is not passed to any reverse proxies. So
redirect URL is correctly computed on Google Cloud Run.
5. All other route configurations are exactly as they are in NGINX
today.
Caddy configuration file is generated in the `caddy-reconfigure.mjs`
script, which will also reload Caddy with the new configuration.
## Description
This PR fixes the experience of Templates forking in self hosted
instances. And also for to Set up a process to keep the embedded DB up
to date with template db schemas.
We have removed the redirection of mockdb end point used in templates
App when forked in self hosted instance from localhost/internal postgres
db.
This also has a migration which is to make sure that none of existing
apps using the internal postgres does not break due to the removal of
redirection. The migration will make sure that existing self hosted
instances using the posgress db and has a datasource with mockdb end
point will be replaces with localhost.
#### PR fixes following issue(s)
Fixes https://github.com/appsmithorg/appsmith/issues/28924
#### Type of change
- Bug fix (non-breaking change which fixes an issue)
## Testing
#### How Has This Been Tested?
- [ ] Manual
#### Test Plan
> Add Testsmith test cases links that relate to this PR
>
>
#### Issues raised during DP testing
> Link issues raised during DP testing for better visiblity and tracking
(copy link from comments dropped on this PR)
>
>
>
## Checklist:
#### Dev activity
- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] PR is being merged under a feature flag
#### QA activity:
- [ ] [Speedbreak
features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-)
have been covered
- [ ] Test plan covers all impacted features and [areas of
interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-)
- [ ] Test plan has been peer reviewed by project stakeholders and other
QA members
- [ ] Manually tested functionality on DP
- [ ] We had an implementation alignment call with stakeholders post QA
Round 2
- [ ] Cypress test cases have been added and approved by SDET/manual QA
- [ ] Added `Test Plan Approved` label after Cypress tests were reviewed
- [ ] Added `Test Plan Approved` label after JUnit tests were reviewed
---------
Co-authored-by: Shrikant Sharat Kandula <shrikant@appsmith.com>
Fix issue with alias names clashing in `keytool -import` command, when
there's more than one cert file in the `ca-certs` folder.
The fix is to explicitly set the alias for each `keytool -import` run,
to the file itself, so clashes don't happen.
Running an Appsmith as a non-root user:
```sh
docker run --name appsmith --user 70:70
```
The `70:70` figures are the UID and GID respectively. It can mostly be
any number, safe to user figures are 70 to 79, or anything above 200 and
below 65000. The important bit, is that it shouldn't change on restart
or manual updates etc.
No product functionality should be affected when running as a non-root
user.
The current latest release at
https://github.com/adoptium/temurin17-binaries/releases/tag/jdk-17.0.9%2B9.1,
doesn't include binaries for Linux, and so the Java download step fails
in our base Docker image build. This PR fixes that.
Sidenote, had we not had this base image and application image
separation, this bug would've blocked all our CI pipelines and the whole
team. 🙂
The layers in the Dockerfile that depend on downloading large files from
external sources, doesn't have to run every day, or at every PR. We
tried using Docker's caching configuration, but it's not as reliable as
we'd have liked.
A separate base image lends us much more control over the how long we
cache the downloaded files and how often we redo this.
This PR only _adds_ the base image. It doesn't change anything in the
build of the existing Docker image. That'll happen once we have the base
images for `release` and `master` already present on DockerHub.
What are we solving here?
1. Installing Java in the `Dockerfile` by using Adoptium's package
repositories is fragile since they've started blocking some IP addresses
used by GitHub Actions runners. We see a message like this:
```
Failed to fetch
https://packages.adoptium.net/artifactory/deb/pool/main/t/temurin-17/temurin-17-jdk_17.0.8.1.0+1_amd64.deb
403 Forbidden [IP: 146.75.107.42 443]
```
We're seeing more and more cases of these and PRs are getting blocked.
2. Installing Java via `apt` also installs other packages like X11
libraries, that aren't really relevant to our usage of Java. Yet, these
packages are present in our Docker image, and are the source of several
CVEs to be reported by scanners on our Docker image.
3. This will give us control over trusted CA certificates, which we can
now perform under `$TMP`, which aligns with our move towards supporting
readonly root filesystem. Which is essentially not write to anything in
the Docker image at runtime, except for under `/tmp` and
`/appsmith-stacks`. This will help us move in that direction.
This removes ~70 medium/low severity CVEs reported on our Docker image,
by removing `build-essential` from being installed by default in the
Docker image.
We only need it when compiling Redis, which is needed on _some_ ARM
systems, that re configured with a page-size of greater than 4096. For
example, CentOS 8.
In the NGINX configuration we generate, we're redirecting _all_ HTTP
requests to HTTPS, when HTTPS is enabled. But the HTTP-01 challenge
works on port 80 and is getting redirected to 443.
This usually fine, as Let's Encrypt respects that redirect and completes
the challenge on port 443. But, if port 443 is blocked to outside
access, the cert renewal will fail. This PR fixes that.
Tested on a server with port 80 open and 443 closed to outside Internet.
Cert renewal fails without this PR's changes, and works with this PR's
changes.
This broke when we changed the way RTS stores version information. This
was never the right way to get the version in the `backup` command and
this PR fixes it, by getting the version from `info.json`.
Failure error:
```
Error: ENOENT: no such file or directory, open '/opt/appsmith/rts/version.js'
```
This fixes RTS build to use `esbuild`.
1. This means the whole `node_modules` won't need to be copied over to
the Docker image. There's unused insignifant _test_ files in there, that
don't add any value, but are causing irrelevant CVEs to be reported on
our Docker image. See example at
https://github.com/appsmithorg/appsmith-ee/pull/2349.
2. Much faster. Not that RTS build is our slow point, but still. Perhaps
we can move client to `esbuild` too. 🙂
## Why are we doing this?
The current method of loading RTS into the Docker image means that _all_
contents of _all_ dependencies are copied over. The whole
`node_modules`. But several of these packages include _test_ files too,
that aren't needed at runtime at all. One of such test files is creating
a false alert for a CVE on our Docker image. Has absolutely no relevance
and impact, but it's there.
To fix that, I [had to `rm -rf /opt/appsmith/rts/node_modules/*/test` in
the Docker
image](https://github.com/appsmithorg/appsmith-ee/pull/2349/files). This
felt very hacky, and very dirty. It felt like we're introducing more
debt and more duct tape around the current build process.
So, `esbuild`.
## Where is `esbuild` coming from?
We're using `esbuild` v0.18.20 only, while the latest is v0.19.3. We
need to update `design-system`'s storybook dependency, I think, to get a
more recent version of `esbuild`. I'm yet to figure this out and can use
some help. 🙂
When running with a custom `PORT` env variable, NGINX server will be
listening on this port. In the backend's startup script, `run-java.sh`,
we're checking for RTS being up or not, at `localhost`. So when the port
is not 80, then this will never succeed, because it'll be looking for
NGINX at the wrong port.
Instead, the fix here will make the backend startup script hit RTS
_directly_ on RTS server's own port, instead of going via NGINX. This
means it's independent of both the `PORT` env variable and the NGINX
server, and only dependent on RTS being up, which is really what we want
here.
When PostgreSQL starts, we see the following errors in the logs:
```
mkdir:
cannot create directory ‘/tmp/appsmith/postgres-stats’
: Permission denied
```
And then this over and over again:
```
postgres stdout | 2023-09-19 15:34:34.504 UTC [1759] LOG: could not open temporary statistics file "/tmp/appsmith/postgres-stats/global.tmp": No such file or directory
```
The problem is that in `postgres.conf`, we set `user=postgres`, which
doesn't have access to create things in `/tmp`.
This PR removes this configuration and lets the default be, which will
be a temp folders _under_ the data directory.
This is part of supporting running Appsmith with readonly root FS. This
moves the supervisord configuration, and runtime files, like the unix
socket file, and the PID file, to `$TMP`.
This is moving the cleanup script to delete old log files, from being a
cron job, to being a scheduled task in the backend server. Why?
1. We want to support running Appsmith with readonly root, which is a
request from security teams at enterprise companies.
2. Cron doesn't play nice. It wants to save a `.pid` file under
`/var/run`, so it fails to start when running with readonly root. This
is not configurable.
3. Since our use of cron is minimal only, we're moving away from it,
especially since the backend server is already capable of running
scheduled tasks.
4. This moves one job, there's still another. Based on experience from
this, we'll work on the other.
Another advantage to doing this is that since this job is now running in
the backend server, which has access to MongoDB and Redis, we can
coordinate when running as a cluster, that only _one_ backend is running
the clean up job. This is for much later though.
---------
Co-authored-by: Nidhi <nidhi@appsmith.com>
This is another step towards supporting running with readonly root FS,
and only making runtime changes in the container in `/tmp` or in
`/appsmith-stacks`, and nowhere else.
