Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

define root level secrets and envFrom support #171

Merged
merged 4 commits into from
Jul 31, 2017

Conversation

concaf
Copy link
Collaborator

@concaf concaf commented Jul 20, 2017

meh, most of it is done, but need to take a final look, will update shortly

This commit allows defining root level secrets, as well as
referencing those from the envFrom directive, which currently
only supports configMaps.

Minor refactors have been made in kubernetes.go to handle envFrom
behavior for ConfigMaps and Secrets in a more consistent manner.

The examples, related READMEs, and the file reference doc have also
been modified to reflect the changes being brought upon.

fixes #128

@kedge-bot
Copy link
Collaborator

@containscafeine, thank you for the pull request! We'll ping some people to review your PR. @surajssd, please review this.

<Kubernetes Secret Definition>
```

The Kubernetes Secret resource is being reused here. More info: https://kubernetes.io/docs/api-reference/v1.6/#secret-v1-core
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aas Kedge is able to "simplify" Kubernetes deployments, it's unclear what's happening here in the documentation. We may need to be more verbose that these are base64 values rather than giving a link to a whole other doc to read. People may be putting their actual credentials / secrets without base64'ing them :(

Copy link
Collaborator Author

@concaf concaf Jul 24, 2017

Choose a reason for hiding this comment

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

Good point, I have added some text around this just after this section, PTAL!

@concaf concaf changed the title implement defining root level secrets define root level secrets and envFrom support Jul 24, 2017
@concaf concaf force-pushed the define_secrets branch 2 times, most recently from 3ed3a23 to 9a5916c Compare July 24, 2017 10:26
@surajssd
Copy link
Member

surajssd commented Jul 24, 2017

This PR is doing two things,

  • adding support for defining secrets
  • adding support for envFrom of secret

@containscafeine do you mind breaking it down in two PRs?

@concaf
Copy link
Collaborator Author

concaf commented Jul 24, 2017

@surajssd agree, I didn't realize this was fixing 2 issues; it's going to be a pain to break this 😩
I have covered this in the commit message, can we get this in this time?

@surajssd
Copy link
Member

Not sure about that, it's hard to see what is needed for pure secret and what is needed for envFrom.

@concaf
Copy link
Collaborator Author

concaf commented Jul 24, 2017

Well, the function names are clear enough to define which one is creating a secret, and which is populating envFrom.
It'll be a pain to break since the changes are in the same files, so will need to copy paste specific snippets and then make it work somehow.
It's doable but will take some time since the PR is huge. If you think spending time on this is worth it, I'll go ahead and do it. Let me know.

Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

need to break it down

@kadel
Copy link
Member

kadel commented Jul 26, 2017

I agree that it would be better to have this in two PRs, but its has already been done here. It will be a pain for @containscafeine to split this now. I think we can keep this PR

secretKeyRef:
name: wordpress
key: MYSQL_ROOT_PASSWORD
- name: APPVERSION
Copy link
Member

Choose a reason for hiding this comment

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

what is APPVERSION?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put this in since I was removing the secretKeyRef part from env variables, and that was the only variable defined under env, hence I needed something else to replace it to demonstrate the env field, and I put in a placeholder variable "APPVERSION". There is no significance to it.
Can you suggest what do I put in its place since we are losing the secretKeyRef part from the env?

Copy link
Member

Choose a reason for hiding this comment

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

We don't need anything here, we can just remove it, that is ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack, removed

|----------|--------------|
| string | yes |

The name of the secret. This is a mandatory field.
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be mandatory.
If there is only one secret it can be without name. Top level name will be used in that case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

secretKeyRef:
name: wordpress
key: MYSQL_ROOT_PASSWORD
- name: VERSION
Copy link
Member

Choose a reason for hiding this comment

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

version of what?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as the APPVERSION thing above, what should I replace this with?

Copy link
Member

Choose a reason for hiding this comment

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

We can remove this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack, removed

@kadel
Copy link
Member

kadel commented Jul 27, 2017

build is failing :-(

go build -ldflags="-w -X github.com/kedgeproject/kedge/cmd.GITCOMMIT=2fc7c41" -o kedge main.go
# github.com/kedgeproject/kedge/pkg/transform/kubernetes
pkg/transform/kubernetes/kubernetes.go:381: cannot use "github.com/kedgeproject/kedge/vendor/k8s.io/client-go/pkg/api/v1".ObjectMeta literal (type "github.com/kedgeproject/kedge/vendor/k8s.io/client-go/pkg/api/v1".ObjectMeta) as type "github.com/kedgeproject/kedge/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ObjectMeta in field value


for _, s := range app.Secrets {
secret := &api_v1.Secret{
ObjectMeta: api_v1.ObjectMeta{
Copy link
Member

Choose a reason for hiding this comment

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

metav1.ObjectMeta should be used instead of api_v1.ObjectMeta

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@concaf
Copy link
Collaborator Author

concaf commented Jul 31, 2017

@kadel made the changes, PTAL

@concaf
Copy link
Collaborator Author

concaf commented Jul 31, 2017

TODO: make sure #186 (comment) is handled
fixed

concaf added 2 commits July 31, 2017 15:43
This commit allows defining root level secrets, as well as
referencing those from the `envFrom` directive, which currently
only supports configMaps.

Minor refactors have been made in kubernetes.go to handle envFrom
behavior for ConfigMaps and Secrets in a more consistent manner.

The examples, related READMEs, and the file reference doc have also
been modified to reflect the changes being brought upon.

fixes kedgeproject#128
fixes kedgeproject#85
@surajssd
Copy link
Member

cool now I can see this code now!

- name: wordpress
data:
MYSQL_ROOT_PASSWORD: YWRtaW4=
MYSQL_PASSWORD: cGFzc3dvcmQ=
Copy link
Member

Choose a reason for hiding this comment

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

Please specify what this value type is, because people will put plain text by default!

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what do you mean ;-(

data is defined as base64 encoded in kubernetes

stringData is plaintext

Copy link
Member

Choose a reason for hiding this comment

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

but people don't know that!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@kadel kadel Jul 31, 2017

Choose a reason for hiding this comment

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

Most important thing is to explain this in file-reference not in the example.

Examples shouldn't supply documentation.

Once someone read file-reference they shouldn't require any other documentation to understand examples.

secrets:
- name: wordpress
data:
MYSQL_ROOT_PASSWORD: YWRtaW4=
Copy link
Member

Choose a reason for hiding this comment

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

The secret example is not deployable:

$ kedge create -f db.yaml -f web.yaml 
deployment "database" created
service "database" created
secret "wordpress" created
deployment "web" created
service "wordpress" created
 
$ k get pods
NAME                        READY     STATUS                                                    RESTARTS   AGE
database-1832364690-2lvr5   1/1       Running                                                   0          8s
web-4102769170-db46t        0/1       Couldn't find key DB_PASSWD in Secret secrete/wordpress   0          7s

Copy link
Member

@kadel kadel Jul 31, 2017

Choose a reason for hiding this comment

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

where did you got web.yaml?

only db is defined there no?

To be honest, that whole example doesn't make sense :-)
We should come up with a better example for file-reference. But that should be in different PR

Copy link
Member

Choose a reason for hiding this comment

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

oops i mapped it at wrong place.

Copy link
Member

Choose a reason for hiding this comment

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

added comment down there!

- name: wordpress
data:
MYSQL_ROOT_PASSWORD: YWRtaW4=
MYSQL_PASSWORD: cGFzc3dvcmQ=
Copy link
Member

Choose a reason for hiding this comment

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

The secret example is not deployable:

$ kedge create -f db.yaml -f web.yaml 
deployment "database" created
service "database" created
secret "wordpress" created
deployment "web" created
service "wordpress" created
 
$ k get pods
NAME                        READY     STATUS                                                    RESTARTS   AGE
database-1832364690-2lvr5   1/1       Running                                                   0          8s
web-4102769170-db46t        0/1       Couldn't find key DB_PASSWD in Secret secrete/wordpress   0          7s

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed, nice catch.

@surajssd
Copy link
Member

It is failing again:

$ k get pods
NAME                        READY     STATUS             RESTARTS   AGE
database-3281411170-6slkw   1/1       Running            0          3m
web-4102769170-bqtz8        0/1       CrashLoopBackOff   2          3m

$ k logs web-4102769170-bqtz8
WordPress not found in /var/www/html - copying now...
Complete! WordPress has been successfully copied to /var/www/html

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)


MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)
Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)

MySQL Connection Error: (1045) Access denied for user 'wordpress'@'172.17.0.7' (using password: YES)

Warning: mysqli::mysqli(): (HY000/1045): Access denied for user 'wordpress'@'172.17.0.7' (using password: YES) in - on line 22

@concaf
Copy link
Collaborator Author

concaf commented Jul 31, 2017

@surajssd not sure why, works for me -

$ minikube ssh
$ curl -L 10.0.0.41:8080
<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" lang="en-US" xml:lang="en-US">
<head>
        <meta name="viewport" content="width=device-width" />
        <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
        <meta name="robots" content="noindex,nofollow" />
        <title>WordPress &rsaquo; Installation</title>
        <link rel='stylesheet' id='buttons-css'  href='http://10.0.0.41:8080/wp-includes/css/buttons.min.
css?ver=4.8' type='text/css' media='all' />
...
...

@concaf
Copy link
Collaborator Author

concaf commented Jul 31, 2017

Forgot to push db.yaml, maybe that's why it failed, can you check now?

@kadel
Copy link
Member

kadel commented Jul 31, 2017

I've just tested and it works for me now

Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

LGTM, but wait for @surajssd approval before merge

@surajssd surajssd merged commit a090296 into kedgeproject:master Jul 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defining secrets
5 participants