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

fix: rateLimitDeployment ignoring pod labels and annotation merge #4228

Merged
merged 19 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
84f6958
fix labels and annotation merges for rate limit deployment
oscarboher Sep 12, 2024
6e5f6a0
fix tests and label merge
oscarboher Sep 12, 2024
c0f680d
fix annotation merge if prometheus was disabled and annotations were …
oscarboher Sep 12, 2024
6a57aeb
renamed labels and annotations to specify they apply to pods only
oscarboher Sep 13, 2024
cd58565
Merge branch 'main' into fix/ratelimit-deployment-labels
oscarboher Sep 13, 2024
45e0263
linter
oscarboher Sep 13, 2024
cfe9bab
Merge branch 'fix/ratelimit-deployment-labels' of github.com:oscarboh…
oscarboher Sep 13, 2024
2db9a57
fix resource provider tests to new annotation behavior
oscarboher Sep 18, 2024
4b86c45
Merge branch 'main' into fix/ratelimit-deployment-labels
oscarboher Sep 18, 2024
138ecda
Merge branch 'main' into fix/ratelimit-deployment-labels
zirain Sep 19, 2024
92a2b13
go linter
oscarboher Sep 24, 2024
1d3f357
Merge branch 'fix/ratelimit-deployment-labels' of github.com:oscarboh…
oscarboher Sep 24, 2024
d831f49
fix gen-check
oscarboher Sep 25, 2024
e005c19
Merge branch 'main' into fix/ratelimit-deployment-labels
oscarboher Sep 25, 2024
a6b00f3
Merge branch 'main' into fix/ratelimit-deployment-labels
arkodg Sep 25, 2024
c3220c6
pod labels selector comment
oscarboher Sep 26, 2024
2e8a961
Merge branch 'fix/ratelimit-deployment-labels' of github.com:oscarboh…
oscarboher Sep 26, 2024
11e8504
Merge branch 'main' into fix/ratelimit-deployment-labels
oscarboher Sep 26, 2024
1eb1c07
Merge branch 'main' into fix/ratelimit-deployment-labels
oscarboher Sep 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions internal/infrastructure/kubernetes/ratelimit/resource_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
_ "embed"
"strconv"

egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
"github.com/envoyproxy/gateway/internal/infrastructure/kubernetes/resource"
"golang.org/x/exp/maps"
appsv1 "k8s.io/api/apps/v1"
autoscalingv2 "k8s.io/api/autoscaling/v2"
corev1 "k8s.io/api/core/v1"
Expand All @@ -17,9 +20,6 @@
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/ptr"

egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
"github.com/envoyproxy/gateway/internal/infrastructure/kubernetes/resource"
)

// ResourceKind indicates the main resources of envoy-ratelimit,
Expand Down Expand Up @@ -186,19 +186,28 @@
// Deployment returns the expected rate limit Deployment based on the provided infra.
func (r *ResourceRender) Deployment() (*appsv1.Deployment, error) {
containers := expectedRateLimitContainers(r.rateLimit, r.rateLimitDeployment, r.Namespace)
labels := rateLimitLabels()
selector := resource.GetSelector(labels)
selector := resource.GetSelector(rateLimitLabels())

var annotations map[string]string
podLabels := rateLimitLabels()
if r.rateLimitDeployment.Pod.Labels != nil {
maps.Copy(podLabels, r.rateLimitDeployment.Pod.Labels)
maps.Copy(podLabels, rateLimitLabels())
Copy link
Contributor

Choose a reason for hiding this comment

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

can L195 be removed ? since L192 is already setting it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So maps.Copy(dest, src) overwrites keys existing in both maps with the src value. I want to make sure noone can override one of the selector labels from the rateLimitLabels, so I'm doing the last maps.Copy to overwrite with the selector labels in case someone added one of those labels in their values.

Also the destination map must not be nil so I cannot do something like:

podLabels := r.rateLimitDeployment.Pod.Labels
maps.Copy(podLabels, rateLimitLabels())

as it will fail if no pod labels are provided.
It looks a bit funny, I just didn't want to implement a function that merges two maps, but maybe it's worth it for clarity's sake. Let me know if you think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah thats a good way to ensure the labels can't be overridden
nit: this as a code comment would be helpful so the next dev gets the "why"

}

var podAnnotations map[string]string
if enablePrometheus(r.rateLimit) {
annotations = map[string]string{
podAnnotations = map[string]string{
"prometheus.io/path": "/metrics",
"prometheus.io/port": strconv.Itoa(PrometheusPort),
"prometheus.io/scrape": "true",
}
}
if r.rateLimitDeployment.Pod.Annotations != nil {
annotations = r.rateLimitDeployment.Pod.Annotations
if podAnnotations != nil {
maps.Copy(podAnnotations, r.rateLimitDeployment.Pod.Annotations)
} else {
podAnnotations = r.rateLimitDeployment.Pod.Annotations

Check warning on line 209 in internal/infrastructure/kubernetes/ratelimit/resource_provider.go

View check run for this annotation

Codecov / codecov/patch

internal/infrastructure/kubernetes/ratelimit/resource_provider.go#L209

Added line #L209 was not covered by tests
}
}

deployment := &appsv1.Deployment{
Expand All @@ -208,16 +217,16 @@
},
ObjectMeta: metav1.ObjectMeta{
Namespace: r.Namespace,
Labels: labels,
Labels: rateLimitLabels(),
},
Spec: appsv1.DeploymentSpec{
Replicas: r.rateLimitDeployment.Replicas,
Strategy: *r.rateLimitDeployment.Strategy,
Selector: selector,
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: selector.MatchLabels,
Annotations: annotations,
Labels: podLabels,
arkodg marked this conversation as resolved.
Show resolved Hide resolved
Annotations: podAnnotations,
},
Spec: corev1.PodSpec{
Containers: containers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ import (
"flag"
"fmt"
"os"
"strconv"
"testing"

egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
"github.com/envoyproxy/gateway/internal/envoygateway/config"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
appsv1 "k8s.io/api/apps/v1"
Expand All @@ -22,9 +25,6 @@ import (
"k8s.io/utils/ptr"
gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
"sigs.k8s.io/yaml"

egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
"github.com/envoyproxy/gateway/internal/envoygateway/config"
)

var overrideTestData = flag.Bool("override-testdata", false, "if override the test output data.")
Expand Down Expand Up @@ -678,6 +678,50 @@ func TestDeployment(t *testing.T) {
},
},
},
{
caseName: "merge-labels",
rateLimit: &egv1a1.RateLimit{
Backend: egv1a1.RateLimitDatabaseBackend{
Type: egv1a1.RedisBackendType,
Redis: &egv1a1.RateLimitRedisSettings{
URL: "redis.redis.svc:6379",
},
},
},
deploy: &egv1a1.KubernetesDeploymentSpec{
Pod: &egv1a1.KubernetesPodSpec{
Labels: map[string]string{
"app.kubernetes.io/name": InfraName,
"app.kubernetes.io/component": "ratelimit",
"app.kubernetes.io/managed-by": "envoy-gateway",
"key1": "value1",
"key2": "value2",
},
},
},
},
{
caseName: "merge-annotations",
rateLimit: &egv1a1.RateLimit{
Backend: egv1a1.RateLimitDatabaseBackend{
Type: egv1a1.RedisBackendType,
Redis: &egv1a1.RateLimitRedisSettings{
URL: "redis.redis.svc:6379",
},
},
},
deploy: &egv1a1.KubernetesDeploymentSpec{
Pod: &egv1a1.KubernetesPodSpec{
Annotations: map[string]string{
"prometheus.io/path": "/metrics",
"prometheus.io/port": strconv.Itoa(PrometheusPort),
"prometheus.io/scrape": "true",
"key1": "value1",
"key2": "value2",
},
},
},
},
}
for _, tc := range cases {
t.Run(tc.caseName, func(t *testing.T) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ spec:
metadata:
annotations:
prometheus.io/scrape: "true"
prometheus.io/path: "/metrics"
prometheus.io/port: "19001"
creationTimestamp: null
labels:
app.kubernetes.io/component: ratelimit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ spec:
metadata:
annotations:
prometheus.io/scrape: "true"
prometheus.io/path: "/metrics"
prometheus.io/port: "19001"
creationTimestamp: null
labels:
app.kubernetes.io/component: ratelimit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ spec:
metadata:
annotations:
prometheus.io/scrape: "true"
prometheus.io/path: "/metrics"
prometheus.io/port: "19001"
creationTimestamp: null
labels:
app.kubernetes.io/component: ratelimit
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
apiVersion: apps/v1
kind: Deployment
metadata:
creationTimestamp: null
labels:
app.kubernetes.io/component: ratelimit
app.kubernetes.io/managed-by: envoy-gateway
app.kubernetes.io/name: envoy-ratelimit
name: envoy-ratelimit
namespace: envoy-gateway-system
ownerReferences:
- apiVersion: apps/v1
kind: Deployment
name: envoy-gateway
uid: test-owner-reference-uid-for-deployment
spec:
progressDeadlineSeconds: 600
revisionHistoryLimit: 10
selector:
matchLabels:
app.kubernetes.io/component: ratelimit
app.kubernetes.io/managed-by: envoy-gateway
app.kubernetes.io/name: envoy-ratelimit
strategy:
type: RollingUpdate
template:
metadata:
annotations:
prometheus.io/path: /metrics
prometheus.io/port: "19001"
prometheus.io/scrape: "true"
key1: value1
key2: value2
creationTimestamp: null
labels:
app.kubernetes.io/component: ratelimit
app.kubernetes.io/managed-by: envoy-gateway
app.kubernetes.io/name: envoy-ratelimit
spec:
automountServiceAccountToken: false
containers:
- command:
- /bin/ratelimit
env:
- name: RUNTIME_ROOT
value: /data
- name: RUNTIME_SUBDIRECTORY
value: ratelimit
- name: RUNTIME_IGNOREDOTFILES
value: "true"
- name: RUNTIME_WATCH_ROOT
value: "false"
- name: LOG_LEVEL
value: info
- name: USE_STATSD
value: "true"
- name: STATSD_PORT
value: "9125"
- name: CONFIG_TYPE
value: GRPC_XDS_SOTW
- name: CONFIG_GRPC_XDS_SERVER_URL
value: envoy-gateway:18001
- name: CONFIG_GRPC_XDS_NODE_ID
value: envoy-ratelimit
- name: GRPC_SERVER_USE_TLS
value: "true"
- name: GRPC_SERVER_TLS_CERT
value: /certs/tls.crt
- name: GRPC_SERVER_TLS_KEY
value: /certs/tls.key
- name: GRPC_SERVER_TLS_CA_CERT
value: /certs/ca.crt
- name: CONFIG_GRPC_XDS_SERVER_USE_TLS
value: "true"
- name: CONFIG_GRPC_XDS_CLIENT_TLS_CERT
value: /certs/tls.crt
- name: CONFIG_GRPC_XDS_CLIENT_TLS_KEY
value: /certs/tls.key
- name: CONFIG_GRPC_XDS_SERVER_TLS_CACERT
value: /certs/ca.crt
- name: FORCE_START_WITHOUT_INITIAL_CONFIG
value: "true"
- name: REDIS_SOCKET_TYPE
value: tcp
- name: REDIS_URL
value: redis.redis.svc:6379
image: envoyproxy/ratelimit:master
imagePullPolicy: IfNotPresent
name: envoy-ratelimit
ports:
- containerPort: 8081
name: grpc
protocol: TCP
readinessProbe:
failureThreshold: 1
httpGet:
path: /healthcheck
port: 8080
scheme: HTTP
periodSeconds: 5
successThreshold: 1
timeoutSeconds: 1
resources:
requests:
cpu: 100m
memory: 512Mi
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
privileged: false
readOnlyRootFilesystem: true
runAsGroup: 65534
runAsNonRoot: true
runAsUser: 65534
seccompProfile:
type: RuntimeDefault
startupProbe:
failureThreshold: 30
httpGet:
path: /healthcheck
port: 8080
scheme: HTTP
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
volumeMounts:
- mountPath: /certs
name: certs
readOnly: true
- command:
- /bin/statsd_exporter
- --web.listen-address=:19001
- --statsd.mapping-config=/etc/statsd-exporter/conf.yaml
image: prom/statsd-exporter:v0.18.0
imagePullPolicy: IfNotPresent
name: prom-statsd-exporter
ports:
- containerPort: 9125
name: statsd
protocol: TCP
- containerPort: 19001
name: metrics
protocol: TCP
resources:
requests:
cpu: 100m
memory: 512Mi
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
privileged: false
readOnlyRootFilesystem: true
runAsGroup: 65534
runAsNonRoot: true
runAsUser: 65534
seccompProfile:
type: RuntimeDefault
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
volumeMounts:
- mountPath: /etc/statsd-exporter
name: statsd-exporter-config
readOnly: true
dnsPolicy: ClusterFirst
restartPolicy: Always
schedulerName: default-scheduler
serviceAccountName: envoy-ratelimit
terminationGracePeriodSeconds: 300
volumes:
- name: certs
secret:
defaultMode: 420
secretName: envoy-rate-limit
- configMap:
defaultMode: 420
name: statsd-exporter-config
optional: true
name: statsd-exporter-config
status: {}
Loading
Loading