Skip to content

Conversation

@enj
Copy link
Contributor

@enj enj commented Dec 14, 2021

Highlights from this dep bump:

  1. Made a copy of the v0.4.0 github.com/go-logr/stdr implementation
    for use in tests. We must bump this dep as Kube code uses a
    newer version now. We would have to rewrite hundreds of test log
    assertions without this copy.
  2. Use github.com/felixge/httpsnoop to undo the changes made by
    fix: force HTTP GET for redirect responses ory/fosite#636 for CLI based login flows. This is required for
    backwards compatibility with older versions of our CLI. A
    separate change after this will update the CLI to be more
    flexible (it is purposefully not part of this change to confirm
    that we did not break anything). For all browser login flows, we
    now redirect using http.StatusSeeOther instead of http.StatusFound.
  3. Drop plog.RemoveKlogGlobalFlags as klog no longer mutates global
    process flags
  4. Only bump github.com/ory/x to v0.0.297 instead of the latest
    v0.0.321 because v0.0.298+ pulls in a newer version of
    go.opentelemetry.io/otel/semconv which breaks k8s.io/apiserver.
    We should update k8s.io/apiserver to use the newer code.
  5. Migrate all code from k8s.io/apimachinery/pkg/util/clock to
    k8s.io/utils/clock and k8s.io/utils/clock/testing
  6. Delete testutil.NewDeleteOptionsRecorder and migrate to the new
    kubetesting.NewDeleteActionWithOptions
  7. Updated ExpectedAuthorizeCodeSessionJSONFromFuzzing caused by
    fosite's new rotated_secrets OAuth client field. This new field
    is currently not relevant to us as we have no private clients.

Signed-off-by: Monis Khan mok@vmware.com

Release note:

Pinniped components now use v0.23.1 of the Kubernetes libraries.

@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #914 (9599ffc) into main (69d5951) will increase coverage by 0.03%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #914      +/-   ##
==========================================
+ Coverage   79.00%   79.04%   +0.03%     
==========================================
  Files         133      133              
  Lines        9776     9771       -5     
==========================================
- Hits         7724     7723       -1     
+ Misses       1789     1785       -4     
  Partials      263      263              
Impacted Files Coverage Δ
cmd/pinniped/cmd/root.go 0.00% <ø> (-40.00%) ⬇️
internal/concierge/server/server.go 26.01% <ø> (-0.60%) ⬇️
...ntroller/impersonatorconfig/impersonator_config.go 93.42% <ø> (ø)
internal/controller/kubecertagent/kubecertagent.go 91.86% <ø> (ø)
...ller/supervisorconfig/federation_domain_watcher.go 98.13% <ø> (ø)
...sitestorage/authorizationcode/authorizationcode.go 88.52% <ø> (ø)
internal/oidc/kube_storage.go 0.00% <0.00%> (ø)
internal/plog/klog.go 87.50% <ø> (+37.50%) ⬆️
internal/concierge/impersonator/impersonator.go 82.84% <100.00%> (ø)
.../controller/supervisorstorage/garbage_collector.go 86.66% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69d5951...9599ffc. Read the comment docs.

issuerEndpointPtr = &issuerEndpoint

testLog := testlogger.New(t)
testLog := testlogger.NewLegacy(t) //nolint: staticcheck // old test with lots of log statements
Copy link
Contributor

Choose a reason for hiding this comment

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

You mentioned on a call that the changes to the logs are pretty small and inconsequential, so it makes more sense to just continue using the old format rather than changing hundreds of test assertions. How will we know if in the future there is a change that's more consequential that we should pick up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mentioned on a call that the changes to the logs are pretty small and inconsequential

Specifically, some white space changes in the logs and the ordering of keys is slightly different now. More than enough to break all of our test assertions in a way that is non-trivial to fix.

so it makes more sense to just continue using the old format rather than changing hundreds of test assertions.

Only for old, preexisting tests.

How will we know if in the future there is a change that's more consequential that we should pick up?

Two things:

  1. I marked testlogger.NewLegacy as deprecated so our linter will get upset if it is used. This is to discourage its use in new tests (they should use testlogger.New instead). This will give us coverage for both the old and new format.
  2. None of this actually matters for the real logs our code emits because those logs use the klog format, which is distinctly different from the stdr log format. I do not know why we chose this format for our tests, but it is what we have today (and it does assert that certain logs happened at certain times). As part of the audit work, I would like to migrate this stuff in a way that it is consistent with the real logs being emitted by our code (which should be some kind of JSON format at that point).

@enj enj disabled auto-merge December 16, 2021 18:25
@enj enj force-pushed the enj/i/bump_0.23.0 branch from b1f5e67 to 956d3c6 Compare December 17, 2021 02:15
Highlights from this dep bump:

1. Made a copy of the v0.4.0 github.com/go-logr/stdr implementation
   for use in tests.  We must bump this dep as Kube code uses a
   newer version now.  We would have to rewrite hundreds of test log
   assertions without this copy.
2. Use github.com/felixge/httpsnoop to undo the changes made by
   ory/fosite#636 for CLI based login flows.  This is required for
   backwards compatibility with older versions of our CLI.  A
   separate change after this will update the CLI to be more
   flexible (it is purposefully not part of this change to confirm
   that we did not break anything).  For all browser login flows, we
   now redirect using http.StatusSeeOther instead of http.StatusFound.
3. Drop plog.RemoveKlogGlobalFlags as klog no longer mutates global
   process flags
4. Only bump github.com/ory/x to v0.0.297 instead of the latest
   v0.0.321 because v0.0.298+ pulls in a newer version of
   go.opentelemetry.io/otel/semconv which breaks k8s.io/apiserver.
   We should update k8s.io/apiserver to use the newer code.
5. Migrate all code from k8s.io/apimachinery/pkg/util/clock to
   k8s.io/utils/clock and k8s.io/utils/clock/testing
6. Delete testutil.NewDeleteOptionsRecorder and migrate to the new
   kubetesting.NewDeleteActionWithOptions
7. Updated ExpectedAuthorizeCodeSessionJSONFromFuzzing caused by
   fosite's new rotated_secrets OAuth client field.  This new field
   is currently not relevant to us as we have no private clients.

Signed-off-by: Monis Khan <mok@vmware.com>
@enj enj force-pushed the enj/i/bump_0.23.0 branch from 956d3c6 to 9599ffc Compare December 17, 2021 02:15
@enj enj changed the title Update all deps to latest where possible, bump Kube deps to v0.23.0 Update all deps to latest where possible, bump Kube deps to v0.23.1 Dec 17, 2021
@enj enj enabled auto-merge December 17, 2021 02:18
@enj enj merged commit adf04d2 into vmware:main Dec 17, 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.

3 participants