Initial work to opensource guardian #9849
Conversation
marvin-tigera
added
release-note-required
Brian-McM
force-pushed
the
bm-add-guardian-to-oss
branch
7 times, most recently
from
8581bb4 to
09d1626
Compare
| ClientCertPath string `json:"clientCertPath"` | ||
| ClientKeyPath string `json:"clientKeyPath"` | ||
|
|
||
| Unauthenticated bool `json:"unauthenticated,omitempty"` |
There was a problem hiding this comment.
I think so... this seems to only apply to the other side of the tunnel upon closer inspection (and probably not even for the tunnel itself).
| ) | ||
|
|
||
| const ( | ||
| tunnelNetwork = "voltron-tunnel" |
There was a problem hiding this comment.
I wonder if there is any benefits to configure this at start up instead of hard-coding it. In case we want to change it, it would be better to not hard-coded.
There was a problem hiding this comment.
For now I'm just going to keep it hardcoded (since this is how it's currently done).
Brian-McM
force-pushed
the
bm-add-guardian-to-oss
branch
2 times, most recently
from
72d8a25 to
8045f6c
Compare
| } else { | ||
| backlog = append(backlog, cmd) | ||
| } | ||
| case cmd := <-executor.backlogChan: |
There was a problem hiding this comment.
Can we have more than 100 tasks in the backlog channel ?
There was a problem hiding this comment.
Not at the same time. Since this loop never blocks it's unlikely we'd every reach that level of requests, and if we do it would be cleared out.
guardian/pkg/asyncutil/async.go
Outdated
|
|
||
| func (executor *commandExecutor[C, R]) Send(params C) <-chan Result[R] { | ||
| cmd, resultChan := NewCommand[C, R](params) | ||
| executor.cmdChan <- cmd |
There was a problem hiding this comment.
Not at the same time, but the other side of the channel shouldn't block so there wouldn't be any issue.
| ) | ||
|
|
||
| type CommandExecutor[C any, R any] interface { | ||
| Send(C) <-chan Result[R] |
There was a problem hiding this comment.
I saw that in most cases, we call this as Send(nill). Do we really need parameter here ?
There was a problem hiding this comment.
I was debating this, I was thinking this would be more general so you can send parameters in other cases. Seemed like it might be overkill to have a different interface just to not send nil. OTOH though it might not be difficult and would make it cleaner (could use the same implementation under the hood?).
There was a problem hiding this comment.
I'd thought about this some more, and I'd like to just stick with having the parameter here. I could make another interface that doesn't take in a parameter, but it seems like that would be unnecessary given that you just pass nil as the parameter as opposed to having different interfaces to work with.
This is intended to be a general use interface as well, and plan on moving it to somewhere more general. This will at least be used for voltron.
| } | ||
|
|
||
| func (cfg *Config) Cert() (string, *x509.CertPool, error) { | ||
| if strings.ToLower(cfg.VoltronCAType) == "public" { |
There was a problem hiding this comment.
What is the difference between public and Tigera CA Type. I assumed Tigera was the public one.
There was a problem hiding this comment.
Not sure tbh, but it's in the current version of guardian so I need to make sure it's in this version.
| ConnectionRetryAttempts int `default:"25" split_words:"true"` | ||
| ConnectionRetryInterval time.Duration `default:"5s" split_words:"true"` | ||
|
|
||
| Listen bool `default:"true"` |
There was a problem hiding this comment.
Do we need this variable ? When can this set to false ?
There was a problem hiding this comment.
I'm not exactly sure, but it's in the existing guardian.
There was a problem hiding this comment.
I was just about to remove this (after our chat yesterday) but I'm realizing that this setting may have some value. From a conceptual POV, it would make sense to not allow guardian to push up to the cluster and have this in some sort of read only state.
Given that this setting is already there (as in production), I'm inclined to leave it.
Brian-McM
added
docs-not-required
Brian-McM
force-pushed
the
bm-add-guardian-to-oss
branch
from
31c441c to
56513a2
Compare
This commit adds guardian, a component that can connect to calico cloud. This facilitates open source clusters connecting to calico free tier.
Brian-McM
force-pushed
the
bm-add-guardian-to-oss
branch
from
e7ed676 to
65c05ba
Compare
| var drainCoordinatorCh <-chan asyncutil.Result[asyncutil.Signaler] | ||
| // Use a FunctionCallRateLimiter to 1) ensure we don't rapidly retry recreating the session if it's constantly | ||
| // disconnecting and 2) return an error if this is called to many times within a 30-second time window. | ||
| drainCoordinator := asyncutil.NewFunctionCallRateLimiter(2*time.Second, 30*time.Second, 5, func(any) (asyncutil.Signaler, error) { |
There was a problem hiding this comment.
Should this values be in the config ?
There was a problem hiding this comment.
Not for right now, I'm not sure if I want this to be configurable or now. I need to think about this more as followup.
|
|
||
| // Close closes the listener. A closed listener cannot be used again | ||
| func (l *listener) Close() error { | ||
| return nil |
There was a problem hiding this comment.
Seems like we are not doing anything here.
There was a problem hiding this comment.
True, but it has to be here since it's implementing the net.Listener interface.
guardian/pkg/asyncutil/types.go
Outdated
| // Clear drains the internal buffer and returns when there's nothing left. | ||
| // Not that writing to the error buffer should not be done while clearing, since if writing is happening as quick | ||
| // as clearing is then the buffer will never be cleared. | ||
| func (b *asyncErrorBuffer) Clear() { |
There was a problem hiding this comment.
Should you add some mutex variables to synchronise write, clear and close ?
There was a problem hiding this comment.
It's not needed, these are writing too and interacting with channels. Can you point out a specific race condition you're seeing?
guardian/pkg/asyncutil/types.go
Outdated
| } | ||
|
|
||
| func NewAsyncErrorBuffer() AsyncErrorBuffer { | ||
| return &asyncErrorBuffer{errs: make(chan error, 1)} |
There was a problem hiding this comment.
Do we always buffer 1 error ? Do we need to buffer multiple errors ?
There was a problem hiding this comment.
For the current use case no we don't. If an error occurs is used to decide whether or not we drain and backlog the executors or kill the tunnel. If multiple errors come in at the same time we only ever use the first error to decide what to do and clear the other errors if we're not exiting.
The reason for this is because we don't want multiple errors from multiple commands triggering a drain and backlog rapidly, we only want one signal to do this and we drain and backlog all executors and assume that's solve the issue for all of them. If it hasn't solve the issue for all of them, then they'll continue to return errors and we'll eventually just shutdown the tunnel.
So whether it doesn't really matter if the buffer is size 1 or 100.
guardian/pkg/asyncutil/types.go
Outdated
| return b.errs | ||
| } | ||
|
|
||
| func (b *asyncErrorBuffer) Close() { |
There was a problem hiding this comment.
Can close be called twice ?
There was a problem hiding this comment.
No and if it is it should panic (that's a developer error if this is called more than once).
| return s.ch | ||
| } | ||
|
|
||
| func (s *signaler) Close() { |
There was a problem hiding this comment.
Can close be called twice ?
There was a problem hiding this comment.
No and if it is it should panic (that's a developer error if this is called more than once).
| } | ||
|
|
||
| // Signaler is an interface used for waiting for and sending simple signals. | ||
| type Signaler interface { |
There was a problem hiding this comment.
What do you think of remaning this to Channel ?
There was a problem hiding this comment.
I think I'd prefer to keep this as Signaler. This isn't inteaded to be used as a Channel, only send / receive a signal with no information.
|
|
||
| // Add all outstanding commands in the backlog or cmd channels to the backlog slice. | ||
| executor.backlog = append(executor.backlog, ReadAll(executor.backlogChan)...) | ||
| executor.backlog = append(executor.backlog, ReadAll(executor.cmdChan)...) |
There was a problem hiding this comment.
I think we need to perform this operation before line 142: https://github.com/projectcalico/calico/pull/9849/files#diff-332e67201f01430847e13e593d0dc866cbb68781014e7dc94babff2de33d1f92R142. Either that or you do not need it.
There was a problem hiding this comment.
It doesn't need to be done before, this is actually intentional. I'm closing the channel so nothing more can be sent over it. Closing the channel doesn't "clear" it, all the items will remain on the channel until they're read off.
This basically forces a panic to happen if somebody writes to the channel after it's closed and we're trying to clean up. If they could write to it after reading everything off, then that would result in some commands hanging after close.
Description
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.