-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add AgentAction controller #73
Conversation
e0756ad
to
274d9b1
Compare
Codecov Report
@@ Coverage Diff @@
## main #73 +/- ##
==========================================
- Coverage 58.00% 53.93% -4.07%
==========================================
Files 6 10 +4
Lines 700 1003 +303
==========================================
+ Hits 406 541 +135
- Misses 254 404 +150
- Partials 40 58 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
1bb8082
to
7eb9ea5
Compare
Add an image reference to the manager image (which has the controllers) to the bundle and then reference it when installing the operator. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
6b2afa9
to
e60c9ff
Compare
@@ -11,8 +11,3 @@ configMapGenerator: | |||
- files: | |||
- controller_manager_config.yaml | |||
name: manager-config | |||
|
|||
images: |
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.
Setting the image of the controller now happens in the bundle itself so that it can use the image distributed with the bundle.
config/samples/porter-hello.yaml
Outdated
@@ -9,5 +9,7 @@ spec: | |||
namespace: operator | |||
name: hello | |||
bundle: | |||
repository: getporter/porter-hello | |||
version: 0.1.1 | |||
repository: carolynvs/porter-hello-nonroot:v0.1.0 |
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.
I switched to a bundle that both takes a parameter and generates outputs to pick up on file permission problems with running as nonroot
@@ -9,5 +9,7 @@ spec: | |||
namespace: operator | |||
name: hello | |||
bundle: | |||
repository: getporter/porter-hello | |||
version: 0.1.1 | |||
repository: ghcr.io/getporter/test/porter-hello |
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 switches to a bundle that was built with the latest nonroot changes.
feed string | ||
version string | ||
}{ | ||
{name: "helm3", feed: "https://mchorfa.github.io/porter-helm3/atom.xml", version: "v0.1.14"}, | ||
{name: "helm3", url: "https://github.com/carolynvs/porter-helm3/releases/download", version: "v0.1.15-8-g864f450"}, |
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 is a build of helm3 that includes a patch for running in a nonroot bundle. MChorfa/porter-helm3#42 I'll submit that PR when we've fully vetted the nonroot change.
kustomize("edit", "set", "image", "manager="+img).In("config/manager").Run() | ||
// Set the image reference in porter.yaml so that the manager image is packaged with the bundle | ||
managerRef := resolveManagerImage() | ||
mgx.Must(shx.Copy("installer/vanilla.porter.yaml", "installer/porter.yaml")) |
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.
I can't wait for this to be settable by a custom build arg! 😁
I've split the installation controller in two, adding a new controller just to manage running the porter agent. The installation controller now only is reponsible for creating an AgentAction resource. This triggers the AgentAction controller to make a porter agent job to execute the command from the AgentAction. It's a bit of an extra layer of indirection but it will simplify additional controllers since they don't need to manage the porter agent job. It also enables users to request that the porter agent is run and can specify arbitrary commands, such as invoke. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
0dc745c
to
739b940
Compare
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.
I did a manual test (by accident) with the wrong version for a bundle in an installation resource. I could not delete the resource via kubectl delete -f
Worked as expected when I fixed the version.
api/v1/agentconfig_types.go
Outdated
@@ -54,7 +54,7 @@ func (c AgentConfigSpec) GetPorterImage() string { | |||
// We don't use a mutable tag like latest, or canary because it's a bad practice that we don't want to encourage. | |||
// As we test out the operator with new versions of Porter, keep this value up-to-date so that the default | |||
// version is guaranteed to work. | |||
version = "v1.0.0-alpha.8" | |||
version = "v1.0.0-alpha.12" |
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.
Should this be set via a const in api/v1/const.go?
args: ["installation", "apply", "installation.yaml"] | ||
files: | ||
# base64 encoded file contents | ||
installation.yaml: c2NoZW1hVmVyc2lvbjogMS4wLjAKbmFtZXNwYWNlOiBvcGVyYXRvcgpuYW1lOiBoZWxsbwpidW5kbGU6CiAgcmVwb3NpdG9yeTogZ2hjci5pby9nZXRwb3J0ZXIvdGVzdC9wb3J0ZXItaGVsbG8KICB2ZXJzaW9uOiAwLjIuMApwYXJhbWV0ZXJzOgogIG5hbWU6IGxsYW1hcyAK |
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.
I see why this needs to be base64 when creating the secret, but are we losing some usability?
I created an installation resource with an incorrect repository. I'm not able to delete the installation and an inspection of the AgentAction requires I base64 decode the installation.yaml
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.
You can always delete a resource by editing it and removing any finalizers on it.
I went with base64 because it allows people to add any files they need into the working directory of the agent, without us having known about it up-front. Do you have a suggestion for how you'd like to see it done differently while still being flexible?
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.
Here's a thought about the b64 encoding. The action.Spec.File data could be tested to see if it's base64. This can't be 100% accurate because some strings could meet the b64 encoding spec. For many string (like multiple line with white space, etc... a b64 encoding check will fail. Then the content can be encode for the user. This should allow a resource like this
---
apiVersion: porter.sh/v1
kind: AgentAction
metadata:
name: agentaction-sample
spec:
args:
- installation
- apply
- installation.yaml
files:
installation.yaml: |
schemaVersion: 1.0.0
namespace: operator
name: hello
bundle:
repository: ghcr.io/getporter/test/porter-hello
version: 0.2.0
parameters:
name: llamas
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.
I am concerned that it may be an indentation yaml bug generator because it isn't embedding a yaml document, it's a formatted yaml string embeded in a yaml file. Though either way it makes it much easier to introduce bugs in the document.
Since k8s users are already familiar with how to decode secrets, I am not too worried about the UX of doing exactly what secrets do for this. Let's go with following k8s existing conventions for now and if we need to revisit in later versions, I'm fine with that. I'd rather get this merged so we can try out a working operator and move forward with more controllers.
Also update the default version to alpha.13 which is the most recent release. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Dockerfile
Outdated
@@ -1,4 +1,4 @@ | |||
# syntax=docker/dockerfile:1.2 | |||
# syntax=docker/dockerfile-upstream:1.4.0-rc2 |
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.
I assume that we can use the official release version now
if job == nil { | ||
action.Status.Job = nil | ||
action.Status.Conditions = nil | ||
log.V(Log5Trace).Info("Cleared status because there is no current job") |
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.
nit: I find it helpful to have a return statement here so that it's clear nothing should happen if no job is found
err := r.Get(ctx, types.NamespacedName{Name: "default", Namespace: operatorNamespace}, systemCfg) | ||
if err != nil && !apierrors.IsNotFound(err) { | ||
return porterv1.AgentConfigSpec{}, errors.Wrap(err, "cannot retrieve system level porter agent configuration") | ||
} |
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 happens if the error is NotFound?
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.
There is nothing for the controller to do. It's not an error case. I can add a comment about 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.
Oh sorry I misunderstood which part of the code you were asking about. In this case, we are checking if there is a config defined, and using it if present. If it's not there, then that's not an error case. It just means we will use the defaults later on in that function.
}, | ||
{ | ||
Name: "JOB_VOLUME_PATH", | ||
Value: "/porter-shared", |
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.
maybe it would be helpful for consistency to declare a constant for 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.
Defer to @VinozzZ for final approval per outstanding feedback, otherwise LGTM
api/v1/agent_types.go
Outdated
@@ -0,0 +1,40 @@ | |||
package v1 | |||
|
|||
// AgentPhase are valid status of a Porter agent job |
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.
// AgentPhase are valid status of a Porter agent job | |
// AgentPhase are valid statuses of a Porter agent job |
api/v1/const.go
Outdated
|
||
// LabelRetry is a label applied to the resources created by the | ||
// Porter Operator, representing the retry attempt identifier. It is used to | ||
// help the operator determine if a resource has |
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.
Looks like the comment is cut off here
err = r.uninstallInstallation(ctx, log, inst) | ||
log.V(Log4Debug).Info("Reconciliation complete: A porter agent has been dispatched to uninstall the installation.") | ||
return ctrl.Result{}, err | ||
} else if isDeleted(inst) { | ||
// This is installation without a finalizer that was deleted | ||
// We remove the finalizer after we successfully uninstall (or someone is manually cleaning things up) | ||
// We remove the finalizer af |
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 just strikes me as an odd comment break point
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
e318f14
to
ca09693
Compare
I've split the installation controller in two, adding a new controller just to manage running the porter agent. The installation controller now only is responsible for creating an AgentAction resource. This triggers the AgentAction controller to make a porter agent job to execute the command from the AgentAction.
It's a bit of an extra layer of indirection but it will simplify additional controllers since they don't need to manage the porter agent job. It also enables users to request that the porter agent is run and can specify arbitrary commands, such as invoke.
Closes #39
This requires that you have porter installed from a custom build of porter with support for nonroot invocation images. The build won't pass until that PR is merged and a new tag created so we can reference it.
TODO