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

change yurthub's protocol from http to https #368

Closed

Conversation

wangchenglong01
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #361

Special notes for your reviewer:

Does this PR introduce a user-facing change?


other Note

@openyurt-bot
Copy link
Collaborator

@wangchenglong01: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #361

Special notes for your reviewer:

Does this PR introduce a user-facing change?


other Note

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openyurt-bot openyurt-bot added the kind/feature kind/feature label Jun 23, 2021
@openyurt-bot openyurt-bot requested review from Fei-Guo and kadisi June 23, 2021 16:08
@openyurt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wangchenglong01
To complete the pull request process, please assign rambohe-ch
You can assign the PR to them by writing /assign @rambohe-ch in a comment when ready.

The full list of commands accepted by this bot can be found 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

@openyurt-bot openyurt-bot added the size/M size/M: 30-99 label Jun 23, 2021
@openyurt-bot
Copy link
Collaborator

Welcome @wangchenglong01! It looks like this is your first PR to openyurtio/openyurt 🎉

@MIBc
Copy link
Member

MIBc commented Jun 24, 2021

If use https, the kubelet and other components config should be changed. We can point out how to change config of these components.

@@ -94,6 +97,9 @@ func Complete(options *options.YurtHubOptions) (*YurtHubConfiguration, error) {
HubAgentDummyIfName: options.HubAgentDummyIfName,
StorageWrapper: storageWrapper,
SerializerManager: serializerManager,
CAFile: options.CAFile,
CertFile: options.CertFile,
KeyFile: options.KeyFile,
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the establishment and configuration of TLS may be better done by yurthub, users do not need to pay attention to the underlying certificate configuration by default.

Copy link
Member

Choose a reason for hiding this comment

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

It's a good idea. I will improve it later.

@luckymrwang
Copy link
Member

If use https, the kubelet and other components config should be changed. We can point out how to change config of these components.

Yes, I'm collecting the changed config.

@rambohe-ch
Copy link
Member

@wangchenglong01 we need to keep proxyPort for http unchanged, and add a new port(like secureProxyPort) for https.

wangchenglong01 and others added 2 commits June 29, 2021 14:32
…enyurt into yurthub-https

# Conflicts:
#	cmd/yurthub/app/config/config.go
#	pkg/yurthub/server/server.go
@openyurt-bot openyurt-bot added size/L size/L: 100-499 and removed size/M size/M: 30-99 labels Jun 29, 2021
@rambohe-ch
Copy link
Member

If use https, the kubelet and other components config should be changed. We can point out how to change config of these components.

The background of support https for yurthub is here: #372

}

// NewYurtHubOptions creates a new YurtHubOptions with a default config.
func NewYurtHubOptions() *YurtHubOptions {
o := &YurtHubOptions{
YurtHubHost: "127.0.0.1",
YurtHubProxySecurePort: "10260",
Copy link
Member

Choose a reason for hiding this comment

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

maybe 10260 is used by k8s, how about 10268?

@@ -72,28 +76,33 @@ func Complete(options *options.YurtHubOptions) (*YurtHubConfiguration, error) {

hubServerAddr := net.JoinHostPort(options.YurtHubHost, options.YurtHubPort)
proxyServerAddr := net.JoinHostPort(options.YurtHubHost, options.YurtHubProxyPort)
proxySecureServerAddr := net.JoinHostPort(options.YurtHubHost, options.YurtHubProxySecurePort)
Copy link
Member

Choose a reason for hiding this comment

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

yurthub's https also need to listen on HubAgentDummyIfIP address

Copy link
Member

Choose a reason for hiding this comment

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

The latest code has been commited. Yurthub's https also listens on HubAgentDummyIfIP address

@@ -130,6 +148,9 @@ func (o *YurtHubOptions) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&o.HubAgentDummyIfIP, "dummy-if-ip", o.HubAgentDummyIfIP, "the ip address of dummy interface that used for container connect hub agent(exclusive ips: 169.254.31.0/24, 169.254.1.1/32)")
fs.StringVar(&o.HubAgentDummyIfName, "dummy-if-name", o.HubAgentDummyIfName, "the name of dummy interface that is used for hub agent")
fs.StringVar(&o.DiskCachePath, "disk-cache-path", o.DiskCachePath, "the path for kubernetes to storage metadata")
fs.StringVar(&o.CAFile, "ca-file", "", "the CA for yurthub to verify client")
fs.StringVar(&o.CertFile, "tls-cert-file", "", "the tls cert of yurthub")
fs.StringVar(&o.KeyFile, "tls-private-key-file", "", "the tls key of yurthub")
Copy link
Member

Choose a reason for hiding this comment

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

It's not convenient for user to setup server certifcates. I think Yurthub shoud generate server certificates by itself.

Copy link
Member

Choose a reason for hiding this comment

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

The latest code has been commited. Yurthub generated server certificates by itself.


// create a certificate manager for the yurthub server and run the csr approver for both yurthub
// and generate a TLS configuration
func genUseCertMgrAndTLSConfig(certificateMgr interfaces.YurtCertificateManager, certDir string, stopCh <-chan struct{}) (*tls.Config, error) {
Copy link
Member

Choose a reason for hiding this comment

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

It's more readable to prepare tls.config after client certificate ready instead of preparing when start server.

and the client certificate preparation ready is here: https://github.com/openyurtio/openyurt/blob/master/cmd/yurthub/app/start.go#L95

@@ -147,8 +147,8 @@ func Run(cfg *config.YurtHubConfiguration, stopCh <-chan struct{}) error {
klog.Infof("%d. new %s server and begin to serve, dummy proxy server: %s", trace, projectinfo.GetHubName(), cfg.YurtHubProxyServerDummyAddr)
Copy link
Member

Choose a reason for hiding this comment

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

please add YurtHubProxyServerSecureDummyAddr here

Client *kubernetes.Clientset
CertDNSNames []string
CertIPs []net.IP
SharedInformerFactory informers.SharedInformerFactory
Copy link
Member

Choose a reason for hiding this comment

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

The following fields should be deleted?

	CertFile                          string
	KeyFile                           string
	KubeConfig                        string
	RootCert                          *x509.CertPool
	Client                            *kubernetes.Clientset
	CertDNSNames                      []string
	CertIPs                           []net.IP
	SharedInformerFactory             informers.SharedInformerFactory

@rambohe-ch
Copy link
Member

@luckymrwang Would you be able to upload the detail logs of creating yurthub server certificates successfully when yurthub startup?

@openyurt-bot openyurt-bot added size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Jul 19, 2021
@wangchenglong01 wangchenglong01 deleted the yurthub-https branch November 23, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature kind/feature size/XL size/XL: 500-999
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Question]Listen protocol of yurthub is http, why not https
6 participants