Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CNI File watcher and pre applying rules setup #1345

Merged
merged 9 commits into from
Aug 16, 2022
Merged

CNI File watcher and pre applying rules setup #1345

merged 9 commits into from
Aug 16, 2022

Conversation

curtbushko
Copy link
Contributor

@curtbushko curtbushko commented Jul 15, 2022 •
edited
Loading

Changes proposed in this PR:

  • 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

How I've tested this PR:

  • unit tests
  • manual testing

How I expect reviewers to test this PR:

  • You can run this in kind by disabling kinds default CNI, installing calico first and installing consul with cni turn on.

Or you can clone my cni-demo repo and run:

cd consul-cni-on-kind; make create-cluster deploy-calico deploy-consul hashicups

Note: the consul-cni will crashloop as I need to add the file watcher to it.

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@curtbushko curtbushko requested review from ishustava and t-eckert July 15, 2022 17:07
@curtbushko curtbushko changed the title Curtbushko/proxyconfig CNI Proxy config Jul 15, 2022
@curtbushko curtbushko force-pushed the curtbushko/proxyconfig branch from e6b21d4 to f9c2da9 Compare July 15, 2022 17:39
t-eckert
t-eckert reviewed Jul 15, 2022
View reviewed changes
curtbushko
curtbushko commented Jul 15, 2022
View reviewed changes
curtbushko
curtbushko commented Jul 15, 2022
View reviewed changes
curtbushko
curtbushko commented Jul 15, 2022
View reviewed changes
t-eckert
t-eckert reviewed Jul 15, 2022
View reviewed changes
Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass with some initial comments! More for me to read and do!

ishustava
ishustava reviewed Jul 19, 2022
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving my comments so far. I didn't get to review the tests yet! Will continue reviewing later today.

control-plane/cni/main.go Outdated
return fmt.Errorf("annotation %s does not exist yet", annotation)
}
return err
}, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), retries))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will wait for 10 seconds for the annotation if I'm reading this correctly. Is this enough? I think on some clouds and depending on your pod, it might definitely take more than that to provision a pod and for endpoints controller to add this annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were correct. The backoff is constant and would last 10 seconds total. I have set this to 30 retries (ie 30 seconds). We could do an ExponentialBackoff (it is a cool package) to slow down the retries but I think being responsive is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed retries.

@curtbushko curtbushko force-pushed the curtbushko/proxyconfig branch from ad2d7e9 to 6fa5393 Compare July 25, 2022 19:14
@curtbushko curtbushko requested a review from ishustava July 25, 2022 19:43
@curtbushko curtbushko self-assigned this Jul 25, 2022
@curtbushko curtbushko force-pushed the curtbushko/proxyconfig branch from c876bb7 to 480a3f2 Compare August 8, 2022 20:54
@curtbushko curtbushko changed the title CNI Proxy config CNI File watcher and pre applying rules setup Aug 9, 2022
@curtbushko curtbushko force-pushed the curtbushko/proxyconfig branch 3 times, most recently from 89105d7 to 760f472 Compare August 9, 2022 18:09
@t-eckert t-eckert self-requested a review August 9, 2022 20:51
t-eckert
t-eckert approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

ishustava
ishustava reviewed Aug 9, 2022
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work Curt!! You did such a great job on testing! I had a few suggestions in-line

control-plane/cni/main.go Outdated
return types.PrintResult(result, cfg.CNIVersion)
}

pod.Annotations[keyTransparentProxyStatus] = waiting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we change it to waiting?

Copy link
Contributor Author

@curtbushko curtbushko Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a simple way to show some sort of status that the CNI plugin is working. I have added a simple updateTransparentProxyStatusAnnotation function to clean it up a bit so that we can have:

consul.hashicorp.com/transparent-proxy-status = waiting and consul.hashicorp.com/transparent-proxy-status = complete

annotations on the pod.

Please let me know if you have better or more consistent status names.

control-plane/cni/main.go Outdated
return fmt.Errorf("annotation %s does not exist yet", annotation)
}
return err
}, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), retries))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to remove

control-plane/subcommand/install-cni/command_test.go Outdated
Comment on lines 90 to 93
time.Sleep(50 * time.Millisecond)
// Filewatcher should have detected change, fixed and appended to the config file. Make sure
// files match.
require.Equal(t, string(expected), string(actual))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
time.Sleep(50 * time.Millisecond)
// Filewatcher should have detected change, fixed and appended to the config file. Make sure
// files match.
require.Equal(t, string(expected), string(actual))
// Filewatcher should have detected change, fixed and appended to the config file. Make sure
// files match.
retry.Run(t, func(r *retry.R) {
require.Equal(r, string(expected), string(actual))
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to a few places that I thought were relevant.

@curtbushko curtbushko requested a review from ishustava August 12, 2022 18:41
ishustava
ishustava approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!! Just one minor suggestion.

Approving, assuming unit and acceptance tests are fixed before merge. For acceptance, you might need to rebase to get latest peering fixes.

curtbushko and others added 7 commits August 15, 2022 19:35
@curtbushko
Initial CNI installer & plugin (#1304)
3c6e110 
* Get structure in place and CNI installer & plugin building
@curtbushko
CNI Helm Charts
1e1873f 
Add helm charts for CNI installer
@curtbushko
Initial CNI installer & plugin (#1304)
77b9849 
* Get structure in place and CNI installer & plugin building
@curtbushko
CNI File watcher and pre applying rules setup
cee83eb 
    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 <[email protected]>
@curtbushko
go mod tidy to get linting passing
c9dd4dc
@curtbushko
Add tests to install-cni/binary and cni/main. Address PR comments
2a4a042
@curtbushko @ishustava
Update comment
2a6759f 
Co-authored-by: Iryna Shustava <[email protected]>
@curtbushko curtbushko force-pushed the curtbushko/proxyconfig branch from 59dab7c to 2a6759f Compare August 15, 2022 23:37
@curtbushko curtbushko merged commit bd5dd79 into cni Aug 16, 2022
@curtbushko curtbushko deleted the curtbushko/proxyconfig branch August 16, 2022 15:17
curtbushko added a commit that referenced this pull request Aug 19, 2022
@curtbushko
CNI File watcher and pre applying rules setup (#1345)
07ccb89 
* 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
curtbushko added a commit that referenced this pull request Aug 19, 2022
@curtbushko
CNI File watcher and pre applying rules setup (#1345)
fc4553c 
* 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
curtbushko added a commit that referenced this pull request Aug 23, 2022
@curtbushko
CNI File watcher and pre applying rules setup (#1345)
fa777da 
* 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
curtbushko added a commit that referenced this pull request Aug 24, 2022
@curtbushko
CNI File watcher and pre applying rules setup (#1345)
79a88b6 
* 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
curtbushko added a commit that referenced this pull request Aug 24, 2022
@curtbushko
CNI File watcher and pre applying rules setup (#1345)
26aad37 
* 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
curtbushko added a commit that referenced this pull request Aug 24, 2022
@curtbushko
CNI File watcher and pre applying rules setup (#1345)
0f33295 
* 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
curtbushko added a commit that referenced this pull request Aug 25, 2022
@curtbushko
CNI File watcher and pre applying rules setup (#1345)
55abb59 
* 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
curtbushko added a commit that referenced this pull request Aug 26, 2022
@curtbushko
CNI File watcher and pre applying rules setup (#1345)
e110c22 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishustava ishustava ishustava approved these changes

@t-eckert t-eckert t-eckert approved these changes

Assignees

@curtbushko curtbushko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@curtbushko @ishustava @t-eckert