-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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.
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
hcp.v2.TelemetryState resource and controller implementation #20257
Conversation
4744ff8
to
205472e
Compare
bb78574
to
ea12838
Compare
@jjti tagged you to review the proto messages to make sure I didn't miss anything you expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
// Check that the link resource indicates the cluster is linked | ||
// If the cluster is not linked, the telemetry-state resource should not exist | ||
if linked, reason := link.IsLinked(res.GetResource()); !linked { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a chance this code will run after the Link is created but before it is linked
for the first time? In that case, I assume the TelemetryState
would be created, then deleted, then recreated once the Link is linked
? Maybe that is okay, I just wanted to understand if we've thought through that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reconciler fires on changes to Link and TelemetryState resources. When the a Link is created for the first time but not linked this will delete any existing TelemetryState (which there shouldn't be yet).
internal/hcp/internal/controllers/telemetrystate/controller_test.go
Outdated
Show resolved
Hide resolved
This looks good to me. I mostly just want to understand if there's any possible issue of TelemetryState being repeatedly created/deleted before approving. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource/controller stuff LGTM!
// TlsInsecureSkipVerify if true will ignore server name verification when making HTTPS requests | ||
bool tls_insecure_skip_verify = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dev/personal-dev environment testing mostly
d926cd9
to
e5aa9bc
Compare
Description
This PR adds a new resource type
hcp.v2.TelemetryState
which is a computed resource used to signal consul-dataplane when and how to forward dataplane telemetry to HCP. When thehcp.v2.Link
resource is set and the cluster has linked successfully with HCP (indicated by condition onLink
resource) theTelemetryState
resource is computed and written.Followup PR will clean up some of the TODOs commented in the controller.
Testing & Reproduction steps
Controller unit tests for various input cases.
Links
PR Checklist