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

Register OSG APs (SOFTWARE-4791) #2032

Merged

Conversation

brianhlin
Copy link
Member

@brianhlin brianhlin commented Aug 30, 2021

Hosts/DNs/contacts mostly grabbed from https://github.com/opensciencegrid/osg-flock/blob/master/flock.opensciencegrid.org/flocking-hosts.yml#L7-L17

@rynge I threw the few that had dev or test in their names into ITB groups

@matyasselmeci
Copy link
Collaborator

This is a lot for one person to review. @edquist can we split this up? You take "topology/American Museum of Natural History through "topology/University of California San Diego" and I take "topology/University of Chicago" through "topology/University of Wisconsin Milwaukee" ?

@edquist
Copy link
Contributor

edquist commented Sep 1, 2021

I did look (gloss) this over, but @brianhlin, how would you like us to review this? Do you want us to verify everything added here against the flocking-hosts.yml you mentioned from the osg-flock repo?

@brianhlin
Copy link
Member Author

Do you want us to verify everything added here against the flocking-hosts.yml you mentioned from the osg-flock repo?

Yeah, that seems like the most reasonable way to do it. I had to make a bunch of educated guesses on new sites/resource groups so if anything looks offensive there, that'd be worth bringing up, too.

@matyasselmeci
Copy link
Collaborator

The half I checked looks good.

@@ -43,6 +44,7 @@ Resources:
Name: Kurt Strosahl
Description: Submit node for JLab
FQDN: scosg20.jlab.org
DN: /CN=scosg16.jlab.org
ID: 1134
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize this ID is not part of the change, but 1134 is already used by a Lamar-Cluster resource.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's concerning that our CI doesn't catch this :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

We test for name uniqueness and ID presence, but not ID uniqueness. I wonder why...

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh huh. Yeah i suppose we should add a unique ID check for the different types of things, ey?

Copy link
Contributor

Choose a reason for hiding this comment

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

Relatedly, @brianhlin & @matyasselmeci, would it make sense to add a check to verify that if a resource has a DN, the /CN part should match the FQDN ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we can assume that: the registered FQDN may correspond to one of the cert's SANs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok.

Also:

It's concerning that our CI doesn't catch this :(

We test for name uniqueness and ID presence, but not ID uniqueness. I wonder why...

Apparently there is a ticket for this - SOFTWARE-3287 specifies a few new unique ID requirements

@matyasselmeci
Copy link
Collaborator

I realized that the split was unfair so I checked Purdue through UCSD as well. Aside from my comments, they looked good.

Copy link
Contributor

@edquist edquist left a comment

Choose a reason for hiding this comment

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

See comments.

Also, i am just reviewing you changes. I have not tried to do any deep analysis of flocking-hosts to try to see if you are missing any resources.

Copy link
Contributor

@edquist edquist left a comment

Choose a reason for hiding this comment

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

No further objections for my half

@brianhlin brianhlin force-pushed the SOFTWARE-4791.register-osg-aps branch from d1f71ea to ae2c7bf Compare September 1, 2021 18:33
@brianhlin
Copy link
Member Author

Rebased off master and force-pushed

@brianhlin brianhlin merged commit c3f6722 into opensciencegrid:master Sep 1, 2021
@brianhlin brianhlin deleted the SOFTWARE-4791.register-osg-aps branch September 1, 2021 18:42
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.

None yet

3 participants