Skip to content

Commit

Permalink
Code review; watch for secret updates
Browse files Browse the repository at this point in the history
  • Loading branch information
sjberman committed Nov 6, 2024
1 parent 6ac55e0 commit 8f676d9
Show file tree
Hide file tree
Showing 34 changed files with 706 additions and 285 deletions.
6 changes: 3 additions & 3 deletions charts/nginx-gateway-fabric/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -268,10 +268,10 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri
| `nginx.image.tag` | | string | `"edge"` |
| `nginx.lifecycle` | The lifecycle of the nginx container. | object | `{}` |
| `nginx.plus` | Is NGINX Plus image being used | bool | `false` |
| `nginx.usage.caSecretName` | The name of the Secret containing the CA cert for verifying the NGINX Plus usage reporting server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` |
| `nginx.usage.clientSSLSecretName` | The name of the Secret containing the client cert/key for communicating with the NGINX Plus usage reporting server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` |
| `nginx.usage.caSecretName` | The name of the Secret containing the CA certificate for verifying the NGINX Plus usage reporting server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` |
| `nginx.usage.clientSSLSecretName` | The name of the Secret containing the client certificate/key for communicating with the NGINX Plus usage reporting server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` |
| `nginx.usage.endpoint` | The endpoint of the NGINX Plus usage reporting server. Default: product.connect.nginx.com | string | `""` |
| `nginx.usage.resolver` | The resolver domain name or IP address with optional port for resolving the endpoint. | string | `""` |
| `nginx.usage.resolver` | The nameserver used to resolve the NGINX Plus usage reporting endpoint. Used with NGINX Instance Manager. | string | `""` |
| `nginx.usage.secretName` | The name of the Secret containing the JWT for NGINX Plus usage reporting. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `"nplus-license"` |
| `nginx.usage.skipVerify` | Disable client verification of the NGINX Plus usage reporting server certificate. | bool | `false` |
| `nginxGateway.config.logging.level` | Log level. | string | `"info"` |
Expand Down
7 changes: 3 additions & 4 deletions charts/nginx-gateway-fabric/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,18 @@ data:
{{- if .Values.nginx.plus }}
mgmt.conf: |
mgmt {
license_token /etc/nginx/license/license.jwt;
{{- if .Values.nginx.usage.endpoint }}
usage_report endpoint={{ .Values.nginx.usage.endpoint }};
{{- end }}
{{- if .Values.nginx.usage.skipVerify }}
ssl_verify off;
{{- end }}
{{- if .Values.nginx.usage.caSecretName }}
ssl_trusted_certificate /etc/nginx/usage-certs/ca/ca.crt;
ssl_trusted_certificate /etc/nginx/certs-bootstrap/ca.crt;
{{- end }}
{{- if .Values.nginx.usage.clientSSLSecretName }}
ssl_certificate /etc/nginx/usage-certs/client/tls.crt;
ssl_certificate_key /etc/nginx/usage-certs/client/tls.key;
ssl_certificate /etc/nginx/certs-bootstrap/tls.crt;
ssl_certificate_key /etc/nginx/certs-bootstrap/tls.key;
{{- end }}
enforce_initial_report off;
}
Expand Down
34 changes: 17 additions & 17 deletions charts/nginx-gateway-fabric/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,12 @@ spec:
{{- if .Values.nginx.plus }}
{{- if .Values.nginx.usage.secretName }}
- name: nginx-plus-license
mountPath: /etc/nginx/license
mountPath: /etc/nginx/license.jwt
subPath: license.jwt
{{- end }}
{{- if .Values.nginx.usage.clientSSLSecretName }}
- name: usage-client-ssl-secret
mountPath: /etc/nginx/usage-certs/client
{{- end }}
{{- if .Values.nginx.usage.caSecretName }}
- name: usage-ca-secret
mountPath: /etc/nginx/usage-certs/ca
{{- if or .Values.nginx.usage.caSecretName .Values.nginx.usage.clientSSLSecretName }}
- name: nginx-plus-usage-certs
mountPath: /etc/nginx/certs-bootstrap/
{{- end }}
{{- end }}
{{- with .Values.nginx.extraVolumeMounts -}}
Expand Down Expand Up @@ -294,15 +291,18 @@ spec:
secret:
secretName: {{ .Values.nginx.usage.secretName }}
{{- end }}
{{- if .Values.nginx.usage.clientSSLSecretName }}
- name: usage-client-ssl-secret
secret:
secretName: {{ .Values.nginx.usage.clientSSLSecretName }}
{{- end }}
{{- if .Values.nginx.usage.caSecretName }}
- name: usage-ca-secret
secret:
secretName: {{ .Values.nginx.usage.caSecretName }}
{{- if or .Values.nginx.usage.caSecretName .Values.nginx.usage.clientSSLSecretName }}
- name: nginx-plus-usage-certs
projected:
sources:
{{- if .Values.nginx.usage.caSecretName }}
- secret:
name: {{ .Values.nginx.usage.caSecretName }}
{{- end }}
{{- if .Values.nginx.usage.clientSSLSecretName }}
- secret:
name: {{ .Values.nginx.usage.clientSSLSecretName }}
{{- end }}
{{- end }}
{{- end }}
{{- with .Values.extraVolumes -}}
Expand Down
1 change: 1 addition & 0 deletions charts/nginx-gateway-fabric/templates/scc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ volumes:
- emptyDir
- secret
- configMap
- projected
users:
- {{ printf "system:serviceaccount:%s:%s" .Release.Namespace (include "nginx-gateway.serviceAccountName" .) }}
allowedCapabilities:
Expand Down
6 changes: 3 additions & 3 deletions charts/nginx-gateway-fabric/values.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -264,14 +264,14 @@
"properties": {
"caSecretName": {
"default": "",
"description": "The name of the Secret containing the CA cert for verifying the NGINX Plus usage reporting\nserver. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
"description": "The name of the Secret containing the CA certificate for verifying the NGINX Plus usage reporting\nserver. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
"required": [],
"title": "caSecretName",
"type": "string"
},
"clientSSLSecretName": {
"default": "",
"description": "The name of the Secret containing the client cert/key for communicating with the NGINX Plus usage reporting\nserver. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
"description": "The name of the Secret containing the client certificate/key for communicating with the NGINX Plus usage reporting\nserver. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
"required": [],
"title": "clientSSLSecretName",
"type": "string"
Expand All @@ -285,7 +285,7 @@
},
"resolver": {
"default": "",
"description": "The resolver domain name or IP address with optional port for resolving the endpoint.",
"description": "The nameserver used to resolve the NGINX Plus usage reporting endpoint. Used with NGINX Instance Manager.",
"required": [],
"title": "resolver",
"type": "string"
Expand Down
6 changes: 3 additions & 3 deletions charts/nginx-gateway-fabric/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -143,17 +143,17 @@ nginx:
# -- The endpoint of the NGINX Plus usage reporting server. Default: product.connect.nginx.com
endpoint: ""

# -- The resolver domain name or IP address with optional port for resolving the endpoint.
# -- The nameserver used to resolve the NGINX Plus usage reporting endpoint. Used with NGINX Instance Manager.
resolver: ""

# -- Disable client verification of the NGINX Plus usage reporting server certificate.
skipVerify: false

# -- The name of the Secret containing the CA cert for verifying the NGINX Plus usage reporting
# -- The name of the Secret containing the CA certificate for verifying the NGINX Plus usage reporting
# server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).
caSecretName: ""

# -- The name of the Secret containing the client cert/key for communicating with the NGINX Plus usage reporting
# -- The name of the Secret containing the client certificate/key for communicating with the NGINX Plus usage reporting
# server. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).
clientSSLSecretName: ""

Expand Down
4 changes: 3 additions & 1 deletion cmd/gateway/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,8 @@ func createStaticModeCommand() *cobra.Command {
"that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway).",
)

cmd.MarkFlagsRequiredTogether(plusFlag, usageReportSecretFlag)

cmd.Flags().Var(
&usageReportEndpoint,
usageReportEndpointFlag,
Expand All @@ -401,7 +403,7 @@ func createStaticModeCommand() *cobra.Command {
cmd.Flags().Var(
&usageReportResolver,
usageReportResolverFlag,
"The nameserver used to resolve the NGINX Plus usage reporting server name.",
"The nameserver used to resolve the NGINX Plus usage reporting endpoint. Used with NGINX Instance Manager.",
)

cmd.Flags().BoolVar(
Expand Down
18 changes: 18 additions & 0 deletions cmd/gateway/commands_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ func TestStaticModeCmdFlagValidation(t *testing.T) {
"--nginx-plus",
"--usage-report-secret=my-secret",
"--usage-report-endpoint=example.com",
"--usage-report-resolver=resolver.com",
"--usage-report-ca-secret=ca-secret",
"--usage-report-client-ssl-secret=client-secret",
"--snippets-filters",
Expand Down Expand Up @@ -344,6 +345,23 @@ func TestStaticModeCmdFlagValidation(t *testing.T) {
expectedErrPrefix: `invalid argument "$*(invalid)" for "--usage-report-endpoint" flag: ` +
`"$*(invalid)" must be a domain name or IP address with optional port`,
},
{
name: "usage-report-resolver is set to empty string",
args: []string{
"--usage-report-resolver=",
},
wantErr: true,
expectedErrPrefix: `invalid argument "" for "--usage-report-resolver" flag: must be set`,
},
{
name: "usage-report-resolveris an invalid endpoint",
args: []string{
"--usage-report-resolver=$*(invalid)",
},
wantErr: true,
expectedErrPrefix: `invalid argument "$*(invalid)" for "--usage-report-resolver" flag: ` +
`"$*(invalid)" must be a domain name or IP address with optional port`,
},
{
name: "usage-report-ca-secret is set to empty string",
args: []string{
Expand Down
4 changes: 2 additions & 2 deletions deploy/experimental-nginx-plus/deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ data:
error_log stderr info;
mgmt.conf: |
mgmt {
license_token /etc/nginx/license/license.jwt;
enforce_initial_report off;
}
kind: ConfigMap
Expand Down Expand Up @@ -323,8 +322,9 @@ spec:
name: nginx-lib
- mountPath: /etc/nginx/includes
name: nginx-includes
- mountPath: /etc/nginx/license
- mountPath: /etc/nginx/license.jwt
name: nginx-plus-license
subPath: license.jwt
initContainers:
- command:
- /usr/bin/gateway
Expand Down
4 changes: 2 additions & 2 deletions deploy/nginx-plus/deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ data:
error_log stderr info;
mgmt.conf: |
mgmt {
license_token /etc/nginx/license/license.jwt;
enforce_initial_report off;
}
kind: ConfigMap
Expand Down Expand Up @@ -317,8 +316,9 @@ spec:
name: nginx-lib
- mountPath: /etc/nginx/includes
name: nginx-includes
- mountPath: /etc/nginx/license
- mountPath: /etc/nginx/license.jwt
name: nginx-plus-license
subPath: license.jwt
initContainers:
- command:
- /usr/bin/gateway
Expand Down
1 change: 1 addition & 0 deletions deploy/openshift/deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -429,3 +429,4 @@ volumes:
- emptyDir
- secret
- configMap
- projected
4 changes: 2 additions & 2 deletions deploy/snippets-filters-nginx-plus/deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ data:
error_log stderr info;
mgmt.conf: |
mgmt {
license_token /etc/nginx/license/license.jwt;
enforce_initial_report off;
}
kind: ConfigMap
Expand Down Expand Up @@ -320,8 +319,9 @@ spec:
name: nginx-lib
- mountPath: /etc/nginx/includes
name: nginx-includes
- mountPath: /etc/nginx/license
- mountPath: /etc/nginx/license.jwt
name: nginx-plus-license
subPath: license.jwt
initContainers:
- command:
- /usr/bin/gateway
Expand Down
4 changes: 2 additions & 2 deletions internal/mode/static/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,15 @@ type ProductTelemetryConfig struct {
type UsageReportConfig struct {
// SecretName is the name of the Secret containing the server credentials.
SecretName string
// ClientSSLSecretName is the name of the Secret containing client cert/key.
// ClientSSLSecretName is the name of the Secret containing client certificate/key.
ClientSSLSecretName string
// CASecretName is the name of the Secret containing the CA certificate.
CASecretName string
// Endpoint is the endpoint of the reporting server.
Endpoint string
// Resolver is the nameserver for resolving the Endpoint.
Resolver string
// SkipVerify controls whether the nginx verifies the server cert.
// SkipVerify controls whether the nginx verifies the server certificate.
SkipVerify bool
}

Expand Down
49 changes: 48 additions & 1 deletion internal/mode/static/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/status"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/telemetry"
)

type handlerMetricsCollector interface {
Expand All @@ -51,8 +52,10 @@ type eventHandlerConfig struct {
serviceResolver resolver.ServiceResolver
// generator is the nginx config generator.
generator ngxConfig.Generator
// k8sClient is a Kubernetes API client
// k8sClient is a Kubernetes API client.
k8sClient client.Client
// k8sReader is a Kubernets API reader.
k8sReader client.Reader
// logLevelSetter is used to update the logging level.
logLevelSetter logLevelSetter
// eventRecorder records events for Kubernetes resources.
Expand All @@ -67,6 +70,8 @@ type eventHandlerConfig struct {
gatewayCtlrName string
// updateGatewayClassStatus enables updating the status of the GatewayClass resource.
updateGatewayClassStatus bool
// plus is whether or not we are running NGINX Plus.
plus bool
}

const (
Expand Down Expand Up @@ -168,6 +173,9 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
case state.EndpointsOnlyChange:
h.version++
cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
if err := h.setDeploymentCtx(ctx, &cfg); err != nil {
logger.Error(err, "error setting deployment context for usage reporting")
}

Check warning on line 178 in internal/mode/static/handler.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/handler.go#L177-L178

Added lines #L177 - L178 were not covered by tests

h.setLatestConfiguration(&cfg)

Expand All @@ -179,6 +187,9 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
case state.ClusterStateChange:
h.version++
cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
if err := h.setDeploymentCtx(ctx, &cfg); err != nil {
logger.Error(err, "error setting deployment context for usage reporting")
}

Check warning on line 192 in internal/mode/static/handler.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/handler.go#L191-L192

Added lines #L191 - L192 were not covered by tests

h.setLatestConfiguration(&cfg)

Expand Down Expand Up @@ -485,6 +496,42 @@ func getGatewayAddresses(
return gwAddresses, nil
}

// setDeploymentCtx sets the deployment context metadata for nginx plus reporting.
func (h *eventHandlerImpl) setDeploymentCtx(ctx context.Context, cfg *dataplane.Configuration) error {
if !h.cfg.plus {
return nil
}

podNSName := types.NamespacedName{
Name: h.cfg.gatewayPodConfig.Name,
Namespace: h.cfg.gatewayPodConfig.Namespace,
}

clusterInfo, err := telemetry.CollectClusterInformation(ctx, h.cfg.k8sReader)
if err != nil {
return fmt.Errorf("error getting cluster information")
}

Check warning on line 513 in internal/mode/static/handler.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/handler.go#L512-L513

Added lines #L512 - L513 were not covered by tests

replicaSet, err := telemetry.GetPodReplicaSet(ctx, h.cfg.k8sReader, podNSName)
if err != nil {
return fmt.Errorf("failed to get replica set for pod %v: %w", podNSName, err)
}

Check warning on line 518 in internal/mode/static/handler.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/handler.go#L517-L518

Added lines #L517 - L518 were not covered by tests

deploymentID, err := telemetry.GetDeploymentID(replicaSet)
if err != nil {
return fmt.Errorf("failed to get NGF deploymentID: %w", err)
}

Check warning on line 523 in internal/mode/static/handler.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/handler.go#L522-L523

Added lines #L522 - L523 were not covered by tests

cfg.DeploymentContext = dataplane.DeploymentContext{
Integration: "ngf",
ClusterID: clusterInfo.ClusterID,
ClusterNodeCount: clusterInfo.NodeCount,
InstallationID: deploymentID,
}

return nil
}

// GetLatestConfiguration gets the latest configuration.
func (h *eventHandlerImpl) GetLatestConfiguration() *dataplane.Configuration {
h.lock.Lock()
Expand Down
Loading

0 comments on commit 8f676d9

Please sign in to comment.