Skip to content

Conversation

@alebedev87
Copy link

@alebedev87 alebedev87 commented Aug 7, 2023

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

Note
kubernetes-sigs#3318 uses TLSOpts of the webhook server, it's a feature which is available in controller-runtime 0.14.x. The currently used controller-runtime doesn't have it, so we cannot specify ciphers (as the upstream PR did), only the minimum TLS version.

Test
Operator e2e (not FIPS): openshift/aws-load-balancer-operator#99
Manual test (FIPS):

# controller is there
$ oc -n aws-load-balancer-operator get pods
NAME                                                             READY   STATUS    RESTARTS   AGE
aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4             1/1     Running   0          2m19s
aws-load-balancer-operator-controller-manager-85d94d56c5-mjpq6   2/2     Running   0          5m11s

# it uses a manually built image (same as in the operator PR, see above)
$ oc -n aws-load-balancer-operator get pods aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4 -o yaml | yq .spec.containers[0].image
quay.io/aws-load-balancer-operator/aws-load-balancer-controller:8.10.234-tls12

# ingress webhook is on
$ oc -n aws-load-balancer-operator logs aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4 | grep 'webhook.*ingress'
{"level":"info","ts":1691706492.4913087,"logger":"controller-runtime.webhook","msg":"registering webhook","path":"/validate-networking-v1-ingress"}

# ingress resource got created with no TLS errors
$ oc -n echoserver get ing
NAME         CLASS   HOSTS   ADDRESS                                                                   PORTS   AGE
echoserver   alb     *       k8s-echoserv-echoserv-314d49d2d2-1425878010.us-east-1.elb.amazonaws.com   80      42s

@openshift-ci-robot openshift-ci-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Aug 7, 2023
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

The goal of this PR is to support environments where TLS versions other than 1.3 are still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

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.

@openshift-ci openshift-ci bot requested review from Miciah, ShudiLi and thejasn August 7, 2023 14:42
@openshift-ci
Copy link

openshift-ci bot commented Aug 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2023
@alebedev87 alebedev87 force-pushed the openshift-webhook-tls-min-version-2 branch from 7ce6271 to a6e019c Compare August 8, 2023 14:24
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

The goal of this PR is to support environments where TLS versions other than 1.3 are still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Upstream PR: kubernetes-sigs#3313

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.

@alebedev87 alebedev87 force-pushed the openshift-webhook-tls-min-version-2 branch 2 times, most recently from 6c1093d to 9a3e54e Compare August 9, 2023 09:31
@alebedev87 alebedev87 changed the title OCPBUGS-14846: new flag to configure the webhook server's minimum TLS version OCPBUGS-14846: webhook - set minimum TLS version back to 1.2 to support FIPS clusters Aug 9, 2023
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

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.

@alebedev87 alebedev87 changed the title OCPBUGS-14846: webhook - set minimum TLS version back to 1.2 to support FIPS clusters [WIP] OCPBUGS-14846: webhook - set minimum TLS version back to 1.2 to support FIPS clusters Aug 9, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2023
@alebedev87
Copy link
Author

kubernetes-sigs#3318 merged, we should carry patch this change, removing WIP.

/retitle OCPBUGS-14846: webhook - set minimum TLS version back to 1.2 to support FIPS clusters

@openshift-ci openshift-ci bot changed the title [WIP] OCPBUGS-14846: webhook - set minimum TLS version back to 1.2 to support FIPS clusters OCPBUGS-14846: webhook - set minimum TLS version back to 1.2 to support FIPS clusters Aug 9, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2023
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

kubernetes-sigs#3318 uses TLSOpts of the webhook server, it's a feature which is available in controller-runtime 0.14.x. The currently used controller-runtime doesn't have it, so we cannot specify ciphers, only the minimum TLS version.

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.

@ShudiLi
Copy link
Member

ShudiLi commented Aug 10, 2023

The issue seemed still there.
`
1.
% oc get clusterversion
NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
version 4.14.0-0.test-2023-08-10-015323-ci-ln-4n63sjk-latest True False 3h4m Cluster version is 4.14.0-0.test-2023-08-10-015323-ci-ln-4n63sjk-latest

% oc get -n aws-load-balancer-operator pods
NAME READY STATUS RESTARTS AGE
aws-load-balancer-controller-cluster-6b8c4db884-q45j5 1/1 Running 0 9m38s
aws-load-balancer-operator-controller-manager-574896dcdc-lnz6j 2/2 Running 0 14m
%

% oc create -f ingress-test-internet-facing.yaml
Error from server (InternalError): error when creating "ingress-test-internet-facing.yaml": Internal error occurred: failed calling webhook "vingress.elbv2.k8s.aws": failed to call webhook: Post "https://aws-load-balancer-controller-cluster.aws-load-balancer-operator.svc:9443/validate-networking-v1-ingress?timeout=10s": remote error: tls: protocol version not supported
%
`

@alebedev87
Copy link
Author

alebedev87 commented Aug 10, 2023

@ShudiLi : you are right. The new image with this controller's code needs to be added to ALBO. A PR for ALBO is coming.

UPD: but first, this PR has to be merged.

@alebedev87 alebedev87 force-pushed the openshift-webhook-tls-min-version-2 branch from 9a3e54e to 40bc8c8 Compare August 10, 2023 20:34
@openshift-ci
Copy link

openshift-ci bot commented Aug 10, 2023

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

