Skip to content
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 command to generate Kubernetes Secrets #202

Merged
merged 25 commits into from
Mar 21, 2023

Conversation

versilis
Copy link
Contributor

@versilis versilis commented Mar 15, 2023

This adds a new command akita kube secret which generates a Kubernetes secret configuration file that stores a user's base-64 encoded Akita API credentials.

To simplify file generation, I've used go's built-in templating utilities; akita-secret.tmpl is used as the template for creating the secret.

Example usage:

% ./bin/akita kube secret -n test -o ./tmp/deployments/configs/akita-secrets.yml
[INFO] Akita Agent 0.0.0
[INFO] Generated Kubernetes secret config to ./tmp/deployments/configs/akita-secrets.yml                                                                                                               

% less ./tmp/deployments/configs/akita-secrets.yml 
apiVersion: v1
kind: Secret
metadata:
  name: akita-secrets
  namespace: test
type: Opaque
data:
  akita-api-key: YXBrXzN1Y2h6WDyiOdyMzg2NldiM3Y0Q2Y=
  akita-api-secret: YmUzMzY5OTllNzNjc2DY3MmI5OWQzMTVmAnGaKmYzN2NlNjc2NWRiZDY4MzNjMWRkMzA4YjFjZDFlNWZkZg==
./tmp/deployments/configs/akita-secrets.yml lines 1-9/9 (END)

Signed-off-by: versilis <versilis@akitasoftware.com>
Signed-off-by: versilis <versilis@akitasoftware.com>
Signed-off-by: versilis <versilis@akitasoftware.com>
Signed-off-by: versilis <versilis@akitasoftware.com>
@versilis versilis self-assigned this Mar 15, 2023
@versilis versilis changed the base branch from main to versilis/kube March 15, 2023 22:34
@versilis versilis changed the title Add Kube Secret Command Add command to generate Kubernetes Secrets Mar 16, 2023
@versilis versilis requested a review from mgritter March 16, 2023 02:37
@versilis versilis marked this pull request as ready for review March 16, 2023 02:37
@versilis versilis added the 3 – Normal Priority Non-blocking review—please turn around quickly label Mar 16, 2023
Copy link
Contributor

@mgritter mgritter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good.

The must-fix items: let's add the ability to write to stdout, and use the default namespace. (And the redundant code in addAgentToECS.)

I would like to see us tackle YAML output with a library here on an easy case before we have to tackle parsing and rewriting a Deployment resource. Unless you were planning on using just string manipulation? But I don't insist on it.

cmd/internal/ecs/ecs.go Outdated Show resolved Hide resolved
cmd/internal/kube/kube.go Outdated Show resolved Hide resolved
cmd/internal/kube/secret.go Outdated Show resolved Hide resolved
cmd/internal/kube/secret.go Outdated Show resolved Hide resolved
cmd/internal/kube/template/akita-secret.tmpl Show resolved Hide resolved
cmd/internal/kube/secret.go Outdated Show resolved Hide resolved
)
_ = secretCmd.MarkFlagRequired("namespace")

secretCmd.Flags().StringVarP(&output, "output", "o", "akita-secret.yml", "File to output the generated secret.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have come up in the design review -- sorry. I think the correct default, which is most idiomatic to Kubernetes tools, is to print to standard output.

The idiomatic usage we are aiming for is something like "akita kube secret | kubectl apply -f -"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented printing in 5acfe06.

To avoid printing pre-run info logs, I've overriden the root command's PersistentPreRun handler. I've also had to make some updates to how we initialize telemetry which I've added as part of this commit: df81990

func init() {
var err error

secretTemplate, err = template.ParseFS(templateFS, "template/akita-secret.tmpl")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly OK with using a template here. But, I think this will be much harder to pull off for the next command, and I would like the two implementations to be consistent. If we have to have YAML parsing and output, let's start here with the easy case. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, there's a discrepancy between the Kubernetes openapi spec and the Go packages API model representation. Here's a link to the issue that covers it: kubernetes/kubernetes#109427

I've opened a separate PR to address using the Kubernetes API for generating secrets: #204

cmd/internal/kube/secret.go Outdated Show resolved Hide resolved
cmd/internal/kube/kube.go Show resolved Hide resolved
Comment on lines 39 to 40
// Output the generated secret to the console
printer.RawOutput(output)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think I was unclear. The two uses cases are:

  1. Apply directly
    akita kube secret | kubectl apply -f -

  2. Apply via a file (convenience, they could always pipe to a file.)
    akita kube secret -f mysecret.yaml
    kubectl apply -f mysecret.yaml

In case #2 we should not print to standard output as well, it should be one or the other. We can support this in a few different ways, I don't much care whether (a) standard out is the default if -f not specified, or (b) -f - writes to standard output..

I kind of think like in case #1 we should not write the file that was not asked for as well.

Come talk to me if what should happen in these two cases are still unclear.

}

// Creates a file at the give path to be used for storing of the generated Secret config
// If any child dicrectories do not exist, it will be created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment correct? I think you mean "it will not be created".

@@ -127,6 +128,7 @@ func runTCPFlowTestCase(c tcpFlowTestCase) error {
}

func TestTCPFlow(t *testing.T) {
telemetry.Init(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand the need here and in pase_http_test since the error functions in parsing send telemetry.

Is there a fix to telemetry that checks whether uninitialized instead? If it's not easy to do, we can keep this, it just seems a bit odd to be initializing telemetry in a situation where we don't really want it.

Copy link
Contributor

@mgritter mgritter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@versilis versilis merged commit a87238f into versilis/kube Mar 21, 2023
@versilis versilis deleted the versilis/kube-secret branch March 21, 2023 23:52
This was referenced Mar 24, 2023
versilis added a commit that referenced this pull request Mar 27, 2023
This adds two new commands, `akita kube inject` and `akita kube secret`,
for simplifying the process of installing Akita as a sidecar in
Kubernetes Deployments.

Changes include:
- #202
- #207
- #206
---------

Signed-off-by: versilis <versilis@akitasoftware.com>
Co-authored-by: Mark Gritter <mgritter@akitasoftware.com>
Co-authored-by: Jed Liu <liujed@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 – Normal Priority Non-blocking review—please turn around quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants