Skip to content

Enable secure API registration for OpenShift apiserver#348

Merged
rootfs merged 2 commits intomainfrom
fix_342
Oct 20, 2021
Merged

Enable secure API registration for OpenShift apiserver#348
rootfs merged 2 commits intomainfrom
fix_342

Conversation

@oglok
Copy link
Contributor

@oglok oglok commented Oct 20, 2021

Signed-off-by: Ricardo Noriega rnoriega@redhat.com

Closes #342

@oglok
Enable secure API registration for OpenShift apiserver
f864152 
Signed-off-by: Ricardo Noriega <rnoriega@redhat.com>
@openshift-ci openshift-ci bot requested review from copejon and sallyom October 20, 2021 11:18
@oglok oglok requested review from fzdarsky and rootfs October 20, 2021 11:21
fzdarsky
fzdarsky reviewed Oct 20, 2021
View reviewed changes
pkg/controllers/ocp-controllers.go
"--logtostderr=" + strconv.FormatBool(cfg.LogDir == "" || cfg.LogAlsotostderr),
"--alsologtostderr=" + strconv.FormatBool(cfg.LogAlsotostderr),
"--v=" + strconv.Itoa(cfg.LogVLevel),
"--vmodule=" + cfg.LogVModule,
Copy link
Contributor

@fzdarsky fzdarsky Oct 20, 2021

Choose a reason for hiding this comment

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

What is the reason to remove the logging-related params (the one here and the two above)?

Copy link
Contributor Author

@oglok oglok Oct 20, 2021

Choose a reason for hiding this comment

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

That's right, let me add them again.

pkg/controllers/ocp-controllers.go
"--requestheader-allowed-names=kube-apiserver-proxy,system:kube-apiserver-proxy,system:openshift-aggregator",
"--requestheader-username-headers=X-Remote-User",
"--requestheader-group-headers=X-Remote-Group",
"--requestheader-extra-headers-prefix=X-Remote-Extra-",
Copy link
Contributor

@fzdarsky fzdarsky Oct 20, 2021

Choose a reason for hiding this comment

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

For the request headers, did you compare notes with OpenShift whether they are not needed? I’ve noticed we’re still configuring them in kube-apiserver via command line args (incl. CA certs) and (due to the analogy between both API servers) I’m wondering whether we’d want our approach to configuring kube-api and openshift-api in sync?

Copy link
Contributor Author

@oglok oglok Oct 20, 2021

Choose a reason for hiding this comment

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

I tried to use the bare minimum number of flags in order to use the config file as much as possible.

@rootfs
Copy link
Member

rootfs commented Oct 20, 2021

@oglok can you post the results from the following commands?

kubectl get apiservices |grep openshift
kubectl api-resources |grep openshift

@oglok
Copy link
Contributor Author

oglok commented Oct 20, 2021

kubectl get apiservices |grep openshift

Sure!

kubectl get apiservices |grep openshift
v1.apps.openshift.io                     default/openshift-apiserver   True        3h
v1.authorization.openshift.io            Local                         True        3h1m
v1.build.openshift.io                    default/openshift-apiserver   True        3h
v1.config.openshift.io                   Local                         True        3h1m
v1.image.openshift.io                    default/openshift-apiserver   True        3h
v1.imageregistry.operator.openshift.io   Local                         True        3h1m
v1.project.openshift.io                  default/openshift-apiserver   True        3h
v1.quota.openshift.io                    Local                         True        3h
v1.route.openshift.io                    default/openshift-apiserver   True        3h
v1.security.openshift.io                 Local                         True        3h1m
v1.template.openshift.io                 default/openshift-apiserver   True        3h
v1alpha1.operator.openshift.io           Local                         True        3h1m

[root@fedora ~]# kubectl api-resources |grep openshift
deploymentconfigs                 dc           apps.openshift.io/v1                     true         DeploymentConfig
rolebindingrestrictions                        authorization.openshift.io/v1            true         RoleBindingRestriction
buildconfigs                      bc           build.openshift.io/v1                    true         BuildConfig
builds                                         build.openshift.io/v1                    true         Build
builds                                         config.openshift.io/v1                   false        Build
images                                         config.openshift.io/v1                   false        Image
proxies                                        config.openshift.io/v1                   false        Proxy
images                                         image.openshift.io/v1                    false        Image
imagesignatures                                image.openshift.io/v1                    false        ImageSignature
imagestreamimages                 isimage      image.openshift.io/v1                    true         ImageStreamImage
imagestreamimports                             image.openshift.io/v1                    true         ImageStreamImport
imagestreammappings                            image.openshift.io/v1                    true         ImageStreamMapping
imagestreams                      is           image.openshift.io/v1                    true         ImageStream
imagestreamtags                   istag        image.openshift.io/v1                    true         ImageStreamTag
imagetags                         itag         image.openshift.io/v1                    true         ImageTag
configs                                        imageregistry.operator.openshift.io/v1   false        Config
imagecontentsourcepolicies                     operator.openshift.io/v1alpha1           false        ImageContentSourcePolicy
projectrequests                                project.openshift.io/v1                  false        ProjectRequest
projects                                       project.openshift.io/v1                  false        Project
clusterresourcequotas                          quota.openshift.io/v1                    false        ClusterResourceQuota
routes                                         route.openshift.io/v1                    true         Route
securitycontextconstraints                     security.openshift.io/v1                 false        SecurityContextConstraints
brokertemplateinstances                        template.openshift.io/v1                 false        BrokerTemplateInstance
processedtemplates                             template.openshift.io/v1                 true         Template
templateinstances                              template.openshift.io/v1                 true         TemplateInstance
templates                                      template.openshift.io/v1                 true         Template

@oglok
Re-enable logging flags
1046251 
Signed-off-by: Ricardo Noriega <rnoriega@redhat.com>
sallyom
sallyom reviewed Oct 20, 2021
View reviewed changes
Copy link
Contributor

@sallyom sallyom left a comment

Choose a reason for hiding this comment

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

see the naming nit, but other than that lgtm

pkg/controllers/apiservice.go
if err != nil {
return err
}
caFile, err := ioutil.ReadFile(cfg.DataDir + "/certs/ca-bundle/ca-bundle.crt")
Copy link
Contributor

@sallyom sallyom Oct 20, 2021

Choose a reason for hiding this comment

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

nit: instead of caFile, caBytes or caData

husky-parul
husky-parul approved these changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@husky-parul husky-parul left a comment

Choose a reason for hiding this comment

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

/LGTM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: husky-parul

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

The pull request process is described here

Details 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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2021
@rootfs rootfs merged commit 81a9e1e into main Oct 20, 2021
@fzdarsky fzdarsky deleted the fix_342 branch October 21, 2021 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sallyom sallyom sallyom left review comments

@fzdarsky fzdarsky fzdarsky left review comments

@copejon copejon Awaiting requested review from copejon

@rootfs rootfs Awaiting requested review from rootfs

+1 more reviewer

@husky-parul husky-parul husky-parul approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@husky-parul husky-parul

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.

[BUG] OpenShift APIs registration does not use TLS certificates

5 participants

@oglok @rootfs @sallyom @fzdarsky @husky-parul