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

Improve SnippetsFilters telemetry collection #2702

Open
bjee19 opened this issue Oct 18, 2024 · 1 comment
Open

Improve SnippetsFilters telemetry collection #2702

bjee19 opened this issue Oct 18, 2024 · 1 comment
Labels
area/nginx-configuration Relates to nginx configuration backlog Currently unprioritized work. May change with user feedback or as the product progresses. bug Something isn't working

Comments

@bjee19
Copy link
Contributor

bjee19 commented Oct 18, 2024

Credit to @pleshakov , the telemetry collection of SnippetsFilters added in this PR has a few edge cases which lead to incorrect behavior. The issue comes up when parsing nginx directives and values. This is the current implementation of parsing Snippet values

func parseSnippetValueIntoDirectives(snippetValue string) []string {
	separatedDirectives := strings.Split(snippetValue, ";")
	directives := make([]string, 0, len(separatedDirectives))

	for _, directive := range separatedDirectives {
		// the strings.TrimSpace is needed in the case of multi-line NGINX Snippet values
		directive = strings.Split(strings.TrimSpace(directive), " ")[0]

		// splitting on the delimiting character can result in a directive being empty or a space/newline character,
		// so we check here to ensure it's not
		if directive != "" {
			directives = append(directives, directive)
		}
	}

	return directives
}

This current implementation is too lax on using the ; character as a separator for directives and leaves room for many edge cases to incorrectly get parsed.

Below are some examples:

Use of the map directive, which not only doesn't have the ; character at the end of the directive, but can have nested ; characters which are not directives.

input:

            {Namespace: "test", Name: "sf-1"}: {
                    Snippets: map[ngfAPI.NginxContext]string{
                            ngfAPI.NginxContextMain:               "worker_priority 0;",
                            ngfAPI.NginxContextHTTP:               "aio on; map $http_x $x {default 0; 'abc' 1;}",
                            ngfAPI.NginxContextHTTPServer:         "auth_delay 10s;",
                            ngfAPI.NginxContextHTTPServerLocation: "keepalive_time 10s;",
                    },

output:

          SnippetsFiltersDirectives: [
              "auth_delay-server",
              "aio-http",
              "keepalive_time-location",
              "worker_priority-main",
              "'abc'-http",
              "client_body_timeout-http",
              "map-http",
              "}-http",
              "allow-location",
              "worker_rlimit_core-main",
              "worker_rlimit_nofile-main",
              "ignore_invalid_headers-server",
          ],

Any example with ; included in the value.

proxy_set_header hello "myvalue;abc";

Comments in general, AND if ; is inside a comment

# this is a nasty; comment

@mpstefan mpstefan added bug Something isn't working area/nginx-configuration Relates to nginx configuration size/small Estimated to be completed within ~2 days refined Requirements are refined and the issue is ready to be implemented. labels Oct 21, 2024
@bjee19 bjee19 assigned bjee19 and unassigned bjee19 Oct 28, 2024
@sindhushiv sindhushiv added this to the v2.0.0 milestone Nov 5, 2024
@sindhushiv sindhushiv removed refined Requirements are refined and the issue is ready to be implemented. size/small Estimated to be completed within ~2 days labels Nov 5, 2024
@mpstefan mpstefan added the backlog Currently unprioritized work. May change with user feedback or as the product progresses. label Nov 18, 2024
@mpstefan mpstefan removed this from the v2.0.0 milestone Nov 18, 2024
@mpstefan
Copy link
Collaborator

Waiting to see what results we get in telemetry first. @kate-osborn May look at before then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/nginx-configuration Relates to nginx configuration backlog Currently unprioritized work. May change with user feedback or as the product progresses. bug Something isn't working
Projects
Status: 🆕 New
Development

No branches or pull requests

3 participants