Conversation
Update to the ssh tests to fix flakeyness and add the audit log testing.
susesgartner
requested review from
Josh-Diamond,
igomez06,
markusewalker and
slickwarren
igomez06
left a comment
igomez06
left a comment
There was a problem hiding this comment.
My question is should the SSH tests themselves even be here in shepherd? Would it make sense to have them in rancher/rancher with _test.go file written for them?
markusewalker
left a comment
markusewalker
left a comment
There was a problem hiding this comment.
Left some comments. Thinking further on Israel's comments, I actually think it does make more sense to have the actual test be in rancher/rancher. We can talk offline further about it if you would like.
| sudo install -m o+w /dev/null /var/lib/rancher/k3s/server/audit.yaml | ||
| sudo chmod o+w /etc/systemd/system/k3s.service | ||
| sed -i '$d' /etc/systemd/system/k3s.service | ||
| sudo echo -e " '--kube-apiserver-arg=audit-log-path=/var/lib/rancher/k3s/server/logs/audit.log' \ \n '--kube-apiserver-arg=audit-policy-file=/var/lib/rancher/k3s/server/audit.yaml'" >> /etc/systemd/system/k3s.service |
There was a problem hiding this comment.
Just a clarifying question, what's the need for the spaces in the beginning of this command?
There was a problem hiding this comment.
That appears to be a copy paste error on my part.
| if newNode.State.Name == runningState { | ||
| return false, nil | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
nit; please put this on a new line for code readability.
There was a problem hiding this comment.
I'll be running our linter against this today once i get it running in vscode and it will catch that
|
|
||
| auditLogPath := rancherDir + cluster.Labels[providerLabel] | ||
| auditLogFile := "" | ||
| if cluster.Labels[providerLabel] == "rke2" { |
There was a problem hiding this comment.
Can you put rke2 in a const block?
There was a problem hiding this comment.
Yes, once my other PR gets merged I plan to rebase and expand those constant files see defaults package in this PR
| auditLogPath = auditLogPath + "/etc/config-files/" + auditLogFile | ||
| } | ||
|
|
||
| if cluster.Labels[providerLabel] == "k3s" { |
There was a problem hiding this comment.
Same comment as above.
| return err | ||
| } | ||
|
|
||
| dirPath := filepath.Join(user.HomeDir, "go/src/github.com/susesgartner/rancher/tests/framework/extensions/provisioning") |
There was a problem hiding this comment.
This is pointing to your forked repo, this needs to be using the official repo.
|
It would probably be a good idea to have some design discussions around the SSH tests. I'm not against moving them but I am not perfectly clear on how we plan to run these tests as part of other tests (their current use) given our current testing architecture. |
slickwarren
left a comment
slickwarren
left a comment
There was a problem hiding this comment.
lgtm, but if it makes sense to move it we can do that too. Perhaps makes more sense to fix this for now, and move it later.
I agree with Caleb, we can get this fixed now. But ultimately we should move these out of here and in rancher/rancher testing. I could be a part of the design discussion if you guys want. |
igomez06
left a comment
igomez06
left a comment
There was a problem hiding this comment.
Do we have a test run to run the new audit log, plus check flakiness? Once that is done I'll give my approval
|
I'll get this run ASAP and send you the job. Dealing with 1 bit of flakeyness I discovered with the a few of the SSH tests. |
4 participants
Update to the ssh tests to fix flakeyness and add the audit log testing.