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

Support NginxProxy CRD and global tracing settings #1870

Merged
merged 15 commits into from
Apr 29, 2024
Merged

Conversation

sjberman
Copy link
Contributor

Problem: As a user of NGF
I want to set the collection point for my traces for my installation of NGF So that I can ensure all my traces are sent to the same collection platform.

Solution: Implement the NginxProxy CRD which contains the fields required to configure the collection point for tracing. This resource is attached to the GatewayClass. If the resource is not found, a condition will be set on the GatewayClass to indicate this. The GatewayClass will continue to be Accepted even if the parametersRef is invalid.

This configuration sets the http context-level otel directives. The otel module is loaded conditionally based on the existence of this configuration.

Note: tracing is not enabled by this configuration, this only sets high level options. #1828 is required to actually enable tracing on a per-route basis.

Testing: Manually verified that the otel configuration is populated in the nginx.conf when appropriate. Conditions are also properly set on the GatewayClass.

Closes #1827
Closes #1775

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

NginxProxy CRD added to configure global settings (such as tracing endpoint) at the GatewayClass level.

@sjberman sjberman requested review from a team as code owners April 22, 2024 19:37
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart labels Apr 22, 2024
@sjberman
Copy link
Contributor Author

Local OSS build works, so not sure about the pipeline failure...

@pleshakov
Copy link
Contributor

@sjberman
looks like OTel module is not available for linux/ppc64le

@sjberman
Copy link
Contributor Author

sjberman commented Apr 22, 2024

@pleshakov Or 390x it seems. I guess we could ping the core team about building the module for those architectures, but who knows when that would actually happen...

We may just have to build it ourselves.

@pleshakov
Copy link
Contributor

We may just have to build it ourselves.

or drop those platforms...

@sjberman
Copy link
Contributor Author

@lucacome What do you think? Do we know if these architectures are popular enough to keep support for them right now?

We could open an issue on the otel-module repo to support these, and we remove support until that point.

@lucacome
Copy link
Member

It looks like it was a choice to only support amd64 and arm64 at least for the Docker image oxpa/docker-official-images@e23a9b5

I've decided to make a separate variant based on the main image instead
of extending it because the module build-depends on a fairly large chunk
of C++ code from multiple projects, which takes around 10 minutes to
compile and link on an 8-core amd64 machine. This is why it's currently
limited to amd64 and arm64v8, which nginx.org provides builds for.
Users can build them on less popular architectures as the instructions
are still provided in the dockerfiles.

Maybe they're going to add the other platforms in the future, we should probably talk to @thresheek

I'm not sure how popular these architectures are...do we have anything in our telemetry data?

build/Dockerfile.nginx Outdated Show resolved Hide resolved
@thresheek
Copy link

It looks like it was a choice to only support amd64 and arm64 at least for the Docker image oxpa/docker-official-images@e23a9b5

Maybe they're going to add the other platforms in the future, we should probably talk to @thresheek

I'm not sure how popular these architectures are...do we have anything in our telemetry data?

Indeed, this was an explicit choice not to overload docker library machines with compiling code on less-than-popular architectures...

I mean we can technically extend this list, but in reality I'd like to see real users/customers demand for otel on s390x or ppc64...

@thresheek
Copy link

thresheek commented Apr 23, 2024

Oh and we definitely didnt build any alpine ppc64le/s390x packages on nginx.org, too

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 99.24812% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.07%. Comparing base (c7fd089) to head (30df54e).

Files Patch % Lines
internal/mode/static/manager.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1870      +/-   ##
==========================================
- Coverage   86.12%   86.07%   -0.06%     
==========================================
  Files          83       85       +2     
  Lines        5586     5492      -94     
  Branches       50        0      -50     
==========================================
- Hits         4811     4727      -84     
+ Misses        726      716      -10     
  Partials       49       49              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.goreleaser.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
internal/mode/static/nginx/conf/nginx.conf Outdated Show resolved Hide resolved
internal/mode/static/state/change_processor.go Outdated Show resolved Hide resolved
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
conformance/provisioner/static-deployment.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/templates/rbac.yaml Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/configuration.go Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/types.go Outdated Show resolved Hide resolved
@sjberman sjberman mentioned this pull request Apr 25, 2024
5 tasks
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
docs/proposals/gateway-settings.md Show resolved Hide resolved
internal/mode/static/nginx/conf/nginx.conf Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/types.go Outdated Show resolved Hide resolved
charts/nginx-gateway-fabric/templates/deployment.yaml Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/configuration_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/types.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM docs-wise but the docs changes are minimal, so I will not give an approval.

Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

LGTM

Problem: As a user of NGF
I want to set the collection point for my traces for my installation of NGF
So that I can ensure all my traces are sent to the same collection platform.

Solution: Implement the NginxProxy CRD which contains the fields required to configure the collection point for tracing. This resource is attached to the GatewayClass. If the resource is not found, a condition will be set on the GatewayClass to indicate this. The GatewayClass will continue to be Accepted even if the parametersRef is invalid.

This configuration sets the `http` context-level otel directives. The otel module is loaded conditionally based on the existence of this configuration.

Note: tracing is not enabled by this configuration, this only sets high level options. #1828 is required to actually enable tracing on a per-route basis.
@sjberman sjberman enabled auto-merge (squash) April 29, 2024 21:56
@sjberman sjberman merged commit 7c3da8d into main Apr 29, 2024
40 checks passed
@sjberman sjberman deleted the enh/nginx-proxy-crd branch April 29, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart release-notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Implement Gateway Settings API [Enhancement Proposal] Gateway Settings Policy
6 participants