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.

Sign up for GitHub

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

Jump to bottom

Watch for Secrets #807

Merged
merged 1 commit into from
Jul 7, 2023
Merged

Watch for Secrets #807

merged 1 commit into from
Jul 7, 2023

Conversation

pleshakov
Copy link
Contributor

Proposed changes

Problem:
NKG doesn't watch for updates of TLS Secrets referenced by Gateway resource.

Solution:

  • Move secrets processing into ChangeProcessor.
  • Introduce helper secretResolver component to resolve Secrets (includes validation) and capture resolved Secrets.
  • When building Gateway Listener, resolve Secrets using secretResolver.
  • When building Graph, add referenced Secrets by Gateway to the Graph, including the ones that don't exists.
  • When Upserting or Deleting a Secret to ChangeProccessor, use Graph to determine if the Secret is referenced by the Graph and thus changes the store.
  • When building Configuration, add all TLS Secrets to it referenced by valid TLS Listeners.
  • Update NGINX file.Manager so that it can deal with multiple files of two types: regular and secret.
  • Remove SecretStore and SecretDiskMemoryManager components.

Testing:

  • Update affected and add new unit tests
  • Manual testing
  • Conformance testing. Relevant tests pass: TestConformance/GatewayInvalidTLSConfiguration

Closes #553
Closes #441

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

@pleshakov pleshakov requested a review from a team as a code owner June 30, 2023 21:38
@github-actions github-actions bot added the enhancement New feature or request label Jun 30, 2023
sjberman
sjberman reviewed Jul 3, 2023
View reviewed changes
internal/events/handler.go Outdated Show resolved Hide resolved
internal/events/handler.go Outdated Show resolved Hide resolved
internal/nginx/config/generator.go Show resolved Hide resolved
internal/nginx/file/folders.go Outdated Show resolved Hide resolved
internal/nginx/file/folders.go Outdated Show resolved Hide resolved
internal/state/change_processor_test.go Outdated Show resolved Hide resolved
internal/state/dataplane/configuration_test.go Outdated Show resolved Hide resolved
internal/state/graph/gateway_test.go Outdated Show resolved Hide resolved
internal/state/graph/graph.go Show resolved Hide resolved
internal/state/graph/graph_test.go Outdated Show resolved Hide resolved
kate-osborn
kate-osborn requested changes Jul 5, 2023
View reviewed changes
internal/events/handler.go Show resolved Hide resolved
internal/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/nginx/file/file_suite_test.go Outdated Show resolved Hide resolved
internal/nginx/file/folders.go Outdated Show resolved Hide resolved
internal/nginx/file/folders.go Outdated Show resolved Hide resolved
internal/nginx/file/manager_test.go Show resolved Hide resolved
internal/state/change_processor.go Outdated Show resolved Hide resolved
internal/state/dataplane/configuration.go Show resolved Hide resolved
internal/state/graph/graph.go Outdated Show resolved Hide resolved
internal/state/store.go Show resolved Hide resolved
This was referenced Jul 6, 2023
Open
Closed
Closed
@pleshakov
Copy link
Contributor Author

not resolving conflicts yet, so that those changes don't clutter the changes based on the feedback so far

kate-osborn
kate-osborn approved these changes Jul 7, 2023
View reviewed changes
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.

🚀

@pleshakov
Watch for Secrets
0262e1d 
Problem:
NKG doesn't watch for updates of TLS Secrets referenced by Gateway
resource.

Solution:
- Move secrets processing into ChangeProcessor.
- Introduce helper secretResolver component to resolve Secrets (includes
validation) and capture resolved Secrets.
- When building Gateway Listener, resolve Secrets using secretResolver.
- When building Graph, add referenced Secrets by Gateway to the Graph,
including the ones that don't exists.
- When Upserting or Deleting a Secret to ChangeProccessor, use Graph
to determine if the Secret is referenced by the Graph and thus changes
the store.
- When building Configuration, add all TLS Secrets to it referenced
by _valid_ TLS Listeners.
- Update NGINX file.Manager so that it can deal with multiple files
of two types: regular and secret.
- Remove SecretStore and SecretDiskMemoryManager components.

Solves nginxinc#553
Solves nginxinc#441

Testing:
- Update affected and add new unit tests
- Manual testing
- Conformance testing. Relevant tests pass:
TestConformance/GatewayInvalidTLSConfiguration
@pleshakov pleshakov merged commit 890fddb into nginxinc:main Jul 7, 2023
16 checks passed
@pleshakov pleshakov deleted the feature/watch-for-secrets branch July 7, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjberman sjberman sjberman approved these changes

@kate-osborn kate-osborn kate-osborn approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
NGINX Gateway Fabric
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dynamic secrets/certificate rotation Refactor how we process Secrets
3 participants
@pleshakov @sjberman @kate-osborn