-
Notifications
You must be signed in to change notification settings - Fork 705
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
Increase debug of helm tests #1153
Conversation
@@ -18,5 +18,5 @@ spec: | |||
command: | |||
- sh | |||
- -c | |||
- "curl -ik -H \"Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)\" $TILLER_PROXY_HOST:$TILLER_PROXY_PORT/v1/releases | grep $KUBEAPPS_RELEASE" | |||
- "curl -v -o /tmp/output -ik -H \"Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)\" $TILLER_PROXY_HOST:$TILLER_PROXY_PORT/v1/releases && cat /tmp/output && cat /tmp/output | grep $KUBEAPPS_RELEASE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these clusters are temporary, but here we may expose sensitive information to the CI logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to say no because this is meant for the local cluster using kind
but the same is executed for GKE and showing the headers would show a valid token for the cluster. Good catch! I am removing the -v
if [[ "$code" != 0 ]]; then | ||
echo "PODS status on failure" | ||
kubectl get pods -n kubeapps | ||
for pod in $(kubectl get po -l release=kubeapps-ci -oname -n kubeapps); do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here about exposing sensitive data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case there is nothing sensitive for the temporary cluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Please, check my comments before merging the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to Angel's question, wondering where this output is going to be publicly visible? CircleCI? What credentials could potentially be exposed, if any?
@@ -16,5 +16,5 @@ spec: | |||
command: | |||
- sh | |||
- -c | |||
- curl $CHARTSVC_HOST:$CHARTSVC_PORT/v1/charts | grep wordpress | |||
- curl -o /tmp/output $CHARTSVC_HOST:$CHARTSVC_PORT/v1/charts && cat /tmp/output && cat /tmp/output | grep wordpress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you cat
ing the whole /tmp/output then also redoing with the pipe? (here and everywhere).
That just means we see some of the output twice, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just do the grep
pipe and the output doesn't contain what we expect it to contain (like for example an error), the actual content would be filtered out. That's why I am showing the output and then do the "grep" as the test.
Yes, in CircleCI. We are showing the logs of the pods of the temporary cluster. We are not showing any credentials there. You can see an example of what's being printed here: (note that I have removed the |
Ref #1146
Sometimes the helm tests fails but since we are not printing the logs we are not sure what could be happening. The goal of this PR is to add verbosity to the tests so we can find the cause of the issue when it appears.