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

Add serverSnippets to TransportServer #1413

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Add serverSnippets to TransportServer #1413

merged 1 commit into from
Mar 11, 2021

Conversation

LorcanMcVeigh
Copy link
Contributor

@LorcanMcVeigh LorcanMcVeigh commented Feb 26, 2021

Proposed changes

This PR adds Snippet feature to the TransportServer resource.

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 master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Feb 26, 2021
internal/configs/transportserver_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

docs-web/configuration/transportserver-resource.md Outdated Show resolved Hide resolved
pkg/apis/configuration/v1alpha1/types.go Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/configurator.go Outdated Show resolved Hide resolved
internal/configs/transportserver_test.go Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
@ogarrett
Copy link

ogarrett commented Mar 1, 2021

Hi @LorcanMcVeigh

Shall we also add streamSnippets field that configures snippets in the stream context ? similar to http-snippets https://deploy-preview-1413--nginx-kubernetes-ingress.netlify.app/nginx-ingress-controller/configuration/virtualserver-and-virtualserverroute-resources/#using-snippets

I agree - there are some directives that can only be specified at the stream context (including geo, keyval and map) and there may be cases where it's convenient to apply a directive there so that all stream servers inherit it

@pleshakov
Copy link
Contributor

Hi @LorcanMcVeigh

the changes look good to me.

However, because of the future streamSnippets, we need to rename snippets to `serverSnippets, which would be similar to http and server snippets for VS :

apiVersion: k8s.nginx.org/v1alpha1
kind: TransportServer
metadata:
  name: dns-tcp
spec:
  streamSnippets: |
    limit_conn_zone $binary_remote_addr zone=addr:10m;
  listener:
    name: dns-tcp 
    protocol: TCP
  serverSnippets: |
    limit_conn addr 1;
    limit_conn_log_level error;
  upstreams:
  - name: dns-app
    service: coredns
    port: 5353
  action:
    pass: dns-app

docs-web/configuration/transportserver-resource.md Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
pkg/apis/configuration/v1alpha1/types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@soneillf5
the last change looks good! however, because of the change, there is some remaining code and logic, which is not really used - that's why a few comments below

internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

🚀

@soneillf5 soneillf5 merged commit 8fdde07 into master Mar 11, 2021
@soneillf5 soneillf5 deleted the ts-snippets branch March 11, 2021 16:55
@lucacome lucacome added the enhancement Pull requests for new features/feature enhancements label Mar 11, 2021
@pleshakov pleshakov changed the title Add snippet feature to TransportServer Add serverSnippets to TransportServer Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants