-
Notifications
You must be signed in to change notification settings - Fork 12
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
OCPBUGS-28614: Use OpenShift generated trust bundle by default #129
Conversation
Hi @montaguethomas. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/retitle OCPBUGS-28614: Use OpenShift generated trust bundle by default |
@montaguethomas: This pull request references Jira Issue OCPBUGS-28614, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
@montaguethomas : can you please update |
/jira refresh |
@alebedev87: This pull request references Jira Issue OCPBUGS-28614, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 openshift-eng/jira-lifecycle-plugin repository. |
relatedImages: | ||
- image: quay.io/aws-load-balancer-operator/aws-load-balancer-controller@sha256:d8b5e9a91aca2a4a4de7f9bd2b614c5ba3d4cc62fa8967e94e9539fd7c1940a9 | ||
name: controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea why make bundle
updated this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a related image discovery mechanism which was added in v1.20.0. Since you used the latest (v1.33.0
) operator-sdk it filled the related images field of CSV. Overall I think it's a good thing however I'm wondering how you got into this situation as make bundle
was supposed to download you the v1.17.0
version of operator-sdk.
I see 2 ways of how we can move on:
- you can regenerate the bundle using the version from
Makefile
(mkae bundle
should do all on its own) - you can update
Makefile
with the latest operator-sdk and put it as a separate commit in your PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally work on a mac. The make target for operator-sdk
wasn't going to work correctly as it downloads the linux_amd64 version. The target does a test if the bin exists in path, so I simply installed it via brew. Of note, this target will redownload the bin every time if it's not in your path.
aws-load-balancer-operator/Makefile
Line 224 in 1a2c320
ifeq (, $(shell which operator-sdk 2>/dev/null)) |
Happy to adjust however you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makefile
is not fully Mac friendly indeed. It would be nice if you could do the same as for opm
target: code. That should cover both of your points: Mac downloads, avoid downloads when the binary is present in bin/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented and regenerated bundle using version specified in Makefile.
@alebedev87 Updated as requested. |
/jira refresh |
Tested it with 4.15.0-0.ci.test-2024-01-31-072323-ci-ln-03nqdbb-latest % oc get clusterversion
% oc -n aws-load-balancer-operator get cm aws-load-balancer-operator-trusted-cabundle -oyaml | grep -A1 "labels:"
% oc get csv aws-load-balancer-operator.v1.1.0 -n aws-load-balancer-operator -oyaml | grep -A3 "deployments:" % oc get csv aws-load-balancer-operator.v1.1.0 -n aws-load-balancer-operator -oyaml | grep -A1 " - name: TRUSTED_CA_CONFIGMAP_NAME" % oc get csv aws-load-balancer-operator.v1.1.0 -n aws-load-balancer-operator -oyaml | grep -A2 "mountPath: /etc/pki/tls/certs/albo-tls-ca-bundle.crt" % oc get csv aws-load-balancer-operator.v1.1.0 -n aws-load-balancer-operator -oyaml | grep -A3 " - configMap:"
% oc -n aws-load-balancer-operator get deployment aws-load-balancer-operator-controller-manager -oyaml | grep -A3 " - configMap:" |
/assign @alebedev87 |
- label: | ||
control-plane: controller-manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This label is new in CSV but it was there in manager.yaml
since the beginning so I suppose it's just a fixed bug in the latest operator-sdk.
docs/proxy.md
Outdated
4. _Optional_: make sure the operator is restarted every time the configmap contents change: | ||
|
||
```bash | ||
oc -n aws-load-balancer-operator rollout restart deployment/aws-load-balancer-operator-controller-manager | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part still needs to be mentioned because the operator doesn't watch for the volume mount's contents changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with this note. Lmk if it looks good.
/ok-to-test |
/test e2e-aws-proxy-operator Delete failed. |
/retest |
1 similar comment
/retest |
tested it with 4.15.0-0.ci.test-2024-02-26-082703-ci-ln-lzcspnt-latest % oc get clusterversion %oc -n aws-load-balancer-operator get pods % oc -n aws-load-balancer-operator get cm aws-load-balancer-operator-trusted-cabundle -oyaml | grep -A1 "labels:" % oc get csv aws-load-balancer-operator.v1.1.0 -n aws-load-balancer-operator -oyaml | grep "operators.operatorframework.io/builder" % oc get csv aws-load-balancer-operator.v1.1.0 -n aws-load-balancer-operator -oyaml | grep -A6 "deployments:" % oc get csv aws-load-balancer-operator.v1.1.0 -n aws-load-balancer-operator -oyaml | grep -A1 " - name: TRUSTED_CA_CONFIGMAP_NAME" % oc get csv aws-load-balancer-operator.v1.1.0 -n aws-load-balancer-operator -oyaml | grep -A2 "mountPath: /etc/pki/tls/certs/albo-tls-ca-bundle.crt" % %oc -n aws-load-balancer-operator get deployment aws-load-balancer-operator-controller-manager -oyaml | grep -A2 " - mountPath: /etc/pki/tls/certs/albo-tls-ca-bundle.crt" % oc -n aws-load-balancer-operator get deployment aws-load-balancer-operator-controller-manager -oyaml | grep -A3 " - configMap:" |
@montaguethomas : Can you please fix up commit to a single one? Also can you mention the bug in the commit message similar to what is done in the other commits? |
@montaguethomas : sorry about a conflict added. If you can fix the merge conflict and fix the commits up I think we can move on with the PR. |
@alebedev87 sorry for the delay. Merge conflict resolved. Are you able to use Github's Squash and Merge on the PR to deal witch squashing the commits into a single commit with an updated commit message as you desire (defaults to the PR's title)? |
@montaguethomas: I can use |
Squashed commit of the following: commit 7190196 Merge: d88ccce 0baccef Author: Thomas Montague <montague.thomas@gmail.com> Date: Tue Apr 23 11:43:43 2024 -0500 Merge branch 'main' into patch-1 commit d88ccce Author: Thomas Montague <montague.thomas@lmbbox.com> Date: Tue Feb 6 10:13:27 2024 -0700 cr commit 0049ecf Author: Thomas Montague <montague.thomas@gmail.com> Date: Tue Feb 6 12:10:11 2024 -0500 Update docs/proxy.md Co-authored-by: Andrey Lebedev <alebedev87@gmail.com> commit e57a790 Author: Thomas Montague <montague.thomas@lmbbox.com> Date: Tue Feb 6 10:08:42 2024 -0700 Update generated bundle. commit bd2e253 Author: Thomas Montague <montague.thomas@lmbbox.com> Date: Tue Feb 6 10:05:16 2024 -0700 Add support for multiple distros/arch in Makefile. commit e2ea121 Author: Thomas Montague <montague.thomas@lmbbox.com> Date: Tue Feb 6 09:57:35 2024 -0700 Add ignores for MacOSX commit d87c76a Author: Thomas Montague <tmontague@palantir.com> Date: Tue Jan 30 11:04:07 2024 -0700 cleanup. commit 39c4fca Author: Thomas Montague <tmontague@palantir.com> Date: Tue Jan 30 11:01:21 2024 -0700 update docs. commit ad42ad0 Author: Thomas Montague <tmontague@palantir.com> Date: Tue Jan 30 10:57:19 2024 -0700 Add updates to config and generate bundle. commit 57b202d Author: Thomas Montague <tmontague@palantir.com> Date: Tue Jan 30 10:36:16 2024 -0700 Revert "Update aws-load-balancer-operator.clusterserviceversion.yaml" This reverts commit b2d11b8. commit b2d11b8 Author: Thomas Montague <montague.thomas@gmail.com> Date: Fri Jan 26 05:06:27 2024 -0500 Update aws-load-balancer-operator.clusterserviceversion.yaml commit 925ac58 Author: Thomas Montague <montague.thomas@gmail.com> Date: Fri Jan 26 05:00:24 2024 -0500 Create aws-load-balancer-operator-trusted-cabundle-config_v1_configmap.yaml
@alebedev87 understood. I've rebased & squash merged changes. |
/lgtm |
[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 |
/retest |
@montaguethomas: 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. |
@montaguethomas: Jira Issue OCPBUGS-28614: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-28614 has been moved to the MODIFIED state. 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 openshift-eng/jira-lifecycle-plugin repository. |
Currently it is required to manually reconfigure the operator installation in order to have it use the OpenShift generated trusted cabundle.
Since this operator is solely for OpenShift [1], there is no reason to have this be a manual, non-default action.
[1]