-
Notifications
You must be signed in to change notification settings - Fork 96
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
Watch for Secrets (draft) #765
Conversation
Problem: Watch for secret updates Solves nginxinc#553 Solution: Watch for secret updates
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.
Overall I think this approach looks good.
return generated | ||
files = append(files, file.File{ | ||
Content: generated, | ||
Path: "/etc/nginx/conf.d/http.conf", |
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 either make this a const, or what if we updated our dataplane.Configuration to have an http
section, and in the future we'd add a stream
section, where each could contain their respective configs and file paths? That's probably out of scope for this, but maybe worth a ticket?
} | ||
|
||
func generateTLSCertPath(id dataplane.TLSCertID) string { | ||
return "/etc/nginx/secrets/" + string(id) + ".pem" |
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.
prefix is probably worth a const
for _, file := range files { | ||
f, err := os.Create(file.Path) | ||
if err != nil { | ||
return fmt.Errorf("failed to create server config %s: %w", file.Path, err) |
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.
return fmt.Errorf("failed to create server config %s: %w", file.Path, err) | |
return fmt.Errorf("failed to create file %s: %w", file.Path, err) |
|
||
_, err = f.Write(file.Content) | ||
if err != nil { | ||
return fmt.Errorf("failed to write server config %s: %w", file.Path, err) |
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.
return fmt.Errorf("failed to write server config %s: %w", file.Path, err) | |
return fmt.Errorf("failed to write file %s: %w", file.Path, err) |
@pleshakov approach looks good to me. Nice work simplifying the relationship capturing 👍 |
closing this in favor of a new non-draft PR |
Proposed changes
Problem:
Watch for secret updates
Solves #553
Solution:
Watch for secret updates
NOTE: this PR is a prototype. Need your comments about the design. (Please ignore low level details).
Key design decisions:
We can further refactor the code to get rid of relationship.Capturer completely in favour of using the Graph as a single source of truth about relationships among the resources.
Before creating a PR, run through this checklist and mark each as complete.