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

start minio server with tls #4991

Merged
merged 9 commits into from
Jan 15, 2022
Merged

start minio server with tls #4991

merged 9 commits into from
Jan 15, 2022

Conversation

clyang82
Copy link
Contributor

@clyang82 clyang82 commented Dec 23, 2021 •
edited
Loading

Signed-off-by: clyang82 [email protected]

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Enable e2e connect MinIO with TLS enabled：

  • Generate certs for MinIO TLS
  • Update S3 Configure to have tls_config field

part of #4820

Verification

@clyang82 clyang82 force-pushed the minio_tls branch 2 times, most recently from fed7d0f to 584f38e Compare December 24, 2021 02:36
@clyang82
Copy link
Contributor Author

/assign @yeya24 Please take a look. Thanks.

GiedriusS
GiedriusS previously approved these changes Jan 3, 2022
View reviewed changes
matej-g
matej-g reviewed Jan 3, 2022
View reviewed changes
test/e2e/e2ethanos/services.go
Readiness: e2e.NewHTTPReadinessProbe("http", "/minio/health/ready", 200, 200),
Command: e2e.NewCommandWithoutEntrypoint("sh", "-c", fmt.Sprintf(strings.Join(commands, " && "), minioKESGithubContent, minioKESGithubContent, bktName, 8090)),
//TODO(@clyang82): This is a temporary workaround for https://github.com/efficientgo/e2e/issues/9
//Readiness: e2e.NewHTTPReadinessProbe("http", "/minio/health/ready", 200, 200),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if this could not increase flakiness somewhere down the road, since we'll be assuming minio is ready straight away 🤔. Could we try to do the readiness probe with the workaround I described in that commented issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean try to do the readiness probe with the workaround here? I think we should support this feature in efficientgo/e2e and then update here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how soon that will be fixed in the upstream, but the flakiness could be immediate. I would agree with either solution (workaround or fix it upstream) as long as it can be done ASAP.

Copy link
Contributor Author

@clyang82 clyang82 Jan 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@matej-g please look at the fix in upstream - efficientgo/e2e#19
I also have a workaround for the readiness probe so that we can merge it this PR firstly. I can open another PR once the upstream fix is acceptable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! Thanks a ton for addressing my concern, this is looking good now 👍. We can replace it with proper option once it lands in the upstream.

@clyang82 clyang82 force-pushed the minio_tls branch 2 times, most recently from 99f049c to 2921651 Compare January 10, 2022 02:16
@clyang82 clyang82 requested a review from GiedriusS January 10, 2022 07:27
matej-g
matej-g reviewed Jan 10, 2022
View reviewed changes
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

This now looks ready to me, I'm just not sure about the test failure, I cannot tell if it's related or not. Otherwise looking good to go 😊.

@clyang82
Copy link
Contributor Author

This now looks ready to me, I'm just not sure about the test failure, I cannot tell if it's related or not. Otherwise looking good to go 😊.

Thanks @matej-g . I did test in my local environment. it works well. so it seems it is not related with my fix.

@matej-g
Copy link
Collaborator

matej-g commented Jan 10, 2022

Thanks @matej-g . I did test in my local environment. it works well. so it seems it is not related with my fix.

From logs it seems the issue is with minio server not being ready, see:

2022-01-10T09:50:19.7465904Z 09:50:19 store-gw-1: level=info name=store-gw-1 ts=2022-01-10T09:50:19.741901135Z caller=grpc.go:131 service=gRPC/server component=store msg="listening for serving gRPC" address=:9091
2022-01-10T09:50:19.7468549Z 09:50:19 store-gw-1: level=info name=store-gw-1 ts=2022-01-10T09:50:19.742995743Z caller=http.go:93 service=http/server component=store msg="internal server is shutdown gracefully" err="bucket store initial sync: sync block: BaseFetcher: iter bucket: Server not initialized, please try again."

Since we're touching the readiness probe, I'm wondering if this is having any effect or the minio readiness is still flaky even in current main with the workaround 🤔. Perhaps rerunning the tests on this PR and seeing if they succeed would help us tell.

@clyang82
Copy link
Contributor Author

clyang82 commented Jan 12, 2022 •
edited
Loading

It passes the thanos end-to-end tests by adding sleep 1 to fix the flakiness of minio. I will update once the upstream PR is merged. so this PR is ready for reviewing again. Thanks.

matej-g
matej-g approved these changes Jan 12, 2022
View reviewed changes
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

🏆

@clyang82
Copy link
Contributor Author

@GiedriusS Can you help to review again? Thanks.

yeya24
yeya24 approved these changes Jan 15, 2022
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM

@yeya24 yeya24 merged commit 4d5f4d9 into thanos-io:main Jan 15, 2022
@clyang82 clyang82 deleted the minio_tls branch January 17, 2022 01:45
@clyang82
Copy link
Contributor Author

Thank you @yeya24

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

@matej-g matej-g matej-g approved these changes

@yeya24 yeya24 yeya24 approved these changes

@GiedriusS GiedriusS Awaiting requested review from GiedriusS

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@clyang82 @matej-g @GiedriusS @yeya24