Copilot commented on code in PR #309:
URL:
https://github.com/apache/trafficserver-ingress-controller/pull/309#discussion_r2517180958
##########
pluginats/connect_redis.lua:
##########
@@ -216,7 +243,7 @@ function do_global_read_request()
ts.error("Redis Lookup Failure: snippet == nil for hostpath")
return 0
end
-
+ ts.debug("Snippet in the Connect Redis lua file " .. snippet)
Review Comment:
Inconsistent indentation: this line uses a tab character while surrounding
lines use spaces. This should use spaces to match the rest of the file's
indentation style.
```suggestion
ts.debug("Snippet in the Connect Redis lua file " .. snippet)
```
##########
tests/suite/test_ingress.py:
##########
@@ -62,9 +68,15 @@ def setup_module(module):
# misc_command('kubectl exec $(kubectl get pod -n trafficserver-test -o
name) -n trafficserver-test -- curl -v $(kubectl get service/appsvc2 -n
trafficserver-test-2 -o jsonpath={.spec.clusterIP}):8080/app1')
def teardown_module(module):
+
+ kubectl_delete('crd atscachingpolicies.k8s.trafficserver.apache.com')
+ kubectl_delete('-f data/setup/ats_caching/ats-cachingpolicy-role.yaml')
+ kubectl_delete('-f data/setup/ats_caching/ats-cachingpolicy-binding.yaml')
kubectl_delete('namespace trafficserver-test-3')
kubectl_delete('namespace trafficserver-test-2')
kubectl_delete('namespace trafficserver-test')
+ kubectl_delete('namespace cache-test-ns')
+
Review Comment:
Trailing whitespace on line 79. Remove the extra spaces after the comment.
```suggestion
```
##########
pluginats/connect_redis.lua:
##########
@@ -186,12 +208,17 @@ function do_global_read_request()
ts.error("Redis Lookup Failure: wrong format - "..ipport)
return 0
end
+
+ -- Setting up cache key
+ ts.debug("setting up the cache url key using the set_cache_url" .. url)
Review Comment:
Missing space after concatenation operator. Should be: `"setting up the
cache url key using the set_cache_url " .. url` (add space before closing
quote).
```suggestion
ts.debug("setting up the cache url key using the set_cache_url " ..
url)
```
##########
tests/suite/test_ingress.py:
##########
@@ -37,16 +37,22 @@ def misc_command(command):
def setup_module(module):
misc_command('openssl req -x509 -sha256 -nodes -days 365 -newkey rsa:2048
-keyout tls.key -out tls.crt -subj "/CN=atssvc/O=atssvc"')
+
kubectl_create('namespace trafficserver-test')
kubectl_create('secret tls tls-secret --key tls.key --cert tls.crt -n
trafficserver-test --dry-run=client -o yaml | kubectl apply -f -')
kubectl_apply('data/setup/configmaps/')
kubectl_apply('data/setup/traffic-server/')
kubectl_apply('data/setup/apps/')
kubectl_apply('data/setup/ingresses/')
+
+ #Applying here as it takes some time for controller to get notification
from kubernetes.
+ kubectl_apply('data/setup/ats_caching/ats-cachingpolicy-role.yaml')
+ kubectl_apply('data/setup/ats_caching/ats-cachingpolicy-binding.yaml')
+ kubectl_apply('data/setup/ats_caching/crd-atscachingpolicy.yaml')
+ kubectl_apply('data/setup/ats_caching/atscachingpolicy.yaml')
Review Comment:
The paths reference `data/setup/ats_caching/` but based on the repository
structure, the ats_caching directory is at the root level (`ats_caching/`), not
under `data/setup/`. These paths should likely be `../ats_caching/` or the
files need to be copied/symlinked to `data/setup/ats_caching/` for the tests to
work correctly.
```suggestion
kubectl_apply('../../ats_caching/ats-cachingpolicy-role.yaml')
kubectl_apply('../../ats_caching/ats-cachingpolicy-binding.yaml')
kubectl_apply('../../ats_caching/crd-atscachingpolicy.yaml')
kubectl_apply('../../ats_caching/atscachingpolicy.yaml')
```
##########
k8s/images/caching-app/package.json:
##########
@@ -0,0 +1,14 @@
+{
+ "name": "dockernode",
+ "version": "1.0.0",
+ "description": "docker-ize a very simple express.js app",
+ "main": "server.js",
+ "scripts": {
Review Comment:
The `package.json` file is missing a `"start"` script in the `"scripts"`
section, but the Dockerfile at line 29 runs `CMD ["npm", "start"]`. This will
cause the container to fail at startup. Add a start script such as:
```json
"scripts": {
"start": "node server.js",
"test": "echo \"Error: no test specified\" && exit 1"
}
```
```suggestion
"scripts": {
"start": "node server.js",
```
##########
tests/suite/test_ingress.py:
##########
@@ -62,9 +68,15 @@ def setup_module(module):
# misc_command('kubectl exec $(kubectl get pod -n trafficserver-test -o
name) -n trafficserver-test -- curl -v $(kubectl get service/appsvc2 -n
trafficserver-test-2 -o jsonpath={.spec.clusterIP}):8080/app1')
def teardown_module(module):
+
+ kubectl_delete('crd atscachingpolicies.k8s.trafficserver.apache.com')
+ kubectl_delete('-f data/setup/ats_caching/ats-cachingpolicy-role.yaml')
+ kubectl_delete('-f data/setup/ats_caching/ats-cachingpolicy-binding.yaml')
Review Comment:
The path references `data/setup/ats_caching/` which doesn't match the
repository structure. This should likely be `../ats_caching/` to correctly
reference the files at the root level.
```suggestion
kubectl_delete('-f ../ats_caching/ats-cachingpolicy-role.yaml')
kubectl_delete('-f ../ats_caching/ats-cachingpolicy-binding.yaml')
```
##########
pluginats/connect_redis_test.lua:
##########
@@ -123,6 +123,52 @@ describe("Unit tests - Lua", function()
stub(ts.client_request, "set_uri")
stub(ts.http, "skip_remapping_set")
stub(ts.http, "set_resp")
+
+ -- calling get_version on client request
+ stub(ts.client_request, "get_version").returns("1.1")
+
+ -- calling get_pristine_url on client request
+ stub(ts.client_request,
"get_pristine_url").returns("http://test.edge.com/app1";)
+
+ -- Used for setting the cache url
+ stub(ts.http, "set_cache_url")
+
+ _G.TS_LUA_HOOK_SEND_RESPONSE_HDR = "TS_LUA_HOOK_SEND_RESPONSE_HDR"
+ stub(ts, "hook")
+ end)
+
+ -- Test - Verify caching for Http/1.1 -------------------------
+
+ it("Test - Verify caching for Http/1.1", function()
+ require("connect_redis")
+ local result = do_global_read_request()
+
+ -- Verifies the HTTP version check and host set for 1.1
+ assert.stub(ts.client_request.get_version).was.called()
+
assert.stub(ts.client_request.set_url_host).was.called_with("test.edge.com")
+
+ -- Verifies that pristine URL is used to set up cache
+
assert.stub(ts.http.set_cache_url).was.called_with("http://test.edge.com/app1";)
+ end)
+
+ -- Test - Verify caching for Http/2.0 -------------------
+
+ it("Test - Verify caching for Http/2.0", function()
+ -- Clear cached module first
+ package.loaded["connect_redis"] = nil
+
+ -- Re-stub get_version to return "2" before requiring
+ stub(ts.client_request, "get_version").returns("2.0")
+
+ -- Reload module
+ require("connect_redis")
+
+ -- Execute target function
+ do_global_read_request()
+
+ -- Assertions
+ assert.stub(ts.client_request.get_version).was.called()
+
assert.stub(ts.http.set_cache_url).was.called_with("http://test.edge.com/app1";)
end)
Review Comment:
Inconsistent indentation in test case: lines 142-152 use 6 spaces for the
`it()` block, while lines 156-172 use 4 spaces. This inconsistency should be
corrected to maintain uniform code style throughout the file.
```suggestion
it("Test - Verify caching for Http/2.0", function()
-- Clear cached module first
package.loaded["connect_redis"] = nil
-- Re-stub get_version to return "2" before requiring
stub(ts.client_request, "get_version").returns("2.0")
-- Reload module
require("connect_redis")
-- Execute target function
do_global_read_request()
-- Assertions
assert.stub(ts.client_request.get_version).was.called()
assert.stub(ts.http.set_cache_url).was.called_with("http://test.edge.com/app1";)
end)
```
##########
tests/data/caching-app/cache-test.yaml:
##########
@@ -0,0 +1,69 @@
+
+apiVersion: v1
+kind: Namespace
+metadata:
+ name: cache-test-ns
+---
+
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: cache-test
+ namespace: cache-test-ns
+spec:
+ replicas: 1
+ selector:
+ matchLabels:
+ app: cache-test
+ template:
+ metadata:
+ labels:
+ app: cache-test
+ spec:
+ containers:
+ - name: cache-test
+ image: caching-app:latest
+ imagePullPolicy: Always
Review Comment:
`imagePullPolicy: Always` will cause failures when using a locally built
image with the `latest` tag. Since the image is built locally in minikube (as
seen in the GitHub workflow), this should be `imagePullPolicy: IfNotPresent` or
`Never` to use the locally built image.
```suggestion
imagePullPolicy: IfNotPresent
```
##########
tests/suite/test_ingress.py:
##########
@@ -215,16 +221,12 @@ def test_cache_app1_beyond_ttl(self, minikubeip):
if resp.__contains__("Age"):
age2 = resp
if resp.__contains__("Date"):
- mod_time2 = resp
- kubectl_delete('crd atscachingpolicies.k8s.trafficserver.apache.com')
+ mod_time2 = resp
Review Comment:
Trailing whitespace on line 224. Remove the extra spaces after the code.
```suggestion
mod_time2 = resp
```
##########
ats_caching/atscachingpolicy.yaml:
##########
@@ -1,19 +1,19 @@
-apiVersion: k8s.trafficserver.apache.com/v1
+apiVersion: k8s.trafficserver.apache.com/v1alpha1
kind: ATSCachingPolicy
metadata:
name: my-app-caching
- namespace: trafficserver-test
+ namespace: caching-ats-new
Review Comment:
The namespace `caching-ats-new` in this policy file doesn't match the
namespace used in the test resources (`cache-test-ns` as defined in
`tests/data/caching-app/cache-test.yaml`). This mismatch will prevent the
caching policy from applying to the test application. Consider updating the
namespace to `cache-test-ns` to match the test infrastructure.
```suggestion
namespace: cache-test-ns
```
##########
k8s/images/caching-app/Dockerfile:
##########
@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+FROM node:8
Review Comment:
The Docker base image `node:8` is extremely outdated and has reached
end-of-life (EOL was 2019-12-31). Node.js 8 has known security vulnerabilities.
Consider upgrading to a more recent LTS version such as `node:20` or `node:18`.
```suggestion
FROM node:20
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
