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

feat: support TLS #95

Merged
merged 12 commits into from
Dec 17, 2020
Merged

feat: support TLS #95

merged 12 commits into from
Dec 17, 2020

Conversation

gxthrj
Copy link
Contributor

@gxthrj gxthrj commented Dec 14, 2020

related #52

@gxthrj gxthrj changed the title WIP: support TLS feat: support TLS Dec 15, 2020
@tokers
Copy link
Contributor

tokers commented Dec 16, 2020

@gxthrj Please resolve the conflicts.

@gxthrj
Copy link
Contributor Author

gxthrj commented Dec 16, 2020

@gxthrj Please resolve the conflicts.

resolved

@codecov-io
Copy link

codecov-io commented Dec 16, 2020

Codecov Report

Merging #95 (e0c6244) into master (53496e6) will increase coverage by 1.44%.
The diff coverage is 91.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   58.28%   59.73%   +1.44%     
==========================================
  Files          15       16       +1     
  Lines         501      524      +23     
==========================================
+ Hits          292      313      +21     
- Misses        185      187       +2     
  Partials       24       24              
Impacted Files Coverage Δ
pkg/ingress/apisix/tls.go 90.90% <90.90%> (ø)
cmd/ingress/ingress.go 74.66% <100.00%> (+0.34%) ⬆️

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 53496e6...e0c6244. Read the comment docs.

@gxthrj gxthrj requested a review from tokers December 16, 2020 13:53
@@ -102,6 +102,8 @@ func NewIngressCommand() *cobra.Command {
c.ApisixUpstream()
// ApisixService
c.ApisixService()
// ApisixTls
c.ApisixTls()
Copy link
Contributor

Choose a reason for hiding this comment

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

What about c.ApisixTLS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, change the method name to ApisixTLS

pkg/ingress/apisix/tls.go Show resolved Hide resolved

type ApisixTlsCRD ingress.ApisixTls

// Convert convert to apisix.Service from ingress.ApisixService CRD
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is not right.

Should be:

// Convert convert to  apisix.Ssl from ingress.ApisixTls CRD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"github.com/stretchr/testify/assert"
"gopkg.in/yaml.v2"
"k8s.io/api/core/v1"
"testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

Standard packages should be put at the beginning of import block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


func (c *ApisixTlsController) Run(stop <-chan struct{}) error {
if ok := cache.WaitForCacheSync(stop); !ok {
glog.Errorf("sync ApisixService cache failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have the pkg/log, we should no longer use glog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
name: apisixtlses.apisix.apache.org
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure whether the es suffix is right. If singular and plural homographs, use apisixtls.apisix.apache.org.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the requirement of CRD , requires to be plural.

samples/deploy/crd/v1beta1/ApisixTls.yaml Show resolved Hide resolved
pkg/ingress/apisix/tls.go Show resolved Hide resolved
pkg/ingress/apisix/tls.go Show resolved Hide resolved
pkg/ingress/controller/apisix_tls.go Show resolved Hide resolved
pkg/ingress/controller/apisix_tls.go Show resolved Hide resolved
@membphis membphis linked an issue Dec 17, 2020 that may be closed by this pull request
@gxthrj gxthrj merged commit 04ab348 into apache:master Dec 17, 2020
@gxthrj gxthrj deleted the kv/retry branch December 17, 2020 08:32
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.

feat: ApisixSSL support
3 participants