kubernetes-sigs#3318 uses TLSOpts of the webhook server, it's a feature which is available in controller-runtime 0.14.x. The currently used controller-runtime doesn't have it, so we cannot specify ciphers, only the minimum TLS version.

Test
Operator e2e (not FIPS): openshift/aws-load-balancer-operator#99
Manual test (FIPS):

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.

@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

Note
kubernetes-sigs#3318 uses TLSOpts of the webhook server, it's a feature which is available in controller-runtime 0.14.x. The currently used controller-runtime doesn't have it, so we cannot specify ciphers (as the upstream PR did), only the minimum TLS version.

Test
Operator e2e (not FIPS): openshift/aws-load-balancer-operator#99
Manual test (FIPS):

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.

@openshift-ci-robot openshift-ci-robot removed the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Aug 10, 2023
@openshift-ci-robot openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Aug 10, 2023
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is invalid:

  • expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

Note
kubernetes-sigs#3318 uses TLSOpts of the webhook server, it's a feature which is available in controller-runtime 0.14.x. The currently used controller-runtime doesn't have it, so we cannot specify ciphers (as the upstream PR did), only the minimum TLS version.

Test
Operator e2e (not FIPS): openshift/aws-load-balancer-operator#99
Manual test (FIPS):

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.

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 10, 2023
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

Note
kubernetes-sigs#3318 uses TLSOpts of the webhook server, it's a feature which is available in controller-runtime 0.14.x. The currently used controller-runtime doesn't have it, so we cannot specify ciphers (as the upstream PR did), only the minimum TLS version.

Test
Operator e2e (not FIPS): openshift/aws-load-balancer-operator#99
Manual test (FIPS):

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.

@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-14846, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

Note
kubernetes-sigs#3318 uses TLSOpts of the webhook server, it's a feature which is available in controller-runtime 0.14.x. The currently used controller-runtime doesn't have it, so we cannot specify ciphers (as the upstream PR did), only the minimum TLS version.

Test
Operator e2e (not FIPS): openshift/aws-load-balancer-operator#99
Manual test (FIPS):

# controller is there
$ oc -n aws-load-balancer-operator get pods
NAME                                                             READY   STATUS    RESTARTS   AGE
aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4             1/1     Running   0          2m19s
aws-load-balancer-operator-controller-manager-85d94d56c5-mjpq6   2/2     Running   0          5m11s

# it uses a manually built image (same as in the operator PR, see above)
$ oc -n aws-load-balancer-operator get pods aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4 -o yaml | yq .spec.containers[0].image
quay.io/aws-load-balancer-operator/aws-load-balancer-controller:8.10.234-tls12

# ingress webhook is on
$ oc -n aws-load-balancer-operator logs aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4 | grep 'webhook.*ingress'
{"level":"info","ts":1691706492.4913087,"logger":"controller-runtime.webhook","msg":"registering webhook","path":"/validate-networking-v1-ingress"}

# ingress resource got created with no TLS errors
$ oc -n echoserver get ing
NAME         CLASS   HOSTS   ADDRESS                                                                   PORTS   AGE
echoserver   alb     *       k8s-echoserv-echoserv-314d49d2d2-1425878010.us-east-1.elb.amazonaws.com   80      42s

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.

@frobware
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2023
@openshift-merge-robot openshift-merge-robot merged commit 54f8897 into openshift:main Aug 11, 2023
@openshift-ci-robot
Copy link

@alebedev87: Jira Issue OCPBUGS-14846: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-14846 has been moved to the MODIFIED state.

In response to this:

The goal of this PR is to support environments where TLS versions 1.2 is still used, mainly FIPS enabled systems. The boringcrypto enabled builds check for the maximum FIPS TLS version which is set to TLS1.2.

Initial upstream PR: kubernetes-sigs#3313
Upstream follow-up PR: kubernetes-sigs#3318

Note
kubernetes-sigs#3318 uses TLSOpts of the webhook server, it's a feature which is available in controller-runtime 0.14.x. The currently used controller-runtime doesn't have it, so we cannot specify ciphers (as the upstream PR did), only the minimum TLS version.

Test
Operator e2e (not FIPS): openshift/aws-load-balancer-operator#99
Manual test (FIPS):

# controller is there
$ oc -n aws-load-balancer-operator get pods
NAME                                                             READY   STATUS    RESTARTS   AGE
aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4             1/1     Running   0          2m19s
aws-load-balancer-operator-controller-manager-85d94d56c5-mjpq6   2/2     Running   0          5m11s

# it uses a manually built image (same as in the operator PR, see above)
$ oc -n aws-load-balancer-operator get pods aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4 -o yaml | yq .spec.containers[0].image
quay.io/aws-load-balancer-operator/aws-load-balancer-controller:8.10.234-tls12

# ingress webhook is on
$ oc -n aws-load-balancer-operator logs aws-load-balancer-controller-cluster-fcbc7bbdb-7rnx4 | grep 'webhook.*ingress'
{"level":"info","ts":1691706492.4913087,"logger":"controller-runtime.webhook","msg":"registering webhook","path":"/validate-networking-v1-ingress"}

# ingress resource got created with no TLS errors
$ oc -n echoserver get ing
NAME         CLASS   HOSTS   ADDRESS                                                                   PORTS   AGE
echoserver   alb     *       k8s-echoserv-echoserv-314d49d2d2-1425878010.us-east-1.elb.amazonaws.com   80      42s

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.

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. jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants