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 deprecated logic and emit clear messages #2211

Merged
merged 12 commits into from
May 26, 2021

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented May 19, 2021

Summary

  • Forcing deprecated values to become unset during template rendering and providing a clear error message if they aren't.
  • Ensuring all messages are shown together to help users fix all failures at once instead of one at the time.
  • Removes no longer needed logic to handle deprecated values

Motivation for breaking changes

  • Complexity reduction of templates by removal of deprecation logic.
  • Future complexity reduction of schema.
    Our deprecation messages are triggered by detecting deprecated values being passed, which requires them to be accepted by the schema. So by ensuring all users have stopped using deprecated values when they upgrade to 1.0.0, we can in 2.0.0 remove them from the schema.

Breaking changes

All of the breaking changes will lead to clear error messages emitted by a fail statement in NOTES.txt. This happens before any communication to the k8s api-server has been made.

  • Previously deprecated options that were required to be falsy, are now required to be unset.
    • auth -> hub.config
    • hub.imagePullSecret -> imagePullSecret (new chart wide config)
    • singleuser.cloudMetadata.enabled -> singleuser.cloudMetadata.blockWithIptables (renamed / inverted value)
    • singleuser.imagePullSecret -> imagePullSecret (new chart wide config)
    • proxy.containerSecurityContext -> proxy.chp.containerSecurityContext (relocated)
    • proxy.networkPolicy -> proxy.chp.networkPolicy (relocated)
    • proxy.pdb -> proxy.chp.networkPolicy (relocated)
  • Previously deprecated options that were still functional are now required to be unset.
    • hub.extraConfigMap is now required to unset and configured using custom (renamed)
    • hub.uid is now required to be unset and configured using hub.containerSecurityContext.runAsUser (coalesced into k8s native config)

Closes #2157 by providing more visually standing out messages for warnings and breaking changes.
Closes #1412 by making NOTES.txt failures grouped into a single message, which I think is sufficient for now.

@consideRatio
Copy link
Member Author

consideRatio commented May 19, 2021

Verification of deprecation logic

This deprecation logic could fail in various ways, for example by the schema validation erroring before the NOTES.txt provides a more helpful message on how to migrate the configuration.

Due to this, I've used the following to test these changes. Everything seems okay.

helm template ./jupyterhub \
    --set hub.extraConfigMap.dummy=dummy \
    --set auth.dummy=dummy \
    --set proxy.containerSecurityContext.dummy=dummy \
    --set proxy.pdb.dummy=dummy \
    --set proxy.networkPolicy.dummy=dummy \
    --set hub.uid=0 \
    --set hub.imagePullSecret.dummy=dummy \
    --set singleuser.imagePullSecret.dummy=dummy \
    --set singleuser.cloudMetadata.enabled=true

#################################################################################
######   BREAKING: The config values passed contained no longer accepted    #####
######             options. See the messages below for more details.        #####
######                                                                      #####
######             To verify your updated config is accepted, you can use   #####
######             the `helm template` command.                             #####
#################################################################################

RENAMED: hub.extraConfigMap has been renamed to custom

The JupyterHub Helm chart's auth config has been reworked and requires changes.

The new way to configure authentication in chart version 0.11.0+ is printed
below for your convenience. The values are not shown by default to ensure no
secrets are exposed, run helm upgrade with --set global.safeToShowValues=true
to show them.

hub:
  config:
    WarningUnrecognizedConfig:
      dummy: '***'

For more details, please see the updated auth config documentation at:
https://zero-to-jupyterhub.readthedocs.io/en/latest/administrator/authentication.html

RENAMED: proxy.containerSecurityContext has been renamed to proxy.chp.containerSecurityContext

RENAMED: proxy.pdb has renamed to proxy.chp.pdb

RENAMED: proxy.networkPolicy has been renamed to proxy.chp.networkPolicy

RENAMED: hub.uid must as of 1.0.0 be configured using hub.containerSecurityContext.runAsUser

REMOVED: hub.imagePullSecret has been removed, but there is now a chart wide wide configuration named imagePullSecret

CHANGED: singleuser.imagePullSecret has been removed, but there is now a chart wide wide configuration named imagePullSecret

CHANGED: singleuser.cloudMetadata.enabled must as of 1.0.0 be configured using singleuser.cloudMetadata.blockWithIptables with the opposite value.

