Skip to content

Filter secret names for registry pod's sa #2165

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

Conversation

hasbro17
Copy link
Contributor

Description of the change:
During the registry server sync the image pull secrets from the catalogsource's spec.secrets are passed
unfiltered to the serviceaccount for the registry pod.

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: ditto-index
  namespace: default
spec:
  sourceType: grpc
  image: quay.io/haseeb_tariq/index-image:ditto-index-0.1
  secrets:
    - ""

Passing an empty string in the secrets list breaks serverside apply for the registry pod which gets created without the metadata.managedFields when it has an empty element in the imagePullSecrets list:

kind: Pod
apiVersion: v1
metadata:
  generateName: ditto-index-
  ...
  # No managedFields on resource
spec:
  imagePullSecrets:
    - {}
    - name: ditto-index-dockercfg-djmz7
...

This prevents the registry pod from being promoted via the SSA client when
there is an update to the index image:

    couldn't ensure registry server - error ensuring updated catalog source pod:
    : detected imageID change: error during update: failed to create manager for
    existing fields: failed to convert new object (/v1, Kind=Pod) to smd typed:
    .spec.imagePullSecrets: element 0: associative list with keys has an element
    that omits key field "name" (and doesn't have default value)

To fix this, the image pull secrets list is filtered for empty strings
before being set on the serviceaccount.

Motivation for the change:
Fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1945548

Similar to kubernetes-sigs/structured-merge-diff#130

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive

@openshift-ci openshift-ci bot requested review from anik120 and awgreene May 19, 2021 00:09
During the registry server sync the image pull secrets from the catalogsource's spec.secrets are passed
unfiltered to the serviceaccount for the registry pod.
Passing an empty string in the secrets list breaks serverside apply for the registry pod
with the following error:

failed to convert new object (/v1, Kind=Pod) to smd typed:
.spec.imagePullSecrets: element 0: associative list with keys has an element
that omits key field "name" (and doesn't have default value)

This prevents the registry pod from being promoted via the SSA client when
there is an update to the index image.

To fix this, the image pull secrets list is filtered for empty strings
before being set on the serviceaccount.

Signed-off-by: Haseeb Tariq <[email protected]>
@hasbro17 hasbro17 force-pushed the fix-ssa-error-from-empty-string branch from 24d9960 to ee40fd9 Compare May 19, 2021 00:11
@joelanford
Copy link
Member

/lgtm

Kudos on the description as well! Super helpful!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2021
@kevinrizza
Copy link
Member

/approve

@openshift-ci
Copy link

openshift-ci bot commented May 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hasbro17, kevinrizza

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 May 19, 2021
@openshift-merge-robot openshift-merge-robot merged commit c376e28 into operator-framework:master May 19, 2021
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants