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

support acme (let's encrypt) (close #2) #391

Merged
merged 13 commits into from
Dec 3, 2021
Merged

support acme (let's encrypt) (close #2) #391

merged 13 commits into from
Dec 3, 2021

Conversation

localvar
Copy link
Collaborator

@localvar localvar commented Dec 1, 2021

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2021

Codecov Report

Merging #391 (8d74483) into main (23cb833) will decrease coverage by 0.20%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #391      +/-   ##
==========================================
- Coverage   80.99%   80.79%   -0.21%     
==========================================
  Files          60       60              
  Lines        6966     6966              
==========================================
- Hits         5642     5628      -14     
- Misses       1036     1047      +11     
- Partials      288      291       +3     
Impacted Files Coverage Δ
pkg/cluster/cluster.go 50.59% <0.00%> (-2.05%) ⬇️
pkg/object/mqttproxy/client.go 79.87% <0.00%> (-1.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d266e79...8d74483. Read the comment docs.

| Name | Type | Description | Required |
| --------------- | ------------------------------------------ | ------------------------------------------------------------------------------------ | ---------------------------------- |
| email | string | An email address for CA account | Yes |
| directoryUrl | string | The endpoint of the CA directory | No (default to use Let's Encrypt) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated.


const (
// Category is the category of AutoCertManager.
// It is a business controller by now, but should be a system controller
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// It is a business controller by now, but should be a system controller
// It is a business controller by now, but should be a system controller.

)

type (
//AutoCertManager is the controller for Automated Certificate Management
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//AutoCertManager is the controller for Automated Certificate Management
//AutoCertManager is the controller for Automated Certificate Management.

Domains []DomainSpec `yaml:"domains" jsonschema:"required"`
}

// DomainSpec is the automate certificate management spec for a domain
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// DomainSpec is the automate certificate management spec for a domain
// DomainSpec is the automated certificate management spec for a domain.

// HTTP-01 challenges requires HTTP server to listen on port 80, but we don't
// know which HTTP server listen on this port (consider there's an nginx sitting
// in front of Easegress), so all HTTP servers need to handle HTTP-01 challenges.
if strings.HasPrefix(stdr.URL.Path, "/.well-known/acme-challenge/") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we can get the port from rules.spec.Port.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We cannot use this configuration here. The port needs to be accessible from the CA server, so even rules.spec.Port is 80, it does not mean the CA server could send a request to this HTTP server, because other reverse proxies like Nginx could get the request and forward it to another HTTP server who does not listen on port 80.

Copy link
Contributor

@xxx7xxxx xxx7xxxx Dec 3, 2021

Choose a reason for hiding this comment

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

Oh we have discussed this topic before, sure.

@xxx7xxxx xxx7xxxx merged commit 262061a into easegress-io:main Dec 3, 2021
@localvar localvar deleted the acme branch December 3, 2021 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants