vmware-tanzu / pinniped

Pinniped is the easy, secure way to log in to your Kubernetes clusters.
https://pinniped.dev
Apache License 2.0
541 stars 65 forks source link

Remove deprecated deploy options #1926

Closed joshuatcasey closed 4 months ago

joshuatcasey commented 4 months ago

Remove deprecated deploy options.

Note that ytt schemas do not allow you to specify unrecognized options, so there's no need to check whether these additional deprecated values have been provided. This likely means we already have some dead code (now removed in this PR).

It also means we don't need code like this to check for deprecated/unrecognized/additional values:

#@ load("@ytt:data", "data")
#@ load("@ytt:assert", "assert")
#@
#@ deprecated_values = (
#@   'service_http_nodeport_port',
#@   'service_http_nodeport_nodeport',
#@   'service_http_loadbalancer_port',
#@   'service_http_clusterip_port',
#@   'deprecated_service_http_nodeport_port',
#@   'deprecated_service_http_nodeport_nodeport',
#@   'deprecated_service_http_loadbalancer_port',
#@   'deprecated_service_http_clusterip_port',
#@   'deprecated_insecure_accept_external_unencrypted_http_requests',
#@   'deprecated_log_format')
#@
#@ for val in deprecated_values:
#@   if hasattr(data.values, val):
#@     assert.fail('value "' + val + '" has been removed')
#@   end
#@ end
codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 29.39%. Comparing base (c0f1e40) to head (ad7df9f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1926 +/- ## ========================================== - Coverage 29.45% 29.39% -0.06% ========================================== Files 351 350 -1 Lines 58549 58519 -30 ========================================== - Hits 17243 17200 -43 - Misses 40790 40803 +13 Partials 516 516 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

joshuatcasey commented 4 months ago

Marking this as draft while I add the code changes

Done

cfryanr commented 4 months ago

@joshuatcasey, I reviewed and saw that you had gone too far in removing the Supervisor's listener configuration setting and the http listener. The Supervisor needs an http listener to support service mesh configurations where the service mesh provides the TLS termination. The part that was deprecated was only allowing the user to say that they want to skip a validation about how that port is bound.

Rather than add a ton of detailed PR comments about suggested changes, I added a new commit to this PR which puts back the Supervisor's http port but still removes the related deprecated configuration knob.

In that commit, I also removed a little more code related to the log formatting that seemed like it would be unused going forward related to the text format, and I did a tiny refactor in concierge/deployment.yaml to remove an unnecessary if statement.

The overall diffs of this PR vs. main should actually be smaller now.