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.

Sign up for GitHub

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

Jump to bottom

OAuthAPIServerController #344

Merged
merged 1 commit into from Mar 30, 2020
Merged

OAuthAPIServerController #344

merged 1 commit into from Mar 30, 2020

Conversation

p0lyn0mial
Copy link
Contributor

@p0lyn0mial p0lyn0mial commented Mar 25, 2020 •
edited

this PR adds OAuthAPIServerController that will manage encryption-config-openshift-oauth-apiserver as described in https://github.com/openshift/enhancements/blob/master/enhancements/etcd/etcd-encryption-for-separate-oauth-apis.md

@p0lyn0mial p0lyn0mial force-pushed the manage-encryption-for-oauth-apiserver branch from b171113 to 5e93a38 Compare March 25, 2020 11:20
@p0lyn0mial p0lyn0mial changed the title wip OAuthAPIServerController Mar 25, 2020
@p0lyn0mial
Copy link
Contributor Author

/assign @sttts

@p0lyn0mial
Copy link
Contributor Author

/test e2e-aws

@p0lyn0mial p0lyn0mial force-pushed the manage-encryption-for-oauth-apiserver branch 3 times, most recently from 804591b to 4b1ed30 Compare March 26, 2020 20:06
sttts
sttts reviewed Mar 27, 2020
View reviewed changes
pkg/operator/oauthapiencryptioncontroller/oauthapiencryptioncontroller.go Outdated
if apierrors.IsNotFound(err) {
// case 1: create with annotation
if _, exists := openshiftAPIServerEncryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey]; !exists {
return fmt.Errorf("%s/%s doesn't contain the required key %q", openshiftAPIServerEncryptionCfg.Name, openshiftAPIServerEncryptionCfg.Namespace, encryptionconfig.EncryptionConfSecretKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

swap ns and name

sttts
sttts reviewed Mar 27, 2020
View reviewed changes
pkg/operator/oauthapiencryptioncontroller/oauthapiencryptioncontroller.go Outdated
encryptionCfg.Annotations[encryptionstate.KubernetesDescriptionKey] = encryptionstate.KubernetesDescriptionScaryValue
encryptionCfg.Data = map[string][]byte{}
encryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey] = openshiftAPIServerEncryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey]
encryptionCfg.Finalizers = []string{encryptionsecret.EncryptionSecretFinalizer}
Copy link
Contributor

Choose a reason for hiding this comment

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

encryptionCfg := &corev1.Secret{
  ObjectMeta: metav1.ObjectMeta{
    Name: ...
    Namespcae: ...
  },
  ...
}

is easier to read IMO

sttts
sttts reviewed Mar 27, 2020
View reviewed changes
pkg/operator/oauthapiencryptioncontroller/oauthapiencryptioncontroller.go Outdated
encryptionCfg.Annotations[encryptionstate.KubernetesDescriptionKey] = encryptionstate.KubernetesDescriptionScaryValue
encryptionCfg.Data = map[string][]byte{}
encryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey] = openshiftAPIServerEncryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey]
encryptionCfg.Finalizers = []string{encryptionsecret.EncryptionSecretFinalizer}
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a controller doing finalizing? Or is it just there to block deletion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to block deletion.

sttts
sttts reviewed Mar 27, 2020
View reviewed changes
pkg/operator/oauthapiencryptioncontroller/oauthapiencryptioncontroller.go
encryptionCfg.Annotations[encryptionConfigManagedBy] = encryptionConfigManagedByValue
encryptionCfg.Annotations[encryptionstate.KubernetesDescriptionKey] = encryptionstate.KubernetesDescriptionScaryValue
encryptionCfg.Data = map[string][]byte{}
encryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey] = openshiftAPIServerEncryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why not whole Data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a map and someone else could have added entries we don't need.

sttts
sttts reviewed Mar 27, 2020
View reviewed changes
pkg/operator/oauthapiencryptioncontroller/oauthapiencryptioncontroller.go Outdated
oasEncryptionCfgData := openshiftAPIServerEncryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey]
if !equality.Semantic.DeepEqual(oauthEncryptionCfgData, oasEncryptionCfgData) {
encryptionCfg := oauthAPIServerEncryptionCfg.DeepCopy()
encryptionCfg.Data[encryptionconfig.EncryptionConfSecretKey] = oasEncryptionCfgData
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't we have any annotations that play a role? Or just for keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we actually use https://github.com/openshift/library-go/blob/master/pkg/operator/encryption/encryptionconfig/secret.go#L71 but the content is the same :) (uff)

@p0lyn0mial p0lyn0mial force-pushed the manage-encryption-for-oauth-apiserver branch from 4b1ed30 to 736bfa9 Compare March 27, 2020 16:45
@sttts
Copy link
Contributor

sttts commented Mar 30, 2020

/retest
/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2020
@openshift-merge-robot openshift-merge-robot merged commit add7cd6 into openshift:master Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@p0lyn0mial @sttts @openshift-ci-robot @openshift-merge-robot