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

lenses/nginx.aug: Allow types block to be defined #844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tupyy
Copy link
Contributor

@tupyy tupyy commented Aug 27, 2024

This PR allow types block to be parsed.

Fixes: #834.

Signed-off-by: Cosmin Tupangiu [email protected]

This commit allow types block to be parsed.

Fixes: hercules-team#834.

Signed-off-by: Cosmin Tupangiu <[email protected]>
@georgehansper
Copy link
Member

Hello @tupyy

I'm starting with a bit of background here, just so you can follow my thinking

The basic idea used for the nginx lens is to use the "directive" as the path label
For example, an nginx directive like these

        error_page 404 /404.html;
        error_page 500 502 503 504 /50x.html;

would become an augeas paths like these

/files/etc/nginx/nginx.conf/http/server/error_page[1] = "404 /404.html"
/files/etc/nginx/nginx.conf/http/server/error_page[2] = "500 502 503 504 /50x.html"

This works quite well in most cases, as nginx directives generally do not contain the path-seperator '/'

Where things get difficult is within "mapping" blocks, such as map or geo or types

For these blocks, the first string on the line is a string (or IP address) that might be contained in a variable.
The range of possible values for this variable is not restricted, and may contain a '/' character

In the case of the geo block, the value may be in CIDR notation, and the existing lens handles this by splitting the key at the '/' and creating a child node 'mask' eg.

/files/etc/nginx/conf.d/test.conf/server/geo
/files/etc/nginx/conf.d/test.conf/server/geo/#geo = "$remote_addr"
/files/etc/nginx/conf.d/test.conf/server/geo/192.168.1.0 = "1"
/files/etc/nginx/conf.d/test.conf/server/geo/192.168.1.0/mask = "24"

This works reasonably well, too, I think splitting the address like this makes it harder to read than the following (hypothetical):

/files/etc/nginx/conf.d/test.conf/server/geo/192.168.1.0/24 = "1"

I'm not suggesting that we change the existing behaviour in general, but we could create a specific case for the (new) types block
For example, the block:

types {
				text/plain   txt text log;
				text/csv     csv;
}

We could generate a tree that looks like this, specifically for the types block:

/files/etc/nginx/mime.types/types/mediatype[1] = "text/plain"
/files/etc/nginx/mime.types/types/extensions[1] = "txt text log"
/files/etc/nginx/mime.types/types/mediatype[2] = "text/csv"
/files/etc/nginx/mime.types/types/extensions[2] = "csv"

Or we could generate a tree that looks like this:

/files/etc/nginx/mime.types/types/text[1]/plain = "txt text log"
/files/etc/nginx/mime.types/types/text[2]/csv = "csv"

Getting to the specifics of this Pull Request, there are a couple issues
Firstly, the definition of sub_type_word needs to be updated, to possibly this

-let sub_type_work = /[A-Za-z.-]+[0-9_]*/
+let sub_type_word = ( /[A-Za-z0-9.-][A-Za-z0-9._+-]*/ - Rx.integer )

With that, the test will pass
FYI: I used the following command to run the individual test: rm tests/lens-nginx.sh.log ; time make -C tests lens-nginx.sh.log

But the lens still fails to load the file /etc/nginx/mime.types when it encounters the following line:

application/opc-nodeset+xml     ;

I am finding this difficult to resolve within the definition of the "simple" lens as it stands

So maybe it is best if we stop trying to use the "simple" definition for every kind of block

I don't like the idea of breaking backwards compatibility, but maybe we need to consider keeping backwards compatability only for the likely use-cases.
By that, I mean that the "mask" child node is only likely to appear in the "geo" block, and handle the other blocks differently

I have tried various things already, and I could not find a "one-size-fits-all" approach that actually worked

At the high level, what I am suggesting is

a) Keep the "simple" lens just for simple things - ie those without a '/' as the first word
b) Create a specific lens for the "types" block
c) Create a specific backwards compatible lens for the "geo" block
d) Do either (b) or (c) for the "map" block

What do you think?

@tupyy
Copy link
Contributor Author

tupyy commented Sep 9, 2024

Hi @georgehansper
My first approach was exactly b. But I thought I'd complicate my life.
I'll give it a try to implement b and c.
Thanks.

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.

The NGINX lens reports "types" as a syntax error incorrectly
2 participants