Bump authn-k8s, conjur-api-go, summon and other dependencies #1446

szh · 2022-01-13T15:04:17Z

Desired Outcome

Use the latest versions of conjur-authn-k8s-client with support for JWT and tracing.
Use the latest version of other dependencies.

Implemented Changes

Updated versions of dependencies
Modified internal/providers/conjur/provider.go to use the new authn flow that supports JWT
Added go:build ignore flags to sample CRD go files to avoid compilation errors. These files are provided for documentation purposes and are not meant to be compiled with the project.

Connected Issue/Story

CyberArk internal issue link: ONYX-16170

Definition of Done

Using latest version of cyberark dependencies
All tests pass

Changelog

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a
CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code
changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR
A follow-up issue to update official docs has been filed here: insert issue ID
This PR does not require updating any documentation

Behavior

This PR changes product behavior and has been reviewed by a PO, or
These changes are part of a larger initiative that will be reviewed later, or
No behavior was changed with this PR

Security

Security architect has reviewed the changes in this PR,
These changes are part of a larger initiative with a separate security review, or
There are no security aspects to these changes

tzheleznyak

LGTM

szh · 2022-01-13T15:52:39Z

Waiting to update until new authn-k8s-client is released

go.mod

CHANGELOG.md

diverdane

Looks great!!! Thanks to you and @tzheleznyak for getting this moving along!
A couple of suggestions, but your call on those.

diverdane · 2022-01-14T16:09:58Z

internal/providers/conjur/provider.go

@@ -97,10 +95,17 @@ func ProviderFactory(options plugin_v1.ProviderOptions) (plugin_v1.Provider, err
 		log.Printf("Info: Conjur provider using Kubernetes authenticator-based authentication")

 		// Load the authenticator with the config from the environment, and log in to Conjur
-		if authenticator, err = loadAuthenticator(provider.AuthnURL, provider.Version, provider.Config); err != nil {
+		conjurAuthenticatorConf, err = authnConfig.NewConfigFromEnv()


Since you're making changes in this section of the code... This may be a personal preference sort of thing, but instead of using a but instead of bunch of if...if-else...if-else statements, we could use a switch:

switch { case provider.Username != "" && provider.APIKey != "": // Conjur provider using API key. Do stuff case tokenFile != "": // Conjur provider using access token. Do stuff case provider.AuthnURL != "" && strings.Contains(provider.AuthnURL, "authn-k8s"): // Conjur provider using authenticator. Do stuff default: // Return error }

For me, the switch is easier to see what's going on, but your call, don't want to hold up on this.

Looking at the Effective Go style guide:

It's therefore possible—and idiomatic—to write an if-else-if-else chain as a switch

So I'll make this change

Nice, it's much easier to read!

diverdane · 2022-01-14T16:20:09Z

CHANGELOG.md

+### Changed
+- Use latest version of conjur-authn-k8s-client which supports JWT loging and tracing.
+  [cyberark/secretless-broker#1446](https://github.com/cyberark/secretless-broker/pull/1446)
+


I'm wondering if we should include a CHANGELOG.md entry to say that we bumped K8s API version to 1.23.
This might be considered a user-facing change, since it affects in which Kubernetes clusters the SP can be run.
(Although this may only affect users who are using CRD/CRs to configure the SP... since I think that's why we're depending on Kubernetes API client and machinery?).

I believe it only affect CRDs

diverdane · 2022-01-14T16:20:59Z

go.mod

@@ -22,10 +22,10 @@ require (
 	github.com/stretchr/testify v1.7.0
 	golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3
 	gopkg.in/yaml.v2 v2.4.0
-	k8s.io/api v0.22.3
+	k8s.io/api v0.23.1


Nice work. These K8s updates are always so tricky.

codeclimate · 2022-01-14T17:06:13Z

Code Climate has analyzed commit 5adc53c and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Style	1

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 54.8% (0.0% change).

View more on Code Climate.

diverdane

LGTM!!!

diverdane · 2022-01-14T17:15:04Z

internal/providers/conjur/provider.go

@@ -97,10 +95,17 @@ func ProviderFactory(options plugin_v1.ProviderOptions) (plugin_v1.Provider, err
 		log.Printf("Info: Conjur provider using Kubernetes authenticator-based authentication")

 		// Load the authenticator with the config from the environment, and log in to Conjur
-		if authenticator, err = loadAuthenticator(provider.AuthnURL, provider.Version, provider.Config); err != nil {
+		conjurAuthenticatorConf, err = authnConfig.NewConfigFromEnv()


Nice, it's much easier to read!

szh mentioned this pull request Jan 13, 2022

Update authn-k8s-client and other dependencies #1445

Closed

13 tasks

szh force-pushed the bump-authn-k8s branch from 7f5f9b2 to 9214258 Compare January 13, 2022 15:11

szh marked this pull request as ready for review January 13, 2022 15:12

szh requested a review from a team as a code owner January 13, 2022 15:12

szh changed the title ~~Bump authn k8s~~ Bump authn-k8s, conjur-api-go, summon and other dependencies Jan 13, 2022

tzheleznyak previously approved these changes Jan 13, 2022

View reviewed changes

tzheleznyak reviewed Jan 13, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

szh dismissed tzheleznyak’s stale review via 4e55c33 January 13, 2022 21:58

codeclimate bot reviewed Jan 13, 2022

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

tzheleznyak force-pushed the bump-authn-k8s branch from 4e55c33 to 8d533a8 Compare January 14, 2022 13:13

szh self-assigned this Jan 14, 2022

tzheleznyak and others added 3 commits January 14, 2022 10:53

Bump authn k8s client and other dependencies

3376368

Add build tag to ignore example go files

f0c301c

Add fixing line for authn configuration

dd0cced

szh force-pushed the bump-authn-k8s branch from 8d533a8 to dd0cced Compare January 14, 2022 15:54

diverdane reviewed Jan 14, 2022

View reviewed changes

Use switch instead of if-else chain

5adc53c

szh requested a review from diverdane January 14, 2022 17:04

diverdane approved these changes Jan 14, 2022

View reviewed changes

szh merged commit eee2c18 into main Jan 14, 2022

szh deleted the bump-authn-k8s branch January 14, 2022 17:45

john-odonnell mentioned this pull request May 17, 2022

Update suite.yml for v1.17.6+suite.1 release cyberark/conjur-oss-suite-release#247

Merged

gl-johnson mentioned this pull request Aug 23, 2022

Update suite.yml for v18.0.0+suite1 release cyberark/conjur-oss-suite-release#250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump authn-k8s, conjur-api-go, summon and other dependencies #1446

Bump authn-k8s, conjur-api-go, summon and other dependencies #1446

szh commented Jan 13, 2022 •

edited

Loading

tzheleznyak left a comment

szh commented Jan 13, 2022

diverdane left a comment

diverdane Jan 14, 2022

szh Jan 14, 2022 •

edited

Loading

diverdane Jan 14, 2022

diverdane Jan 14, 2022

szh Jan 14, 2022

diverdane Jan 14, 2022

codeclimate bot commented Jan 14, 2022

diverdane left a comment

diverdane Jan 14, 2022

Bump authn-k8s, conjur-api-go, summon and other dependencies #1446

Bump authn-k8s, conjur-api-go, summon and other dependencies #1446

Conversation

szh commented Jan 13, 2022 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

tzheleznyak left a comment

Choose a reason for hiding this comment

szh commented Jan 13, 2022

diverdane left a comment

Choose a reason for hiding this comment

diverdane Jan 14, 2022

Choose a reason for hiding this comment

szh Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

diverdane Jan 14, 2022

Choose a reason for hiding this comment

diverdane Jan 14, 2022

Choose a reason for hiding this comment

szh Jan 14, 2022

Choose a reason for hiding this comment

diverdane Jan 14, 2022

Choose a reason for hiding this comment

codeclimate bot commented Jan 14, 2022

diverdane left a comment

Choose a reason for hiding this comment

diverdane Jan 14, 2022

Choose a reason for hiding this comment

szh commented Jan 13, 2022 •

edited

Loading

szh Jan 14, 2022 •

edited

Loading