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.

Sign up for GitHub

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

Jump to bottom

minio: Set secure=true to enable TLS by default #3168

Merged
merged 2 commits into from
Feb 27, 2020
Merged

minio: Set secure=true to enable TLS by default #3168

merged 2 commits into from
Feb 27, 2020

Conversation

discordianfish
Copy link
Member

@discordianfish discordianfish commented Feb 25, 2020 •
edited by jlewi
Loading

Not using TLS is a security concern, especially if using cloud storage
like S3. This should be set to secure to avoid people unknowingly not
using TLS.

To make the bundled minio still work, I've submitted
kubeflow/manifests#950 to set secure=false in
this case explicitly.

This change is Reviewable

@discordianfish
minio: Set secure=true to enable TLS by default
40191b7 
Not using TLS is a security concern, especially if using cloud storage
like S3. This should be set to secure to avoid people unknowingly not
using TLS.

To make the bundled minio still work, I've submitted
kubeflow/manifests#950 to set secure=false in
this case explicitly.
@discordianfish
Copy link
Member Author

Not sure how to read the test response. Is it failing because the python dependencies? If so, I'll put this on hold and try to get #3161 in first.

2 similar comments
@discordianfish
Copy link
Member Author

Not sure how to read the test response. Is it failing because the python dependencies? If so, I'll put this on hold and try to get #3161 in first.

@discordianfish
Copy link
Member Author

Not sure how to read the test response. Is it failing because the python dependencies? If so, I'll put this on hold and try to get #3161 in first.

@numerology
Copy link

Hi @discordianfish , sorry for the noisy test log. Usually we'll need to click into Raw build_log.txt to figure out what's going on there.

For this specific question I think it's more likely to be caused by backend server init failure.

@discordianfish
Copy link
Member Author

@numerology This makes sense, it probably can't connect to minion using TLS. I'm still haven't been able to figure out which manifests revision it uses though. If it's current master, merging kubeflow/manifests#950 should fix this test.

@discordianfish discordianfish mentioned this pull request Feb 26, 2020
Merged
@discordianfish
Copy link
Member Author

Ah great, okay it looks like updating the manifests here fixed the test errors. Should be ready to get merged now.

@discordianfish
Copy link
Member Author

/assign @IronPan

@IronPan
Copy link
Member

IronPan commented Feb 27, 2020

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@numerology
Copy link

/retest

@k8s-ci-robot k8s-ci-robot merged commit 5cb158d into kubeflow:master Feb 27, 2020
Bobgy added a commit that referenced this pull request Feb 28, 2020
@Bobgy
Revert "minio: Set secure=true to enable TLS by default (#3168)"
cf80663 
This reverts commit 5cb158d.
@Bobgy Bobgy mentioned this pull request Feb 28, 2020
Merged
Bobgy added a commit that referenced this pull request Feb 28, 2020
@Bobgy
Revert "minio: Set secure=true to enable TLS by default (#3168)" (#3192)
743746b 
This reverts commit 5cb158d.
k8s-ci-robot pushed a commit that referenced this pull request Feb 28, 2020
@rmgogogo
revert #3168 as it doesn't work, put myself to reviewer (#3191)
4cb81ea 
Co-authored-by: renmingu <[email protected]>
@discordianfish discordianfish deleted the api-service-minio-default-secure-true branch February 29, 2020 17:35
@discordianfish discordianfish restored the api-service-minio-default-secure-true branch February 29, 2020 17:36
@discordianfish
Copy link
Member Author

@Bobgy Why was this reverted?

@Bobgy
Copy link
Contributor

Bobgy commented Mar 1, 2020

We found a bug in recent release.
I will help you fix it and merge again after the release

@discordianfish
Copy link
Member Author

@Bobgy Can you be more specific? Maybe next time you revert a commit, you can mention the reason in the commit log, then I could already go ahead and look into fixing it.

@Bobgy
Copy link
Contributor

Bobgy commented Mar 2, 2020

@discordianfish I mentioned the reason in #3192 (comment) (Referenced comment)

Bobgy added a commit to Bobgy/pipelines that referenced this pull request Mar 5, 2020
@Bobgy
Revert "Revert "minio: Set secure=true to enable TLS by default (kube…
8435299 
…flow#3168)" (kubeflow#3192)"

This reverts commit 743746b.
@Bobgy Bobgy mentioned this pull request Mar 5, 2020
Merged
k8s-ci-robot pushed a commit that referenced this pull request Mar 12, 2020
@Bobgy
[Deployment] Add secure=false explicitly in manifests for better obse…
f2beb96 
…rvability (#3217)

* Revert "Revert "minio: Set secure=true to enable TLS by default (#3168)" (#3192)"

This reverts commit 743746b.

* Fix managed storage specific manifest

* Update pipeline.yaml

* Update client_manager.go
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
@discordianfish @Jeffwan
minio: Set secure=true to enable TLS by default (kubeflow#3168)
b9c46bf 
* minio: Set secure=true to enable TLS by default

Not using TLS is a security concern, especially if using cloud storage
like S3. This should be set to secure to avoid people unknowingly not
using TLS.

To make the bundled minio still work, I've submitted
kubeflow/manifests#950 to set secure=false in
this case explicitly.

* minio: secure=false in GCP & standalone manifests
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
@Bobgy @Jeffwan
Revert "minio: Set secure=true to enable TLS by default (kubeflow#3168)…
95bea5e 
…" (kubeflow#3192)

This reverts commit 5cb158d.
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
@rmgogogo @Jeffwan
revert kubeflow#3168 as it doesn't work, put myself to reviewer (kube…
4808bdd 
…flow#3191)

Co-authored-by: renmingu <[email protected]>
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
@Bobgy @Jeffwan
[Deployment] Add secure=false explicitly in manifests for better obse…
faa768a 
…rvability (kubeflow#3217)

* Revert "Revert "minio: Set secure=true to enable TLS by default (kubeflow#3168)" (kubeflow#3192)"

This reverts commit 743746b.

* Fix managed storage specific manifest

* Update pipeline.yaml

* Update client_manager.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IronPan IronPan Awaiting requested review from IronPan

@neuromage neuromage Awaiting requested review from neuromage

Assignees

@IronPan IronPan

Labels
approved lgtm size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@discordianfish @numerology @IronPan @k8s-ci-robot @Bobgy