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 domain support for certgen.sh #3808

Merged
merged 1 commit into from
Apr 26, 2022
Merged

add domain support for certgen.sh #3808

merged 1 commit into from
Apr 26, 2022

Conversation

lwabish
Copy link
Contributor

@lwabish lwabish commented Apr 21, 2022

What type of PR is this?
/kind bug

What this PR does / why we need it:
the original script failed when providing a domain name.
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@kubeedge-bot kubeedge-bot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 21, 2022
@kubeedge-bot kubeedge-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 21, 2022
Copy link
Member

@fisherxu fisherxu left a comment

Choose a reason for hiding this comment

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

@lwabish Why do you build the certs manually instead of using the certs generted by cloudcore?

@lwabish
Copy link
Contributor Author

lwabish commented Apr 21, 2022

@lwabish Why do you build the certs manually instead of using the certs generted by cloudcore?

I was following the Enable kubectl logs Feature steps after install cloudcore and init my edge node. @fisherxu

@fisherxu
Copy link
Member

Ok, it should only use the stream func.

@lwabish
Copy link
Contributor Author

lwabish commented Apr 21, 2022

Ok, it should only use the stream func.

yes,indeed. So should I leave the genCert() funciton as it was?

@fisherxu
Copy link
Member

stream is only used to the communition between cloudcore and apiserver, I think the domain name is used to communicate between cloudhub and edgehub, so it's no need to add the domain name in stream's certs. And genCert haven't been used now..

@lwabish
Copy link
Contributor Author

lwabish commented Apr 22, 2022

oh I see.
In this case, I deployed cloudcore in k8s,so I need to use cloudcore.kubeedge to generate the stream cert so that apiserver can locate cloudcore, is this right?

@kubeedge-bot
Copy link
Collaborator

@ramezanius: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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.

@fisherxu
Copy link
Member

In this case, I deployed cloudcore in k8s,so I need to use cloudcore.kubeedge to generate the stream cert so that apiserver can locate cloudcore, is this right?

Yes, if the cloudocre domain used for edgeside, the domain name maybe a public domain. If apiserver connects to the cloudcore, it may not have to use that public domin, domain name incluster should be also fine, like svc.ns.cluster.local.

Anyway, set the domain name in stream's certs is reasonable, and can we set another env value like CLOUDCORE_DOMAINS? And the docs also need to be updated :) @lwabish

Copy link
Member

@fisherxu fisherxu left a comment

Choose a reason for hiding this comment

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

Looks good, and could you please squash the 4 commits? Thanks.

@lwabish
Copy link
Contributor Author

lwabish commented Apr 26, 2022

previous commits squashed.
is the "fetch upstream commit" necessary to be squashed too?

@fisherxu
Copy link
Member

previous commits squashed. is the "fetch upstream commit" necessary to be squashed too?

Yes, fetch upstream commit is no needed here :)

1. when CLOUDCOREIPS env contains spaces, `-z ${CLOUDCOREIPS}` could lead to error, double quote is indispensable.
2. in some cases domain names are provided when running `./certgen.sh stream`

Signed-off-by: lwabish <wubw@pku.edu.cn>
@lwabish
Copy link
Contributor Author

lwabish commented Apr 26, 2022

previous commits squashed. is the "fetch upstream commit" necessary to be squashed too?

Yes, fetch upstream commit is no needed here :)

done

@fisherxu
Copy link
Member

Copy link
Member

@fisherxu fisherxu left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2022
@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fisherxu, ramezanius

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

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2022
@kubeedge-bot kubeedge-bot merged commit 089d580 into kubeedge:master Apr 26, 2022
@lwabish
Copy link
Contributor Author

lwabish commented Apr 26, 2022

Thanks, and would you like to update the docs? Ref: https://github.com/kubeedge/website/blob/master/content/en/docs/setup/keadm.md#enable-kubectl-logs-feature

sure, i'll work on it later.
I also found that the stream certs of cloudcore deployed using helm was generated within the helm chart, running certgen.sh stream is not needed in this case. So maybe I could also try improving the doc about stream certs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants