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

Gardener config credential repository #55

Merged
merged 32 commits into from
Sep 2, 2022
Merged

Gardener config credential repository #55

merged 32 commits into from
Sep 2, 2022

Conversation

jschicktanz
Copy link
Contributor

@jschicktanz jschicktanz commented Aug 11, 2022

What this PR does / why we need it:
Add Gardener config credential repository which can read credentials in a data format used by the Gardener Pipelines (see https://github.com/gardener/cc-utils/tree/master/model). Credentials can be handed to the client either via local files or a http server (see https://github.com/gardener/cc-utils/blob/master/ccc/secrets_server.py#L29).

Which issue(s) this PR fixes:
Fixes #34

Special notes for your reviewer:

Release note:

- Add Gardener config credential repository

@gardener-robot
Copy link
Contributor

@jschicktanz You need rebase this pull request with latest master branch. Please check.

@Skarlso
Copy link
Contributor

Skarlso commented Aug 12, 2022

Hello 👋 :)

Please comment exported fields and try using fmt.Errorf for returned errors instead of simple return err to aid understanding where an error came from. :) Also, try avoiding names such as gardenercfg_cpi for packages and import statements. Go is camelCased/mixedCased.

Thanks! :)

@gardener-robot
Copy link
Contributor

@jschicktanz You need rebase this pull request with latest master branch. Please check.

@jschicktanz jschicktanz requested a review from mandelsoft August 18, 2022 14:56
pkg/utils/url.go Show resolved Hide resolved
@gardener-robot
Copy link
Contributor

@enrico-kaack-comp, @reshnm You have pull request review open invite, please check

@jschicktanz jschicktanz requested a review from mandelsoft August 22, 2022 09:32

r.creds = map[string]core.Credentials{}
for _, cred := range creds {
credName := cred.Name()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! :) That's better. :)

Skarlso
Skarlso previously approved these changes Aug 26, 2022
@Skarlso
Copy link
Contributor

Skarlso commented Aug 26, 2022

@jschicktanz Yo 👋 :) I resolved the conflict for you :)

@jschicktanz
Copy link
Contributor Author

@jschicktanz Yo 👋 :) I resolved the conflict for you :)

Thanks @Skarlso, and sry for not responding. I had to completely setup my local machine from scratch because of a failed OS update🙈

"password": "123",
}

creds := `{
Copy link
Contributor

Choose a reason for hiding this comment

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

may be rename it to something like gardenconfig. Creds may be misunderstood as credentials in the sense of a credential context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


specdata := fmt.Sprintf(specTemplate, svr.URL, local.Plaintext)

repo, err := defaultContext.RepositoryForConfig([]byte(specdata), nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to instantiate the repository, if the server is currently not available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


Expect(credentials.Properties()).To(Equal(props))
})

Copy link
Contributor

Choose a reason for hiding this comment

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

you should test whether the provided credentials are properly propagated to their consumer ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Expect(credentials.Properties()).To(Equal(props))
Expect(credentialsFromRepo).To(Equal(expectedCreds))

credSrc, err := defaultContext.GetCredentialsForConsumer(expectedConsumerId, hostpath.IdentityMatcher(identity.CONSUMER_TYPE))
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use your identity matcher object provided by the package

Copy link
Contributor

Choose a reason for hiding this comment

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

you should bundle the consumer test in a dedicated test with the focus of testing the consumer propagation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jschicktanz jschicktanz requested a review from Skarlso September 2, 2022 12:58
@jschicktanz jschicktanz merged commit e0a958c into main Sep 2, 2022
@jschicktanz jschicktanz deleted the js-work branch September 2, 2022 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secret Server (cc-utils) Credential Repository
5 participants