USHIFT-6276: introduce hosts plugin coreDNS RF tests #5657
Conversation
Signed-off-by: Evgeny Slutsky <[email protected]>
Signed-off-by: Evgeny Slutsky <[email protected]>
Signed-off-by: Evgeny Slutsky <[email protected]>
Signed-off-by: Evgeny Slutsky <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eslutsky The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
eslutsky
force-pushed
the
add-tests
branch
4 times, most recently
from
f4a94e9 to
34f7bf6
Compare
pkg/components/controllers.go
Outdated
There was a problem hiding this comment.
pkg/config/dns.go
Outdated
There was a problem hiding this comment.
Few comments for this function:
- Use a switch case, maybe?
- If
Enabledwe should validate the file exists and we can read it. - We are allowing the exposure of critical files (such as
/etc/passwd) because we are not checking the actual file path. Same with relative paths, we should sanitize and require absolute paths here. - File size. This will go to a config map, meaning we are limited to 1Mi in total size.
Just an idea: Should we restrict the files we can load to a handful of paths? For example, either /etc/hosts or something outside system paths?
There was a problem hiding this comment.
I am missing RBAC for this specific config map, only CoreDNS pods should be able to read it and only MicroShift should be able to write it.
pkg/controllers/hostswatcher.go
Outdated
There was a problem hiding this comment.
This is constant, should it be a member of the struct?
pkg/controllers/hostswatcher.go
Outdated
There was a problem hiding this comment.
This is constant, should it be a member of the struct?
There was a problem hiding this comment.
Should these be constants?
There was a problem hiding this comment.
Should it signal readiness here instead? One it reaches this point you know you have at least one update to the configmap plus the file being watched.
There was a problem hiding this comment.
Should it close the watcher?
There was a problem hiding this comment.
Should it signal the removal/rename of the file? At that point it will fail in the next restart but remain working until then.
pkg/controllers/hostswatcher.go
Outdated
There was a problem hiding this comment.
Remove the V(2) so that it always show up?
eslutsky
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@eslutsky: This pull request references USHIFT-6276 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
eslutsky
force-pushed
the
add-tests
branch
2 times, most recently
from
9b89e4b to
03c2ffa
Compare
|
/retest |
|
@pacevedom this PR was opened for the tests but its based on the #5491 commits , so i will address the code changed there. |
Signed-off-by: Evgeny Slutsky <[email protected]>
Signed-off-by: Evgeny Slutsky <[email protected]>
eslutsky
force-pushed
the
add-tests
branch
2 times, most recently
from
251cd99 to
1be960c
Compare
Signed-off-by: Evgeny Slutsky <[email protected]>
|
@eslutsky: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Which issue(s) this PR addresses:
Closes #