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 kompose.service.expose.tls-secret #896

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Add kompose.service.expose.tls-secret #896

merged 1 commit into from
Dec 20, 2017

Conversation

Code0x58
Copy link
Contributor

@Code0x58 Code0x58 commented Dec 17, 2017

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 17, 2017
@cdrage
Copy link
Member

cdrage commented Dec 18, 2017

Tests pass (despite us having a hard barrier-to-entry to creating them, haha), code looks great, name makes sense. This LGTM. 👍 thanks a lot @Code0x58 !

Edit:

So after looking at the code. Correct me if I'm wrong, but this essentially grabs a secret from Kubernetes from whatever name you set it as? (for this example, test-secret).

The problem with this is that it isn't exactly 1-1 conversion from Docker Compose to Kubernetes. You would already have to have a secret setup on Kubernetes in order to set this up, which doesn't fit with the whole "convert 1-1 from docker-compose to kubernetes".

I have no problem merging this in, but this makes #296 a higher priority now, as ideally, you'd be able to define this secret within Docker Compose and then it's automatically created within Kubernetes (without having to define it manually), does that make sense? Let me know your thoughts 👍

@Code0x58
Copy link
Contributor Author

Code0x58 commented Dec 19, 2017

Thanks, I definitely made use of CI on here while working out the tests - sempahore shows 12 builds.

I think ingress TLS secrets won't map nicely between docker-compose/swarm and Kubernetes as it would be an application concern in compose/swam, so it will probably always be stuck in labels/workarounds like this. Pod ImagePullSecrets are probably in the same boat.

I image that down the line when compose secrets are supported, you could specify your secret within the compose file, then refer to it in this label without any additional work - so this should be a stable approach. You may want to check that this refers to a defined secret, which could be an external one to work as it does now.

@cdrage
Copy link
Member

cdrage commented Dec 19, 2017

Yeah, there's absolutely no way to map it correctly without doing something really hacky. I agree that labels are the right way to go.

This LGTM 👍 Thanks for clarifying regarding the secret and TLS.

IMO this should have another review. @kadel @containscafeine @surajnarwade can you have a look?

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

@cdrage
Copy link
Member

cdrage commented Dec 20, 2017

Mergin' thanks @Code0x58 !

@cdrage cdrage merged commit c1f1813 into kubernetes:master Dec 20, 2017
@Code0x58 Code0x58 deleted the add-ingress-tls branch December 20, 2017 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants