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

Replace http with https and fake cert in local VMs #896

Merged
merged 7 commits into from
Oct 2, 2023

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Sep 28, 2023

Most local machines had their own nginx config to use only http. But the lxc host was missing that config which led me to these tasks:

  • Create a common config for all local containers/VMs/hosts.
  • Use the standard nginx config with the snake oil certificate in local containers.
  • Get rid of all the duplicate config for the local containers.
  • Get rid of outdated development scripts because ofn-install is not for a development environment.

The config is smaller, easier to maintain and covers all local
containers. It also makes sure that we use the real production config
for Nginx and that the local configs don't get out of date.

It does mean that you need to accept an insecure certificate when you
access the website locally but then the connection is encrypted. This
also enables you to test browser behaviour around redirects to https and
security settings only available with https.
We state in the Readme that ofn-install is meant for setting up staging
and production servers. The openfoodnetwork repository has instructions
for development setups.

Our deploy scripts assume staging or production and fail if the
environment is set to development. It makes the code easier.
It can be the same for every local host. I also updated some values to
be the same as in the openfoodnetwork development environment.
@mkllnk mkllnk self-assigned this Sep 28, 2023
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great cleanup! All the updates make sense to me, I love seeing less code, doing the same thing 🤩

I have a couple of minor comment suggestions, but it's otherwise all good

admin_email: [email protected]
mail_domain: example.com

# Add missing vars to emulate /local_vagrant/secrets.yml
Copy link
Member

Choose a reason for hiding this comment

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

I think the exact path of secrets.yml is unimportant here:

Suggested change
# Add missing vars to emulate /local_vagrant/secrets.yml
# Add missing vars to emulate secrets.yml

@@ -18,3 +18,21 @@ developer_email: [email protected]

users_sysadmin:
- "{{ core_devs }}"

# Test host configuration
Copy link
Member

Choose a reason for hiding this comment

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

Seeing as the whole section below is pertaining to test/local hosts, maybe we can make this look more like section comment:

Suggested change
# Test host configuration
# *** Test host configuration ***

(I'm not sure if we have another convention in this codebase already, or you have a different preference. If so pls update!)

Copy link
Contributor

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning that up 🙏 ! I remember having to fix some configuration when I used vagrant to test the new relic setup, but I never had the chance to go back apply my changes. Anyway that's miles better that what I would have done 😄

@rioug
Copy link
Contributor

rioug commented Sep 29, 2023

I cancelled the checks as it wants to use Ubuntu 18 runners which I believe github doesn't offer any more.
@mkllnk you might want to address David's comment, otherwise it's ready to merge.

@mkllnk
Copy link
Member Author

mkllnk commented Oct 2, 2023

Thank you! I updated the comments. Merging.

@mkllnk mkllnk merged commit 3323f8a into openfoodfoundation:master Oct 2, 2023
@mkllnk mkllnk deleted the local branch October 2, 2023 00:57
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