Skip to content

feat: nginx loadbalancing validation - #778

Draft
buchdag wants to merge 4 commits into
mainfrom
feat/loadbalance-validation
Draft

feat: nginx loadbalancing validation#778
buchdag wants to merge 4 commits into
mainfrom
feat/loadbalance-validation

Conversation

@buchdag

@buchdag buchdag commented Aug 8, 2026

Copy link
Copy Markdown
Member

No description provided.

@buchdag buchdag self-assigned this Aug 8, 2026
@buchdag
buchdag requested a balanced review from Copilot August 8, 2026 08:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds validation and normalization for NGINX load-balancing directives.

Changes:

  • Implements parsers for supported load-balancing methods.
  • Adds table-driven parser tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
internal/template/nginx_validation.go Adds load-balancing validation logic.
internal/template/nginx_validation_test.go Tests accepted and rejected directives.
Suppressed comments (1)

internal/template/nginx_validation.go:112

  • The NGINX hash directive requires a key (hash key [consistent]), so accepting the one-element case returns a bare hash directive that NGINX will reject when validating the generated configuration. Return an error when the key is missing and update the test that currently accepts this input.
	case 1:
		// no parameters

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/template/nginx_validation.go Outdated
Comment thread internal/template/nginx_validation.go Outdated
Comment thread internal/template/nginx_validation.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/template/nginx_validation.go:91

  • nginxMustParseLoadbalance uses strings.Split(value, " ") without trimming/field-normalizing first. This mis-parses directives with multiple spaces/tabs or leading/trailing whitespace (creating empty elements) and also prevents hash keys containing spaces from reaching nginxQuote as a single parameter (e.g. hash foo bar{} becomes 3 tokens and is rejected). Consider trimming first, using strings.Fields for method selection, and preserving the full remainder string for the hash key (with optional trailing consistent).
func nginxMustParseLoadbalance(value string) (string, error) {
	value = strings.TrimSuffix(value, ";")
	methodAndParameters := strings.Split(value, " ")
	method := strings.TrimSpace(methodAndParameters[0])

internal/template/nginx_validation_test.go:188

  • The new load-balance parser tests don’t cover common whitespace normalization cases (leading/trailing whitespace, multiple spaces) or a hash key containing spaces (which the implementation intends to support via nginxQuote). Adding a couple of cases here would help prevent regressions in the tokenizer/normalization logic.
		{
			name:      "trailing semicolon is normalized",
			input:     "least_conn;",
			wantValue: "least_conn",
		},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants