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

Pass Redis URL as Env Var to Limitador deployment. #109

Merged
merged 6 commits into from
Nov 15, 2023

Conversation

Boomatang
Copy link
Contributor

@Boomatang Boomatang commented Oct 24, 2023

Closes: #106

This change pass the Redis URL to the limitador-server as a Environment Variable in the deployment config.

Verification

Verification steps requires Redis to be configured in the cluster so the running limitador does not have connection error logs.

Set up the kind cluster with this deployment.

make local-setup

Create redis deployment, service and secret. The secret will be used in the limitador deployment and should be created in the same namespace. See changes in docs about this.

echo "
apiVersion: v1
kind: Secret
metadata:
  name: redisconfig
stringData:
  URL: redis://redis.default.svc.cluster.local/0
type: Opaque
---
apiVersion: v1
kind: Service
metadata:
  name: redis
spec:
  ports:
  - port: 6379
    protocol: TCP
    targetPort: 6379
  selector:
    app: redis
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: redis
spec:
  replicas: 1
  selector:
    matchLabels:
      app: redis
  template:
    metadata:
      labels:
        app: redis
    spec:
      containers:
      - image: redis:latest
        name: redis
        ports:
        - containerPort: 6379
  " | kubectl apply -f -

Verify the redis connections work.

Deploy limitador with redis.

echo "
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  storage:
    redis:
      configSecretRef: 
        name: redisconfig
" | kubectl apply -f -

Once deployment is complete:

  • check the limitador deployment
kubectl get deployment limitador-sample -n default -o yaml

Expected output to contain

spec:
      containers:
      - command:
        - limitador-server
        - /home/limitador/etc/limitador-config.yaml
        - redis
        - $(LIMITIADOR_OPERATOR_REDIS_URL)
        env:
        - name: LIMITIADOR_OPERATOR_REDIS_URL
          valueFrom:
            secretKeyRef:
              key: URL
              name: redisconfig

Expect logs not to contain any errors

kubectl logs deployment/limitador-sample -n default

Verify the rediscache connections work.

Deploy limitador with rediscache.

echo "
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  storage:
    redis-cached:
      configSecretRef: 
        name: redisconfig
      options:
        ttl: 1000
        max-cached: 5000
" | kubectl apply -f -

Once deployment is complete:

  • check the limitador deployment
kubectl get deployment limitador-sample -n default -o yaml

Expected output to contain

spec:
   containers:
      - command:
        - limitador-server
        - /home/limitador/etc/limitador-config.yaml
        - redis_cached
        - $(LIMITIADOR_OPERATOR_REDIS_URL)
        - --ttl
        - "1000"
        - --max-cached
        - "5000"
        env:
        - name: LIMITIADOR_OPERATOR_REDIS_URL
          valueFrom:
              secretKeyRef:
                 key: URL
                 name: redisconfig

Expect logs not to contain any errors

kubectl logs deployment/limitador-sample -n default

Verify the disk connections work.

This does not use the redis but sets custom command arguments.

Deploy limitador with disk storage.

echo "
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  storage:
    disk:
       optimize: disk
" | kubectl apply -f -

Once deployment is complete:

  • check the limitador deployment
kubectl get deployment limitador-sample -n default -o yaml

Expected output to contain

 spec:
      containers:
      - command:
        - limitador-server
        - /home/limitador/etc/limitador-config.yaml
        - disk
        - --optimize
        - disk
        - /var/lib/limitador/data

Expect logs not to contain any errors

kubectl logs deployment/limitador-sample -n default

Verify the default / in-memory deployment.

This does not use the redis and does not set custom commands.

Deploy limitador with disk storage.

echo "
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  storage: null
" | kubectl apply -f -

Once deployment is complete:

  • check the limitador deployment
kubectl get deployment limitador-sample -n default -o yaml

Expected output to contain

    spec:
      containers:
      - command:
        - limitador-server
        - /home/limitador/etc/limitador-config.yaml
        - memory

Expect logs not to contain any errors

kubectl logs deployment/limitador-sample -n default

Error logs in Operator

If redis secret is mising:

2023-11-14T15:57:55Z    ERROR    Reconciler error    {"controller": "limitador", "controllerGroup": "limitador.kuadrant.io", "controllerKind": "Limitador", "Limitador": {"name":"limitador-sample","namespace":"default"}, "namespace": "default", "name": "limitador-sample", "reconcileID": "a5fcf752-3209-4665-9b4a-aa5861b8ccdb", "error": "Secret \"redisconfig\" not found"}

If URL field is missing from the secret:

023-11-14T15:54:05Z    ERROR    Reconciler error    {"controller": "limitador", "controllerGroup": "limitador.kuadrant.io", "controllerKind": "Limitador" , "Limitador": {"name":"limitador-sample","namespace":"default"}, "namespace": "default", "name": "limitador-sample", "reconcileID": "0ec16398-6b0c-4af2-b22a-372c2f9fd300", "error": "the storage config Secret doesn't have the `URL` field"}

Copy link
Member

@alexsnaps alexsnaps left a comment

Choose a reason for hiding this comment

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

I won't block this, but I wonder whether only inserting the creds in the redis URL if needed wouldn't be preferable. I don't know if setting URLs like this is usually done using a secret... if that's a known pattern, please ignore me. @guicassolato might have a better understanding. (was this comment even useful now?!)

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

Changes looks good overall, some comments dropped to enhance the implementation (IMO)

pkg/limitador/redis_cache_storage_options_test.go Outdated Show resolved Hide resolved
pkg/limitador/redis_storage_options.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2023

Codecov Report

Merging #109 (c172cc7) into main (5c9f159) will increase coverage by 0.96%.
The diff coverage is 94.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
+ Coverage   68.59%   69.56%   +0.96%     
==========================================
  Files          14       14              
  Lines         987     1025      +38     
==========================================
+ Hits          677      713      +36     
- Misses        269      270       +1     
- Partials       41       42       +1     
Flag Coverage Δ
integration 66.47% <78.57%> (+0.40%) ⬆️
unit 71.13% <100.00%> (+1.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1alpha1 (u) 100.00% <ø> (ø)
pkg/helpers (u) ∅ <ø> (∅)
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 46.12% <100.00%> (+1.92%) ⬆️
pkg/limitador (u) 88.54% <100.00%> (+0.86%) ⬆️
controllers (i) 66.47% <78.57%> (+0.40%) ⬆️
Files Coverage Δ
api/v1alpha1/limitador_types.go 100.00% <ø> (ø)
pkg/limitador/deployment_options.go 92.10% <ø> (ø)
pkg/limitador/k8s_objects.go 81.46% <100.00%> (+0.09%) ⬆️
pkg/limitador/redis_cache_storage_options.go 100.00% <100.00%> (ø)
pkg/limitador/redis_storage_options.go 100.00% <100.00%> (+3.33%) ⬆️
pkg/reconcilers/deployment.go 69.00% <100.00%> (+2.69%) ⬆️
controllers/limitador_controller.go 61.78% <78.57%> (+0.84%) ⬆️

@Boomatang Boomatang force-pushed the GH106/URL_from_envvar branch 2 times, most recently from 7c666fb to ecd2b83 Compare November 7, 2023 13:44
@alexsnaps alexsnaps added this to the v0.7.0 milestone Nov 13, 2023
@alexsnaps
Copy link
Member

It'd be great to have this in the 0.7 release... wdyt? Can we get to a good state in time?

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

Looking good, just one comment

doc/storage.md Show resolved Hide resolved
@Boomatang Boomatang merged commit f661116 into main Nov 15, 2023
8 checks passed
@eguzki eguzki deleted the GH106/URL_from_envvar branch November 15, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: To test
Development

Successfully merging this pull request may close these issues.

Storage redis URL from Secret is transformed into plain container command leaking possible password
4 participants