-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: Automatically trigger pod rollout for appsv1 resources when AuthProxyWorkload changes. #197
Conversation
b70585b
to
e3ee326
Compare
|
||
var status metav1.ConditionStatus | ||
var result ctrl.Result | ||
if upToDate { |
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.
Small Go nit:
status := metav1.ConditionFalse
result := requeueNow
if upToDate {
status = metav1.ConditionTrue
result = ctrl.Result{}
}
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.
Fixed.
internal/workload/podspec_updates.go
Outdated
// on the pod. | ||
func PodAnnotation(r *cloudsqlapi.AuthProxyWorkload) (string, string) { | ||
return fmt.Sprintf("%s/%s", cloudsqlapi.AnnotationPrefix, r.Name), | ||
strconv.FormatInt(r.Generation, 10) |
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.
What is 10
?
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.
10 is "format this int64 in base 10"
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.
🤦 Let's just use fmt.Sprintf("%d", r.Generation)
in that. I somehow totally read over FormatInt.
} | ||
|
||
// test that annotation was set properly | ||
if !reflect.DeepEqual(wl.PodTemplateAnnotations(), wantAnnotations) { |
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.
reflect.DeepEqual is fine here, but FYI there is https://github.com/google/go-cmp which is helpful for more sophistication comparisons.
|
||
// Check if the correct annotation exists | ||
an := wl.PodTemplateAnnotations() | ||
if an != nil { |
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.
if an != nil && an[k] == v {
return false
}
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.
Fixed.
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.
Updated and ready for a new review.
|
||
// Check if the correct annotation exists | ||
an := wl.PodTemplateAnnotations() | ||
if an != nil { |
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.
Fixed.
|
||
var status metav1.ConditionStatus | ||
var result ctrl.Result | ||
if upToDate { |
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.
Fixed.
internal/workload/podspec_updates.go
Outdated
// on the pod. | ||
func PodAnnotation(r *cloudsqlapi.AuthProxyWorkload) (string, string) { | ||
return fmt.Sprintf("%s/%s", cloudsqlapi.AnnotationPrefix, r.Name), | ||
strconv.FormatInt(r.Generation, 10) |
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.
10 is "format this int64 in base 10"
/gcbrun |
b1d3823
to
ddbb207
Compare
/gcbrun |
Features - Add new field RolloutStrategy control automatic rollout (#202) (090b88d) - Add new terraform project for e2e test resources (#181) (0140592) - Add script to run terraform with input validation. (#182) (857444a) - Add support for Unix sockets. (#205) (8177a35), closes #47 - Add telemetry settings to configure health check port (#210) (3ede42d) - Add the e2e test job for Cloud Build (#184) (dc2990c) - Automatic changes to workloads when an AuthProxyWorload is deleted (#200) (e11caed) - Automatically trigger pod rollout for appsv1 resources when AuthProxyWorkload changes. (#197) (3b0359b) - Separate terraform for project setup and permissions (#179) (8f43657) - Validate AuthProxyWorkload spec.selector field (#209) (98c460b) - Validate AuthProxyWorkload updates to prevent changes to the workload selector. (#211) (4304283) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Release PR Generate Bot action release-please[bot] <release-please[bot]@users.noreply.github.com>
Given an AuthProxyWorkload resource that has been applied to StatefulSet, DaemonSet,
ReplicaSet, and/or Deployment workloads, when that AuthProxyWorkload is updated, the operator
will add or modify an annotation on the PodTemplateSpec of that workload, causing k8s to
replace the workload's pods following the workload's rollout strategy, thus applying the
AuthProxyWorkload's configuration change.
Related to #187.
The code for this feature will be split across a few PRs for clarity. These are the first 4:
AutomaticRolloutEnabled
field to the AuthProxyWorkload resource.