-
Notifications
You must be signed in to change notification settings - Fork 40
define root level secrets and envFrom support #171
Conversation
@containscafeine, thank you for the pull request! We'll ping some people to review your PR. @surajssd, please review this. |
docs/file-reference.md
Outdated
<Kubernetes Secret Definition> | ||
``` | ||
|
||
The Kubernetes Secret resource is being reused here. More info: https://kubernetes.io/docs/api-reference/v1.6/#secret-v1-core |
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.
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 :(
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.
Good point, I have added some text around this just after this section, PTAL!
3ed3a23
to
9a5916c
Compare
This PR is doing two things,
@containscafeine do you mind breaking it down in two PRs? |
@surajssd agree, I didn't realize this was fixing 2 issues; it's going to be a pain to break this 😩 |
Not sure about that, it's hard to see what is needed for pure |
Well, the function names are clear enough to define which one is creating a secret, and which is populating envFrom. |
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.
need to break it down
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 |
docs/file-reference.md
Outdated
secretKeyRef: | ||
name: wordpress | ||
key: MYSQL_ROOT_PASSWORD | ||
- name: APPVERSION |
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 is APPVERSION?
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 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?
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.
We don't need anything here, we can just remove it, that is ok.
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.
ack, removed
docs/file-reference.md
Outdated
|----------|--------------| | ||
| string | yes | | ||
|
||
The name of the secret. This is a mandatory field. |
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 shouldn't be mandatory.
If there is only one secret it can be without name. Top level name will be used in that case
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.
fixed
docs/file-reference.md
Outdated
secretKeyRef: | ||
name: wordpress | ||
key: MYSQL_ROOT_PASSWORD | ||
- name: VERSION |
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.
version of what?
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.
same as the APPVERSION thing above, what should I replace this with?
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.
We can remove 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.
ack, removed
build is failing :-(
|
|
||
for _, s := range app.Secrets { | ||
secret := &api_v1.Secret{ | ||
ObjectMeta: api_v1.ObjectMeta{ |
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.
metav1.ObjectMeta
should be used instead of api_v1.ObjectMeta
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.
fixed
@kadel made the changes, PTAL |
|
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
cool now I can see this code now! |
- name: wordpress | ||
data: | ||
MYSQL_ROOT_PASSWORD: YWRtaW4= | ||
MYSQL_PASSWORD: cGFzc3dvcmQ= |
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.
Please specify what this value type is, because people will put plain text by default!
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 don't understand what do you mean ;-(
data is defined as base64 encoded in kubernetes
stringData is plaintext
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.
but people don't know 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.
fixed
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.
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= |
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.
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
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.
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
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.
oops i mapped it at wrong place.
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.
added comment down there!
- name: wordpress | ||
data: | ||
MYSQL_ROOT_PASSWORD: YWRtaW4= | ||
MYSQL_PASSWORD: cGFzc3dvcmQ= |
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.
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
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.
fixed, nice catch.
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 |
@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 › 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' />
...
... |
Forgot to push db.yaml, maybe that's why it failed, can you check now? |
I've just tested and it works for me now |
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.
LGTM, but wait for @surajssd approval before merge
meh, most of it is done, but need to take a final look, will update shortlyThis commit allows defining root level secrets, as well as
referencing those from the
envFrom
directive, which currentlyonly 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