Use --debug flag to render out invalid YAML

@consideRatio consideRatio requested review from yuvipanda and manics May 19, 2021 01:16
@consideRatio consideRatio added this to the 1.0.0 milestone May 19, 2021
@consideRatio consideRatio deleted the branch jupyterhub:main May 22, 2021 17:29
@consideRatio consideRatio reopened this May 22, 2021
@consideRatio
Copy link
Member Author

consideRatio commented May 22, 2021

@yuvipanda @manics I've updated the PR summary to be a bit quicker and easier to read. This should be part of 1.0.0.

@manics
Copy link
Member

manics commented May 24, 2021

For clarity what do you think about changing HARD DEPRECATION: to BREAKING CHANGE: or CHANGED: as there doesn't seem to be a clear definition of "hard deprecation" e.g. https://www.google.com/search?q=%22hard+deprecation%22

@consideRatio
Copy link
Member Author

@manics absolutely - hard deprecation is not easy to understand or well defined! Perhaps we want to write a title of "BREAKING CHANGES", and then we write individual messages with "REMOVED" for example?

Btw, do you agree on this understanding of the word "Deprecated" - do we use that word whenever we mean something that still will remain functional but is an option that we mark as something that users should transition away from?

@manics
Copy link
Member

manics commented May 24, 2021

For me "deprecated" always means it still works for existing uses, but you probably shouldn't use it for anything new, and should consider updating existing uses to a replacement. Adding an adjective in front conveys (in an ill-defined manner since there's no standard terminology 😃) how soon you should plan to move to something else.

@consideRatio
Copy link
Member Author

consideRatio commented May 25, 2021

@manics okay updated! See #2211 (comment) for a rendered output of the breaking changes failure message.

I made changes to make the warning messages stand out visually, and here is such output rendered.

helm upgrade --install jupyterhub jupyterhub/ --dry-run --namespace test --set proxy.https.type=offload --set scheduling.userPlaceholder.replicas=1 --set scheduling.userPlaceholder.enabled=true

NOTES:
Thank you for installing JupyterHub!

Your release is named "jupyterhub" and installed into the namespace "test".

You can check whether the hub and proxy are ready by running:

 kubectl --namespace=test get pod

and watching for both those pods to be in status 'Running'.

You can find the public (load-balancer) IP of JupyterHub by running:

  kubectl -n test get svc proxy-public -o jsonpath='{.status.loadBalancer.ingress[].ip}'

It might take a few minutes for it to appear!

To get full information about the JupyterHub proxy service run:

  kubectl --namespace=test get svc proxy-public

If you have questions, please:

  1. Read the guide at https://z2jh.jupyter.org
  2. Ask for help or chat to us on https://discourse.jupyter.org/
  3. If you find a bug please report it at https://github.com/jupyterhub/zero-to-jupyterhub-k8s/issues

#################################################################################
######   WARNING: You are using user placeholders without pod priority      #####
######            enabled*, either enable pod priority or stop using the    #####
######            user placeholders** to avoid having placeholders that     #####
######            refuse to make room for a real user.                      #####
######                                                                      #####
######            *scheduling.podPriority.enabled                           #####
######            **scheduling.userPlaceholder.enabled                      #####
######            **scheduling.userPlaceholder.replicas                     #####
#################################################################################

#################################################################################
######   WARNING: proxy.https.enabled is set to false by default since      #####
######            version 0.10.0. It is now set to false but proxy.https    #####
######            seem to have been modified indicating you may want it     #####
######            enabled.                                                  #####
#################################################################################

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

LGTM, but will give @maincs a chance to merge based on prior review.

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

A couple of very minor suggestions, otherwise feel free to merge!

jupyterhub/templates/NOTES.txt Outdated Show resolved Hide resolved
jupyterhub/templates/NOTES.txt Outdated Show resolved Hide resolved
@consideRatio consideRatio force-pushed the pr/hard-deprecation branch 2 times, most recently from f1b9628 to f30cc97 Compare May 26, 2021 13:51
@consideRatio consideRatio force-pushed the pr/hard-deprecation branch from b7fda72 to 4f28655 Compare May 26, 2021 13:54
@consideRatio consideRatio merged commit 714a3d2 into jupyterhub:main May 26, 2021
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visually standing out warnings / deprecations etc in NOTES.txt System to deprecate Helm value config
3 participants