-
Notifications
You must be signed in to change notification settings - Fork 42
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
Bump authn-k8s, conjur-api-go, summon and other dependencies #1446
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Waiting to update until new authn-k8s-client is released |
4e55c33
to
8d533a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!!! Thanks to you and @tzheleznyak for getting this moving along!
A couple of suggestions, but your call on those.
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, it's much easier to read!
### 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) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it only affect CRDs
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. These K8s updates are always so tricky.
Code Climate has analyzed commit 5adc53c and detected 1 issue on this pull request. Here's the issue category breakdown:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!!
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, it's much easier to read!
Desired Outcome
Implemented Changes
internal/providers/conjur/provider.go
to use the new authn flow that supports JWTgo: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
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
README
s) were updated in this PRBehavior
Security