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 #1274 - various nginx batch improvements #1286

Merged
merged 7 commits into from
Mar 26, 2024
Merged

Fix #1274 - various nginx batch improvements #1286

merged 7 commits into from
Mar 26, 2024

Conversation

mxsasha
Copy link
Collaborator

@mxsasha mxsasha commented Feb 20, 2024

No description provided.

@mxsasha mxsasha self-assigned this Feb 20, 2024
@mxsasha mxsasha linked an issue Feb 20, 2024 that may be closed by this pull request
@mxsasha
Copy link
Collaborator Author

mxsasha commented Feb 20, 2024

@bwbroersma I'm having some trouble getting batch auth apply to /, conditional on the batch setting. I bet I'll figure it out eventually, but you might be a bit more familiar with it? The templating is also really limited. The changes in the last commit definitely do not work right.

@mxsasha mxsasha added the discuss Requires further team discussion and decisions label Mar 4, 2024
@bwbroersma
Copy link
Collaborator

@mxsasha sorry for the late reply:

  1. It's pretty tricky to use if correctly in nginx, see these examples 'why if is considered evil' in nginx location context, instead of if map can be used.
  2. env can be used directly if specified, which might be useful sometimes.
  3. Another option would be to use include, since the default nginx.conf is not overwritten, it already includes a include /etc/nginx/conf.d/*.conf; in the http-context, so it just works by adding a conf file, like authentication.sh is already doing.

I think BATCH and BASIC_AUTH should work similar, so I would go for option 3, but it's also an option to rewrite the BASIC_AUTH logic. It would be nice to add a hint about htpasswd and the link to authentication.sh, since it took me a few seconds the first time.

@mxsasha mxsasha added backport Fix to be backported and removed discuss Requires further team discussion and decisions labels Mar 7, 2024
@mxsasha mxsasha force-pushed the nginx-fixes branch 5 times, most recently from 5686208 to 8e137e2 Compare March 8, 2024 12:43
@mxsasha
Copy link
Collaborator Author

mxsasha commented Mar 8, 2024

@bwbroersma any clue what I am missing in 0c6e797 ?
I have confirmed that mime.types with my YAML line is in the container, and nginx.conf has include /etc/nginx/mime.types;

@mxsasha mxsasha force-pushed the nginx-fixes branch 3 times, most recently from e218824 to 6b1e70b Compare March 25, 2024 19:23
@mxsasha mxsasha merged commit e761d30 into main Mar 26, 2024
15 checks passed
@mxsasha mxsasha deleted the nginx-fixes branch March 26, 2024 08:20
mxsasha added a commit that referenced this pull request Mar 26, 2024
* The content-type for openapi.yaml must be application/yaml
* Security.txt and openapi.yaml are always accessible.
* We will have a single list of users and passwords in a htpasswd file, editable through the commands we recently added.
* There are three modes:
  * single test open (enable_batch=False, so no batch URLs available) has no auth at all
  * batch mode (enable_batch=True) has auth on all URLs except result reports
  * single test closed (auth_all_urls=True, enable_batch=False) has auth on all URLs.
* The settings BASIC_AUTH and BASIC_AUTH_RAW are removed.

(cherry picked from commit e761d30)
mxsasha added a commit that referenced this pull request Mar 26, 2024
* The content-type for openapi.yaml must be application/yaml
* Security.txt and openapi.yaml are always accessible.
* We will have a single list of users and passwords in a htpasswd file, editable through the commands we recently added.
* There are three modes:
  * single test open (enable_batch=False, so no batch URLs available) has no auth at all
  * batch mode (enable_batch=True) has auth on all URLs except result reports
  * single test closed (auth_all_urls=True, enable_batch=False) has auth on all URLs.
* The settings BASIC_AUTH and BASIC_AUTH_RAW are removed.

(cherry picked from commit e761d30)
@mxsasha mxsasha removed the backport Fix to be backported label Mar 26, 2024
mxsasha added a commit that referenced this pull request Mar 26, 2024
* The content-type for openapi.yaml must be application/yaml
* Security.txt and openapi.yaml are always accessible.
* We will have a single list of users and passwords in a htpasswd file, editable through the commands we recently added.
* There are three modes:
  * single test open (enable_batch=False, so no batch URLs available) has no auth at all
  * batch mode (enable_batch=True) has auth on all URLs except result reports
  * single test closed (auth_all_urls=True, enable_batch=False) has auth on all URLs.
* The settings BASIC_AUTH and BASIC_AUTH_RAW are removed.

(cherry picked from commit e761d30)
mxsasha added a commit that referenced this pull request Mar 26, 2024
mxsasha added a commit that referenced this pull request Mar 26, 2024
mxsasha added a commit that referenced this pull request Mar 26, 2024
mxsasha added a commit that referenced this pull request Mar 26, 2024
* The content-type for openapi.yaml must be application/yaml
* Security.txt and openapi.yaml are always accessible.
* We will have a single list of users and passwords in a htpasswd file, editable through the commands we recently added.
* There are three modes:
  * single test open (enable_batch=False, so no batch URLs available) has no auth at all
  * batch mode (enable_batch=True) has auth on all URLs except result reports
  * single test closed (auth_all_urls=True, enable_batch=False) has auth on all URLs.
* The settings BASIC_AUTH and BASIC_AUTH_RAW are removed.

(cherry picked from commit e761d30)
mxsasha added a commit that referenced this pull request Mar 26, 2024
@mxsasha mxsasha mentioned this pull request Jul 24, 2024
@mxsasha mxsasha mentioned this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Nginx batch issues
2 participants