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

Implements handling of insecure TLS connections in Mehkit #373

Closed
wants to merge 24 commits into from
Closed

Implements handling of insecure TLS connections in Mehkit #373

wants to merge 24 commits into from

Conversation

yash37158
Copy link
Member

@yash37158 yash37158 commented Sep 20, 2023

Description

This PR fixes #8619

Implements handling of insecure TLS connections in Meshkit.

When connecting to Kubernetes clusters with a kubeconfig that has the insecure-skip-tls-verify: true setting configured, Meshery Server will now respect that setting and ignore TLS/SSL issues.

This feature is implemented in MeshKit and includes the following acceptance tests:

  • Meshkit now enforces or ignores SSL errors on a per Kubernetes context basis, allowing import of a multi-context
    kubeconfig with the same or different insecure-skip-tls-verify settings per context.

  • A manifest of all attempted, ignored, and successful connections are returned to the client in a Meshery Event (/api/events). But I still need a better understanding of how can i achieve this, on digging I came to know that Meshery uses pub/sub for passing events. but I am not sure if that is correct also how can I return it client(/api/event) and populate the ui? if someone would like to contribute or have a better approach for passing the status to the client.

Here is the test results for the desired state:
Screenshot 2023-09-21 at 12 51 12 AM

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@welcome
Copy link

welcome bot commented Sep 20, 2023

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while peforming a commit.

@leecalcote
Copy link
Member

Whoo-hoo, @yash37158 👍

utils/kubernetes/client.go Outdated Show resolved Hide resolved
Copy link

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

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

@yash37158 The current behaviour is if you provide the flag but your kube config has the
certificate-authority-data for the cluster the client-go (k8s package) will ignore the flag, were as if you remove the certificate-authority-data and then use the flag, it will be respected.

This behaviour is consistent with kubectl as well as k8s-client-go.

So I don't think this change is required Or you can do it in this way, if the flag is present remove the certificate-authority-data.

@Philip-21
Copy link
Member

@yash37158 The current behaviour is if you provide the flag but your kube config has the certificate-authority-data for the cluster the client-go (k8s package) will ignore the flag, were as if you remove the certificate-authority-data and then use the flag, it will be respected.

This behaviour is consistent with kubectl as well as k8s-client-go.

So I don't think this change is required Or you can do it in this way, if the flag is present remove the certificate-authority-data.

@MUzairS15 i don't think its best he removes the certificate-authority-data . When users are running mesheryctl on a k8 cluster, they may perform tasks that require certificate-authority-data. .
What do you think ??

Copy link
Member

@Philip-21 Philip-21 left a comment

Choose a reason for hiding this comment

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

@yash37158 kindly work on your indents and newline spacing, meshkit is very sensitive to this.

@MUzairS15
Copy link

@Philip-21 I agree, but the testcases he have, doesn’t include certificate-data hence the tests passes, (which comply with the default behaviour of client-go)
If we use the flag along with having certificate-data unless it is removed manually (which is not good) the behaviour will be overriden.

@Philip-21
Copy link
Member

@Philip-21 I agree, but the testcases he have, doesn’t include certificate-data hence the tests passes, (which comply with the default behaviour of client-go) If we use the flag along with having certificate-data unless it is removed manually (which is not good) the behaviour will be overriden.

Alright . So what do you think he should do, should he leave the way it was or he should make the certificate-data. Which is better

@yash37158
Copy link
Member Author

yash37158 commented Sep 22, 2023

I appreciate everyone's review and suggestions. I'm working on the changes and will push them as soon as possible.
@Philip-21 @MUzairS15 @leecalcote

@yash37158
Copy link
Member Author

yash37158 commented Sep 25, 2023

@MUzairS15, if removing certificate-authority-data is a good approach as Philips said removing may affect the mesheryctl operations. how should i go about this ?

@yash37158
Copy link
Member Author

Hey @Philip-21 @MUzairS15, I have made the necessary changes suggested by @MUzairS15

Copy link

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

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

@yash37158 Thanks for working on this, going through reviews and making changes.

The proposed fix adds convenience, but because most of the users will be accustomed to what is already present in k8s The merge of this PR would require a PR explaining this behaviour in meshery docs.

utils/kubernetes/client.go Show resolved Hide resolved
utils/kubernetes/client.go Outdated Show resolved Hide resolved
utils/kubernetes/client.go Outdated Show resolved Hide resolved
utils/kubernetes/client.go Outdated Show resolved Hide resolved
utils/kubernetes/client.go Outdated Show resolved Hide resolved
yash37158 and others added 2 commits October 27, 2023 00:52
Co-authored-by: Aisuko <[email protected]>
Signed-off-by: Yash Sharma  <[email protected]>
Co-authored-by: Aisuko <[email protected]>
Signed-off-by: Yash Sharma  <[email protected]>
@yash37158
Copy link
Member Author

yash37158 commented Oct 26, 2023

Thanks for your review @Aisuko I have added the changes you asked for. are we good to merge this now?

@Aisuko
Copy link
Member

Aisuko commented Oct 26, 2023

@yash37158
Copy link
Member Author

Hey @Aisuko, I already signoff my latest commit. I have added your suggestion from the github itself. below is the screenshot attached. where did I go wrong?
Screenshot 2023-10-28 at 12 20 29 AM

utils/kubernetes/client.go Outdated Show resolved Hide resolved
utils/kubernetes/client.go Outdated Show resolved Hide resolved
utils/kubernetes/client.go Outdated Show resolved Hide resolved
utils/kubernetes/client_test.go Outdated Show resolved Hide resolved
utils/kubernetes/client_test.go Show resolved Hide resolved
utils/kubernetes/client_test.go Show resolved Hide resolved
utils/kubernetes/kompose/convert.go Outdated Show resolved Hide resolved
@yash37158
Copy link
Member Author

@Aisuko, Are we good now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support insecure-skip-tls-verify: true in kubeconfig
5 participants