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

⚠ Remove TLSVerify/PullSecret fields #369

Conversation

thetechnick
Copy link
Contributor

Remove fields that will not have long term support committed to them to prepare for the v1.0 release.

Removed Fields:

  • spec.source.image.insecureSkipTLSVerify
  • spec.source.image.pullSecret

GH Issue Ref: #355

Draft due to pending e2e test changes.

@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 Sep 4, 2024
@joelanford
Copy link
Member

I am working on switching operator-controller to containers/image library here: operator-framework/operator-controller#1194

We are also planning to port similar logic to catalogd (and we separately need to decide if we want to somehow centralize/generalize the image unpacking library bits)

I bring this up because there will be a way to inject pull secrets and insecure registry configurations to resolve the e2e issues (e.g. by mounting a configmap that contains the analogous configuration files for insecure registries and credentials)

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.48%. Comparing base (dfe121e) to head (c6303d6).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
+ Coverage   34.41%   34.48%   +0.07%     
==========================================
  Files          17       17              
  Lines         738      722      -16     
==========================================
- Hits          254      249       -5     
+ Misses        452      443       -9     
+ Partials       32       30       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@everettraven
Copy link
Collaborator

I am working on switching operator-controller to containers/image library here: operator-framework/operator-controller#1194

We are also planning to port similar logic to catalogd (and we separately need to decide if we want to somehow centralize/generalize the image unpacking library bits)

I bring this up because there will be a way to inject pull secrets and insecure registry configurations to resolve the e2e issues (e.g. by mounting a configmap that contains the analogous configuration files for insecure registries and credentials)

@joelanford There was also changes to the way the e2es are configured in operator-controller recently that we might be able to update catalogd to follow. IIRC the changes in operator-controller made it so that we didn't need to skip TLS verification (since there is no way to set that in the ClusterExtension API)

@thetechnick
Copy link
Contributor Author

Trying to just get the TLS/Cert changes from operator-controller in, before continuing here:
#377

@thetechnick
Copy link
Contributor Author

PR to trust certs from image-registry: (Needs to be merged before and this rebased on top)
#377

@perdasilva perdasilva force-pushed the remove-insecure-and-pull-secret-fields branch from 182a8ac to 093582b Compare September 12, 2024 12:11
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2024
@perdasilva perdasilva force-pushed the remove-insecure-and-pull-secret-fields branch from 093582b to 26e7e4d Compare September 12, 2024 12:13
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2024
@perdasilva
Copy link
Contributor

@thetechnick I've taken the liberty of rebasing your PR (please double check that my conflict resolution was what you had in mind =D)

Remove fields that will not have long term support committed to them to
prepare for the v1.0 release.

Removed Fields:
- spec.source.image.insecureSkipTLSVerify
- spec.source.image.pullSecret

GH Issue Ref: operator-framework#355

Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the remove-insecure-and-pull-secret-fields branch from 26e7e4d to c6303d6 Compare September 12, 2024 12:32
@thetechnick thetechnick marked this pull request as ready for review September 12, 2024 12:55
@thetechnick thetechnick requested a review from a team as a code owner September 12, 2024 12:55
@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 Sep 12, 2024
Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2024
@perdasilva perdasilva added this pull request to the merge queue Sep 12, 2024
Merged via the queue into operator-framework:main with commit 1e962d2 Sep 12, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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