Add redirect traffic config and apply it#1432
Merged
curtbushko merged 10 commits intocnifrom Aug 24, 2022
Merged
Conversation
ishustava
reviewed
Aug 17, 2022
Contributor
ishustava
left a comment
ishustava
left a comment
There was a problem hiding this comment.
Great work!! Leaving some comments in-line.
t-eckert
reviewed
Aug 22, 2022
Comment on lines
+11
to
+15
| // If the src file does not exist then either the incorrect command line argument was used or | ||
| // the docker container we built is broken somehow. | ||
| if _, err := os.Stat(srcFile); os.IsNotExist(err) { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
Instead of having this info in a comment, I think it would be valuable to append it to the existing error so that it's useful for debugging.
Suggested change
| // If the src file does not exist then either the incorrect command line argument was used or | |
| // the docker container we built is broken somehow. | |
| if _, err := os.Stat(srcFile); os.IsNotExist(err) { | |
| return err | |
| } | |
| // If the src file does not exist then either the incorrect command line argument was used or | |
| // the docker container we built is broken somehow. | |
| if _, err := os.Stat(srcFile); os.IsNotExist(err) { | |
| return fmt.Errorf("%w. Check that the correct command line argument was used and verify that the container image does not contain the source file.", err) | |
| } |
t-eckert
reviewed
Aug 22, 2022
|
|
||
| // Check if the user bit is enabled in file permission. | ||
| if info.Mode().Perm()&(1<<(uint(7))) == 0 { | ||
| return fmt.Errorf("cannot write to destination directory %s", destDir) |
Contributor
There was a problem hiding this comment.
Might change this to say "user does not have permissions to..." so that this doesn't get confused with another failure mode.
t-eckert
approved these changes
Aug 22, 2022
Contributor
t-eckert
left a comment
t-eckert
left a comment
There was a problem hiding this comment.
Great work! Weird that GitHub shows that this PR changes 57 files.
curtbushko
force-pushed
the
curtbushko/cni-redirect-traffic-config
branch
2 times, most recently
from
8496bd9 to
82e70f6
Compare
ishustava
approved these changes
Aug 23, 2022
Contributor
ishustava
left a comment
ishustava
left a comment
There was a problem hiding this comment.
Looks good! Approving, assuming the init container user will be changed along with the tests for it.
* Get structure in place and CNI installer & plugin building
* Add file watcher to CNI installer to watch for config file changes and repair breakages. * Wait for CNI config file to show up on the host file system before attempting to install consul-cni configuration. * Add some code to get ready for the next PR that applying iptables rules * Unit tests for installer and plugin scenarios
* Get structure in place and CNI installer & plugin building
* Add file watcher to CNI installer to watch for config file changes and repair breakages. * Wait for CNI config file to show up on the host file system before attempting to install consul-cni configuration. * Add some code to get ready for the next PR that applying iptables rules * Unit tests for installer and plugin scenarios
Add helm charts for CNI installer
* Get structure in place and CNI installer & plugin building
increase limits for CNI plugin so that it runs on GKE
add annotations for transparent proxy status (enabled, waiting)
Initial setup (CNI_ARGS) for getting information to the CNI plugin
file watcher for config file changes and for when the config file does not exists
added wait for annotation to be used before applying ipconfig traffic redirection
Co-Authored-By: Thomas Eckert <teckert@hashicorp.com>
…y when a valid annotation is used Add redirect traffic config and apply it in the plugin
curtbushko
force-pushed
the
curtbushko/cni-redirect-traffic-config
branch
from
82e70f6 to
9606072
Compare
curtbushko
added a commit
that referenced
this pull request
Aug 25, 2022
* Add redirect traffic config and apply it * Unit tests for redirect traffic and webhook changes
curtbushko
added a commit
that referenced
this pull request
Aug 26, 2022
* Add redirect traffic config and apply it * Unit tests for redirect traffic and webhook changes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR:
NOTE: CircleCI is failing to build this right now with a weird error (below). I will look at this error when I am back on Thursday. It builds locally for me.
cp: cannot stat '/home/circleci/go/bin//control-plane': No such file or directoryHow I've tested this PR:
How I expect reviewers to test this PR:
👀
Checklist: