Logstash controller unit and integration tests#6575
Logstash controller unit and integration tests#6575robbavey merged 11 commits intoelastic:feature/logstashfrom
Conversation
thbkrkr
changed the title
kaisecheng
left a comment
kaisecheng
left a comment
There was a problem hiding this comment.
Many thanks for adding the tests. Mostly looks good to me. I left three comments for you to consider.
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "configRef takes precedence", |
There was a problem hiding this comment.
should be "config takes precedence"
pkg/controller/logstash/pod.go
Outdated
|
|
||
| // GetLogstashContainer returns the Logstash container from the given podSpec. | ||
| func GetLogstashContainer(podSpec corev1.PodSpec) *corev1.Container { | ||
| return pod.ContainerByName(podSpec, "logstash") |
There was a problem hiding this comment.
there is a constant for the container name https://github.com/elastic/cloud-on-k8s/blob/feature/logstash/pkg/apis/logstash/v1alpha1/logstash_types.go#L17
| }, | ||
| }, | ||
| } | ||
| for _, tt := range tests { |
There was a problem hiding this comment.
Can we add test coverage for custom ports, annotation and readinessProbe?
Added tests for readiness probes and annotations. Updated readiness probe to pick up port from API service
robbavey
force-pushed
the
controller_tests
branch
from
bfda607 to
c8523ce
Compare
Also updated to take account of pipelines
robbavey
force-pushed
the
controller_tests
branch
from
55a0b93 to
d2dd060
Compare
robbavey
force-pushed
the
controller_tests
branch
from
cd90d6a to
873bb47
Compare
|
@barkbay Ready for another round |
|
@elasticmachine run elasticsearch-ci/docs |
| var port = network.HTTPPort | ||
| for _, service := range logstash.Spec.Services { | ||
| if service.Name == LogstashAPIServiceName { | ||
| port = int(service.Service.Spec.Ports[0].Port) |
There was a problem hiding this comment.
I missed it during the first review but .Service.Spec.Ports can be empty. It is possible for a user to crash the operator by creating a Service with an empty slice, for example:
apiVersion: logstash.k8s.elastic.co/v1alpha1
kind: Logstash
metadata:
name: logstash-sample
spec:
count: 2
version: 8.6.1
config:
log.level: info
api.http.host: "0.0.0.0"
api.http.port: 9601
queue.type: memory
services:
- name: api
service:
spec:
type: ExternalName
externalName: foo
ports: []The slice length must be checked before accessing the index.
| var port = network.HTTPPort | ||
| for _, service := range logstash.Spec.Services { | ||
| if service.Name == ls.LogstashAPIServiceName { | ||
| port = int(service.Service.Spec.Ports[0].Port) |
There was a problem hiding this comment.
Same here, we should check the length of the slice before accessing the index 0
|
@barkbay Nice find! I've pushed a fix and added a test to verify |
4 participants
This commit adds extra unit tests for the Logstash Controller, including tests for config, controller, pods and services