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 CLI options to run metrics endpoint over HTTPS #2014

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

ConnorJC3
Copy link
Contributor

@ConnorJC3 ConnorJC3 commented Apr 19, 2024

Is this a bug fix or adding new feature?

New feature (and related bugfix)

What is this PR about? / Why do we need it?

Add options to run metrics endpoint over HTTPS

Adds the ability to run the metrics server via HTTPS by supplying a certificate and key.

Validate options in main.go

Adds missing Validate() call to main.go

What testing is done?

CI/manual/Added unit tests

$ AWS_REGION="us-west-2" ./bin/aws-ebs-csi-driver controller --http-endpoint=":6443" --metrics-cert-file=`pwd`/server.crt --metrics-key-file=`pwd`/server.key 
I0419 18:04:46.442557   29484 metrics.go:96] "Metric server listening" address=":6443" path="/metrics"
I0419 18:04:46.444976   29484 driver.go:68] "Driver Information" Driver="ebs.csi.aws.com" Version="v1.29.1"

$ curl -kv https://localhost:6443
*   Trying 127.0.0.1:6443...
* Connected to localhost (127.0.0.1) port 6443
* ALPN: curl offers h2,http/1.1
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN: server accepted h2
* Server certificate:
*  subject: C=XX; L=Default City; O=Default Company Ltd
*  start date: Apr 19 18:02:41 2024 GMT
*  expire date: Apr 17 18:02:41 2034 GMT
*  issuer: C=XX; L=Default City; O=Default Company Ltd
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
* using HTTP/2
* [HTTP/2] [1] OPENED stream for https://localhost:6443/
* [HTTP/2] [1] [:method: GET]
* [HTTP/2] [1] [:scheme: https]
* [HTTP/2] [1] [:authority: localhost:6443]
* [HTTP/2] [1] [:path: /]
* [HTTP/2] [1] [user-agent: curl/8.3.0]
* [HTTP/2] [1] [accept: */*]
> GET / HTTP/2
> Host: localhost:6443
> User-Agent: curl/8.3.0
> Accept: */*
> 
< HTTP/2 404 
< content-type: text/plain; charset=utf-8
< x-content-type-options: nosniff
< content-length: 19
< date: Fri, 19 Apr 2024 18:05:05 GMT
< 
404 page not found

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 19, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 19, 2024
Signed-off-by: Connor Catlett <[email protected]>
Copy link

Code Coverage Diff

File Old Coverage New Coverage Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/metrics/metrics.go 72.0% 66.7% -5.3

Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

Code itself lgtm other than missing options documentation page updates

Thank you for removing the 2 panics.

pkg/driver/options.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2024
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2024
@ConnorJC3
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit b977f78 into kubernetes-sigs:master Apr 19, 2024
19 checks passed
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants