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

fix: consistent imageCredentials in KubeEnforcer #110

Closed

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Aug 5, 2020

Description

  • KubeEnforcer used a different, older imagePullSecret name than the
    other charts

    • this would cause bugs and result in the image being unable to be
      pulled if one followed the directions and used the
      aqua-registry-secret name
    • 38f48d6 changed it from
      csp-registry-secret to aqua-registry-secret
    • and the docs said the default was aqua-image-pull-secret, which
      wasn't what it was set to nor the new name, it was neither
      • seemed like the Enforcer used this incorrect name in its docs
        until 38f48d6 as well
  • the create default was set to true, but is false in the other charts,
    so this sets it to false to make it consistent

  • it also specified an email for some reason, while none of the other
    charts do so

  • docs didn't specify repositoryUriPrefix or registry, but Enforcer
    and Server do, so add those for consistency

    • also because it's a bit misleading and confusing to not add all
      the configurable values in the docs if a "see more in values.yaml"
      or something isn't specified

Tags

38f48d6 changed the secret name in charts per the description.
Fixes inconsistencies introduced in #86, #93, and #94

Like #103, which also made a correction to the imagePullSecret naming, this type of bug is really easy to hit, get confused by, and spend a lot of unnecessary time on

Review Notes

This is a bugfix but is technically slightly breaking as well for anyone that relied on previous behavior. The KubeEnforcer chart still has yet to be published per #96 though

- KubeEnforcer used a different, older imagePullSecret name than the
  other charts
  - this would cause bugs and result in the image being unable to be
    pulled if one followed the directions and used the
    aqua-registry-secret name
  - 38f48d6 changed it from
    csp-registry-secret to aqua-registry-secret
  - and the docs said the default was aqua-image-pull-secret, which
    wasn't what it was set to nor the new name, it was neither
    - seemed like the Enforcer used this incorrect name in its docs
      until 38f48d6 as well

- the create default was set to true, but is false in the other charts,
  so this sets it to false to make it consistent
- it also specified an email for some reason, while none of the other
  charts do so
- docs didn't specify repositoryUriPrefix or registry, but Enforcer
  and Server do, so add those for consistency
  - also because it's a bit misleading and confusing to not add _all_
    the configurable values in the docs if a "see more in values.yaml"
    or something isn't specified
@agilgur5
Copy link
Contributor Author

This now has merge conflicts because #121 was merged beforehand, despite my comments on #121 pointing out the duplications it made to this PR

@KoppulaRajender
Copy link
Collaborator

fixed on all active branches, closing this PR

@agilgur5
Copy link
Contributor Author

fixed on all active branches

@KoppulaRajender the current branches actually don't have this code. #121 was merged (despite my comments) but that only duplicated one part of this PR and not the rest...

For instance, imageCredentials.create is still true for the KubeEnforcer, yet it's false in other charts like the Enforcer

The README here also still doesn't have repositoryUriPrefix or registry listed, despite those having defaults and being inconsistent with the Server and Enforcer per my opening message...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants