plugin/k8s: support sidecar containers #2428
Conversation
There was a problem hiding this comment.
Yo! This looks like a good start 🎉 🚗 I haven't given this a shot yet, so giving some early code review feedback right now. Let me know if you have any questions.
Some high level things, it looks like maybe the PR for enabling dynamic configs got mixed into this and the commits got duplicated in the history? Rebasing to fix that up would be good to do before merge. Then I think some of Evans demo fixups and UI improvements got deleted which should go back as well
builtin/k8s/platform.go
Outdated
| } | ||
|
|
||
| // SidecarContainer describes the configuration for the sidecar container | ||
| type SidecarContainer struct { |
There was a problem hiding this comment.
Doing that would let people double-specify a bunch of fields: for example, they could give ProbePath in the top-level config, and then again under the Pod.Container struct.
I really don't love how much duplication there is here between fields in SidecarContainer and other fields elsewhere in the config. We explored a bunch of ideas to make this cleaner, but couldn't find anything that wouldn't introduce user confusion or destroy backwards compat. Open to other ideas though!
xiaolin-ninja
force-pushed
the
plugin/k8-sidecar
branch
from
77035c4
to
e42b30f
Compare
|
Ok I'm going to try to fix this weird merge history. I rebased before making the PR, but I think now that Izaak's PR is merged the rebase should be cleaner. |
xiaolin-ninja
force-pushed
the
plugin/k8-sidecar
branch
from
e42b30f
to
58875fe
Compare
izaaklauer
force-pushed
the
plugin/k8-sidecar
branch
from
58875fe
to
fdadd9a
Compare
Also some other minor refactoring
Which I feel like was harder to discover how to use than it needs to be
That is reused between the app container and sidecars
- Adding validators - Adding field docs - Hard deprecating top level Ports, and adding a helpful error
izaaklauer
force-pushed
the
plugin/k8-sidecar
branch
from
2b46af4
to
7db9a0f
Compare
|
Examples here: hashicorp/waypoint-examples#83 |
.changelog/2428.txt
Outdated
| @@ -0,0 +1,8 @@ | |||
| ```release-note:feature | |||
| plugin/k8s: Allows advanced users to add sidecar containers to apps using the k8s plugin config. | |||
There was a problem hiding this comment.
Nitpick but I don't think we need to say "advanced users" here, just "Allows users to".
builtin/k8s/platform.go
Outdated
| ), | ||
| )) | ||
| if err != nil { | ||
| return status.Errorf(codes.InvalidArgument, fmt.Sprintf("Invalid kubernetes platform plugin config: %s", err.Error())) |
There was a problem hiding this comment.
Small nit: since it's already Errorf you can just do this, I think 🤔
| return status.Errorf(codes.InvalidArgument, fmt.Sprintf("Invalid kubernetes platform plugin config: %s", err.Error())) | |
| return status.Errorf(codes.InvalidArgument, "Invalid kubernetes platform plugin config: %s", err.Error()) |
builtin/k8s/platform.go
Outdated
| Containers: []corev1.Container{container}, | ||
| // App container must have some kind of port | ||
| if len(appContainerSpec.Ports) == 0 { | ||
| ui.Output(fmt.Sprintf("No ports defined - defaulting to http on port %d", DefaultServicePort), terminal.WithWarningStyle()) |
There was a problem hiding this comment.
I think ui.Output should also let you format without fmt.Sprintf if I remember right
| ui.Output(fmt.Sprintf("No ports defined - defaulting to http on port %d", DefaultServicePort), terminal.WithWarningStyle()) | |
| ui.Output("No ports defined - defaulting to http on port %d", DefaultServicePort, terminal.WithWarningStyle()) |
Issue
RFC
cc: @izaaklauer
This PR introduces a new feature, allowing advanced Kubernetes users to add sidecar containers to their apps by adding a new “sidecar” stanza to the k8s plugin config.
This PR introduces several new structs, and breaks backwards compatibility by refactoring the "Ports" config.
See usage examples in RFC.